IFC-1238: Fix failure when removing generic attribute and its uniqueness constraint#9288
IFC-1238: Fix failure when removing generic attribute and its uniqueness constraint#9288solababs wants to merge 2 commits into
Conversation
github-actions
Bot
added
the
group/backend
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 3 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. This change adds a targeted filter to remove uniqueness constraint paths that reference attributes being deleted, preventing a 500 error during generic schema updates, and includes a thorough integration test to confirm the fix.
Re-trigger cubic
ajtmccarty
left a comment
ajtmccarty
left a comment
There was a problem hiding this comment.
can you update the description to explain why this works for a Node schema but does not for a Generic schema? I don't really understand why one works but not the other
| async def test_final_validate(self, db: InfrahubDatabase) -> None: | ||
| await verify_no_duplicate_relationships(db=db) | ||
| await verify_no_edges_added_after_node_delete(db=db) | ||
| await assert_no_virtual_schema_relationships_in_db(db=db) |
| paths | ||
| for paths in updated_node.uniqueness_constraints | ||
| if not any(path.split("__", maxsplit=1)[0] in deleted_names for path in paths) | ||
| ] or None |
There was a problem hiding this comment.
I'm not certain this is what we want. for example, if I have uniqueness_constraints like this [["name__value", "color__value"], ["model__value", "color__value"]] and I remove color from the schema, then this logic would remove all the uniqueness constraints. at least, that's what it looks like to me, correct me if I'm wrong.
I think we need the user to decide what should happen to multi-element uniqueness constraints that include an attribute being deleted, so raising an error is probably the correct thing to do. Although I think it should be a ValueError and not a 500 Server Error
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would auto-approve. The one-line change in schema_branch.py filters out uniqueness constraint paths referencing attributes marked as absent, preventing the 500 error while keeping the logic narrow and safe, and the new integration tests thoroughly validate both the rejection of partial removal and success of...
Re-trigger cubic
ajtmccarty
left a comment
ajtmccarty
left a comment
There was a problem hiding this comment.
it looks like this PR just adds a blank line and a test
2 participants
Why
When an attribute and its
uniqueness_constraintsentry are removed from a generic schema in a single update, the schema reload raises a 500 Internal Server Error. The error is:Root cause:
process_attributes_state()removes ABSENT attributes from the schema object but leaves stale paths inuniqueness_constraints. The immediately followingsync_uniqueness_constraints_and_unique_attributes()then callsparse_schema_path()on a path whose attribute no longer exists, triggeringAttributePathParsingError. The same operation works for regular node schemas and succeeds when split into two sequential updates.Closes #5735
How to test
uv run pytest backend/tests/integration/schema_lifecycle/test_migration_uniqueness_constraints.py -v -k "TestGenericRemoveAttributesAndConstraints"Checklist
changelog/5735.fixed.md)Summary by cubic
Fixes a 500 error when removing a generic attribute and its uniqueness constraint in the same schema update. We now drop constraint paths that reference attributes marked
absent, so the update succeeds in one pass (addresses IFC-1238).process_attributes_state(), filter outuniqueness_constraintsthat reference attributes with stateabsentto avoid stale paths andAttributePathParsingError.TestGenericRemoveAttributesAndConstraintsto assert rejection when only the attribute is removed and success when attributes and constraints are removed together.Written for commit 308f39d. Summary will update on new commits. Review in cubic