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

Issue:
Verify that missing values are not returned in comparisons
> (MongoDB's NOT will also return documents with missing values which is not allowed accoording the OPTMIADE standard. An alternative would be to add an extra expression to check wether a certain entry is present in a Document.

This is a good point that we may not be testing currently (I believe we only test "required" fields in our filter tests, which cannot be null/missing). As you say, a remedy may be to add an `{"$exists": True}` to every comparison we perform. I think I will raise another issue for this, as we should also test that the elasticsearch backend behaves appropriately.

_Originally posted by @ml-evs and @JPBergsma in https://github.com/Materials-Consortia/optimade-python-tools/issues/79#issuecomment-834327481_

Pull request:
Handling null fields in the filtertransformer and validator
Closes #792, as reported by @JPBergsma.

This PR:

- adds validation that no filters on a field returns entries with null values for that field. This can occur in MongoDB as null values match filters that test for inequality (i.e. `null != 1` evaluates to true).
- add additional `"$ne": null` expressions to all "$ne" filters in the `MongoTransformer`.
- add additional `"exists": True` expression to all "!=" filters in the `ElasticsearchTransformer`.
- add some null fields to our server test data so that we can verify this for other backends
- updates CODEOWNERS

Patch:
diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS
index 52864936..af9d90a4 100644
--- a/.github/CODEOWNERS
+++ b/.github/CODEOWNERS
@@ -1,3 +1,3 @@
-* @CasperWA @ml-evs
-*mongo* @ml-evs @shyamd
-*elastic* @ml-evs @markus1978
+* @CasperWA @ml-evs @JPBergsma
+*mongo* @CasperWA @JPBergsma @ml-evs @shyamd
+*elastic* @CasperWA @JPBergsma @ml-evs @markus1978
diff --git a/optimade/filtertransformers/elasticsearch.py b/optimade/filtertransformers/elasticsearch.py
index a38396c6..0314683c 100644
--- a/optimade/filtertransformers/elasticsearch.py
+++ b/optimade/filtertransformers/elasticsearch.py
@@ -109,9 +109,11 @@ class ElasticTransformer(BaseTransformer):
             return Q(query_type, **{field: value})
 
         if op == "!=":
-            return ~Q(  # pylint: disable=invalid-unary-operand-type
-                query_type, **{field: value}
-            )
+            # != queries must also include an existence check
+            # Note that for MongoDB, `$exists` will include null-valued fields,
+            # where as in ES `exists` excludes them.
+            # pylint: disable=invalid-unary-operand-type
+            return ~Q(query_type, **{field: value}) & Q("exists", field=field)
 
     def _has_query_op(self, quantities, op, predicate_zip_list):
         """
diff --git a/optimade/filtertransformers/mongo.py b/optimade/filtertransformers/mongo.py
index 53ba5f3a..b86addca 100755
--- a/optimade/filtertransformers/mongo.py
+++ b/optimade/filtertransformers/mongo.py
@@ -84,6 +84,12 @@ class MongoTransformer(BaseTransformer):
     def property_first_comparison(self, arg):
         # property_first_comparison: property ( value_op_rhs | known_op_rhs | fuzzy_string_op_rhs | set_op_rhs |
         # set_zip_op_rhs | length_op_rhs )
+
+        # Awkwardly, MongoDB will match null fields in $ne filters,
+        # so we need to add a check for null equality in evey $ne query.
+        if "$ne" in arg[1]:
+            return {"$and": [{arg[0]: arg[1]}, {arg[0]: {"$ne": None}}]}
+
         return {arg[0]: arg[1]}
 
     def constant_first_comparison(self, arg):
diff --git a/optimade/validator/validator.py b/optimade/validator/validator.py
index 84aa1b07..866061a6 100644
--- a/optimade/validator/validator.py
+++ b/optimade/validator/validator.py
@@ -827,6 +827,15 @@ class ImplementationValidator:
                             "returning different results each time."
                         )
 
+                # check that the filter returned no entries that had a null or missing value for the filtered property
+                if any(
+                    entry["attributes"].get(prop, entry.get(prop, None)) is None
+                    for entry in reversed_response.get("data", [])
+                ):
+                    raise ResponseError(
+                        f"Filter {reversed_query!r} on field {prop!r} returned entries that had null or missing values for the field."
+                    )
+
             excluded = operator in exclusive_operators
             # if we have all results on this page, check that the blessed ID is in the response
             if not response["meta"]["more_data_available"]:
@@ -848,6 +857,15 @@ class ImplementationValidator:
                         f"(with field '{prop} = {test_value}') potentially indicating a problem with filtering on this field."
                     )
 
+            # check that the filter returned no entries that had a null or missing value for the filtered property
+            if any(
+                entry["attributes"].get(prop, entry.get(prop, None)) is None
+                for entry in response.get("data", [])
+            ):
+                raise ResponseError(
+                    f"Filter {query!r} on field {prop!r} returned entries that had null or missing values for the field."
+                )
+
         if prop in CONF.unique_properties:
             if num_data_returned["="] is not None and num_data_returned["="] == 0:
                 raise ResponseError(


Test patch:
diff --git a/optimade/server/data/test_structures.json b/optimade/server/data/test_structures.json
index 0c4981c4..18a7eba0 100644
--- a/optimade/server/data/test_structures.json
+++ b/optimade/server/data/test_structures.json
@@ -3,6 +3,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700d02"
     },
+    "assemblies": null,
     "chemsys": "Ac",
     "cartesian_site_positions": [
       [
@@ -78,6 +79,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700d03"
     },
+    "assemblies": null,
     "chemsys": "Ac-Ag-Ir",
     "cartesian_site_positions": [
       [
@@ -193,6 +195,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700d04"
     },
+    "assemblies": null,
     "chemsys": "Ac-Ag-Pb",
     "cartesian_site_positions": [
       [
@@ -308,6 +311,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700d18"
     },
+    "assemblies": null,
     "chemsys": "Ac-Mg",
     "cartesian_site_positions": [
       [
@@ -396,6 +400,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700d1f"
     },
+    "assemblies": null,
     "chemsys": "Ac-O",
     "cartesian_site_positions": [
       [
@@ -496,6 +501,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700d6f"
     },
+    "assemblies": null,
     "chemsys": "Ac-Cu-F-O",
     "cartesian_site_positions": [
       [
@@ -618,6 +624,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700dc9"
     },
+    "assemblies": null,
     "chemsys": "Ag",
     "cartesian_site_positions": [
       [
@@ -683,6 +690,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700ddd"
     },
+    "assemblies": null,
     "chemsys": "Ag-Br-Cl-Te",
     "cartesian_site_positions": [
       [
@@ -871,6 +879,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700e04"
     },
+    "assemblies": null,
     "chemsys": "Ag-C-Cl-N-O-S",
     "cartesian_site_positions": [
       [
@@ -1045,6 +1054,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700e11"
     },
+    "assemblies": null,
     "chemsys": "Ag-C-Cl-H-N",
     "cartesian_site_positions": [
       [
@@ -1292,6 +1302,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700e15"
     },
+    "assemblies": null,
     "chemsys": "Ag-C-H-N-O",
     "cartesian_site_positions": [
       [
@@ -1545,6 +1556,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700e1a"
     },
+    "assemblies": null,
     "chemsys": "Ag-C-Cl-H-N-O-S",
     "cartesian_site_positions": [
       [
@@ -1808,6 +1820,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700ebf"
     },
+    "assemblies": null,
     "chemsys": "Ag-Br-Cl-Hg-I-S",
     "cartesian_site_positions": [
       [
@@ -2030,6 +2043,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700f28"
     },
+    "assemblies": null,
     "chemsys": "Ag-B-C-Cl-H-N-O-P",
     "cartesian_site_positions": [
       [
@@ -2610,6 +2624,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410700f79"
     },
+    "assemblies": null,
     "chemsys": "Ag-C-Cl-H-N-O-S",
     "cartesian_site_positions": [
       [
@@ -2909,6 +2924,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410701bdc"
     },
+    "assemblies": null,
     "chemsys": "Ba-Ce-Fe-H-Na-O-Si-Ti",
     "cartesian_site_positions": [
       [
@@ -3309,6 +3325,7 @@
     "_id": {
       "$oid": "5cfb441f053b174410701bec"
     },
+    "assemblies": null,
     "chemsys": "Ba-F-H-Mn-Na-O-Re-Si-Ti",
     "cartesian_site_positions": [
       [
diff --git a/tests/filtertransformers/test_mongo.py b/tests/filtertransformers/test_mongo.py
index cd1070c7..0f6de6ee 100644
--- a/tests/filtertransformers/test_mongo.py
+++ b/tests/filtertransformers/test_mongo.py
@@ -91,7 +91,9 @@ class TestMongoTransformer:
         assert self.transform("a>3") == {"a": {"$gt": 3}}
         assert self.transform("a>=3") == {"a": {"$gte": 3}}
         assert self.transform("a=3") == {"a": {"$eq": 3}}
-        assert self.transform("a!=3") == {"a": {"$ne": 3}}
+        assert self.transform("a!=3") == {
+            "$and": [{"a": {"$ne": 3}}, {"a": {"$ne": None}}]
+        }
 
     def test_id(self):
         assert self.transform('id="example/1"') == {"id": {"$eq": "example/1"}}
@@ -135,7 +137,12 @@ class TestMongoTransformer:
         ) == {
             "$and": [
                 {"chemical_formula_hill": {"$eq": "H2O"}},
-                {"chemical_formula_anonymous": {"$ne": "AB"}},
+                {
+                    "$and": [
+                        {"chemical_formula_anonymous": {"$ne": "AB"}},
+                        {"chemical_formula_anonymous": {"$ne": None}},
+                    ]
+                },
             ]
         }
         assert self.transform(
@@ -149,7 +156,12 @@ class TestMongoTransformer:
                         {"nelements": {"$gte": 10}},
                         {
                             "$nor": [
-                                {"_exmpl_x": {"$ne": "Some string"}},
+                                {
+                                    "$and": [
+                                        {"_exmpl_x": {"$ne": "Some string"}},
+                                        {"_exmpl_x": {"$ne": None}},
+                                    ]
+                                },
                                 {"_exmpl_a": {"$not": {"$eq": 7}}},
                             ]
                         },
@@ -443,7 +455,12 @@ class TestMongoTransformer:
 
         assert transformer.transform(
             parser.parse('immutable_id != "5cfb441f053b174410700d02"')
-        ) == {"_id": {"$ne": ObjectId("5cfb441f053b174410700d02")}}
+        ) == {
+            "$and": [
+                {"_id": {"$ne": ObjectId("5cfb441f053b174410700d02")}},
+                {"_id": {"$ne": None}},
+            ]
+        }
 
         for op in ("CONTAINS", "STARTS WITH", "ENDS WITH", "HAS"):
             with pytest.raises(
diff --git a/tests/server/routers/test_structures.py b/tests/server/routers/test_structures.py
index a1a601ac..b6258f84 100644
--- a/tests/server/routers/test_structures.py
+++ b/tests/server/routers/test_structures.py
@@ -154,3 +154,32 @@ class TestMultiStructureWithOverlappingRelationships(RegularEndpointTests):
         assert len(self.json_response["data"]) == 2
         assert "included" in self.json_response
         assert len(self.json_response["included"]) == 2
+
+
+class TestStructuresWithNullFieldsDoNotMatchNegatedFilters(RegularEndpointTests):
+    """Tests that structures with e.g., `'assemblies':null` do not get
+    returned for negated queries like `filter=assemblies != 1`, as mandated
+    by the specification.
+
+    """
+
+    request_str = "/structures?filter=assemblies != 1"
+    response_cls = StructureResponseMany
+
+    def test_structures_endpoint_data(self):
+        """Check that no structures are returned."""
+        assert len(self.json_response["data"]) == 0
+
+
+class TestStructuresWithNullFieldsMatchUnknownFilter(RegularEndpointTests):
+    """Tests that structures with e.g., `'assemblies':null` do get
+    returned for queries testing for "UNKNOWN" fields.
+
+    """
+
+    request_str = "/structures?filter=assemblies IS UNKNOWN"
+    response_cls = StructureResponseMany
+
+    def test_structures_endpoint_data(self):
+        """Check that all structures are returned."""
+        assert len(self.json_response["data"]) == 17