Skip to content

Commit ef878e4

Browse files
authored
fix(dumpkit): write plist dates to whole seconds so plistlib reads them (#1448) (#1450)
dictdumper's PLIST._append_date writes '%Y-%m-%dT%H:%M:%S.%fZ', which the property list <date> 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.
1 parent b14d676 commit ef878e4

6 files changed

Lines changed: 193 additions & 54 deletions

File tree

‎docs/source/contributing/pep.rst‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -594,16 +594,19 @@ Anyone wanting to run the tests wants the repository, which is where they are.
594594

595595
.. note::
596596

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

608611
Reassembly Beyond IP and TCP
609612
----------------------------

‎pcapkit/dumpkit/common.py‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,11 @@ def make_dumper(output: 'Type[ABCDumper]') -> 'Type[ABCDumper]':
335335
# report has (GitHub issue #1257). The defect is :mod:`dictdumper`'s, so it
336336
# is worked around here.
337337
keep_none = issubclass(output, dictdumper.plist.PLIST)
338+
# NOTE: :meth:`~dictdumper.plist.PLIST._append_date` writes every date with
339+
# six fractional digits, which the property list ``<date>`` grammar does not
340+
# admit, so :func:`plistlib.load` rejects every report (GitHub issue #1448).
341+
# The defect is :mod:`dictdumper`'s, so it is worked around here.
342+
plist_dates = issubclass(output, dictdumper.plist.PLIST)
338343
# NOTE: :meth:`~dictdumper.tree.Tree._append_number` hands every number to
339344
# :func:`math.isnan`, which raises :exc:`TypeError` for a :class:`complex`
340345
# one, although :class:`~dictdumper.tree.Tree` routes :class:`complex` there
@@ -524,6 +529,41 @@ def _append_array(self, value: 'list[Any]', file: 'TextIO') -> 'None':
524529
value = [self._encode_value(item) if item is None else item for item in value]
525530
super()._append_array(value, file)
526531

532+
if plist_dates:
533+
def _append_date(self, value: 'datetime.date', file: 'TextIO') -> 'None':
534+
"""Call this function to write date contents.
535+
536+
Args:
537+
self: Dumper instance.
538+
value: Content to be dumped.
539+
file: Output file.
540+
541+
Notes:
542+
The date is written in UTC to whole seconds, as
543+
:func:`plistlib.dump` writes one: an aware value is
544+
converted to UTC first, a naive one is taken as UTC already,
545+
and the sub-second part of the :class:`~datetime.datetime`
546+
is truncated. That value is already rounded to the
547+
microsecond where it is parsed, so within half a microsecond
548+
of a second boundary the date is one second past the floor
549+
of a finer ``*_epoch``. The exact value is not
550+
lost: every timestamp pcapkit reports sits next to a field
551+
that carries it exactly -- a :class:`~decimal.Decimal`
552+
``*_epoch`` for frames and PCAP-NG blocks and options, e.g.
553+
``time_epoch``, and the raw ``ntp_timestamp`` or
554+
``pmip_timestamp`` for the two MH options.
555+
556+
"""
557+
if isinstance(value, datetime.datetime) and value.tzinfo is not None:
558+
value = value.astimezone(datetime.timezone.utc)
559+
# NOTE: not :meth:`~datetime.date.strftime`, whose ``%Y`` does not
560+
# zero-pad a year below 1000 on every platform.
561+
hour, minute, second = (getattr(value, name, 0)
562+
for name in ('hour', 'minute', 'second'))
563+
tabs = '\t' * self._tctr
564+
file.write(f'{tabs}<date>{value.year:04d}-{value.month:02d}-{value.day:02d}'
565+
f'T{hour:02d}:{minute:02d}:{second:02d}Z</date>\n')
566+
527567
if escape_json_keys:
528568
def _append_string(self, value: 'str', file: 'TextIO') -> 'None':
529569
"""Call this function to write string contents.

‎tests/dumpkit/test_capture_dump_runtime.py‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -59,17 +59,7 @@ class Gap(NamedTuple):
5959

6060

6161
#: Every check that fails today, by test method; each applies to every capture.
62-
KNOWN_FAILURES = {
63-
'test_plist_dump_reads_back_through_plistlib': Gap(
64-
1448, 'AttributeError',
65-
"dictdumper/plist.py:278 writes each timestamp as '%Y-%m-%dT%H:%M:%S.%fZ', "
66-
'and a property list <date> has no fractional part, so plistlib.load '
67-
"raises AttributeError: 'NoneType' object has no attribute 'groupdict' on "
68-
"every capture's first frame. Repro: extract(fin='in.pcap', fout='o', "
69-
"format='plist'); plistlib.load(open('o.plist', 'rb')). Also skipped, "
70-
'citing #1448, in tests/integration/test_output_formats.py PlistRoundTripTests.',
71-
),
72-
}
62+
KNOWN_FAILURES = {} # type: dict[str, Gap]
7363

7464

7565
def captures(pcapng: 'bool' = True) -> 'list[str]':
@@ -107,8 +97,7 @@ def sections(fmt: 'str', path: 'str') -> 'list[str]':
10797
with open(path, encoding='utf-8') as file:
10898
return list(json.load(file))
10999
if fmt == 'plist':
110-
# ElementTree, not plistlib, which cannot read the dump at all; see
111-
# :data:`KNOWN_FAILURES`.
100+
# ElementTree, so the plistlib test below is checked against an independent reader.
112101
root = ET.parse(path).getroot()
113102
return [key.text or '' for key in root[0].findall('key')]
114103
with open(path, encoding='utf-8') as file:
Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
# -*- coding: utf-8 -*-
2+
"""``<date>`` values in a ``plist`` report (GitHub issue #1448).
3+
4+
:meth:`dictdumper.plist.PLIST._append_date` writes every date as
5+
``%Y-%m-%dT%H:%M:%S.%fZ``. The property list ``<date>`` grammar has no
6+
fractional part, so :func:`plistlib.load` failed on every report with
7+
``AttributeError: 'NoneType' object has no attribute 'groupdict'``. These tests
8+
dump through :func:`~pcapkit.dumpkit.common.make_dumper` and read the file back
9+
with :mod:`plistlib`, so they need no capture. :func:`plistlib.loads` is
10+
called without ``aware_datetime``, which is new in Python 3.13, so every
11+
date comes back as a naive UTC value. :mod:`pcapkit` is imported
12+
inside each test, after :func:`~tests._support.reimport_once_per_class`.
13+
14+
"""
15+
from __future__ import annotations
16+
17+
import datetime
18+
import decimal
19+
import importlib.util
20+
import os
21+
import plistlib
22+
import tempfile
23+
import unittest
24+
25+
from tests._support import reimport_once_per_class
26+
27+
RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper')
28+
HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS)
29+
30+
UTC = datetime.timezone.utc
31+
32+
33+
@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
34+
class TestPlistDate(unittest.TestCase):
35+
"""Every ``<date>`` the customised ``PLIST`` writer emits parses."""
36+
37+
def setUp(self) -> None:
38+
reimport_once_per_class(self)
39+
tmpdir = tempfile.TemporaryDirectory(prefix='pcapkit-1448-') # pylint: disable=consider-using-with
40+
self.addCleanup(tmpdir.cleanup)
41+
self.tmpdir = tmpdir.name
42+
43+
def dump(self, kind: 'str', value: 'dict[str, object]') -> 'bytes':
44+
"""Dump ``value`` through the customised ``dictdumper`` writer ``kind``."""
45+
import dictdumper
46+
47+
from pcapkit.dumpkit.common import make_dumper
48+
49+
path = os.path.join(self.tmpdir, f'out.{kind.lower()}')
50+
make_dumper(getattr(dictdumper, kind))(path)(value, name='x')
51+
with open(path, 'rb') as file:
52+
return file.read()
53+
54+
def load(self, value: 'dict[str, object]') -> 'dict[str, object]':
55+
"""Dump ``value`` as ``plist`` and read it back with :func:`plistlib.loads`."""
56+
return plistlib.loads(self.dump('PLIST', value))['x']
57+
58+
def test_sub_second_date_is_written_to_whole_seconds(self) -> None:
59+
"""The regression: a fractional date parses, truncated, not rounded."""
60+
stamp = datetime.datetime(2017, 11, 19, 15, 49, 5, 999999, tzinfo=UTC)
61+
62+
raw = self.dump('PLIST', {'time': stamp})
63+
64+
self.assertIn(b'<date>2017-11-19T15:49:05Z</date>', raw)
65+
self.assertEqual(self.load({'time': stamp}),
66+
{'time': datetime.datetime(2017, 11, 19, 15, 49, 5)})
67+
68+
def test_exact_sibling_survives_alongside_the_date(self) -> None:
69+
"""The exact value is carried by the ``*_epoch`` sibling, as a frame reports it."""
70+
epoch = decimal.Decimal('1511106545.471719')
71+
stamp = datetime.datetime(2017, 11, 19, 15, 49, 5, 471719, tzinfo=UTC)
72+
73+
got = self.load({'time': stamp, 'time_epoch': epoch})
74+
75+
self.assertEqual(got['time'], datetime.datetime(2017, 11, 19, 15, 49, 5))
76+
self.assertEqual(decimal.Decimal(got['time_epoch']), epoch)
77+
78+
def test_aware_date_is_converted_to_utc(self) -> None:
79+
"""``Z`` means UTC, so an offset is applied rather than dropped."""
80+
tz = datetime.timezone(datetime.timedelta(hours=8))
81+
stamp = datetime.datetime(2020, 1, 1, 8, 0, 0, 500000, tzinfo=tz)
82+
83+
self.assertEqual(self.load({'t': stamp}),
84+
{'t': datetime.datetime(2020, 1, 1, 0, 0, 0)})
85+
86+
def test_naive_date_and_plain_date_are_written_as_is(self) -> None:
87+
"""A naive value is taken as UTC already, as :func:`plistlib.dump` takes it."""
88+
got = self.load({
89+
'naive': datetime.datetime(1999, 12, 31, 23, 59, 59, 1),
90+
'day': datetime.date(2000, 2, 29),
91+
'early': datetime.datetime(5, 1, 2, 3, 4, 5),
92+
})
93+
94+
self.assertEqual(got, {
95+
'naive': datetime.datetime(1999, 12, 31, 23, 59, 59),
96+
'day': datetime.datetime(2000, 2, 29),
97+
'early': datetime.datetime(5, 1, 2, 3, 4, 5),
98+
})
99+
100+
def test_json_and_tree_keep_the_fractional_date(self) -> None:
101+
"""Only the ``plist`` writer is changed; the others keep full precision."""
102+
stamp = datetime.datetime(2017, 11, 19, 15, 49, 5, 471719, tzinfo=UTC)
103+
104+
self.assertIn(b'2017-11-19T15:49:05.471719', self.dump('JSON', {'time': stamp}))
105+
self.assertIn(b'2017-11-19T15:49:05.471719', self.dump('Tree', {'time': stamp}))
106+
107+
108+
if __name__ == '__main__':
109+
unittest.main()

‎tests/dumpkit/test_plist_escaping_regression.py‎

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import importlib.util
2626
import json
2727
import pathlib
28+
import plistlib
2829
import tempfile
2930
import unittest
3031
import xml.etree.ElementTree as ET
@@ -71,23 +72,29 @@ def report(self, capture: 'str', fmt: 'str') -> 'pathlib.Path':
7172
return output
7273

7374
def test_the_plist_report_of_a_bytes_keyed_mapping_parses(self) -> None:
74-
"""The regression itself: the document has to be well-formed XML.
75+
"""The regression itself: the document has to be a readable property list.
7576
76-
:func:`plistlib.load` cannot stand in for the parser here --
77-
``dictdumper`` writes a ``<date>`` with fractional seconds, which it
78-
rejects for reasons that have nothing to do with #772 (see
79-
``PlistRoundTripTests`` in
80-
:file:`tests/integration/test_output_formats.py`) -- so this asserts
81-
well-formedness with :mod:`xml.etree.ElementTree` and then reads the key
82-
back out of the tree rather than out of the text, which is the part a
83-
string comparison would not have caught.
77+
This reads the report back with :func:`plistlib.load` and then takes the
78+
key from the parsed mappings rather than from the text, which is the
79+
part a string comparison would not have caught.
8480
8581
"""
8682
report = self.report('test.pcapng', 'plist')
8783

88-
keys = [element.text for element in ET.parse(report).iter('key')]
84+
keys = [] # type: list[str]
85+
86+
def collect(node: 'object') -> 'None':
87+
if isinstance(node, dict):
88+
keys.extend(node)
89+
node = list(node.values())
90+
if isinstance(node, list):
91+
for item in node:
92+
collect(item)
93+
94+
with report.open('rb') as stream:
95+
collect(plistlib.load(stream))
8996
self.assertIn(ESCAPED_BYTES_KEY, report.read_text(encoding='utf-8'))
90-
# ElementTree resolves the entities, so the key comes back as the octets
97+
# The parser resolves the entities, so the key comes back as the octets
9198
# the fixture holds: escaping is a transport detail, not a rename.
9299
self.assertIn(RAW_BYTES_KEY, keys)
93100

‎tests/integration/test_output_formats.py‎

Lines changed: 12 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -110,33 +110,21 @@ def test_plist_report_is_well_formed_xml_with_one_key_per_frame(self) -> None:
110110
self.assertEqual(plist_keys(extractor.output), IN_PCAP_SECTIONS)
111111

112112

113+
@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
113114
class PlistRoundTripTests(EndToEndTestCase):
114-
"""The property list report against a real property list reader.
115-
116-
Kept apart from :class:`PlistReportTests` so that the skip below covers only
117-
the reader, and the structural assertions above keep running.
118-
119-
"""
115+
"""The property list report against a real property list reader."""
120116

121-
@unittest.skip('blocked on dictdumper writing <date> values with fractional seconds, '
122-
'which plistlib rejects (dictdumper/plist.py:278, #1448)')
123117
def test_plist_report_round_trips_through_plistlib(self) -> None:
124-
"""A ``plist`` report should be readable by :func:`plistlib.load`.
125-
126-
It is not. ``dictdumper/plist.py:278`` formats every timestamp as
127-
``'%Y-%m-%dT%H:%M:%S.%fZ'``, and a property list ``<date>`` carries no
128-
fractional part, so :func:`plistlib.load` fails on the first frame's
129-
``time`` with ``AttributeError: 'NoneType' object has no attribute
130-
'groupdict'`` -- its date pattern simply does not match. Reproduce with::
131-
132-
>>> import dictdumper, datetime, plistlib
133-
>>> dictdumper.PLIST('probe.plist')({'time': datetime.datetime.now()})
134-
>>> plistlib.load(open('probe.plist', 'rb'))
118+
"""A ``plist`` report is readable by :func:`plistlib.load` (GitHub issue #1448).
135119
136-
The assertions below are what a fixed writer should satisfy, so this
137-
test can simply be un-skipped once the dumper emits a conformant date.
120+
``dictdumper`` writes a ``<date>`` with fractional seconds, which
121+
:mod:`plistlib` rejects; :mod:`pcapkit.dumpkit` writes it to whole
122+
seconds instead, and the exact value stays in ``time_epoch``. The date
123+
is compared as naive UTC, since ``aware_datetime`` is new in Python 3.13.
138124
139125
"""
126+
import datetime
127+
import decimal
140128
import plistlib
141129

142130
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:
147135

148136
self.assertEqual(list(report), IN_PCAP_SECTIONS)
149137
self.assertEqual(report['Frame 1']['protocols'], IN_PCAP_FIRST_CHAIN)
138+
self.assertEqual(report['Frame 1']['time'],
139+
datetime.datetime(2017, 11, 19, 15, 49, 5))
140+
self.assertEqual(decimal.Decimal(report['Frame 1']['time_epoch']), decimal.Decimal('1511106545.471719'))
150141

151142

152143
@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')

0 commit comments

Comments
 (0)