From 2ab8bbe83cf08a779b8e94c4031dfa4fa6e033a5 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 8 Oct 2026 21:19:58 -0400 Subject: [PATCH] fix(dumpkit): write plist dates to whole seconds so plistlib reads them (#1448) dictdumper's PLIST._append_date writes '%Y-%m-%dT%H:%M:%S.%fZ', which the property list grammar does not admit, so plistlib.load failed on the plist/xml report of every capture. - Override _append_date in make_dumper for PLIST writers: convert an aware value to UTC, take a naive one as UTC, truncate to whole seconds, and zero-pad the year, as plistlib.dump does. - The exact value is kept by the sibling field every reported timestamp already has (time_epoch / timestamp_epoch, or the MH option's raw ntp_timestamp / pmip_timestamp), so no key is added. - Un-skip PlistRoundTripTests in tests/integration/test_output_formats.py and add tests/dumpkit/test_plist_date_unit.py. - Read the #772 regression's plist report back with plistlib.load, and update the note in docs/source/contributing/pep.rst. - Drop the #1448 row from KNOWN_FAILURES in tests/dumpkit/test_capture_dump_runtime.py, which now passes. json and tree output is unchanged. --- docs/source/contributing/pep.rst | 21 ++-- pcapkit/dumpkit/common.py | 40 +++++++ tests/dumpkit/test_capture_dump_runtime.py | 15 +-- tests/dumpkit/test_plist_date_unit.py | 109 ++++++++++++++++++ .../dumpkit/test_plist_escaping_regression.py | 29 +++-- tests/integration/test_output_formats.py | 33 ++---- 6 files changed, 193 insertions(+), 54 deletions(-) create mode 100644 tests/dumpkit/test_plist_date_unit.py diff --git a/docs/source/contributing/pep.rst b/docs/source/contributing/pep.rst index 642166bfa..1e638ccef 100644 --- a/docs/source/contributing/pep.rst +++ b/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 ```` 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 `__. + :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 ```` pattern admits none, so its - match returns ``None``. The fix belongs upstream, as `JarryShaw/DictDumper#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 ---------------------------- diff --git a/pcapkit/dumpkit/common.py b/pcapkit/dumpkit/common.py index 48aae0da8..95edee840 100644 --- a/pcapkit/dumpkit/common.py +++ b/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 ```` 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}{value.year:04d}-{value.month:02d}-{value.day:02d}' + f'T{hour:02d}:{minute:02d}:{second:02d}Z\n') + if escape_json_keys: def _append_string(self, value: 'str', file: 'TextIO') -> 'None': """Call this function to write string contents. diff --git a/tests/dumpkit/test_capture_dump_runtime.py b/tests/dumpkit/test_capture_dump_runtime.py index 993df8684..ccc6f7d7a 100644 --- a/tests/dumpkit/test_capture_dump_runtime.py +++ b/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 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: diff --git a/tests/dumpkit/test_plist_date_unit.py b/tests/dumpkit/test_plist_date_unit.py new file mode 100644 index 000000000..1cd223345 --- /dev/null +++ b/tests/dumpkit/test_plist_date_unit.py @@ -0,0 +1,109 @@ +# -*- coding: utf-8 -*- +"""```` 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 ```` 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 ```` 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'2017-11-19T15:49:05Z', 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() diff --git a/tests/dumpkit/test_plist_escaping_regression.py b/tests/dumpkit/test_plist_escaping_regression.py index df6fb8bac..b8562c8d4 100644 --- a/tests/dumpkit/test_plist_escaping_regression.py +++ b/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 ```` 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) diff --git a/tests/integration/test_output_formats.py b/tests/integration/test_output_formats.py index 730c8c18a..53d364b2f 100644 --- a/tests/integration/test_output_formats.py +++ b/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 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 ```` 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 ```` 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')