Repository: Materials-Consortia/optimade-python-tools

Issue:
Missing support for timestamps/datetime in grammar
The grammar is currently missing support for timestamps (or `datetime.datetime` in terms of Python types).

The relevant section in the spec may be found [here](https://github.com/Materials-Consortia/OPTiMaDe/blob/develop/optimade.rst#type-handling-and-conversions-in-comparisons).

Specifically we need to recognize timestamps as values being of the form specified by [RFC 3339 Internet Date/Time Format](https://tools.ietf.org/html/rfc3339#section-5.6) and then turn them into `datetime.datetime` objects if they are OK, otherwise, the implementation should return a `400 Bad Request`.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/590?src=pr&el=h1) Report
> Merging [#590](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/590?src=pr&el=desc) (7dd4647) into [master](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/commit/7e932df10c1159249b4096c255a284d5efa2ad4e?el=desc) (7e932df) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/590/graphs/tree.svg?width=650&height=150&src=pr&token=UJAtmqkZZO)](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/590?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
+ Coverage   92.11%   92.14%   +0.02%     
==========================================
  Files          61       61              
  Lines        3210     3222      +12     
==========================================
+ Hits         2957     2969      +12     
  Misses        253      253              
```

| Flag | Coverage Δ | |
|---|---|---|
| project | `92.14% <100.00%> (+0.02%)` | :arrow_up: |
| validator | `65.05% <66.66%> (+0.13%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/590?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [optimade/filtertransformers/mongo.py](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/590/diff?src=pr&el=tree#diff-b3B0aW1hZGUvZmlsdGVydHJhbnNmb3JtZXJzL21vbmdvLnB5) | `97.50% <100.00%> (+0.13%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/590?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/590?src=pr&el=footer). Last update [7e932df...7dd4647](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/590?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ml-evs: I think this was uncontroversial after our discussion on Slack a few weeks ago, so I'll merge these changes later today unless anyone stops me.
ml-evs: > I think this was uncontroversial after our discussion on Slack a few weeks ago, so I'll merge these changes later today unless anyone stops me.

Forgot about this PR, @shyamd would be good if you could have a look as this will affect MP

Patch:
diff --git a/optimade/filtertransformers/mongo.py b/optimade/filtertransformers/mongo.py
index 9b09844a..eb806e7a 100755
--- a/optimade/filtertransformers/mongo.py
+++ b/optimade/filtertransformers/mongo.py
@@ -1,7 +1,8 @@
 import copy
+import warnings
 from lark import v_args, Token
 from optimade.filtertransformers.base_transformer import BaseTransformer
-from optimade.server.exceptions import BadRequest
+from optimade.server.warnings import TimestampNotRFCCompliant
 
 __all__ = ("MongoTransformer",)
 
@@ -42,6 +43,7 @@ class MongoTransformer(BaseTransformer):
         query = self._apply_length_operators(query)
         query = self._apply_unknown_or_null_filter(query)
         query = self._apply_mongo_id_filter(query)
+        query = self._apply_mongo_date_filter(query)
 
         return query
 
@@ -395,8 +397,8 @@ class MongoTransformer(BaseTransformer):
                 if operator not in ("$eq", "$ne"):
                     if self.mapper is not None:
                         prop = self.mapper.alias_of(prop)
-                    raise BadRequest(
-                        detail=f"Operator not supported for query on field {prop!r}, can only test for equality"
+                    raise NotImplementedError(
+                        f"Operator {operator} not supported for query on field {prop!r}, can only test for equality"
                     )
                 if isinstance(val, str):
                     subdict[prop][operator] = ObjectId(val)
@@ -406,6 +408,44 @@ class MongoTransformer(BaseTransformer):
             filter_, check_for_id_key, replace_str_id_with_objectid
         )
 
+    def _apply_mongo_date_filter(self, filter_: dict) -> dict:
+        """This method loops through the query and replaces any operations
+        on suspected timestamp properties with the corresponding operation
+        on a BSON `DateTime` type.
+        """
+
+        def check_for_timestamp_field(prop, _):
+            """ Find cases where the query dict is operating on a timestamp field. """
+            if self.mapper is not None:
+                prop = self.mapper.alias_of(prop)
+            return prop == "last_modified"
+
+        def replace_str_date_with_datetime(subdict, prop, expr):
+            """Encode suspected dates in with BSON. """
+            import bson.json_util
+
+            for operator in subdict[prop]:
+                query_datetime = bson.json_util.loads(
+                    bson.json_util.dumps({"$date": subdict[prop][operator]}),
+                    json_options=bson.json_util.DEFAULT_JSON_OPTIONS.with_options(
+                        tz_aware=True, tzinfo=bson.tz_util.utc
+                    ),
+                )
+                if query_datetime.microsecond != 0:
+                    warnings.warn(
+                        f"Query for timestamp {subdict[prop][operator]!r} for field {prop!r} contained microseconds, which is not RFC3339 compliant. "
+                        "This may cause undefined behaviour for the underlying database.",
+                        TimestampNotRFCCompliant,
+                    )
+
+                subdict[prop][operator] = query_datetime
+
+            return subdict
+
+        return recursive_postprocessing(
+            filter_, check_for_timestamp_field, replace_str_date_with_datetime
+        )
+
 
 def recursive_postprocessing(filter_, condition, replacement):
     """Recursively descend into the query, checking each dictionary
diff --git a/optimade/models/jsonapi.py b/optimade/models/jsonapi.py
index 910fc8ed..7938dffb 100644
--- a/optimade/models/jsonapi.py
+++ b/optimade/models/jsonapi.py
@@ -352,6 +352,13 @@ class Response(BaseModel):
         return values
 
     class Config:
+        """The specification mandates that datetimes must be encoded following
+        [RFC3339](https://tools.ietf.org/html/rfc3339), which does not support
+        fractional seconds, thus they must be stripped in the response. This can
+        cause issues when the underlying database contains fields that do include
+        microseconds, as filters may return unexpected results.
+        """
+
         json_encoders = {
             datetime: lambda v: v.astimezone(timezone.utc).strftime(
                 "%Y-%m-%dT%H:%M:%SZ"
diff --git a/optimade/server/warnings.py b/optimade/server/warnings.py
index 7ce40c8b..4fb60658 100644
--- a/optimade/server/warnings.py
+++ b/optimade/server/warnings.py
@@ -42,3 +42,10 @@ class QueryParamNotUsed(OptimadeWarning):
 class MissingExpectedField(OptimadeWarning):
     """A field was provided with a null value when a related field was provided
     with a value."""
+
+
+class TimestampNotRFCCompliant(OptimadeWarning):
+    """A timestamp has been used in a filter that contains microseconds and is thus not
+    RFC 3339 compliant. This may cause undefined behaviour in the query results.
+
+    """
diff --git a/optimade/validator/config.py b/optimade/validator/config.py
index de626cfa..cf499a16 100644
--- a/optimade/validator/config.py
+++ b/optimade/validator/config.py
@@ -71,14 +71,11 @@ _INCLUSIVE_OPERATORS = {
         "ENDS",
     ),
     DataType.TIMESTAMP: (
-        "=",
-        "<=",
+        # "=" and "<=" are disabled due to issue with microseconds stored in database vs API response (see Materials-Consortia/optimade-python-tools/#606)
+        # ">=" is fine as all microsecond trimming will round times down
+        # "=",
+        # "<=",
         ">=",
-        "CONTAINS",
-        "STARTS WITH",
-        "STARTS",
-        "ENDS WITH",
-        "ENDS",
     ),
     DataType.INTEGER: (
         "=",
@@ -97,7 +94,7 @@ exclusive_ops = ("!=", "<", ">")
 
 _EXCLUSIVE_OPERATORS = {
     DataType.STRING: exclusive_ops,
-    DataType.TIMESTAMP: exclusive_ops,
+    DataType.TIMESTAMP: (),
     DataType.FLOAT: exclusive_ops,
     DataType.INTEGER: exclusive_ops,
     DataType.LIST: (),
diff --git a/optimade/validator/validator.py b/optimade/validator/validator.py
index 337137dd..959297e2 100644
--- a/optimade/validator/validator.py
+++ b/optimade/validator/validator.py
@@ -621,7 +621,7 @@ class ImplementationValidator:
                 else:
                     _test_value = test_value[0]
 
-        elif prop_type == DataType.STRING:
+        elif prop_type in (DataType.STRING, DataType.TIMESTAMP):
             _test_value = f'"{test_value}"'
 
         else:
@@ -700,7 +700,7 @@ class ImplementationValidator:
                 self._log.warning(msg)
                 return None, msg
 
-        if prop_type in (DataType.DICTIONARY, DataType.TIMESTAMP):
+        if prop_type in (DataType.DICTIONARY,):
             msg = f"Not testing queries on field {prop} of type {prop_type}."
             self._log.warning(msg)
             return None, msg
@@ -823,7 +823,7 @@ class ImplementationValidator:
                     chosen_entry["id"] in set(entry["id"] for entry in response["data"])
                 ):
                     raise ResponseError(
-                        f"Objects {excluded} were not necessarily excluded by {query}"
+                        f"Object {chosen_entry['id']} with value {test_value} was not excluded by {query}"
                     )
 
             # check that at least the archetypal structure was returned, unless we are using a fallback value


Test patch:
diff --git a/optimade/server/data/test_structures.json b/optimade/server/data/test_structures.json
index 6bd7d291..0c4981c4 100644
--- a/optimade/server/data/test_structures.json
+++ b/optimade/server/data/test_structures.json
@@ -3562,7 +3562,7 @@
     ],
     "formula_anonymous": "A26B8C2D2E2FGHI",
     "last_modified": {
-      "$date": "2019-06-08T05:13:37.945Z"
+      "$date": "2018-06-08T05:13:37.945Z"
     },
     "lattice_vectors": [
       [
diff --git a/tests/filtertransformers/test_mongo.py b/tests/filtertransformers/test_mongo.py
index 6bcc9892..0696d1d4 100644
--- a/tests/filtertransformers/test_mongo.py
+++ b/tests/filtertransformers/test_mongo.py
@@ -3,7 +3,6 @@ import pytest
 from lark.exceptions import VisitError
 
 from optimade.filterparser import LarkParser, ParserError
-from optimade.server.exceptions import BadRequest
 
 
 class TestMongoTransformer:
@@ -361,6 +360,54 @@ class TestMongoTransformer:
             parser.parse("cartesian_site_positions LENGTH >= 3")
         ) == {"nsites": {"$gte": 3}}
 
+    def test_suspected_timestamp_fields(self, mapper):
+        import datetime
+        import bson.tz_util
+        from optimade.filtertransformers.mongo import MongoTransformer
+        from optimade.server.warnings import TimestampNotRFCCompliant
+
+        example_RFC3339_date = "2019-06-08T04:13:37Z"
+        example_RFC3339_date_2 = "2019-06-08T04:13:37"
+        example_non_RFC3339_date = "2019-06-08T04:13:37.123Z"
+
+        expected_datetime = datetime.datetime(
+            year=2019,
+            month=6,
+            day=8,
+            hour=4,
+            minute=13,
+            second=37,
+            microsecond=0,
+            tzinfo=bson.tz_util.utc,
+        )
+
+        assert self.transform(f'last_modified > "{example_RFC3339_date}"') == {
+            "last_modified": {"$gt": expected_datetime}
+        }
+        assert self.transform(f'last_modified > "{example_RFC3339_date_2}"') == {
+            "last_modified": {"$gt": expected_datetime}
+        }
+
+        non_rfc_datetime = expected_datetime.replace(microsecond=123000)
+
+        with pytest.warns(TimestampNotRFCCompliant):
+            assert self.transform(f'last_modified > "{example_non_RFC3339_date}"') == {
+                "last_modified": {"$gt": non_rfc_datetime}
+            }
+
+        class MyMapper(mapper("StructureMapper")):
+            ALIASES = (("last_modified", "ctime"),)
+
+        transformer = MongoTransformer(mapper=MyMapper())
+        parser = LarkParser(version=self.version, variant=self.variant)
+
+        assert transformer.transform(
+            parser.parse(f'last_modified > "{example_RFC3339_date}"')
+        ) == {"ctime": {"$gt": expected_datetime}}
+        assert transformer.transform(
+            parser.parse(f'last_modified > "{example_RFC3339_date_2}"')
+        ) == {"ctime": {"$gt": expected_datetime}}
+
     def test_unaliased_length_operator(self):
         assert self.transform("cartesian_site_positions LENGTH <= 3") == {
             "cartesian_site_positions.4": {"$exists": False}
@@ -400,7 +447,7 @@ class TestMongoTransformer:
 
         for op in ("CONTAINS", "STARTS WITH", "ENDS WITH", "HAS"):
             with pytest.raises(
-                BadRequest,
+                NotImplementedError,
                 match=r".*not supported for query on field 'immutable_id', can only test for equality.*",
             ):
                 transformer.transform(parser.parse(f'immutable_id {op} "abcdef"'))
diff --git a/tests/server/query_params/test_filter.py b/tests/server/query_params/test_filter.py
index 2633d162..285145dc 100644
--- a/tests/server/query_params/test_filter.py
+++ b/tests/server/query_params/test_filter.py
@@ -1,4 +1,5 @@
 """Make sure response_fields is handled correctly"""
+import pytest
 
 
 def test_custom_field(check_response):
@@ -167,6 +168,30 @@ def test_list_correlated(check_error_response):
     # check_response(request, expected_ids)
 
 
+def test_timestamp_query(check_response):
+    from optimade.server.warnings import TimestampNotRFCCompliant
+
+    request = '/structures?filter=last_modified="2019-06-08T05:13:37.331Z"&page_limit=5'
+    expected_ids = ["mpf_1", "mpf_2", "mpf_3"]
+    with pytest.warns(TimestampNotRFCCompliant):
+        check_response(request, expected_ids, expected_as_is=True)
+
+    request = '/structures?filter=last_modified<"2019-06-08T05:13:37.331Z"&page_limit=5'
+    expected_ids = ["mpf_3819"]
+    with pytest.warns(TimestampNotRFCCompliant):
+        check_response(request, expected_ids, expected_as_is=True)
+
+    request = '/structures?filter=last_modified="2018-06-08T05:13:37.945Z"&page_limit=5'
+    expected_ids = ["mpf_3819"]
+    with pytest.warns(TimestampNotRFCCompliant):
+        check_response(request, expected_ids, expected_as_is=True)
+
+    request = '/structures?filter=last_modified>"2018-06-08T05:13:37.945Z" AND last_modified<="2019-06-08T05:13:37.331Z"&page_limit=5'
+    expected_ids = ["mpf_1", "mpf_2", "mpf_3"]
+    with pytest.warns(TimestampNotRFCCompliant):
+        check_response(request, expected_ids, expected_as_is=True)
+
+
 def test_is_known(check_response):
     request = "/structures?filter=nsites IS KNOWN AND nsites>=44"
     expected_ids = ["mpf_551", "mpf_3803", "mpf_3819"]