fix(dumpkit): write plist dates to whole seconds so plistlib reads them (#1448)

docs/source/contributing/pep.rst

@@ -594,16 +594,19 @@ Anyone wanting to run the tests wants the repository, which is where they are.
 
 .. note::
 
-   One test is skipped on an upstream defect rather than a pcapkit one.
+   ``dictdumper``'s ``PLIST`` writer formats every :class:`~datetime.datetime` as
+   ``'%Y-%m-%dT%H:%M:%S.%fZ'``, always with six fractional digits, and
+   :mod:`plistlib`'s ``<date>`` pattern admits none, so :func:`plistlib.load`
+   failed on every ``format='plist'`` report with ``AttributeError: 'NoneType'
+   object has no attribute 'groupdict'``. The upstream defect is
+   `JarryShaw/DictDumper#122 <https://github.com/JarryShaw/DictDumper/issues/122>`__.
+   :func:`pcapkit.dumpkit.common.make_dumper` works around it (:issue:`1448`): it
+   writes each date in UTC to whole seconds, as :func:`plistlib.dump` does. The
+   exact value stays in the sibling field every timestamp already has, such as
+   ``time_epoch``.
    ``PlistRoundTripTests.test_plist_report_round_trips_through_plistlib``, in
-   ``tests/integration/test_output_formats.py``, reads a ``format='plist'`` report
-   back with :func:`plistlib.load`, which fails with ``AttributeError: 'NoneType'
-   object has no attribute 'groupdict'``. ``dictdumper``'s ``PLIST`` writer formats
-   every :class:`~datetime.datetime` as ``'%Y-%m-%dT%H:%M:%S.%fZ'``, always with six
-   fractional digits, and :mod:`plistlib`'s ``<date>`` pattern admits none, so its
-   match returns ``None``. The fix belongs upstream, as `JarryShaw/DictDumper#122
-   <https://github.com/JarryShaw/DictDumper/issues/122>`__; the test asserts what a
-   fixed writer should produce, so un-skipping it is the whole of pcapkit's side.
+   ``tests/integration/test_output_formats.py``, now runs rather than being
+   skipped.
 
 Reassembly Beyond IP and TCP
 ----------------------------

pcapkit/dumpkit/common.py

@@ -335,6 +335,11 @@ def make_dumper(output: 'Type[ABCDumper]') -> 'Type[ABCDumper]':
     # report has (GitHub issue #1257). The defect is :mod:`dictdumper`'s, so it
     # is worked around here.
     keep_none = issubclass(output, dictdumper.plist.PLIST)
+    # NOTE: :meth:`~dictdumper.plist.PLIST._append_date` writes every date with
+    # six fractional digits, which the property list ``<date>`` grammar does not
+    # admit, so :func:`plistlib.load` rejects every report (GitHub issue #1448).
+    # The defect is :mod:`dictdumper`'s, so it is worked around here.
+    plist_dates = issubclass(output, dictdumper.plist.PLIST)
     # NOTE: :meth:`~dictdumper.tree.Tree._append_number` hands every number to
     # :func:`math.isnan`, which raises :exc:`TypeError` for a :class:`complex`
     # one, although :class:`~dictdumper.tree.Tree` routes :class:`complex` there
@@ -524,6 +529,41 @@ def _append_array(self, value: 'list[Any]', file: 'TextIO') -> 'None':
                 value = [self._encode_value(item) if item is None else item for item in value]
                 super()._append_array(value, file)
 
+        if plist_dates:
+            def _append_date(self, value: 'datetime.date', file: 'TextIO') -> 'None':
+                """Call this function to write date contents.
+
+                Args:
+                    self: Dumper instance.
+                    value: Content to be dumped.
+                    file: Output file.
+
+                Notes:
+                    The date is written in UTC to whole seconds, as
+                    :func:`plistlib.dump` writes one: an aware value is
+                    converted to UTC first, a naive one is taken as UTC already,
+                    and the sub-second part of the :class:`~datetime.datetime`
+                    is truncated. That value is already rounded to the
+                    microsecond where it is parsed, so within half a microsecond
+                    of a second boundary the date is one second past the floor
+                    of a finer ``*_epoch``. The exact value is not
+                    lost: every timestamp pcapkit reports sits next to a field
+                    that carries it exactly -- a :class:`~decimal.Decimal`
+                    ``*_epoch`` for frames and PCAP-NG blocks and options, e.g.
+                    ``time_epoch``, and the raw ``ntp_timestamp`` or
+                    ``pmip_timestamp`` for the two MH options.
+
+                """
+                if isinstance(value, datetime.datetime) and value.tzinfo is not None:
+                    value = value.astimezone(datetime.timezone.utc)
+                # NOTE: not :meth:`~datetime.date.strftime`, whose ``%Y`` does not
+                # zero-pad a year below 1000 on every platform.
+                hour, minute, second = (getattr(value, name, 0)
+                                        for name in ('hour', 'minute', 'second'))
+                tabs = '\t' * self._tctr
+                file.write(f'{tabs}<date>{value.year:04d}-{value.month:02d}-{value.day:02d}'
+                           f'T{hour:02d}:{minute:02d}:{second:02d}Z</date>\n')
+
         if escape_json_keys:
             def _append_string(self, value: 'str', file: 'TextIO') -> 'None':
                 """Call this function to write string contents.

tests/dumpkit/test_capture_dump_runtime.py

@@ -59,17 +59,7 @@ class Gap(NamedTuple):
 
 
 #: Every check that fails today, by test method; each applies to every capture.
-KNOWN_FAILURES = {
-    'test_plist_dump_reads_back_through_plistlib': Gap(
-        1448, 'AttributeError',
-        "dictdumper/plist.py:278 writes each timestamp as '%Y-%m-%dT%H:%M:%S.%fZ', "
-        'and a property list <date> has no fractional part, so plistlib.load '
-        "raises AttributeError: 'NoneType' object has no attribute 'groupdict' on "
-        "every capture's first frame. Repro: extract(fin='in.pcap', fout='o', "
-        "format='plist'); plistlib.load(open('o.plist', 'rb')). Also skipped, "
-        'citing #1448, in tests/integration/test_output_formats.py PlistRoundTripTests.',
-    ),
-}
+KNOWN_FAILURES = {}  # type: dict[str, Gap]
 
 
 def captures(pcapng: 'bool' = True) -> 'list[str]':
@@ -107,8 +97,7 @@ def sections(fmt: 'str', path: 'str') -> 'list[str]':
         with open(path, encoding='utf-8') as file:
             return list(json.load(file))
     if fmt == 'plist':
-        # ElementTree, not plistlib, which cannot read the dump at all; see
-        # :data:`KNOWN_FAILURES`.
+        # ElementTree, so the plistlib test below is checked against an independent reader.
         root = ET.parse(path).getroot()
         return [key.text or '' for key in root[0].findall('key')]
     with open(path, encoding='utf-8') as file:

tests/dumpkit/test_plist_date_unit.py

@@ -0,0 +1,109 @@
+# -*- coding: utf-8 -*-
+"""``<date>`` values in a ``plist`` report (GitHub issue #1448).
+
+:meth:`dictdumper.plist.PLIST._append_date` writes every date as
+``%Y-%m-%dT%H:%M:%S.%fZ``. The property list ``<date>`` grammar has no
+fractional part, so :func:`plistlib.load` failed on every report with
+``AttributeError: 'NoneType' object has no attribute 'groupdict'``. These tests
+dump through :func:`~pcapkit.dumpkit.common.make_dumper` and read the file back
+with :mod:`plistlib`, so they need no capture. :func:`plistlib.loads` is
+called without ``aware_datetime``, which is new in Python 3.13, so every
+date comes back as a naive UTC value. :mod:`pcapkit` is imported
+inside each test, after :func:`~tests._support.reimport_once_per_class`.
+
+"""
+from __future__ import annotations
+
+import datetime
+import decimal
+import importlib.util
+import os
+import plistlib
+import tempfile
+import unittest
+
+from tests._support import reimport_once_per_class
+
+RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper')
+HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS)
+
+UTC = datetime.timezone.utc
+
+
+@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
+class TestPlistDate(unittest.TestCase):
+    """Every ``<date>`` the customised ``PLIST`` writer emits parses."""
+
+    def setUp(self) -> None:
+        reimport_once_per_class(self)
+        tmpdir = tempfile.TemporaryDirectory(prefix='pcapkit-1448-')  # pylint: disable=consider-using-with
+        self.addCleanup(tmpdir.cleanup)
+        self.tmpdir = tmpdir.name
+
+    def dump(self, kind: 'str', value: 'dict[str, object]') -> 'bytes':
+        """Dump ``value`` through the customised ``dictdumper`` writer ``kind``."""
+        import dictdumper
+
+        from pcapkit.dumpkit.common import make_dumper
+
+        path = os.path.join(self.tmpdir, f'out.{kind.lower()}')
+        make_dumper(getattr(dictdumper, kind))(path)(value, name='x')
+        with open(path, 'rb') as file:
+            return file.read()
+
+    def load(self, value: 'dict[str, object]') -> 'dict[str, object]':
+        """Dump ``value`` as ``plist`` and read it back with :func:`plistlib.loads`."""
+        return plistlib.loads(self.dump('PLIST', value))['x']
+
+    def test_sub_second_date_is_written_to_whole_seconds(self) -> None:
+        """The regression: a fractional date parses, truncated, not rounded."""
+        stamp = datetime.datetime(2017, 11, 19, 15, 49, 5, 999999, tzinfo=UTC)
+
+        raw = self.dump('PLIST', {'time': stamp})
+
+        self.assertIn(b'<date>2017-11-19T15:49:05Z</date>', raw)
+        self.assertEqual(self.load({'time': stamp}),
+                         {'time': datetime.datetime(2017, 11, 19, 15, 49, 5)})
+
+    def test_exact_sibling_survives_alongside_the_date(self) -> None:
+        """The exact value is carried by the ``*_epoch`` sibling, as a frame reports it."""
+        epoch = decimal.Decimal('1511106545.471719')
+        stamp = datetime.datetime(2017, 11, 19, 15, 49, 5, 471719, tzinfo=UTC)
+
+        got = self.load({'time': stamp, 'time_epoch': epoch})
+
+        self.assertEqual(got['time'], datetime.datetime(2017, 11, 19, 15, 49, 5))
+        self.assertEqual(decimal.Decimal(got['time_epoch']), epoch)
+
+    def test_aware_date_is_converted_to_utc(self) -> None:
+        """``Z`` means UTC, so an offset is applied rather than dropped."""
+        tz = datetime.timezone(datetime.timedelta(hours=8))
+        stamp = datetime.datetime(2020, 1, 1, 8, 0, 0, 500000, tzinfo=tz)
+
+        self.assertEqual(self.load({'t': stamp}),
+                         {'t': datetime.datetime(2020, 1, 1, 0, 0, 0)})
+
+    def test_naive_date_and_plain_date_are_written_as_is(self) -> None:
+        """A naive value is taken as UTC already, as :func:`plistlib.dump` takes it."""
+        got = self.load({
+            'naive': datetime.datetime(1999, 12, 31, 23, 59, 59, 1),
+            'day': datetime.date(2000, 2, 29),
+            'early': datetime.datetime(5, 1, 2, 3, 4, 5),
+        })
+
+        self.assertEqual(got, {
+            'naive': datetime.datetime(1999, 12, 31, 23, 59, 59),
+            'day': datetime.datetime(2000, 2, 29),
+            'early': datetime.datetime(5, 1, 2, 3, 4, 5),
+        })
+
+    def test_json_and_tree_keep_the_fractional_date(self) -> None:
+        """Only the ``plist`` writer is changed; the others keep full precision."""
+        stamp = datetime.datetime(2017, 11, 19, 15, 49, 5, 471719, tzinfo=UTC)
+
+        self.assertIn(b'2017-11-19T15:49:05.471719', self.dump('JSON', {'time': stamp}))
+        self.assertIn(b'2017-11-19T15:49:05.471719', self.dump('Tree', {'time': stamp}))
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/dumpkit/test_plist_escaping_regression.py

@@ -25,6 +25,7 @@
 import importlib.util
 import json
 import pathlib
+import plistlib
 import tempfile
 import unittest
 import xml.etree.ElementTree as ET
@@ -71,23 +72,29 @@ def report(self, capture: 'str', fmt: 'str') -> 'pathlib.Path':
         return output
 
     def test_the_plist_report_of_a_bytes_keyed_mapping_parses(self) -> None:
-        """The regression itself: the document has to be well-formed XML.
+        """The regression itself: the document has to be a readable property list.
 
-        :func:`plistlib.load` cannot stand in for the parser here --
-        ``dictdumper`` writes a ``<date>`` with fractional seconds, which it
-        rejects for reasons that have nothing to do with #772 (see
-        ``PlistRoundTripTests`` in
-        :file:`tests/integration/test_output_formats.py`) -- so this asserts
-        well-formedness with :mod:`xml.etree.ElementTree` and then reads the key
-        back out of the tree rather than out of the text, which is the part a
-        string comparison would not have caught.
+        This reads the report back with :func:`plistlib.load` and then takes the
+        key from the parsed mappings rather than from the text, which is the
+        part a string comparison would not have caught.
 
         """
         report = self.report('test.pcapng', 'plist')
 
-        keys = [element.text for element in ET.parse(report).iter('key')]
+        keys = []  # type: list[str]
+
+        def collect(node: 'object') -> 'None':
+            if isinstance(node, dict):
+                keys.extend(node)
+                node = list(node.values())
+            if isinstance(node, list):
+                for item in node:
+                    collect(item)
+
+        with report.open('rb') as stream:
+            collect(plistlib.load(stream))
         self.assertIn(ESCAPED_BYTES_KEY, report.read_text(encoding='utf-8'))
-        # ElementTree resolves the entities, so the key comes back as the octets
+        # The parser resolves the entities, so the key comes back as the octets
         # the fixture holds: escaping is a transport detail, not a rename.
         self.assertIn(RAW_BYTES_KEY, keys)
 

tests/integration/test_output_formats.py

@@ -110,33 +110,21 @@ def test_plist_report_is_well_formed_xml_with_one_key_per_frame(self) -> None:
         self.assertEqual(plist_keys(extractor.output), IN_PCAP_SECTIONS)
 
 
+@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
 class PlistRoundTripTests(EndToEndTestCase):
-    """The property list report against a real property list reader.
-
-    Kept apart from :class:`PlistReportTests` so that the skip below covers only
-    the reader, and the structural assertions above keep running.
-
-    """
+    """The property list report against a real property list reader."""
 
-    @unittest.skip('blocked on dictdumper writing <date> values with fractional seconds, '
-                   'which plistlib rejects (dictdumper/plist.py:278, #1448)')
     def test_plist_report_round_trips_through_plistlib(self) -> None:
-        """A ``plist`` report should be readable by :func:`plistlib.load`.
-
-        It is not. ``dictdumper/plist.py:278`` formats every timestamp as
-        ``'%Y-%m-%dT%H:%M:%S.%fZ'``, and a property list ``<date>`` carries no
-        fractional part, so :func:`plistlib.load` fails on the first frame's
-        ``time`` with ``AttributeError: 'NoneType' object has no attribute
-        'groupdict'`` -- its date pattern simply does not match. Reproduce with::
-
-            >>> import dictdumper, datetime, plistlib
-            >>> dictdumper.PLIST('probe.plist')({'time': datetime.datetime.now()})
-            >>> plistlib.load(open('probe.plist', 'rb'))
+        """A ``plist`` report is readable by :func:`plistlib.load` (GitHub issue #1448).
 
-        The assertions below are what a fixed writer should satisfy, so this
-        test can simply be un-skipped once the dumper emits a conformant date.
+        ``dictdumper`` writes a ``<date>`` with fractional seconds, which
+        :mod:`plistlib` rejects; :mod:`pcapkit.dumpkit` writes it to whole
+        seconds instead, and the exact value stays in ``time_epoch``. The date
+        is compared as naive UTC, since ``aware_datetime`` is new in Python 3.13.
 
         """
+        import datetime
+        import decimal
         import plistlib
 
         extractor = self.extract(fin=sample_path('in.pcap'), fout=self.out('report'),
@@ -147,6 +135,9 @@ def test_plist_report_round_trips_through_plistlib(self) -> None:
 
         self.assertEqual(list(report), IN_PCAP_SECTIONS)
         self.assertEqual(report['Frame 1']['protocols'], IN_PCAP_FIRST_CHAIN)
+        self.assertEqual(report['Frame 1']['time'],
+                         datetime.datetime(2017, 11, 19, 15, 49, 5))
+        self.assertEqual(decimal.Decimal(report['Frame 1']['time_epoch']), decimal.Decimal('1511106545.471719'))
 
 
 @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')