IFC-2048: Remove optional and unique attributes from profile schema#8577
IFC-2048: Remove optional and unique attributes from profile schema#8577
Conversation
github-actions
bot
added
the
group/backend
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change prevents generated Profile schemas from including attributes that are optional and part of a node's uniqueness constraints. It adds two helper methods on BaseNodeSchema to detect uniqueness-constraint membership and to decide if an attribute should support profiles. Profile-generation checks were updated to use that helper across schema generation and migration queries. A new migration (node.uniqueness_constraints.update) and accompanying tests were added to handle uniqueness-constraint membership changes; migration registry, docs, and OpenAPI metadata for 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/infrahub/core/schema/schema_branch.py (1)
2369-2376: Document the new helper's matching rules.Please add a docstring here. The helper now defines non-trivial behavior (
attrandattr__fieldmatching), and backend Python changes are supposed to document public methods consistently.As per coding guidelines, "Always use triple quotes (""") for Python docstrings" and "Follow Google-style docstring format in Python".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/core/schema/schema_branch.py` around lines 2369 - 2376, Add a Google-style triple-quoted docstring to the function check_attr_in_uniqueness_constraint describing its purpose and matching rules: it checks whether a given attribute name (attr) appears in any NodeSchema.uniqueness_constraints either exactly or as a prefix for nested fields using the "attr__field" pattern (i.e., constraint_path == attr or constraint_path.startswith(f"{attr}__")). Document the parameters (attr: str, node_schema: NodeSchema), the boolean return value, and a brief example or note about the expected uniqueness_constraints structure to clarify behavior.changelog/7644.added.md (1)
1-1: Clarify changelog entry wording.The current wording could be more precise about when attributes are excluded. Based on the PR objectives, attributes are excluded when they are both optional AND participate in uniqueness constraints (not just when they have
unique=True).✍️ Suggested rewording for clarity
-Added condition to restrict adding attributes that are unique and optional to generated profile schemas +Restrict optional attributes that participate in uniqueness constraints from being added to generated profile schemasor alternatively:
-Added condition to restrict adding attributes that are unique and optional to generated profile schemas +Profile schemas now exclude attributes that are both optional and part of uniqueness constraintsThis makes it clearer that the restriction applies specifically to attributes in
uniqueness_constraints, which aligns with the implementation and test case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@changelog/7644.added.md` at line 1, Update the changelog sentence to explicitly state that attributes are excluded only when they are both optional and part of uniqueness constraints (i.e., participate in uniqueness_constraints), not merely when they have unique=True; mention "optional and participate in uniqueness_constraints" and optionally reference the `uniqueness_constraints` term and `unique=True` to clarify the distinction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/infrahub/core/schema/schema_branch.py`:
- Around line 2369-2376: The helper check_attr_in_uniqueness_constraint
currently only inspects node_schema.uniqueness_constraints, so optional
attributes with unique=True can slip through before constraints are
materialized; update check_attr_in_uniqueness_constraint to also return True if
the node_schema defines the attribute with unique=True (e.g., inspect
node_schema.attributes[attr].unique or equivalent attribute metadata) so profile
generation/pre-validation will treat it as constrained; apply the same change at
the parallel location referenced around the lines handling the same logic (the
duplicate block near 2429-2431) so both checks consider attribute-level
unique=True in addition to node_schema.uniqueness_constraints.
In `@backend/tests/component/core/schema_manager/test_manager_schema.py`:
- Around line 4667-4682: Add a Google-style docstring to the test function
test_profile_does_not_contain_optional_unique_attributes describing its intent:
that when a schema defines a unique attribute marked optional (attribute "name"
in the Router schema), the generated ProfileNetworkRouter should not expose that
optional unique attribute and attempting to access it via
Profile.get_attribute("name") should raise a ValueError; place the docstring
immediately below the async def line and use one brief summary sentence followed
by a short "Args" section if desired to document any parameters (none for pytest
tests).
---
Nitpick comments:
In `@backend/infrahub/core/schema/schema_branch.py`:
- Around line 2369-2376: Add a Google-style triple-quoted docstring to the
function check_attr_in_uniqueness_constraint describing its purpose and matching
rules: it checks whether a given attribute name (attr) appears in any
NodeSchema.uniqueness_constraints either exactly or as a prefix for nested
fields using the "attr__field" pattern (i.e., constraint_path == attr or
constraint_path.startswith(f"{attr}__")). Document the parameters (attr: str,
node_schema: NodeSchema), the boolean return value, and a brief example or note
about the expected uniqueness_constraints structure to clarify behavior.
In `@changelog/7644.added.md`:
- Line 1: Update the changelog sentence to explicitly state that attributes are
excluded only when they are both optional and part of uniqueness constraints
(i.e., participate in uniqueness_constraints), not merely when they have
unique=True; mention "optional and participate in uniqueness_constraints" and
optionally reference the `uniqueness_constraints` term and `unique=True` to
clarify the distinction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3258911-bbf8-45c5-ba94-67a4c6cbb26a
📒 Files selected for processing (3)
backend/infrahub/core/schema/schema_branch.pybackend/tests/component/core/schema_manager/test_manager_schema.pychangelog/7644.added.md
| async def test_profile_does_not_contain_optional_unique_attributes() -> None: | ||
| schema = { | ||
| "namespace": "Network", | ||
| "name": "Router", | ||
| "uniqueness_constraints": [["name__value"]], | ||
| "human_friendly_id": ["name__value"], | ||
| "display_label": "name__value", | ||
| "attributes": [{"name": "name", "kind": "Text", "optional": True}], | ||
| } | ||
| schema_branch = SchemaBranch(cache={}, name="test") | ||
| schema_branch.load_schema(schema=SchemaRoot(nodes=[schema])) | ||
| schema_branch.process() | ||
|
|
||
| network_router_profile = schema_branch.get(name="ProfileNetworkRouter", duplicate=False) | ||
| with pytest.raises(ValueError, match="Unable to find the attribute name"): | ||
| network_router_profile.get_attribute("name") |
There was a problem hiding this comment.
Add docstring to document test purpose.
The test function lacks a docstring explaining its purpose, which makes it harder for other developers to understand what behavior is being tested and why.
📝 Proposed fix to add docstring
async def test_profile_does_not_contain_optional_unique_attributes() -> None:
+ """Test that optional attributes in uniqueness constraints are excluded from profile schemas.
+
+ When an attribute is both optional and participates in a uniqueness constraint,
+ it should not be included in the generated profile schema node to prevent
+ profiles from allowing values that would violate uniqueness semantics.
+ """
schema = {As per coding guidelines, Python functions should include docstrings following Google-style format with a brief description.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def test_profile_does_not_contain_optional_unique_attributes() -> None: | |
| schema = { | |
| "namespace": "Network", | |
| "name": "Router", | |
| "uniqueness_constraints": [["name__value"]], | |
| "human_friendly_id": ["name__value"], | |
| "display_label": "name__value", | |
| "attributes": [{"name": "name", "kind": "Text", "optional": True}], | |
| } | |
| schema_branch = SchemaBranch(cache={}, name="test") | |
| schema_branch.load_schema(schema=SchemaRoot(nodes=[schema])) | |
| schema_branch.process() | |
| network_router_profile = schema_branch.get(name="ProfileNetworkRouter", duplicate=False) | |
| with pytest.raises(ValueError, match="Unable to find the attribute name"): | |
| network_router_profile.get_attribute("name") | |
| async def test_profile_does_not_contain_optional_unique_attributes() -> None: | |
| """Test that optional attributes in uniqueness constraints are excluded from profile schemas. | |
| When an attribute is both optional and participates in a uniqueness constraint, | |
| it should not be included in the generated profile schema node to prevent | |
| profiles from allowing values that would violate uniqueness semantics. | |
| """ | |
| schema = { | |
| "namespace": "Network", | |
| "name": "Router", | |
| "uniqueness_constraints": [["name__value"]], | |
| "human_friendly_id": ["name__value"], | |
| "display_label": "name__value", | |
| "attributes": [{"name": "name", "kind": "Text", "optional": True}], | |
| } | |
| schema_branch = SchemaBranch(cache={}, name="test") | |
| schema_branch.load_schema(schema=SchemaRoot(nodes=[schema])) | |
| schema_branch.process() | |
| network_router_profile = schema_branch.get(name="ProfileNetworkRouter", duplicate=False) | |
| with pytest.raises(ValueError, match="Unable to find the attribute name"): | |
| network_router_profile.get_attribute("name") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/component/core/schema_manager/test_manager_schema.py` around
lines 4667 - 4682, Add a Google-style docstring to the test function
test_profile_does_not_contain_optional_unique_attributes describing its intent:
that when a schema defines a unique attribute marked optional (attribute "name"
in the Router schema), the generated ProfileNetworkRouter should not expose that
optional unique attribute and attempting to access it via
Profile.get_attribute("name") should raise a ValueError; place the docstring
immediately below the async def line and use one brief summary sentence followed
by a short "Args" section if desired to document any parameters (none for pytest
tests).
| assert pool_rel.peer == InfrahubKind.NUMBERPOOL | ||
|
|
||
|
|
||
| async def test_profile_does_not_contain_optional_unique_attributes() -> None: |
There was a problem hiding this comment.
test should cover uniqueness_constraints with multiple values too
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/infrahub/core/schema/attribute_schema.py (1)
83-85: Redundant condition insupport_profiles.The clause
not (self.optional and self.unique)is alwaysTruewhenself.unique is False(already checked earlier in the expression), making it a no-op. If this was intended to document intent, consider a comment instead. If the logic needs to change, the condition should be revisited.# Current: always True when self.unique is False return self.read_only is False and self.unique is False and not (self.optional and self.unique) # Simplified equivalent: return self.read_only is False and self.unique is False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/core/schema/attribute_schema.py` around lines 83 - 85, The support_profiles property contains a redundant condition: "not (self.optional and self.unique)" is always true when "self.unique is False", so update the boolean expression in support_profiles to remove that clause (i.e., return self.read_only is False and self.unique is False) to simplify logic; if the extra clause was intentional for documentation, instead keep the simplified expression and add a brief inline comment inside the support_profiles property explaining the intended constraint involving optional/unique to preserve intent.backend/infrahub/core/schema/basenode_schema.py (1)
690-697: LGTM with a minor suggestion.The implementation correctly identifies attributes in uniqueness constraints by matching both exact names and
attr__fieldstyle paths. Consider adding a brief docstring for clarity.📝 Optional: Add docstring
def check_attr_in_uniqueness_constraint(self, attr: str) -> bool: + """Check if an attribute participates in any uniqueness constraint. + + Args: + attr: The attribute name to check. + + Returns: + True if the attribute appears in any uniqueness constraint path. + """ if not self.uniqueness_constraints: return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/core/schema/basenode_schema.py` around lines 690 - 697, Add a brief docstring to the method check_attr_in_uniqueness_constraint explaining its purpose and behavior: state that it checks whether a given attribute name appears in any uniqueness constraint (either as an exact match or as a prefix in "attr__field" style paths), document the parameter attr: str and the boolean return value, and note that it returns False when self.uniqueness_constraints is empty; place the docstring directly above the def check_attr_in_uniqueness_constraint line.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/infrahub/core/relationship/constraints/profiles_removal.py`:
- Around line 39-43: The condition in the profiles removal check is wrong:
because it requires not attr_schema.optional, the nested clause
attr_schema.optional and schema.check_attr_in_uniqueness_constraint(...) is
unreachable. Update the boolean logic in the function that contains this
condition (the block using attr_schema.support_profiles and
schema.check_attr_in_uniqueness_constraint) so the uniqueness-constraint check
is actually evaluated—for example, remove the contradictory not
attr_schema.optional requirement or rewrite the predicate as
"attr_schema.support_profiles and not (attr_schema.optional and
schema.check_attr_in_uniqueness_constraint(attr=attr_schema.name))" if you want
to allow required attrs unless they are in a uniqueness constraint, or as
"attr_schema.support_profiles and not
schema.check_attr_in_uniqueness_constraint(attr=attr_schema.name)" if required
attrs should also be excluded; adjust the condition in the same function where
attr_schema and schema are referenced.
---
Nitpick comments:
In `@backend/infrahub/core/schema/attribute_schema.py`:
- Around line 83-85: The support_profiles property contains a redundant
condition: "not (self.optional and self.unique)" is always true when
"self.unique is False", so update the boolean expression in support_profiles to
remove that clause (i.e., return self.read_only is False and self.unique is
False) to simplify logic; if the extra clause was intentional for documentation,
instead keep the simplified expression and add a brief inline comment inside the
support_profiles property explaining the intended constraint involving
optional/unique to preserve intent.
In `@backend/infrahub/core/schema/basenode_schema.py`:
- Around line 690-697: Add a brief docstring to the method
check_attr_in_uniqueness_constraint explaining its purpose and behavior: state
that it checks whether a given attribute name appears in any uniqueness
constraint (either as an exact match or as a prefix in "attr__field" style
paths), document the parameter attr: str and the boolean return value, and note
that it returns False when self.uniqueness_constraints is empty; place the
docstring directly above the def check_attr_in_uniqueness_constraint line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 383dcee9-cb7a-4515-8d76-ba890f3e125b
📒 Files selected for processing (7)
backend/infrahub/core/migrations/schema/attribute_supports_generated_schema.pybackend/infrahub/core/migrations/schema/node_attribute_add.pybackend/infrahub/core/relationship/constraints/profiles_removal.pybackend/infrahub/core/schema/attribute_schema.pybackend/infrahub/core/schema/basenode_schema.pybackend/infrahub/core/schema/schema_branch.pybackend/tests/component/core/schema_manager/test_manager_schema.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/component/core/schema_manager/test_manager_schema.py
|
I think the full fix for this requires keep profile schemas correctly updated when a schema's |
…tributes-from-profile-schema-ifc-2048
…optional-and-unique-attributes-from-profile-schema-ifc-2048' into sb-11032025-remove-optional-and-unique-attributes-from-profile-schema-ifc-2048
github-actions
bot
added
the
type/documentation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/component/core/migrations/schema/test_node_uniqueness_constraints_update.py (1)
104-267: Solid migration regression coverage; add one case for required attributes.The suite thoroughly exercises optional-attribute transitions. Please add a no-op test where a non-optional
support_profilesattribute is in/out ofuniqueness_constraintsand assert no profile attribute removal/add migration is executed.Suggested additional test shape
+async def test_migration_no_change_for_required_attr_in_uniqueness_constraint( + db: InfrahubDatabase, + default_branch: Branch, + car_person_schema: SchemaBranch, +) -> None: + schema = registry.schema.get_schema_branch(name=default_branch.name) + prev_car_schema = schema.get(name="TestCar") + candidate_schema = schema.duplicate() + new_car_schema = candidate_schema.get(name="TestCar") + + # pick a support_profiles attribute that is non-optional in this fixture + # then toggle uniqueness membership and assert migration count stays 0 + migration = NodeUniquenessConstraintsUpdateMigration( + previous_node_schema=prev_car_schema, + new_node_schema=new_car_schema, + schema_path=_make_schema_path("TestCar"), + ) + execution_result = await migration.execute(migration_input=MigrationInput(db=db), branch=default_branch) + assert not execution_result.errors + assert execution_result.nbr_migrations_executed == 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/component/core/migrations/schema/test_node_uniqueness_constraints_update.py` around lines 104 - 267, Add a new test that mirrors the optional-attribute cases but uses a required support_profiles attribute and asserts no migrations run: duplicate the pattern in test_migration_no_change_when_optional_attr_not_affected, obtain prev_car_schema via registry.schema.get_schema_branch(...).get("TestCar"), create candidate_schema = schema.duplicate(), modify new_car_schema.uniqueness_constraints to add/remove "support_profiles__value" while ensuring support_profiles is non-optional on the schema, instantiate NodeUniquenessConstraintsUpdateMigration(previous_node_schema=prev_car_schema, new_node_schema=new_car_schema, schema_path=_make_schema_path("TestCar")), execute it with MigrationInput(db=db) and assert execution_result.nbr_migrations_executed == 0 and no errors, and finally call await assert_attribute_path_status(...) for the ProfileTestCar support_profiles path expecting it to remain "active".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/infrahub/core/relationship/constraints/profiles_removal.py`:
- Around line 39-43: The current conditional in profiles_removal.py is excluding
required attributes because it combines attr_schema.support_profiles, not
attr_schema.optional and not schema.check_attr_in_uniqueness_constraint(...) —
change the logic so the uniqueness-constraint check only causes exclusion for
optional attributes: only skip removal protection when
attr_schema.support_profiles is true AND attr_schema.optional is true AND
schema.check_attr_in_uniqueness_constraint(attr=attr_schema.name) is true;
update the conditional (referencing attr_schema.support_profiles,
attr_schema.optional and schema.check_attr_in_uniqueness_constraint) accordingly
so required (not optional) attributes remain protected even if they appear in
uniqueness constraints.
---
Nitpick comments:
In
`@backend/tests/component/core/migrations/schema/test_node_uniqueness_constraints_update.py`:
- Around line 104-267: Add a new test that mirrors the optional-attribute cases
but uses a required support_profiles attribute and asserts no migrations run:
duplicate the pattern in
test_migration_no_change_when_optional_attr_not_affected, obtain prev_car_schema
via registry.schema.get_schema_branch(...).get("TestCar"), create
candidate_schema = schema.duplicate(), modify
new_car_schema.uniqueness_constraints to add/remove "support_profiles__value"
while ensuring support_profiles is non-optional on the schema, instantiate
NodeUniquenessConstraintsUpdateMigration(previous_node_schema=prev_car_schema,
new_node_schema=new_car_schema, schema_path=_make_schema_path("TestCar")),
execute it with MigrationInput(db=db) and assert
execution_result.nbr_migrations_executed == 0 and no errors, and finally call
await assert_attribute_path_status(...) for the ProfileTestCar support_profiles
path expecting it to remain "active".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f32c8e6c-9f8e-4a86-8387-02bd7d101796
⛔ Files ignored due to path filters (1)
backend/infrahub/core/schema/generated/base_node_schema.pyis excluded by!**/generated/**
📒 Files selected for processing (8)
backend/infrahub/core/migrations/__init__.pybackend/infrahub/core/migrations/schema/node_uniqueness_constraints_update.pybackend/infrahub/core/relationship/constraints/profiles_removal.pybackend/infrahub/core/schema/definitions/internal.pybackend/infrahub/core/schema/schema_branch.pybackend/tests/component/core/migrations/schema/test_node_uniqueness_constraints_update.pychangelog/7644.added.mddocs/docs/reference/schema/validator-migration.mdx
✅ Files skipped from review due to trivial changes (3)
- changelog/7644.added.md
- docs/docs/reference/schema/validator-migration.mdx
- backend/infrahub/core/migrations/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/infrahub/core/schema/schema_branch.py
- backend/infrahub/core/migrations/schema/node_uniqueness_constraints_update.py
backend/infrahub/core/relationship/constraints/profiles_removal.py
Outdated
Show resolved
Hide resolved
ajtmccarty
left a comment
ajtmccarty
left a comment
There was a problem hiding this comment.
not sure if it is in scope for this change or if it should be tracked with another issue (if it doesn't already exist), but the docs say that the same constraint should apply to relationships used in uniqueness_constraints too.
backend/infrahub/core/migrations/schema/node_uniqueness_constraints_update.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/infrahub/core/migrations/schema/attribute_supports_generated_schema.py`:
- Around line 127-138: The previous profile-support check currently uses
self.new_schema; change it to use the pre-update schema snapshot
(previous_schema) when computing previous_profile_support_condition so it
reflects the old constraints: call
previous_schema.check_if_attr_supports_profiles(attribute_schema=self.previous_attribute_schema)
for previous_profile_support_condition and keep new_profile_support_condition
using
self.new_schema.check_if_attr_supports_profiles(attribute_schema=self.new_attribute_schema),
ensuring the if-block comparing previous_profile_support_condition !=
new_profile_support_condition correctly detects uniqueness_constraint changes.
In `@schema/openapi.json`:
- Line 2421: This change edits the generated OpenAPI artifact directly by
modifying the literal mapping "update": "migration_required" in
schema/openapi.json; instead update the original source schema (e.g., the API
model/definition that defines the "update" response or enum) where that value is
defined, then run the project's OpenAPI generation workflow (the generator
script/Makefile task used to produce schema/openapi.json) to regenerate the
JSON; revert any direct edits in schema/openapi.json and ensure the source
definition (the schema file or annotation that controls "update") is the one
updated so the generated artifact reflects the intended change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1954375c-becc-4478-9399-9008aee68dde
📒 Files selected for processing (6)
backend/infrahub/core/migrations/schema/attribute_supports_generated_schema.pybackend/infrahub/core/migrations/schema/node_attribute_add.pybackend/infrahub/core/relationship/constraints/profiles_removal.pybackend/infrahub/core/schema/basenode_schema.pybackend/infrahub/core/schema/schema_branch.pyschema/openapi.json
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/infrahub/core/schema/schema_branch.py
- backend/infrahub/core/relationship/constraints/profiles_removal.py
- backend/infrahub/core/schema/basenode_schema.py
backend/infrahub/core/migrations/schema/attribute_supports_generated_schema.py
Outdated
Show resolved
Hide resolved
| "title": "Uniqueness Constraints", | ||
| "description": "List of multi-element uniqueness constraints that can combine relationships and attributes", | ||
| "update": "validate_constraint" | ||
| "update": "migration_required" |
There was a problem hiding this comment.
Avoid direct edits in generated OpenAPI artifact.
These updates are in schema/openapi.json, which is generated output. Please apply the change in the source schema definitions and regenerate this file via the project’s generation workflow instead of editing it directly.
As per coding guidelines, "Do not edit schema/openapi.json - this is an auto-generated OpenAPI schema file".
Also applies to: 2707-2707, 3032-3032, 3303-3303, 4632-4632, 5401-5401
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schema/openapi.json` at line 2421, This change edits the generated OpenAPI
artifact directly by modifying the literal mapping "update":
"migration_required" in schema/openapi.json; instead update the original source
schema (e.g., the API model/definition that defines the "update" response or
enum) where that value is defined, then run the project's OpenAPI generation
workflow (the generator script/Makefile task used to produce
schema/openapi.json) to regenerate the JSON; revert any direct edits in
schema/openapi.json and ensure the source definition (the schema file or
annotation that controls "update") is the one updated so the generated artifact
reflects the intended change.
@ajtmccarty what docs? as the attributes of a relationship schema are already handled when checking the attributes |
…tributes-from-profile-schema-ifc-2048
backend/infrahub/core/migrations/schema/node_uniqueness_constraints_update.py
Show resolved
Hide resolved
| if not attr.support_profiles or not attr.optional: | ||
| continue | ||
|
|
||
| previous_in_constraint = self.previous_schema.check_attr_in_uniqueness_constraint(attr=attr.name) | ||
| new_in_constraint = self.new_schema.check_attr_in_uniqueness_constraint(attr=attr.name) |
There was a problem hiding this comment.
I think this should use check_if_attr_supports_profiles instead of breaking the logic apart
| else: | ||
| all_queries.append(ProfilesAttributeRemoveMigrationQuery) | ||
|
|
||
| # Check template support changes |
There was a problem hiding this comment.
I think we might need to do this same update for templates, b/c which attributes are included on a template schema are also affected by the uniqueness_constraints
this could be a separate follow-up issue. but the template problem should probably be fixed in the same release as this profile one
There was a problem hiding this comment.
@wvandeun please confirm the same behavior should be used for template
There was a problem hiding this comment.
If we have the same issue with templates, then yes!
…tributes-from-profile-schema-ifc-2048
ajtmccarty
left a comment
ajtmccarty
left a comment
There was a problem hiding this comment.
just some test comments
not sure what is going wrong with the component test failures
it would be a good to add an integration test that confirms the new uniqueness constraint migration is triggered and runs correctly on a schema update that includes a uniqueness constraint change
| """ | ||
|
|
||
|
|
||
| async def assert_attribute_path_status( |
There was a problem hiding this comment.
- the cypher query for this function will skip Nodes that do not have an attribute with the matching name and I think it would want to catch those. the fix is to start with a MATCH on the vertex label and then update the MATCHes in the subqueries to be OPTIONAL MATCH
- it should probably also include an
r1/2.status DESCin the ORDER BY clauses in the subqueries to make sure that active edges are preferred when edges are added and deleted at the same time - it would also be good to confirm that the
totime on the edge isnullto confirm that it is still active - looking at this some more, it looks like we do something pretty similar in some other tests. this PR has already getting kind of big, so maybe best to leave those other tests for a different PR
There was a problem hiding this comment.
thinking about this a third time, maybe it is all right as is if it is only checking that an attribute was added that did not exist before. if it is testing updates, then it is probably not sufficient, but if it is only testing create it is probably fine
| car_profile1_main: Node, | ||
| ) -> None: | ||
| """Adding nbr_seats to a uniqueness constraint removes it from profile nodes.""" | ||
| await assert_attribute_path_status( |
There was a problem hiding this comment.
I think a cleaner way to handle these assertions is to get the ProfileTestCar schema and verify that it does or does not have the nbr_seats attribute
then get any instances of the ProfileTestCar created for or during the test and check the same thing
There was a problem hiding this comment.
actually you might need to test it at the database level if you need to confirm that a new attribute was actually added with a NULL value b/c I think attribute loading will fill in missing attributes if they exist on the schema
he best way to confirm is probably to set an actual value on the new attribute on the template and verify that can be saved and retrieved
3 participants
Why
Optional attributes that participate in uniqueness constraints must not appear in profile schemas. When a profile sets such an attribute, every node using that profile would share the same value, violating the uniqueness constraint at scale.
Closes #7644 and #7646
What changed
uniqueness_constraintsentry are now excluded when generating a profile from a node schema.SchemaBranch.check_attr_in_uniqueness_constraintto check whether an attribute name appears in any uniqueness constraint path (handles both bare attribute names andattr__fieldpaths).generate_profile_from_nodenow skips an attribute if it is bothoptionaland in a uniqueness constraint.Suggested review order
SchemaBranch.check_attr_in_uniqueness_constraint— the new helperifcondition ingenerate_profile_from_nodetest_profile_does_not_contain_optional_unique_attributes— the regression testHow to review
Focus on the condition logic in
generate_profile_from_node: an attribute is skipped whenoptional=Trueand it appears in a uniqueness constraint. Non-optional attributes in constraints are intentionally unaffected.The constraint path matching handles both
attr__valuestyle paths and bareattrnames viastartswith(f"{attr}__") or constraint_path == attr.How to test
Expected: the
nameattribute is absent fromProfileNetworkRouterwhen it is optional and listed inuniqueness_constraints.Impact & rollout
Checklist
uv run towncrier create ...)Summary by CodeRabbit
New Features
Bug Fixes
Documentation