Allow microseconds in datetimes and allow identity-function parsing. (#991)

* Allow microseconds in datetimes, allow identity-function parsing.

* Touch up expected schema in test data.

* Switch from arrow to ciso8601 for date/datetime parsing.

* Allow dates to be passed in place of datetimes, since the conversion is widening.

* Delint.

* Update scalar type docstrings in test schema.

* Remove pytz imports.

* Resolve lint issues.

* Tighten mypy.ini file.

* Explicitly mention date/datetime format is ciso8601 in error messages.

Author: obi1kenobi

Pipfile

@@ -14,7 +14,7 @@ redis = ">=3.2.1,<4"
 redisgraph = ">=1.7,<1.9"
 
 # Linters and other development tools
-bandit = ">=1.5.1,<2"
+bandit = ">=1.5.1,<1.7"  # 1.7 has a blocking regression: https://github.com/PyCQA/bandit/issues/658
 black = "==20.8b1"  # This is still marked as a beta release, pin it explicitly: https://github.com/pypa/pipenv/issues/1760
 codecov = ">=2.0.15,<3"
 flake8 = ">=3.6.0,<4"
@@ -39,10 +39,9 @@ sphinx-rtd-theme = ">=0.4.3,<1"
 sphinx = ">=1.8,<2"
 
 [packages]  # Make sure to keep in sync with setup.py requirements.
-arrow = ">=0.15.0,<1"
+ciso8601 = ">=2.1.3,<3"
 funcy = ">=1.7.3,<2"
 graphql-core = ">=3.1.2,<3.2"  # minor versions sometimes contain breaking changes
-pytz = ">=2017.2"
 six = ">=1.10.0"
 sqlalchemy = ">=1.3.0,<2"
 

Pipfile.lock

@@ -1,5 +1,6 @@
 # Copyright 2020-present Kensho Technologies, LLC.
 """Convert values to their underlying GraphQLType."""
+from datetime import date, datetime
 from types import MappingProxyType
 from typing import Any, Callable, Mapping, Tuple, Type
 
@@ -21,8 +22,8 @@
 
 _ALLOWED_SCALAR_TYPES: Mapping[str, Tuple[Type, ...]] = MappingProxyType(
     {
-        GraphQLDate.name: (str,),
-        GraphQLDateTime.name: (str,),
+        GraphQLDate.name: (str, date),
+        GraphQLDateTime.name: (str, date, datetime),
         GraphQLFloat.name: (str, float, int),
         GraphQLDecimal.name: (str, float, int),
         GraphQLInt.name: (int, str),
@@ -124,6 +125,18 @@ def deserialize_scalar_value(expected_type: GraphQLScalarType, value: Any) -> An
             f"Cannot deserialize boolean value {value} to non-GraphQLBoolean type {expected_type}."
         )
 
+    # Explicitly disallow passing datetime objects as date objects.
+    # In Python, datetime subclasses date and therefore datetimes are treated as dates implicitly
+    # by truncating the time and timezone components. However, this is a loss of precision,
+    # and implicitly losing precision like this is undesirable for our purposes.
+    if isinstance(value, datetime) and is_same_type(GraphQLDate, expected_type):
+        raise ValueError(
+            f"Cannot use the datetime object {value} as a GraphQL Date value. While Python "
+            f"datetimes are subclasses of date, the default behavior of simply truncating the time "
+            f"and time zone data is undesirable as an implicit default. Please instead use "
+            f"a date object or a string representing a date in ISO-8601 'YYYY-MM-DD' format."
+        )
+
     # Ensure value has an appropriate type and deserialize the value.
     expected_python_types, deserialization_function = types_and_deserialization
     if not isinstance(value, expected_python_types):

graphql_compiler/deserialization.py

@@ -4,7 +4,6 @@
 import decimal
 from typing import Any, Collection, Dict, Mapping, NoReturn, Type
 
-import arrow
 from graphql import (
     GraphQLBoolean,
     GraphQLFloat,
@@ -102,8 +101,8 @@ def validate_argument_type(name: str, expected_type: QueryArgumentGraphQLType, v
         except ValueError as e:
             raise GraphQLInvalidArgumentError(e)
     elif is_same_type(GraphQLDateTime, stripped_type):
-        if not isinstance(value, (datetime.date, arrow.Arrow)):
-            _raise_invalid_type_error(name, (datetime.date, arrow.Arrow), value)
+        if not isinstance(value, datetime.datetime):
+            _raise_invalid_type_error(name, (datetime.datetime,), value)
         try:
             GraphQLDateTime.serialize(value)
         except ValueError as e:

graphql_compiler/query_formatting/common.py

@@ -3,7 +3,6 @@
 import json
 from string import Template
 
-import arrow
 from graphql import GraphQLBoolean, GraphQLFloat, GraphQLID, GraphQLInt, GraphQLList, GraphQLString
 import six
 
@@ -100,9 +99,7 @@ def _safe_cypher_argument(expected_type, argument_value):
     elif is_same_type(GraphQLDate, expected_type):
         return _safe_cypher_date_and_datetime(expected_type, (datetime.date,), argument_value)
     elif is_same_type(GraphQLDateTime, expected_type):
-        return _safe_cypher_date_and_datetime(
-            expected_type, (datetime.datetime, arrow.Arrow), argument_value
-        )
+        return _safe_cypher_date_and_datetime(expected_type, (datetime.datetime,), argument_value)
     elif isinstance(expected_type, GraphQLList):
         return _safe_cypher_list(expected_type.of_type, argument_value)
     else:

graphql_compiler/query_formatting/cypher_formatting.py

@@ -6,7 +6,8 @@
 from itertools import chain
 from typing import Any, FrozenSet, Iterable
 
-import arrow
+# C-based module confuses pylint, which is why we disable the check below.
+from ciso8601 import parse_datetime  # pylint: disable=no-name-in-module
 from graphql import (
     DirectiveLocation,
     GraphQLArgument,
@@ -340,45 +341,92 @@ def _serialize_date(value: Any) -> str:
 
 def _parse_date_value(value: Any) -> date:
     """Deserialize a Date object from its proper ISO-8601 representation."""
-    return arrow.get(value, "YYYY-MM-DD").date()
+    if type(value) == date:
+        # We prefer exact type equality instead of isinstance() because datetime objects
+        # are subclasses of date but are not interchangeable for dates for our purposes.
+        return value
+    elif isinstance(value, str):
+        # ciso8601 only supports parsing into datetime objects, not date objects.
+        # This is not a problem in itself: "YYYY-MM-DD" strings will get parsed into datetimes
+        # with hour/minute/second/microsecond set to 0, and tzinfo=None.
+        # We don't want our parsing to implicitly lose precision, so before we convert the parsed
+        # datetime into a date value, we just assert that these fields are set as expected.
+        dt = parse_datetime(value)  # This will raise ValueError in case of bad ISO 8601 formatting.
+        if (
+            dt.hour != 0
+            or dt.minute != 0
+            or dt.second != 0
+            or dt.microsecond != 0
+            or dt.tzinfo is not None
+        ):
+            raise ValueError(
+                f"Expected an ISO-8601 date string in 'YYYY-MM-DD' format, but got a datetime "
+                f"string with a non-empty time component. This is not supported, since converting "
+                f"it to a date would result in an implicit loss of precision. Received value "
+                f"{repr(value)}, parsed as {dt}."
+            )
+
+        return dt.date()
+    else:
+        raise ValueError(
+            f"Expected a date object or its ISO-8601 'YYYY-MM-DD' string representation. "
+            f"Got {value} of type {type(value)} instead."
+        )
 
 
 def _serialize_datetime(value: Any) -> str:
     """Serialize a DateTime object to its proper ISO-8601 representation."""
-    # Python datetime.datetime is a subclass of datetime.date, but in this case, the two are not
-    # interchangeable. Rather than using isinstance, we will therefore check for exact type
-    # equality.
-    #
-    # We don't allow Arrow objects as input since it seems that Arrow objects are always tz aware.
-    # This is supported by the fact that the `.naive` Arrow method returns a datetime object instead
-    # of an Arrow object.
-    if type(value) == datetime and value.tzinfo is None:
+    if isinstance(value, datetime) and value.tzinfo is None:
         return value.isoformat()
     else:
         raise ValueError(
-            f"Expected a timezone naive datetime object. Got {value} of type {type(value)} instead."
+            f"Expected a timezone-naive datetime object. Got {value} of type {type(value)} instead."
         )
 
 
 def _parse_datetime_value(value: Any) -> datetime:
-    """Deserialize a DateTime object from its proper ISO-8601 representation."""
-    # attempt to parse with microsecond information
-    try:
-        arrow_result = arrow.get(value, "YYYY-MM-DDTHH:mm:ss")
-    except arrow.parser.ParserMatchError:
-        arrow_result = arrow.get(value, "YYYY-MM-DDTHH:mm:ss.S")
-
-    # arrow parses datetime naive strings into Arrow objects with a UTC timezone.
-    return arrow_result.naive
+    """Deserialize a DateTime object from a date/datetime or a ISO-8601 string representation."""
+    if isinstance(value, datetime) and value.tzinfo is None:
+        return value
+    elif isinstance(value, str):
+        dt = parse_datetime(value)  # This will raise ValueError in case of bad ISO 8601 formatting.
+        if dt.tzinfo is not None:
+            raise ValueError(
+                f"Expected a timezone-naive datetime value, but got a timezone-aware datetime "
+                f"string. This is not supported, since discarding the timezone component would "
+                f"result in an implicit loss of precision. Received value {repr(value)}, "
+                f"parsed as {dt}."
+            )
+
+        return dt
+    elif type(value) == date:
+        # The date type is a supertype of datetime. We check for exact type equality
+        # rather than using isinstance(), to avoid having this branch get hit
+        # by timezone-aware datetimes (i.e. ones that fail the value.tzinfo is None check above).
+        #
+        # This is a widening conversion (there's no loss of precision) so we allow it to be implicit
+        # since use ciso8601 parsing logic for parsing datetimes, and ciso8601 successfully parses
+        # datetimes that only have data down to day precision.
+        return datetime(value.year, value.month, value.day)
+    else:
+        raise ValueError(
+            f"Expected a timezone-naive datetime or an ISO-8601 string representation parseable "
+            f"by the ciso8601 library. Got {value} of type {type(value)} instead."
+        )
 
 
 GraphQLDate = GraphQLScalarType(
     name="Date",
     description=(
         "The `Date` scalar type represents day-accuracy date objects."
         "Values are serialized following the ISO-8601 datetime format specification, "
-        'for example "2017-03-21". The year, month and day fields must be included, '
-        "and the format followed exactly, or the behavior is undefined."
+        'for example "2017-03-21". Serialization and parsing support is guaranteed for the format '
+        "described here, with the year, month and day fields included and separated by dashes as "
+        "in the example. Implementations are allowed to support additional serialization formats, "
+        "if they so choose."
+        # GraphQL compiler's implementation of GraphQL-based querying uses the ciso8601 library
+        # for date and datetime parsing, so it additionally supports the subset of the ISO-8601
+        # standard supported by that library.
     ),
     serialize=_serialize_date,
     parse_value=_parse_date_value,
@@ -389,10 +437,16 @@ def _parse_datetime_value(value: Any) -> datetime:
 GraphQLDateTime = GraphQLScalarType(
     name="DateTime",
     description=(
-        "The `DateTime` scalar type represents timezone-naive second-accuracy timestamps."
-        "Values are serialized following the ISO-8601 datetime format specification, "
-        'for example "2017-03-21T12:34:56". All of these fields must be included, '
-        "including the seconds, and the format followed exactly, or the behavior is undefined."
+        "The `DateTime` scalar type represents timezone-naive timestamps with up to microsecond "
+        "accuracy. Values are serialized following the ISO-8601 datetime format specification, "
+        'for example "2017-03-21T12:34:56.012345" or "2017-03-21T12:34:56". Serialization and '
+        "parsing support is guaranteed for the format described here, with all fields down to "
+        "and including seconds required to be included, and fractional seconds optional, as in "
+        "the example. Implementations are allowed to support additional serialization formats, "
+        "if they so choose."
+        # GraphQL compiler's implementation of GraphQL-based querying uses the ciso8601 library
+        # for date and datetime parsing, so it additionally supports the subset of the ISO-8601
+        # standard supported by that library.
     ),
     serialize=_serialize_datetime,
     parse_value=_parse_datetime_value,

graphql_compiler/schema/__init__.py

@@ -1,11 +1,10 @@
 # Copyright 2019-present Kensho Technologies, LLC.
-from datetime import date, datetime
+from datetime import date, datetime, timedelta, timezone
 import math
 from typing import Any, Dict, List
 import unittest
 
 import pytest
-import pytz
 
 from .. import test_input_data
 from ...compiler.metadata import FilterInfo
@@ -1849,11 +1848,22 @@ def test_int_value_conversion_datetime(self):
         datetime_values = [
             datetime(2000, 1, 1),
             datetime(3000, 1, 1, tzinfo=None),
-            datetime(1000, 1, 1, tzinfo=pytz.utc),
-            datetime(1, 1, 1, tzinfo=pytz.utc),
-            datetime(2000, 1, 1, 20, 55, 40, 877633, tzinfo=pytz.utc),
-            datetime(2000, 1, 1, 20, 55, 40, 877633, tzinfo=pytz.timezone("GMT")),
-            datetime(2000, 1, 1, 20, 55, 40, 877633, tzinfo=pytz.timezone("America/New_York")),
+            datetime(1000, 1, 1, tzinfo=timezone.utc),
+            datetime(1, 1, 1, tzinfo=timezone.utc),
+            datetime(2000, 1, 1, 20, 55, 40, 877633, tzinfo=timezone.utc),
+            datetime(
+                2000, 1, 1, 20, 55, 40, 877633, tzinfo=timezone(timedelta(hours=0), name="GMT")
+            ),
+            datetime(
+                2000,
+                1,
+                1,
+                20,
+                55,
+                40,
+                877633,
+                tzinfo=timezone(timedelta(hours=-4), name="America/New_York"),
+            ),
         ]
         for datetime_value in datetime_values:
             int_value = convert_field_value_to_int(

graphql_compiler/tests/snapshot_tests/test_cost_estimation.py

@@ -14,7 +14,6 @@
     GraphQLScalarType,
     GraphQLString,
 )
-import pytz
 import six
 
 from .. import graphql_to_gremlin, graphql_to_match
@@ -227,9 +226,16 @@ def test_argument_types(self) -> None:
                 (
                     "2007-12-06T16:29:43",
                     datetime.date(2007, 12, 6),
-                    datetime.datetime(2008, 12, 6, 16, 29, 43, 79043, tzinfo=pytz.utc),
+                    datetime.datetime(2008, 12, 6, 16, 29, 43, 79043, tzinfo=datetime.timezone.utc),
                     datetime.datetime(
-                        2009, 12, 6, 16, 29, 43, 79043, tzinfo=pytz.timezone("US/Eastern")
+                        2009,
+                        12,
+                        6,
+                        16,
+                        29,
+                        43,
+                        79043,
+                        tzinfo=datetime.timezone(datetime.timedelta(hours=-4), name="US/Eastern"),
                     ),
                 ),
             ),
@@ -272,9 +278,19 @@ def test_date_deserialization(self) -> None:
         self.assertEqual(datetime.date(2014, 2, 5), value)
 
     def test_datetime_deserialization(self) -> None:
-        # No time provided
-        with self.assertRaises(GraphQLInvalidArgumentError):
-            deserialize_argument("birth_time", GraphQLDateTime, "2014-02-05")
+        # No time provided, but still acceptable with zero time components.
+        value = deserialize_argument("birth_time", GraphQLDateTime, "2014-02-05")
+        self.assertEqual(datetime.datetime(2014, 2, 5), value)
+
+        # Time component with excess precision is truncated (not rounded!) down to microseconds.
+        # This example has 7 decimal places, whereas Python supports a maximum of 6.
+        value = deserialize_argument("birth_time", GraphQLDateTime, "2000-02-29T13:02:27.0018349")
+        self.assertEqual(datetime.datetime(2000, 2, 29, 13, 2, 27, 1834), value)
+
+        # Allow dates to be implicitly converted into datetimes, since this is a lossless,
+        # widening conversion.
+        value = deserialize_argument("birth_time", GraphQLDateTime, datetime.date(2000, 2, 29))
+        self.assertEqual(datetime.datetime(2000, 2, 29), value)
 
         # With timezone
         with self.assertRaises(GraphQLInvalidArgumentError):

graphql_compiler/tests/test_end_to_end.py

@@ -130,19 +130,23 @@
     }
 
     \"\"\"
-    The `Date` scalar type represents day-accuracy date objects.Values are
-    serialized following the ISO-8601 datetime format specification, for example
-    "2017-03-21". The year, month and day fields must be included, and the format
-    followed exactly, or the behavior is undefined.
+    The `Date` scalar type represents day-accuracy date objects.
+    Values are serialized following the ISO-8601 datetime format specification,
+    for example "2017-03-21". Serialization and parsing support is guaranteed for the format
+    described here, with the year, month and day fields included and separated by dashes as
+    in the example. Implementations are allowed to support additional serialization formats,
+    if they so choose.
     \"\"\"
     scalar Date
 
     \"\"\"
-    The `DateTime` scalar type represents timezone-naive second-accuracy
-    timestamps.Values are serialized following the ISO-8601 datetime format
-    specification, for example "2017-03-21T12:34:56". All of these fields must
-    be included, including the seconds, and the format followed
-    exactly, or the behavior is undefined.
+    The `DateTime` scalar type represents timezone-naive timestamps with up to microsecond
+    accuracy. Values are serialized following the ISO-8601 datetime format specification,
+    for example "2017-03-21T12:34:56.012345" or "2017-03-21T12:34:56". Serialization and
+    parsing support is guaranteed for the format described here, with all fields down to
+    and including seconds required to be included, and fractional seconds optional, as in
+    the example. Implementations are allowed to support additional serialization formats,
+    if they so choose.
     \"\"\"
     scalar DateTime
 

graphql_compiler/tests/test_helpers.py

@@ -320,7 +320,7 @@ def test_convert_basic_datetime(self):
             {
                 "child_datetime_fields": [
                     datetime.datetime(2020, 1, 1, 5, 45),
-                    datetime.datetime(2000, 2, 29, 13, 2, 27, 1835),
+                    datetime.datetime(2000, 2, 29, 13, 2, 27, 1834),  # truncated, not rounded
                     None,
                 ],
             }