From 93de4c0c21d2acb511928f2c460654871aa9e99f Mon Sep 17 00:00:00 2001 From: poocog Date: Mon, 3 Aug 2026 11:46:37 +0200 Subject: [PATCH 1/2] Add validation for reverse connections pointing through reverse properties MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Detect when a reverse direct relation's through property is itself a reverse relation, including cyclic A↔B cases, with a clear error explaining the mapping never resolves to a container. De-duplicate REVERSE-003 for this case and add snapshot tests. Co-authored-by: Cursor --- .../neat/_data_model/rules/dms/__init__.py | 2 + .../_data_model/rules/dms/_connections.py | 118 +++++++++++++++++- docs/validation/index.md | 3 +- .../neat-dms-connections-reverse-010.md | 16 +++ .../containers.yaml | 18 +++ .../data_model.yaml | 8 ++ .../bi_directional_connections/views.yaml | 58 +++++++++ ...test_bidirectional_connection_validator.py | 39 ++++++ 8 files changed, 260 insertions(+), 2 deletions(-) create mode 100644 docs/validation/neat-dms-connections-reverse-010.md diff --git a/cognite/neat/_data_model/rules/dms/__init__.py b/cognite/neat/_data_model/rules/dms/__init__.py index 18fe5f670..a16f582fe 100644 --- a/cognite/neat/_data_model/rules/dms/__init__.py +++ b/cognite/neat/_data_model/rules/dms/__init__.py @@ -21,6 +21,7 @@ ReverseConnectionSourceViewMissing, ReverseConnectionTargetMismatch, ReverseConnectionTargetMissing, + ReverseConnectionThroughPropertyIsReverse, ) from ._consistency import ViewSpaceVersionInconsistentWithDataModel from ._containers import ( @@ -76,6 +77,7 @@ "ReverseConnectionSourceViewMissing", "ReverseConnectionTargetMismatch", "ReverseConnectionTargetMissing", + "ReverseConnectionThroughPropertyIsReverse", "SuboptimalRequiresConstraint", "UnresolvableQueryPerformance", "ViewContainerCountIsOutOfLimits", diff --git a/cognite/neat/_data_model/rules/dms/_connections.py b/cognite/neat/_data_model/rules/dms/_connections.py index 5a79137e8..ed3820595 100644 --- a/cognite/neat/_data_model/rules/dms/_connections.py +++ b/cognite/neat/_data_model/rules/dms/_connections.py @@ -7,7 +7,10 @@ ViewDirectReference, ViewReference, ) -from cognite.neat._data_model.models.dms._view_property import ViewCorePropertyRequest +from cognite.neat._data_model.models.dms._view_property import ( + ReverseDirectRelationProperty, + ViewCorePropertyRequest, +) from cognite.neat._data_model.rules.dms._base import DataModelRule from cognite.neat._issues import ConsistencyError, Recommendation @@ -254,6 +257,9 @@ def validate(self) -> list[ConsistencyError]: source_property = source_view_expanded.properties[through.identifier] + if isinstance(source_property, ReverseDirectRelationProperty): + continue # Handled by ReverseConnectionThroughPropertyIsReverse + if not isinstance(source_property, ViewCorePropertyRequest): errors.append( ConsistencyError( @@ -538,3 +544,113 @@ def validate(self) -> list[Recommendation]: ) return recommendations + + +class ReverseConnectionThroughPropertyIsReverse(DataModelRule): + """Validates that reverse connections do not point through other reverse direct relations. + + ## What it does + Checks that the property referenced in a reverse connection's 'through' clause + is a direct relation property, not another reverse direct relation property. + + ## Why is this bad? + Reverse direct relation properties do not map to a container; they rely on the + direct relation property they reverse. Pointing two reverse properties at each other + creates a cycle that never resolves to container storage, even when the containers + have correct direct relation properties defined. + + ## Example + If ViewA has reverse property `items` through ViewB's reverse property `owners`, + and ViewB has reverse property `owners` through ViewA's reverse property `items`, + neither reverse connection maps down to a container direct relation. + """ + + code = f"{BASE_CODE}-REVERSE-010" + issue_type = ConsistencyError + + def validate(self) -> list[ConsistencyError]: + errors: list[ConsistencyError] = [] + + for (target_view_ref, reverse_prop_name), ( + source_view_ref, + through, + ) in self.validation_resources.reverse_to_direct_mapping.items(): + through = self.validation_resources.normalize_through_reference(source_view_ref, through) + source_view = self.validation_resources.select_view(source_view_ref, through.identifier) + + if not source_view: + continue # Handled by ReverseConnectionSourceViewMissing + + if not (source_view_expanded := self.validation_resources.expand_view_properties(source_view_ref)): + raise RuntimeError(f"{type(self).__name__}: View {source_view_ref!s} not found. This is a bug in NEAT.") + + if not source_view_expanded.properties or through.identifier not in source_view_expanded.properties: + continue # Handled by ReverseConnectionSourcePropertyMissing + + source_property = source_view_expanded.properties[through.identifier] + + if not isinstance(source_property, ReverseDirectRelationProperty): + continue + + cycle_path = self._find_reverse_cycle_path(target_view_ref, reverse_prop_name) + cycle_suffix = "" + if cycle_path: + path_str = " -> ".join(f"{view!s}.{prop}" for view, prop in cycle_path) + cycle_suffix = f"This forms a cycle of reverse connections: {path_str}." + + errors.append( + ConsistencyError( + message=( + f"Reverse connection '{reverse_prop_name}' in view {target_view_ref!s} " + f"points through '{through.identifier}' in view {source_view_ref!s}, " + f"but '{through.identifier}' is itself a reverse direct relation " + f"(not a direct relation that maps to a container). " + f"Reverse connections must reference a direct relation property. {cycle_suffix}" + ), + fix=( + "Update the reverse connection to point through the corresponding " + "direct relation property on the source view (not another reverse property)." + ), + code=self.code, + ) + ) + + return errors + + def _find_reverse_cycle_path( + self, + start_target_view: ViewReference, + start_reverse_prop: str, + ) -> list[tuple[ViewReference, str]] | None: + """Return the cycle path if reverse-through-reverse forms a cycle, else None.""" + seen: list[tuple[ViewReference, str]] = [] + current = (start_target_view, start_reverse_prop) + + while True: + if current in seen: + return seen + [current] + + mapping = self.validation_resources.reverse_to_direct_mapping.get(current) + if not mapping: + return None + + source_view_ref, through = mapping + through = self.validation_resources.normalize_through_reference(source_view_ref, through) + + source_view_expanded = self.validation_resources.expand_view_properties(source_view_ref) + if not source_view_expanded or not source_view_expanded.properties: + return None + + if through.identifier not in source_view_expanded.properties: + return None + + through_property = source_view_expanded.properties[through.identifier] + + if isinstance(through_property, ViewCorePropertyRequest): + return None + + if not isinstance(through_property, ReverseDirectRelationProperty): + return None + + seen.append(current) + current = (source_view_ref, through.identifier) diff --git a/docs/validation/index.md b/docs/validation/index.md index c9dec7bb1..00d457561 100644 --- a/docs/validation/index.md +++ b/docs/validation/index.md @@ -1,4 +1,4 @@ -**Neat supports 38 validation rules** for data modeling. These rules are learned +**Neat supports 39 validation rules** for data modeling. These rules are learned from best practice, knowledge of the Cognite Data Fusion data modeling service, and practical experience from helping customers build and maintain their data models. @@ -36,6 +36,7 @@ Validators for connections in data model specifications. | NEAT-DMS-CONNECTIONS-REVERSE-007 | [ReverseConnectionTargetMissing](neat-dms-connections-reverse-007.md) | Validates that the direct connection in reverse connection pair have target views specified. | | NEAT-DMS-CONNECTIONS-REVERSE-008 | [ReverseConnectionPointsToAncestor](neat-dms-connections-reverse-008.md) | Validates that direct connections point to specific views rather than ancestors. | | NEAT-DMS-CONNECTIONS-REVERSE-009 | [ReverseConnectionTargetMismatch](neat-dms-connections-reverse-009.md) | Validates that direct connections point to the correct target views. | +| NEAT-DMS-CONNECTIONS-REVERSE-010 | [ReverseConnectionThroughPropertyIsReverse](neat-dms-connections-reverse-010.md) | Validates that reverse connections do not point through other reverse direct relations. | ### Consistency (NEAT-DMS-CONSISTENCY) diff --git a/docs/validation/neat-dms-connections-reverse-010.md b/docs/validation/neat-dms-connections-reverse-010.md new file mode 100644 index 000000000..9597b4058 --- /dev/null +++ b/docs/validation/neat-dms-connections-reverse-010.md @@ -0,0 +1,16 @@ +Validates that reverse connections do not point through other reverse direct relations. + +## What it does +Checks that the property referenced in a reverse connection's 'through' clause +is a direct relation property, not another reverse direct relation property. + +## Why is this bad? +Reverse direct relation properties do not map to a container; they rely on the +direct relation property they reverse. Pointing two reverse properties at each other +creates a cycle that never resolves to container storage, even when the containers +have correct direct relation properties defined. + +## Example +If ViewA has reverse property `items` through ViewB's reverse property `owners`, +and ViewB has reverse property `owners` through ViewA's reverse property `items`, +neither reverse connection maps down to a container direct relation. \ No newline at end of file diff --git a/tests/data/snapshots/local/bi_directional_connections/containers.yaml b/tests/data/snapshots/local/bi_directional_connections/containers.yaml index 16f60fea4..a2764e935 100644 --- a/tests/data/snapshots/local/bi_directional_connections/containers.yaml +++ b/tests/data/snapshots/local/bi_directional_connections/containers.yaml @@ -95,3 +95,21 @@ type: type: direct list: true +- space: my_space + externalId: CyclicContainerA + usedFor: node + properties: + cyclicDirectAStorage: + nullable: true + type: + type: direct + list: false +- space: my_space + externalId: CyclicContainerB + usedFor: node + properties: + cyclicDirectBStorage: + nullable: true + type: + type: direct + list: false diff --git a/tests/data/snapshots/local/bi_directional_connections/data_model.yaml b/tests/data/snapshots/local/bi_directional_connections/data_model.yaml index 0f75dfda8..6b61b48a3 100644 --- a/tests/data/snapshots/local/bi_directional_connections/data_model.yaml +++ b/tests/data/snapshots/local/bi_directional_connections/data_model.yaml @@ -50,3 +50,11 @@ views: externalId: ReverseToListDirectView version: v1 type: view + - space: my_space + externalId: CyclicViewA + version: v1 + type: view + - space: my_space + externalId: CyclicViewB + version: v1 + type: view diff --git a/tests/data/snapshots/local/bi_directional_connections/views.yaml b/tests/data/snapshots/local/bi_directional_connections/views.yaml index 60229fc60..a1f230b74 100644 --- a/tests/data/snapshots/local/bi_directional_connections/views.yaml +++ b/tests/data/snapshots/local/bi_directional_connections/views.yaml @@ -378,3 +378,61 @@ version: v1 type: view identifier: listDirectRelationTarget +- space: my_space + externalId: CyclicViewA + version: v1 + properties: + cyclicDirectA: + container: + space: my_space + externalId: CyclicContainerA + type: container + containerPropertyIdentifier: cyclicDirectAStorage + source: + space: my_space + externalId: CyclicViewB + version: v1 + type: view + cyclicReverseA: + connectionType: single_reverse_direct_relation + source: + space: my_space + externalId: CyclicViewB + version: v1 + type: view + through: + source: + space: my_space + externalId: CyclicViewB + version: v1 + type: view + identifier: cyclicReverseB +- space: my_space + externalId: CyclicViewB + version: v1 + properties: + cyclicDirectB: + container: + space: my_space + externalId: CyclicContainerB + type: container + containerPropertyIdentifier: cyclicDirectBStorage + source: + space: my_space + externalId: CyclicViewA + version: v1 + type: view + cyclicReverseB: + connectionType: single_reverse_direct_relation + source: + space: my_space + externalId: CyclicViewA + version: v1 + type: view + through: + source: + space: my_space + externalId: CyclicViewA + version: v1 + type: view + identifier: cyclicReverseA diff --git a/tests/tests_unit/test_data_model/test_rules/dms/test_bidirectional_connection_validator.py b/tests/tests_unit/test_data_model/test_rules/dms/test_bidirectional_connection_validator.py index 32a5940a9..1eef89bbb 100644 --- a/tests/tests_unit/test_data_model/test_rules/dms/test_bidirectional_connection_validator.py +++ b/tests/tests_unit/test_data_model/test_rules/dms/test_bidirectional_connection_validator.py @@ -17,6 +17,7 @@ ReverseConnectionSourceViewMissing, ReverseConnectionTargetMismatch, ReverseConnectionTargetMissing, + ReverseConnectionThroughPropertyIsReverse, ) from tests.data import SNAPSHOT_CATALOG @@ -34,6 +35,7 @@ ReverseConnectionTargetMissing: {"reverseToAttribute", "reverseToDirectWithoutTyping"}, ReverseConnectionPointsToAncestor: {"innerReflection"}, ReverseConnectionTargetMismatch: {"reverseSourceToTargetViewConnection"}, + ReverseConnectionThroughPropertyIsReverse: {"cyclicReverseA", "cyclicReverseB"}, } @@ -101,3 +103,40 @@ def test_validation_deep( break assert found_problematic_reversals == actual_problematic_reversal + + +@pytest.mark.parametrize("profile", ["deep-additive", "legacy-additive"]) +def test_cyclic_reverse_relation_validator_message( + profile: Literal["deep-additive", "legacy-additive"], +) -> None: + config = internal_profiles()[profile] + mode = config.modeling.mode + can_run_validator = config.validation.can_run_validator + + local_snapshot, cdf_snapshot = SNAPSHOT_CATALOG.load_scenario( + "bi_directional_connections", "for_validators", modus_operandi=mode, include_cdm=False, format="snapshots" + ) + data_model = SNAPSHOT_CATALOG.snapshot_to_request_schema(local_snapshot) + + orchestrator = DmsDataModelRulesOrchestrator( + cdf_snapshot=cdf_snapshot, + limits=SchemaLimits(), + modus_operandi=mode, + can_run_validator=can_run_validator, + ) + orchestrator.run(data_model) + by_code = orchestrator.issues.by_code() + + if can_run_validator(ReverseConnectionThroughPropertyIsReverse.code, ReverseConnectionThroughPropertyIsReverse.issue_type): + cyclic_issues = by_code[ReverseConnectionThroughPropertyIsReverse.code] + cyclic_messages = [issue.message for issue in cyclic_issues if "cyclicReverse" in issue.message] + + assert len(cyclic_messages) == 2 + for message in cyclic_messages: + assert "reverse direct relation" in message + assert "cycle of reverse connections" in message + assert "cyclicReverseA" in message or "cyclicReverseB" in message + + wrong_type_issues = by_code.get(ReverseConnectionSourcePropertyWrongType.code, []) + wrong_type_cyclic = [issue for issue in wrong_type_issues if "cyclicReverse" in issue.message] + assert wrong_type_cyclic == [] From 40621066040096b56885046d60b02e9fce9637ed Mon Sep 17 00:00:00 2001 From: poocog <73483823+poocog@users.noreply.github.com> Date: Mon, 3 Aug 2026 10:00:40 +0000 Subject: [PATCH 2/2] Linting and static code checks --- .../test_rules/dms/test_bidirectional_connection_validator.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/tests_unit/test_data_model/test_rules/dms/test_bidirectional_connection_validator.py b/tests/tests_unit/test_data_model/test_rules/dms/test_bidirectional_connection_validator.py index 1eef89bbb..78f31ab44 100644 --- a/tests/tests_unit/test_data_model/test_rules/dms/test_bidirectional_connection_validator.py +++ b/tests/tests_unit/test_data_model/test_rules/dms/test_bidirectional_connection_validator.py @@ -127,7 +127,9 @@ def test_cyclic_reverse_relation_validator_message( orchestrator.run(data_model) by_code = orchestrator.issues.by_code() - if can_run_validator(ReverseConnectionThroughPropertyIsReverse.code, ReverseConnectionThroughPropertyIsReverse.issue_type): + if can_run_validator( + ReverseConnectionThroughPropertyIsReverse.code, ReverseConnectionThroughPropertyIsReverse.issue_type + ): cyclic_issues = by_code[ReverseConnectionThroughPropertyIsReverse.code] cyclic_messages = [issue.message for issue in cyclic_issues if "cyclicReverse" in issue.message]