docs: add new comments and fix existing #265

datagateway_api/src/search_api/models.py

@@ -39,9 +39,9 @@ def from_icat(cls, icat_data, required_related_fields):  # noqa: B902, N805
 
         model_data = {}
         for field in model_fields:
-            # Some fields have aliases so we must use them when creating a model instance.
-            # If a field does not have an alias then the `alias` property holds the name
-            # of the field
+            # Some fields have aliases so we must use them when creating a model
+            # instance. If a field does not have an alias then the `alias` property
+            # holds the name of the field
             field_alias = cls.__fields__[field].alias
 
             panosc_entity_name, icat_field_name = mappings.get_icat_mapping(
@@ -58,16 +58,26 @@ def from_icat(cls, icat_data, required_related_fields):  # noqa: B902, N805
                     if field_value:
                         break
                 except KeyError:
+                    # If an icat value cannot be found for the ICAT field name in the
+                    # provided ICAT data then ignore the error. The field name could
+                    # simply be a mapping of an optional PaNOSC entity field so ICAT
+                    # may not return data for it which is fine. It could also be a list
+                    # of mappings which is the case with the `value` field of the
+                    # PaNOSC entity. When this is the case, ICAT only returns data for
+                    # one of the mappings from the list so we can ignore the error.
+                    # This also ignores errors for mandatory fields but this is not a
+                    # problem because pydantic is responsible for validating whether
+                    # data for mandatory fields is missing.
                     continue
 
             if not field_value:
                 continue
 
             if panosc_entity_name != cls.__name__:
-                # If we are here, it means that the field references another model so we
-                # have to get hold of its class definition and call its `from_icat` method
-                # to create an instance of itself with the ICAT data provided. Doing this
-                # allows for recursion.
+                # If we are here, it means that the field references another model so
+                # we have to get hold of its class definition and call its `from_icat`
+                # method to create an instance of itself with the ICAT data provided.
+                # Doing this allows for recursion.
                 data = field_value
                 if not isinstance(data, list):
                     data = [data]
@@ -113,9 +123,10 @@ def from_icat(cls, icat_data, required_related_fields):  # noqa: B902, N805
                 and required_related_field not in model_data
             ):
                 # If we are here, it means that a related entity, which has a minimum
-                # cardinality of one, has been specified to be included as part of the entity
-                # but the relevant ICAT data needed for its creation cannot be found in the
-                # provided ICAT response. Because of this, a ValidationError is raised.
+                # cardinality of one, has been specified to be included as part of the
+                # entity but the relevant ICAT data needed for its creation cannot be
+                # found in the provided ICAT response. Because of this, a
+                # `ValidationError` is raised.
                 error_wrapper = ErrorWrapper(
                     TypeError("field required"), loc=required_related_field,
                 )
@@ -292,16 +303,27 @@ class Parameter(PaNOSCAttribute):
     dataset: Optional[Dataset] = None
     document: Optional[Document] = None
 
-    # @root_validator(skip_on_failure=True)
-    # def validate_dataset_and_document(cls, values):  # noqa: B902, N805
-    #     if values["dataset"] is None and values["document"] is None:
-    #         raise TypeError("must have a dataset or document")
-
-    #     if values["dataset"] is not None and values["document"] is not None:
-    #         # TODO - Should an exception be raised here instead?
-    #         values["Document"] = None
-
-    #     return values
+    """
+    Validator commented as it was decided to be disabled for the time being. The Data
+    Model states that a Parameter must be related to a Dataset or Document, however
+    considering that there is not a Parameter endpoint, it means that a Parameter can
+    only be included via Dataset or Document. It's unclear why anyone would query for
+    a Dataset or Document that includes Parameters which in turn includes a Dataset or
+    Document that are the same as the top level ones. To avoid errors being raised
+    as a result of Parameters not containing ICAT data for a Dataset or Document, the
+    validator has been disabled.
+
+    @root_validator(skip_on_failure=True)
+    def validate_dataset_and_document(cls, values):  # noqa: B902, N805
+        if values["dataset"] is None and values["document"] is None:
+            raise TypeError("must have a dataset or document")
+
+        if values["dataset"] is not None and values["document"] is not None:
+            # TODO - Should an exception be raised here instead?
+            values["Document"] = None
+
+        return values
+    """
 
     @classmethod
     def from_icat(cls, icat_data, required_related_fields):