Merge pull request #4427 from opsmill/lgu-load-incorrect-order-by

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

backend/infrahub/core/constants/schema.py

@@ -27,13 +27,29 @@ class UpdateValidationErrorType(Enum):
 
 
 class SchemaElementPathType(Flag):
-    ATTR = auto()
-    REL_ONE_NO_ATTR = auto()
-    REL_ONE_ATTR = auto()
+    ATTR_WITH_PROP = auto()
+    ATTR_NO_PROP = auto()
+    ATTR = ATTR_WITH_PROP | ATTR_NO_PROP
+
     REL_MANY_NO_ATTR = auto()
     REL_MANY_ATTR = auto()
-    ALL_RELS = REL_ONE_NO_ATTR | REL_MANY_NO_ATTR | REL_ONE_ATTR | REL_MANY_ATTR
-    RELS_ATTR = REL_ONE_ATTR | REL_MANY_ATTR
-    RELS_NO_ATTR = REL_ONE_NO_ATTR | REL_MANY_NO_ATTR
-    REL_ONE = REL_ONE_NO_ATTR | REL_ONE_ATTR
     REL_MANY = REL_MANY_NO_ATTR | REL_MANY_ATTR
+
+    REL_ONE_MANDATORY_NO_ATTR = auto()
+    REL_ONE_MANDATORY_ATTR_WITH_PROP = auto()
+    REL_ONE_MANDATORY_ATTR_NO_PROP = auto()
+    REL_ONE_MANDATORY_ATTR = REL_ONE_MANDATORY_ATTR_WITH_PROP | REL_ONE_MANDATORY_ATTR_NO_PROP
+    REL_ONE_MANDATORY = REL_ONE_MANDATORY_ATTR | REL_ONE_MANDATORY_NO_ATTR
+    REL_ONE_OPTIONAL_ATTR_WITH_PROP = auto()
+    REL_ONE_OPTIONAL_ATTR_NO_PROP = auto()
+    REL_ONE_OPTIONAL_ATTR = REL_ONE_OPTIONAL_ATTR_WITH_PROP | REL_ONE_OPTIONAL_ATTR_NO_PROP
+    REL_ONE_OPTIONAL_NO_ATTR = auto()
+    REL_ONE_OPTIONAL = REL_ONE_OPTIONAL_ATTR | REL_ONE_OPTIONAL_NO_ATTR
+    REL_ONE = REL_ONE_MANDATORY | REL_ONE_OPTIONAL
+    REL_ONE_ATTR_WITH_PROP = REL_ONE_MANDATORY_ATTR_WITH_PROP | REL_ONE_OPTIONAL_ATTR_WITH_PROP
+    REL_ONE_ATTR = REL_ONE_MANDATORY_ATTR | REL_ONE_OPTIONAL_ATTR
+    REL_ONE_NO_ATTR = REL_ONE_MANDATORY_NO_ATTR | REL_ONE_OPTIONAL_NO_ATTR
+
+    REL = REL_ONE | REL_MANY
+    REL_ATTR = REL_ONE_MANDATORY_ATTR | REL_ONE_OPTIONAL_ATTR | REL_MANY_ATTR
+    RELS_NO_ATTR = REL_ONE_MANDATORY_NO_ATTR | REL_ONE_OPTIONAL_NO_ATTR | REL_MANY_NO_ATTR

backend/infrahub/core/schema_manager.py

@@ -595,31 +595,55 @@ def validate_schema_path(
         if not (SchemaElementPathType.ATTR & allowed_path_types) and schema_attribute_path.is_type_attribute:
             raise ValueError(f"{error_header}: this property only supports relationships not attributes")
 
-        if not (SchemaElementPathType.ALL_RELS & allowed_path_types) and schema_attribute_path.is_type_relationship:
+        if not (SchemaElementPathType.REL & allowed_path_types) and schema_attribute_path.is_type_relationship:
             raise ValueError(f"{error_header}: this property only supports attributes, not relationships")
 
-        if schema_attribute_path.is_type_relationship and schema_attribute_path.relationship_schema:
-            if (
-                schema_attribute_path.relationship_schema.cardinality == RelationshipCardinality.ONE
-                and not SchemaElementPathType.REL_ONE & allowed_path_types
-            ):
+        if not (SchemaElementPathType.ATTR_NO_PROP & allowed_path_types) and schema_attribute_path.is_type_attribute:
+            required_properties = tuple(
+                schema_attribute_path.attribute_schema.get_class().get_allowed_property_in_path()
+            )
+            if schema_attribute_path.attribute_property_name not in required_properties:
                 raise ValueError(
-                    f"{error_header}: cannot use {schema_attribute_path.relationship_schema.name} relationship,"
-                    " relationship must be of cardinality many"
+                    f"{error_header}: invalid attribute, it must end with one of the following properties:"
+                    f" {', '.join(required_properties)}. (`{path}`)"
                 )
+
+        if schema_attribute_path.is_type_relationship:
+            if schema_attribute_path.relationship_schema.cardinality == RelationshipCardinality.ONE:
+                if not SchemaElementPathType.REL_ONE & allowed_path_types:
+                    raise ValueError(
+                        f"{error_header}: cannot use {schema_attribute_path.relationship_schema.name} relationship,"
+                        f" relationship must be of cardinality many. (`{path}`)"
+                    )
+                if (
+                    not SchemaElementPathType.REL_ONE_OPTIONAL & allowed_path_types
+                    and schema_attribute_path.relationship_schema.optional
+                    and not (
+                        schema_attribute_path.relationship_schema.name == "ip_namespace"
+                        and isinstance(node_schema, NodeSchema)
+                        and (node_schema.is_ip_address() or node_schema.is_ip_prefix)
+                    )
+                ):
+                    raise ValueError(
+                        f"{error_header}: cannot use {schema_attribute_path.relationship_schema.name} relationship,"
+                        f" relationship must be mandatory. (`{path}`)"
+                    )
+
             if (
                 schema_attribute_path.relationship_schema.cardinality == RelationshipCardinality.MANY
                 and not SchemaElementPathType.REL_MANY & allowed_path_types
             ):
                 raise ValueError(
                     f"{error_header}: cannot use {schema_attribute_path.relationship_schema.name} relationship,"
-                    " relationship must be of cardinality one"
+                    f" relationship must be of cardinality one (`{path}`)"
                 )
 
-            if schema_attribute_path.has_property and not SchemaElementPathType.RELS_ATTR & allowed_path_types:
-                raise ValueError(f"{error_header}: cannot use attributes of related node, only the relationship")
+            if schema_attribute_path.has_property and not SchemaElementPathType.REL_ATTR & allowed_path_types:
+                raise ValueError(
+                    f"{error_header}: cannot use attributes of related node, only the relationship. (`{path}`)"
+                )
             if not schema_attribute_path.has_property and not SchemaElementPathType.RELS_NO_ATTR & allowed_path_types:
-                raise ValueError(f"{error_header}: Must use attributes of related node")
+                raise ValueError(f"{error_header}: Must use attributes of related node. (`{path}`)")
 
         return schema_attribute_path
 
@@ -669,34 +693,15 @@ def validate_uniqueness_constraints(self) -> None:
 
             for constraint_paths in node_schema.uniqueness_constraints:
                 for constraint_path in constraint_paths:
-                    schema_path = self.validate_schema_path(
+                    element_name = "uniqueness_constraints"
+                    self.validate_schema_path(
                         node_schema=node_schema,
                         path=constraint_path,
-                        allowed_path_types=SchemaElementPathType.ATTR | SchemaElementPathType.REL_ONE_NO_ATTR,
-                        element_name="uniqueness_constraints",
+                        allowed_path_types=SchemaElementPathType.ATTR_WITH_PROP
+                        | SchemaElementPathType.REL_ONE_MANDATORY_NO_ATTR,
+                        element_name=element_name,
                     )
 
-                    if schema_path.is_type_attribute:
-                        required_properties = tuple(
-                            schema_path.attribute_schema.get_class().get_allowed_property_in_path()
-                        )
-                        if schema_path.attribute_property_name not in required_properties:
-                            raise ValueError(
-                                f"{node_schema.kind} uniqueness contraint is invalid at attribute '{schema_path.attribute_schema.name}', it must "
-                                f"end with one of the following properties: {', '.join(required_properties)}"
-                            )
-
-                    if schema_path.is_type_relationship and schema_path.relationship_schema:
-                        if schema_path.relationship_schema.optional and not (
-                            schema_path.relationship_schema.name == "ip_namespace"
-                            and isinstance(node_schema, NodeSchema)
-                            and (node_schema.is_ip_address() or node_schema.is_ip_prefix)
-                        ):
-                            raise ValueError(
-                                f"Only mandatory relation of cardinality one can be used in uniqueness_constraints,"
-                                f" {schema_path.relationship_schema.name} is not mandatory. ({constraint_path})"
-                            )
-
     def validate_display_labels(self) -> None:
         for name in self.all_names:
             node_schema = self.get(name=name, duplicate=False)
@@ -727,13 +732,14 @@ def validate_order_by(self) -> None:
             if not node_schema.order_by:
                 continue
 
-            allowed_types = SchemaElementPathType.ATTR | SchemaElementPathType.REL_ONE
+            allowed_types = SchemaElementPathType.ATTR_WITH_PROP | SchemaElementPathType.REL_ONE_ATTR_WITH_PROP
             for order_by_path in node_schema.order_by:
+                element_name = "order_by"
                 self.validate_schema_path(
                     node_schema=node_schema,
                     path=order_by_path,
                     allowed_path_types=allowed_types,
-                    element_name="order_by",
+                    element_name=element_name,
                 )
 
     def validate_default_filters(self) -> None:
@@ -778,36 +784,18 @@ def validate_human_friendly_id(self) -> None:
             if not node_schema.human_friendly_id:
                 continue
 
-            allowed_types = SchemaElementPathType.ATTR | SchemaElementPathType.REL_ONE_ATTR
-            for item in node_schema.human_friendly_id:
+            allowed_types = SchemaElementPathType.ATTR_WITH_PROP | SchemaElementPathType.REL_ONE_MANDATORY_ATTR
+            for hfid_path in node_schema.human_friendly_id:
                 schema_path = self.validate_schema_path(
                     node_schema=node_schema,
-                    path=item,
+                    path=hfid_path,
                     allowed_path_types=allowed_types,
                     element_name="human_friendly_id",
                 )
 
                 if schema_path.is_type_attribute:
-                    required_properties = tuple(schema_path.attribute_schema.get_class().get_allowed_property_in_path())
-                    if schema_path.attribute_property_name not in required_properties:
-                        raise ValueError(
-                            f"{node_schema.kind} HFID is invalid at attribute '{schema_path.attribute_schema.name}', it must end with one of the "
-                            f"following properties: {', '.join(required_properties)}"
-                        )
                     hf_attr_names.add(schema_path.attribute_schema.name)
 
-                if schema_path.is_type_relationship and schema_path.relationship_schema:
-                    if schema_path.relationship_schema.optional and not (
-                        schema_path.relationship_schema.name == "ip_namespace"
-                        and isinstance(node_schema, NodeSchema)
-                        and (node_schema.is_ip_address() or node_schema.is_ip_prefix)
-                    ):
-                        raise ValueError(
-                            f"Only mandatory relationship of cardinality one can be used in human_friendly_id, "
-                            f"{schema_path.relationship_schema.name} is not mandatory on {schema_path.relationship_schema.kind} for "
-                            f"{node_schema.kind}. ({item})"
-                        )
-
     def validate_required_relationships(self) -> None:
         reverse_dependency_map: dict[str, set[str]] = {}
         for name in self.node_names + self.generic_names:

backend/tests/unit/core/schema_manager/test_manager_schema.py

@@ -1,4 +1,5 @@
 import copy
+import re
 import uuid
 
 import pytest
@@ -1155,11 +1156,17 @@ async def test_validate_exception_ipam_ip_namespace(
         ),
         (
             [["mybool__value", "status__name__value"]],
-            "InfraGenericInterface.uniqueness_constraints: cannot use attributes of related node, only the relationship",
+            "InfraGenericInterface.uniqueness_constraints: cannot use status relationship, relationship must be mandatory. (`status__name__value`)",
         ),
         (
             [["mybool", "status__name__value"]],
-            "InfraGenericInterface uniqueness contraint is invalid at attribute 'mybool', it must end with one of the following properties: value",
+            "InfraGenericInterface.uniqueness_constraints: invalid attribute, "
+            "it must end with one of the following properties: value. (`mybool`)",
+        ),
+        (
+            [["status__name"]],
+            "InfraGenericInterface.uniqueness_constraints: cannot use status relationship, "
+            "relationship must be mandatory. (`status__name`)",
         ),
     ],
 )
@@ -1170,7 +1177,7 @@ async def test_validate_uniqueness_constraints_error(schema_all_in_one, uniquene
     schema = SchemaBranch(cache={}, name="test")
     schema.load_schema(schema=SchemaRoot(**schema_all_in_one))
 
-    with pytest.raises(ValueError, match=expected_error):
+    with pytest.raises(ValueError, match=re.escape(expected_error)):
         schema.validate_uniqueness_constraints()
 
 
@@ -1263,6 +1270,11 @@ async def test_validate_order_by_success(schema_all_in_one, order_by):
             "InfraGenericInterface.order_by: cannot use badges relationship, relationship must be of cardinality one",
         ),
         (["status__name__nothing"], "InfraGenericInterface.order_by: nothing is not a valid property of name"),
+        (
+            ["my_generic_name"],
+            "InfraGenericInterface.order_by: invalid attribute, it must end "
+            "with one of the following properties: value. (`my_generic_name`)",
+        ),
     ],
 )
 async def test_validate_order_by_error(schema_all_in_one, order_by, expected_error):
@@ -1272,7 +1284,7 @@ async def test_validate_order_by_error(schema_all_in_one, order_by, expected_err
     schema = SchemaBranch(cache={}, name="test")
     schema.load_schema(schema=SchemaRoot(**schema_all_in_one))
 
-    with pytest.raises(ValueError, match=expected_error):
+    with pytest.raises(ValueError, match=re.escape(expected_error)):
         schema.validate_order_by()
 
 

backend/tests/unit/core/test_relationship.py

@@ -36,7 +36,7 @@ async def test_relationship_init(
     assert rel.node_id == person_jack_main.id
 
     rel_node = await rel.get_node(db=db)
-    assert type(rel_node) == Node
+    assert type(rel_node) is Node
     assert rel_node.id == person_jack_main.id
 
 

changelog/4323.fixed.md

@@ -0,0 +1 @@
+Loading a schema with an invalid order_by field raise a proper error