Repository: Materials-Consortia/optimade-python-tools

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_

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/796?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia) Report
> Merging [#796](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/796?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia) (77da01a) into [master](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/commit/37e3c9739faf17889eba1f79e5e97aa8cba97b60?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia) (37e3c97) will **decrease** coverage by `0.12%`.
> The diff coverage is `28.57%`.

[![Impacted file tree graph](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/796/graphs/tree.svg?width=650&height=150&src=pr&token=UJAtmqkZZO&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia)](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/796?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia)

```diff
@@            Coverage Diff             @@
##           master     #796      +/-   ##
==========================================
- Coverage   92.71%   92.59%   -0.13%     
==========================================
  Files          68       68              
  Lines        3666     3673       +7     
==========================================
+ Hits         3399     3401       +2     
- Misses        267      272       +5     
```

| Flag | Coverage Δ | |
|---|---|---|
| project | `92.59% <28.57%> (-0.13%)` | :arrow_down: |
| validator | `92.59% <28.57%> (-0.13%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/796?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia) | Coverage Δ | |
|---|---|---|
| [optimade/validator/validator.py](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia#diff-b3B0aW1hZGUvdmFsaWRhdG9yL3ZhbGlkYXRvci5weQ==) | `81.64% <28.57%> (-0.72%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/796?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/796?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia). Last update [37e3c97...77da01a](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/796?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia).

ml-evs: I'll wait for @JPBergsma to have a look at this before merging!
JPBergsma: In the file mongo.py line 91 it should be null instead of None.
The same goes for lines 95,143,162,461  in test_mongo.py
CasperWA: > I think Mongo DB uses null for missing values.
> In the file mongo.py line 91 it should be null instead of None.
> The same goes for lines 95,143,162,461 in test_mongo.py
> Other than that I did not spot anything wrong.

The Python special NoneType value `None` is parsed as `null` for the MongoDB backend when needed through pymongo. So I think this should be fine.

Essentially, MongoDB `null` is represented as `None` in Python.
JPBergsma: @CasperWA
Yes you are right.  Pymongo does the conversion from None to null for us.

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