Repository: Materials-Consortia/optimade-python-tools (python)

Issue:
Too strict validator for top-level fields
I think the root validator `either_data_meta_or_errors_must_be_set()` for `Response` is a bit too strict? While either of those MUST be set, they should be able to have a "null" value, either an empty dict, list or similar or `None`.
So basically the validator should be changed to just check for the existence of the top-level names as keys in the passed dict `values`, and not check their values - or did I misunderstand something here? Do they _need_ to have none-"null" content as well?

Pull request:
Relax response top-level root validator
Fixes #902.

Add test that checks a `Response` can be initialized with "null" values for the "required" top-level fields, while still raising a `ValidationError` if the "required" top-level fields are left out.

Note: See the issue to discuss whether this fix should be implemented or not - this PR is simply here to be able to fix the issue if we agree that it _is_ an issue and currently violates the specification.

Patch:
diff --git a/openapi/index_openapi.json b/openapi/index_openapi.json
index 50a372d8..d540e638 100644
--- a/openapi/index_openapi.json
+++ b/openapi/index_openapi.json
@@ -433,7 +433,7 @@
           "description": {
             "title": "Description",
             "type": "string",
-            "description": "OPTIONAL human-readable description of the relationship"
+            "description": "OPTIONAL human-readable description of the relationship."
           }
         },
         "description": "Specific meta field for base relationship resource"
@@ -684,7 +684,7 @@
                 "$ref": "#/components/schemas/ResponseMeta"
               }
             ],
-            "description": "A meta object containing non-standard information"
+            "description": "A meta object containing non-standard information."
           },
           "errors": {
             "title": "Errors",
@@ -958,7 +958,7 @@
                 "$ref": "#/components/schemas/IndexInfoResource"
               }
             ],
-            "description": "Index meta-database /info data"
+            "description": "Index meta-database /info data."
           },
           "meta": {
             "title": "Meta",
@@ -1248,7 +1248,7 @@
                 }
               }
             ],
-            "description": "List of unique OPTIMADE links resource objects"
+            "description": "List of unique OPTIMADE links resource objects."
           },
           "meta": {
             "title": "Meta",
@@ -1457,7 +1457,7 @@
             "description": "a meta object that contains non-standard meta-information about the relationship."
           }
         },
-        "description": "Similar to normal JSON API relationship, but with addition of OPTIONAL meta field for a resource"
+        "description": "Similar to normal JSON API relationship, but with addition of OPTIONAL meta field for a resource."
       },
       "RelatedLinksResource": {
         "title": "RelatedLinksResource",
@@ -1757,7 +1757,7 @@
             "description": "a meta object that contains non-standard meta-information about the relationship."
           }
         },
-        "description": "Similar to normal JSON API relationship, but with addition of OPTIONAL meta field for a resource"
+        "description": "Similar to normal JSON API relationship, but with addition of OPTIONAL meta field for a resource."
       },
       "ToplevelLinks": {
         "title": "ToplevelLinks",
diff --git a/openapi/openapi.json b/openapi/openapi.json
index 4984ea39..bcdf4aff 100644
--- a/openapi/openapi.json
+++ b/openapi/openapi.json
@@ -1528,7 +1528,7 @@
           "description": {
             "title": "Description",
             "type": "string",
-            "description": "OPTIONAL human-readable description of the relationship"
+            "description": "OPTIONAL human-readable description of the relationship."
           }
         },
         "description": "Specific meta field for base relationship resource"
@@ -1669,7 +1669,7 @@
                 "$ref": "#/components/schemas/EntryInfoResource"
               }
             ],
-            "description": "OPTIMADE information for an entry endpoint"
+            "description": "OPTIMADE information for an entry endpoint."
           },
           "meta": {
             "title": "Meta",
@@ -1935,7 +1935,7 @@
                 "$ref": "#/components/schemas/ResponseMeta"
               }
             ],
-            "description": "A meta object containing non-standard information"
+            "description": "A meta object containing non-standard information."
           },
           "errors": {
             "title": "Errors",
@@ -2095,7 +2095,7 @@
                 "$ref": "#/components/schemas/BaseInfoResource"
               }
             ],
-            "description": "The implementations /info data"
+            "description": "The implementations /info data."
           },
           "meta": {
             "title": "Meta",
@@ -2366,7 +2366,7 @@
                 }
               }
             ],
-            "description": "List of unique OPTIMADE links resource objects"
+            "description": "List of unique OPTIMADE links resource objects."
           },
           "meta": {
             "title": "Meta",
@@ -2609,7 +2609,7 @@
             "description": "a meta object that contains non-standard meta-information about the relationship."
           }
         },
-        "description": "Similar to normal JSON API relationship, but with addition of OPTIONAL meta field for a resource"
+        "description": "Similar to normal JSON API relationship, but with addition of OPTIONAL meta field for a resource."
       },
       "ReferenceResource": {
         "title": "ReferenceResource",
@@ -2849,7 +2849,7 @@
                 }
               }
             ],
-            "description": "List of unique OPTIMADE references entry resource objects"
+            "description": "List of unique OPTIMADE references entry resource objects."
           },
           "meta": {
             "title": "Meta",
@@ -2926,7 +2926,7 @@
                 "type": "object"
               }
             ],
-            "description": "A single references entry resource"
+            "description": "A single references entry resource."
           },
           "meta": {
             "title": "Meta",
@@ -3334,7 +3334,7 @@
             "description": "a meta object that contains non-standard meta-information about the relationship."
           }
         },
-        "description": "Similar to normal JSON API relationship, but with addition of OPTIONAL meta field for a resource"
+        "description": "Similar to normal JSON API relationship, but with addition of OPTIONAL meta field for a resource."
       },
       "StructureResource": {
         "title": "StructureResource",
@@ -3587,7 +3587,7 @@
                 }
               }
             ],
-            "description": "List of unique OPTIMADE structures entry resource objects"
+            "description": "List of unique OPTIMADE structures entry resource objects."
           },
           "meta": {
             "title": "Meta",
@@ -3664,7 +3664,7 @@
                 "type": "object"
               }
             ],
-            "description": "A single structures entry resource"
+            "description": "A single structures entry resource."
           },
           "meta": {
             "title": "Meta",
diff --git a/optimade/models/jsonapi.py b/optimade/models/jsonapi.py
index 7938dffb..f6724e01 100644
--- a/optimade/models/jsonapi.py
+++ b/optimade/models/jsonapi.py
@@ -345,10 +345,12 @@ class Response(BaseModel):
     @root_validator(pre=True)
     def either_data_meta_or_errors_must_be_set(cls, values):
         required_fields = ("data", "meta", "errors")
-        if not any(values.get(field) for field in required_fields):
+        if not any(field in values for field in required_fields):
             raise ValueError(
                 f"At least one of {required_fields} MUST be specified in the top-level response"
             )
+        if "errors" in values and not values.get("errors"):
+            raise ValueError("Errors MUST NOT be an empty or 'null' value.")
         return values
 
     class Config:
diff --git a/optimade/models/optimade_json.py b/optimade/models/optimade_json.py
index 26a73999..92757fc7 100644
--- a/optimade/models/optimade_json.py
+++ b/optimade/models/optimade_json.py
@@ -340,16 +340,16 @@ class Success(jsonapi.Response):
 
     @root_validator(pre=True)
     def either_data_meta_or_errors_must_be_set(cls, values):
-        """Overwriting the existing validation function, since 'errors' MUST NOT be set"""
+        """Overwriting the existing validation function, since 'errors' MUST NOT be set."""
         required_fields = ("data", "meta")
-        if not any(values.get(field) for field in required_fields):
+        if not any(field in values for field in required_fields):
             raise ValueError(
-                f"At least one of {required_fields} MUST be specified in the top-level response"
+                f"At least one of {required_fields} MUST be specified in the top-level response."
             )
 
         # errors MUST be skipped
-        if values.get("errors", None) is not None:
-            raise ValueError("'errors' MUST be skipped for a successful response")
+        if "errors" in values:
+            raise ValueError("'errors' MUST be skipped for a successful response.")
 
         return values
 
@@ -358,7 +358,7 @@ class BaseRelationshipMeta(jsonapi.Meta):
     """Specific meta field for base relationship resource"""
 
     description: str = StrictField(
-        ..., description="OPTIONAL human-readable description of the relationship"
+        ..., description="OPTIONAL human-readable description of the relationship."
     )
 
 
@@ -372,7 +372,7 @@ class BaseRelationshipResource(jsonapi.BaseResource):
 
 
 class Relationship(jsonapi.Relationship):
-    """Similar to normal JSON API relationship, but with addition of OPTIONAL meta field for a resource"""
+    """Similar to normal JSON API relationship, but with addition of OPTIONAL meta field for a resource."""
 
     data: Optional[
         Union[BaseRelationshipResource, List[BaseRelationshipResource]]
diff --git a/optimade/models/responses.py b/optimade/models/responses.py
index 6662fa26..c9959650 100644
--- a/optimade/models/responses.py
+++ b/optimade/models/responses.py
@@ -33,7 +33,7 @@ class ErrorResponse(Response):
     """errors MUST be present and data MUST be skipped"""
 
     meta: ResponseMeta = StrictField(
-        ..., description="A meta object containing non-standard information"
+        ..., description="A meta object containing non-standard information."
     )
     errors: List[OptimadeError] = StrictField(
         ...,
@@ -43,26 +43,26 @@ class ErrorResponse(Response):
 
     @root_validator(pre=True)
     def data_must_be_skipped(cls, values):
-        if values.get("data", None) is not None:
-            raise ValueError("data MUST be skipped for failures reporting errors")
+        if "data" in values:
+            raise ValueError("data MUST be skipped for failures reporting errors.")
         return values
 
 
 class IndexInfoResponse(Success):
     data: IndexInfoResource = StrictField(
-        ..., description="Index meta-database /info data"
+        ..., description="Index meta-database /info data."
     )
 
 
 class EntryInfoResponse(Success):
     data: EntryInfoResource = StrictField(
-        ..., description="OPTIMADE information for an entry endpoint"
+        ..., description="OPTIMADE information for an entry endpoint."
     )
 
 
 class InfoResponse(Success):
     data: BaseInfoResource = StrictField(
-        ..., description="The implementations /info data"
+        ..., description="The implementations /info data."
     )
 
 
@@ -85,34 +85,34 @@ class EntryResponseMany(Success):
 class LinksResponse(EntryResponseMany):
     data: Union[List[LinksResource], List[Dict[str, Any]]] = StrictField(
         ...,
-        description="List of unique OPTIMADE links resource objects",
+        description="List of unique OPTIMADE links resource objects.",
         uniqueItems=True,
     )
 
 
 class StructureResponseOne(EntryResponseOne):
     data: Union[StructureResource, Dict[str, Any], None] = StrictField(
-        ..., description="A single structures entry resource"
+        ..., description="A single structures entry resource."
     )
 
 
 class StructureResponseMany(EntryResponseMany):
     data: Union[List[StructureResource], List[Dict[str, Any]]] = StrictField(
         ...,
-        description="List of unique OPTIMADE structures entry resource objects",
+        description="List of unique OPTIMADE structures entry resource objects.",
         uniqueItems=True,
     )
 
 
 class ReferenceResponseOne(EntryResponseOne):
     data: Union[ReferenceResource, Dict[str, Any], None] = StrictField(
-        ..., description="A single references entry resource"
+        ..., description="A single references entry resource."
     )
 
 
 class ReferenceResponseMany(EntryResponseMany):
     data: Union[List[ReferenceResource], List[Dict[str, Any]]] = StrictField(
         ...,
-        description="List of unique OPTIMADE references entry resource objects",
+        description="List of unique OPTIMADE references entry resource objects.",
         uniqueItems=True,
     )


Test patch:
diff --git a/tests/models/test_jsonapi.py b/tests/models/test_jsonapi.py
index 3d684666..9cb66d2e 100644
--- a/tests/models/test_jsonapi.py
+++ b/tests/models/test_jsonapi.py
@@ -1,9 +1,12 @@
 from pydantic import ValidationError
 import pytest
-from optimade.models.jsonapi import Error, ToplevelLinks
 
 
 def test_hashability():
+    """Check a list of errors can be converted to a set,
+    i.e., check that Errors can be hashed."""
+    from optimade.models.jsonapi import Error
+
     error = Error(id="test")
     assert set([error])
 
@@ -14,6 +17,8 @@ def test_toplevel_links():
     can be validated as a URL or a Links object too.
 
     """
+    from optimade.models.jsonapi import ToplevelLinks
+
     test_links = {
         "first": {"href": "http://example.org/structures?page_limit=3&page_offset=0"},
         "last": {"href": "http://example.org/structures?page_limit=3&page_offset=10"},
@@ -55,3 +60,22 @@ def test_toplevel_links():
 
     with pytest.raises(ValidationError):
         ToplevelLinks(**{"base_url": {"href": "not a link"}})
+
+
+def test_response_top_level():
+    """Ensure a response with "null" values can be created."""
+    from optimade.models.jsonapi import Response
+
+    assert isinstance(Response(data=[]), Response)
+    assert isinstance(Response(data=None), Response)
+    assert isinstance(Response(meta={}), Response)
+    assert isinstance(Response(meta=None), Response)
+
+    # "errors" MUST NOT be an empty or `null` value if given.
+    with pytest.raises(ValidationError, match=r"Errors MUST NOT be an empty.*"):
+        assert isinstance(Response(errors=[]), Response)
+    with pytest.raises(ValidationError, match=r"Errors MUST NOT be an empty.*"):
+        assert isinstance(Response(errors=None), Response)
+
+    with pytest.raises(ValidationError, match=r"At least one of .*"):
+        Response(links={})