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

Restrict size of common string attribute kinds #4584

Merged
merged 2 commits into from
Oct 9, 2024

Conversation

LucasG0
Copy link
Contributor

@LucasG0 LucasG0 commented Oct 8, 2024

Would fix #4432

@github-actions github-actions bot added the group/backend Issue related to the backend (API Server, Git Agent) label Oct 8, 2024
@LucasG0 LucasG0 force-pushed the lgu-restrict-attribute-size branch 2 times, most recently from d6da0e4 to 2671a7f Compare October 8, 2024 23:41
Copy link

codspeed-hq bot commented Oct 8, 2024

CodSpeed Performance Report

Merging #4584 will not alter performance

Comparing lgu-restrict-attribute-size (1b038be) with develop (4b059e1)

Summary

✅ 10 untouched benchmarks

@LucasG0 LucasG0 force-pushed the lgu-restrict-attribute-size branch from 2671a7f to 292bb88 Compare October 9, 2024 00:11
Copy link
Collaborator

@dgarros dgarros left a comment

Choose a reason for hiding this comment

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

Great work
Please can you add a newsfragment

Copy link
Contributor

@ogenstad ogenstad left a comment

Choose a reason for hiding this comment

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

Nice!

@classmethod
def validate_content(cls, value: Any, name: str, schema: AttributeSchema) -> None:
super().validate_content(value, name, schema)
if ATTRIBUTE_TYPES[schema.kind] in [Text, Password]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add URL here as well, don't think it will ever be relevant but you never know..

Copy link
Collaborator

Choose a reason for hiding this comment

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

I missed this one, I don't think we should hardcode the list if kinds that will be validated here, mostlikely this list will grow in the future and we'll forget about it
Might be better to have a list of kinds that are exempted from the check

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking closer into into it, ideally we should apply the validation to all attributes, after serialize, (except if the kind is exempted from the check)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, added modifications to validate any non-large attribute. This is done after serializing to avoid an extra serialization.

@LucasG0 LucasG0 force-pushed the lgu-restrict-attribute-size branch from 4f8c85a to 302f474 Compare October 9, 2024 09:52
@LucasG0 LucasG0 force-pushed the lgu-restrict-attribute-size branch from 302f474 to 1b038be Compare October 9, 2024 09:54
@LucasG0 LucasG0 requested review from ogenstad and dgarros October 9, 2024 10:02
@LucasG0 LucasG0 merged commit 8151582 into develop Oct 9, 2024
30 checks passed
@LucasG0 LucasG0 deleted the lgu-restrict-attribute-size branch October 9, 2024 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
group/backend Issue related to the backend (API Server, Git Agent)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: Restrict size of common attribute kinds
3 participants