Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add UidBone #1131

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

ArneGudermann
Copy link
Contributor

Proposal for #1117

@ArneGudermann ArneGudermann added the viur-meeting Issues to discuss in the next ViUR meeting label Apr 11, 2024
@ArneGudermann
Copy link
Contributor Author

Some Points for the viur meeting

  1. should the value for an add be rendered out?
  2. readonly ?
  3. lang/multiple allowed?

@sveneberth sveneberth linked an issue Apr 12, 2024 that may be closed by this pull request
@ArneGudermann ArneGudermann removed the viur-meeting Issues to discuss in the next ViUR meeting label Apr 18, 2024
@ArneGudermann ArneGudermann marked this pull request as ready for review April 18, 2024 08:05
src/viur/core/bones/uid.py Outdated Show resolved Hide resolved
src/viur/core/bones/uid.py Outdated Show resolved Hide resolved
src/viur/core/bones/uid.py Outdated Show resolved Hide resolved
src/viur/core/bones/uid.py Outdated Show resolved Hide resolved
src/viur/core/bones/uid.py Outdated Show resolved Hide resolved
src/viur/core/bones/uid.py Outdated Show resolved Hide resolved
src/viur/core/bones/uid.py Outdated Show resolved Hide resolved
src/viur/core/bones/uid.py Outdated Show resolved Hide resolved
@ArneGudermann
Copy link
Contributor Author

mhh i have found an interesting bug. So if I call up a skel view in which this bone exists but it has not yet been written, a uid is automatically assigned but not written in the skel. This leads to the number not being used.

@ArneGudermann ArneGudermann added viur-meeting Issues to discuss in the next ViUR meeting and removed viur-meeting Issues to discuss in the next ViUR meeting labels Aug 1, 2024
@ArneGudermann
Copy link
Contributor Author

mhh i have found an interesting bug. So if I call up a skel view in which this bone exists but it has not yet been written, a uid is automatically assigned but not written in the skel. This leads to the number not being used.

Fixed by: 041da3b

Copy link
Member

@sveneberth sveneberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worked in my test, but I didn't any pentest. But tests in production are usually the best test anyways 😆

pattern = pattern()
self.pattern = str(pattern)
if self.pattern.count("*") != 1:
raise ValueError("Only one Wildcard (*) is allowed in the pattern")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raise ValueError("Only one Wildcard (*) is allowed in the pattern")
raise ValueError("Only one wildcard (*) is allowed and required in the pattern")

Comment on lines +88 to +89
if len(self.fillchar) != 1:
raise ValueError("Only one char is allowed as fillchar")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ValueError is also raised on an empty fillchar, which is the default. Is this wanted?

Comment on lines +78 to +79
if not self.readOnly:
self.readOnly = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you solve this differently? Default readOnly flag shall be True, and changing it should raise a ValueError. The user should know that this bone can't be set readOnly=False.

"""
Initializes a new UidBone.


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

@phorward phorward added this to the ViUR-core v3.7 milestone Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Implement UidBone
3 participants