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.

Sign up for GitHub

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

Jump to bottom

Loading a schema with an incorrect order_by field raise a proper error #4427

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Loading a schema with an incorrect order_by field raise a proper error #4427

merged 1 commit into from
Sep 25, 2024

Conversation

LucasG0
Copy link
Contributor

@LucasG0 LucasG0 commented Sep 23, 2024

No description provided.

@LucasG0 LucasG0 requested a review from ajtmccarty September 23, 2024 15:52
@github-actions github-actions bot added the group/backend Issue related to the backend (API Server, Git Agent) label Sep 23, 2024
ajtmccarty
ajtmccarty reviewed Sep 23, 2024
View reviewed changes
Copy link
Contributor

@ajtmccarty ajtmccarty left a comment

Choose a reason for hiding this comment

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

all look good, just one question on an updated test

backend/tests/unit/core/schema_manager/test_manager_schema.py Outdated
@@ -1226,7 +1233,6 @@ async def test_validate_display_labels_error(schema_all_in_one, display_labels,
["my_generic_name__value", "mybool__value"],
["my_generic_name__value"],
["primary_tag__name__value"],
["status__name__value", "mybool__value"],
Copy link
Contributor

Choose a reason for hiding this comment

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

did this one start to fail? it seems like it should still be allowed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it failed because Status is an optional relationship of GenericInterface. Not sure what is the expected behavior here, this pattern is currently not supported by uniqueness_constraints

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, order_by should support optional cardinality-one relationship

dgarros
dgarros reviewed Sep 23, 2024
View reviewed changes
backend/infrahub/core/schema_manager.py Outdated
@@ -523,6 +523,30 @@ def generate_identifiers(self) -> None:
rel.identifier = str("__".join(sorted([node.kind, rel.peer]))).lower()
self.set(name=name, schema=node)

@staticmethod
def validate_attr_or_mandatory_rel_path(
schema_path: SchemaAttributePath, node_schema: MainSchemaTypes, element_name: str, element_path: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you considered using the function SchemaBranch.validate_schema_path here ? it has been designed specifically so you can provide the criteria using flags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but some criteria seem to be significantly specific to certain fields. For instance, uniqueness_constraints raises on optional relationship while order_by should not, so it might be relevant to have specific logic in dedicated functions

@dgarros
Copy link
Collaborator

dgarros commented Sep 23, 2024

Also, would be good to add a newsfragment for the changelog

@LucasG0 LucasG0 force-pushed the lgu-load-incorrect-order-by branch from ec80aac to 1e2ad37 Compare September 24, 2024 15:27
@github-actions github-actions bot added the type/documentation Improvements or additions to documentation label Sep 24, 2024
@LucasG0 LucasG0 force-pushed the lgu-load-incorrect-order-by branch from 1e2ad37 to f60b4b5 Compare September 24, 2024 15:28
@github-actions github-actions bot removed the type/documentation Improvements or additions to documentation label Sep 24, 2024
@LucasG0 LucasG0 force-pushed the lgu-load-incorrect-order-by branch 3 times, most recently from 768db23 to 3667c0f Compare September 24, 2024 15:42
ajtmccarty
ajtmccarty approved these changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@ajtmccarty ajtmccarty left a comment

Choose a reason for hiding this comment

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

big improvements all around

dgarros
dgarros approved these changes Sep 24, 2024
View reviewed changes
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 @LucasG0

@LucasG0 LucasG0 force-pushed the lgu-load-incorrect-order-by branch 2 times, most recently from 8a85590 to 834724b Compare September 24, 2024 16:37
@LucasG0 LucasG0 force-pushed the lgu-load-incorrect-order-by branch from 834724b to f1ce2f6 Compare September 25, 2024 08:36
@LucasG0 LucasG0 merged commit b4ee01b into stable Sep 25, 2024
31 checks passed
@LucasG0 LucasG0 deleted the lgu-load-incorrect-order-by branch September 25, 2024 14:37
@LucasG0 LucasG0 mentioned this pull request Sep 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajtmccarty ajtmccarty ajtmccarty approved these changes

@dgarros dgarros dgarros approved these changes

Assignees
No one assigned
Labels
group/backend Issue related to the backend (API Server, Git Agent)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LucasG0 @dgarros @ajtmccarty