From 00718012f5056c8c0db24ebaad422517c4b56099 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 24 Sep 2026 11:00:51 -0400 Subject: [PATCH] fix(foundation): add the #718 identity guard to the five registrars it missed register_engine, register_reassembly, register_traceflow, and both register_dumper copies (Extractor and TraceFlowBase) warned on mere key presence, so re-registering the exact same class or descriptor read as "overwriting X" even though nothing was displaced. #718 fixed this for ten code-keyed registrars but excluded these five because their message wording falls outside its criterion, even though they share the defect. - Replace each `if key in cls.__dict__:` guard with `incumbent = cls.__dict__ .get(key)` + `incumbent is not None and incumbent is not value`, matching the identity-guard shape #726 gives the ten sibling registrars. Message text is unchanged. - register_engine/reassembly/traceflow: all three seed dicts hold only ModuleDescriptors, never a class, so the *first* real registration (descriptor -> class) still legitimately warns; only a *repeat* registration is now silent. Plain dicts, so `.get()`/`in` are both non-inserting. - register_dumper (both copies): __output__ maps format -> (dumper, ext), so the identity check compares the incumbent dumper (index 0), not the pair -- a re-registration that only changes ext is still identity-equal on the dumper and stays silent. __output__ is a `collections.defaultdict`; measured that `.get()` does not invoke the default factory the way a subscript access would (confirmed: len() unchanged on a `.get()`/`in` miss, +1 on a `d[missing]` subscript). `incumbent_entry[0]` is safe on a factory-produced miss too, since the factory itself returns a 2-tuple. - Add tests pinning, per registrar: same-object re-registration is silent, different-object replacement still warns with the same message, and the registry write happens on both paths. Verified all five fail against the unfixed guard. Ran tests/foundation/test_extraction.py + tests/foundation/traceflow/ test_traceflow_base.py: 16 -> 21 passed (28 subtests, unchanged). coverage: extraction.py 367 -> 372 stmts (miss unchanged at 7, 96%), traceflow.py 121 -> 123 stmts (0 miss, 100%) -- all new statements fully exercised. Fixes #739. --- pcapkit/foundation/extraction.py | 47 ++++- pcapkit/foundation/traceflow/traceflow.py | 20 +- tests/foundation/test_extraction.py | 198 ++++++++++++++++++ .../traceflow/test_traceflow_base.py | 39 ++++ 4 files changed, 299 insertions(+), 5 deletions(-) diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index b4a150ac40..9460a1fe83 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -373,6 +373,22 @@ def register_dumper(cls, format: 'str', dumper: 'ModuleDescriptor[Dumper] | Type The full qualified class name of the new dumper class should be as ``{dumper.module}.{dumper.name}``. + The overwrite guard fires only when the incumbent dumper differs + from the replacement, so re-registering the exact same object is + a silent no-op rather than a warning about nothing displaced -- + the identity guard GitHub issue #718 gave the code-keyed + registrars, extended here by GitHub issue #739. ``__output__`` + maps each format to a ``(dumper, ext)`` pair, so the identity + check compares the incumbent *dumper* (index ``0``), not the + pair -- a re-registration that only changes ``ext`` is still + identity-equal on the dumper and stays silent, since the dumper + is what "the same object" means here, not the pair as a whole. + ``__output__`` is also a :class:`collections.defaultdict`, + unlike the other three + registrars this issue touches; :meth:`dict.get` does not invoke + the default factory the way ``cls.__output__[format]`` would, so + it stays non-inserting here as well. + Arguments: format: format name dumper: module descriptor or a :class:`dictdumper.dumper.Dumper` subclass @@ -383,7 +399,9 @@ def register_dumper(cls, format: 'str', dumper: 'ModuleDescriptor[Dumper] | Type dumper = dumper.klass if not issubclass(dumper, Dumper): raise RegistryError(f'dumper must be a Dumper subclass, not {dumper!r}') - if format in cls.__output__: + incumbent_entry = cls.__output__.get(format) + incumbent = incumbent_entry[0] if incumbent_entry is not None else None + if incumbent is not None and incumbent is not dumper: warn(f'dumper {format} already registered, overwriting', RegistryWarning) cls.__output__[format] = (dumper, ext) @@ -395,6 +413,12 @@ def register_engine(cls, name: 'str', engine: 'ModuleDescriptor[Engine] | Type[E The full qualified class name of the new extraction engine should be as ``{engine.module}.{engine.name}``. + The overwrite guard fires only when the incumbent differs from + the replacement, so re-registering the exact same object is a + silent no-op rather than a warning about nothing displaced -- + the identity guard GitHub issue #718 gave the code-keyed + registrars, extended here by GitHub issue #739. + Arguments: name: engine name engine: module descriptor or an @@ -410,7 +434,8 @@ def register_engine(cls, name: 'str', engine: 'ModuleDescriptor[Engine] | Type[E # classes. ``Engine`` is itself an ``EngineBase``, so this only widens. See #513. if not issubclass(engine, EngineBase): raise RegistryError(f'engine must be an Engine subclass, not {engine!r}') - if name in cls.__engine__: + incumbent = cls.__engine__.get(name) + if incumbent is not None and incumbent is not engine: warn(f'engine {name} already registered, overwriting', RegistryWarning) cls.__engine__[name] = engine @@ -422,6 +447,12 @@ def register_reassembly(cls, protocol: 'str', reassembly: 'ModuleDescriptor[Reas The full qualified class name of the new reassembly engine should be as ``{reassembly.module}.{reassembly.name}``. + The overwrite guard fires only when the incumbent differs from + the replacement, so re-registering the exact same object is a + silent no-op rather than a warning about nothing displaced -- + the identity guard GitHub issue #718 gave the code-keyed + registrars, extended here by GitHub issue #739. + Arguments: protocol: protocol name reassembly: module descriptor or a @@ -434,7 +465,8 @@ def register_reassembly(cls, protocol: 'str', reassembly: 'ModuleDescriptor[Reas # :meth:`register_engine` above -- see #513. if not issubclass(reassembly, ReassemblyBase): raise RegistryError(f'reassembly must be a Reassembly subclass, not {reassembly!r}') - if protocol in cls.__reassembly__: + incumbent = cls.__reassembly__.get(protocol) + if incumbent is not None and incumbent is not reassembly: warn(f'reassembly {protocol} already registered, overwriting', RegistryWarning) cls.__reassembly__[protocol] = reassembly @@ -446,6 +478,12 @@ def register_traceflow(cls, protocol: 'str', traceflow: 'ModuleDescriptor[TraceF The full qualified class name of the new flow tracing engine should be as ``{traceflow.module}.{traceflow.name}``. + The overwrite guard fires only when the incumbent differs from + the replacement, so re-registering the exact same object is a + silent no-op rather than a warning about nothing displaced -- + the identity guard GitHub issue #718 gave the code-keyed + registrars, extended here by GitHub issue #739. + Arguments: protocol: protocol name traceflow: module descriptor or a @@ -458,7 +496,8 @@ def register_traceflow(cls, protocol: 'str', traceflow: 'ModuleDescriptor[TraceF # :meth:`register_engine` above -- see #513. if not issubclass(traceflow, TraceFlowBase): raise RegistryError(f'traceflow must be a TraceFlow subclass, not {traceflow!r}') - if protocol in cls.__traceflow__: + incumbent = cls.__traceflow__.get(protocol) + if incumbent is not None and incumbent is not traceflow: warn(f'traceflow {protocol} already registered, overwriting', RegistryWarning) cls.__traceflow__[protocol] = traceflow diff --git a/pcapkit/foundation/traceflow/traceflow.py b/pcapkit/foundation/traceflow/traceflow.py index 6eaa1375b9..82a37966a1 100644 --- a/pcapkit/foundation/traceflow/traceflow.py +++ b/pcapkit/foundation/traceflow/traceflow.py @@ -210,6 +210,22 @@ def register_dumper(cls, format: 'str', dumper: 'ModuleDescriptor[Dumper] | Type The full qualified class name of the new dumper class should be as ``{dumper.module}.{dumper.name}``. + The overwrite guard fires only when the incumbent dumper differs + from the replacement, so re-registering the exact same object is + a silent no-op rather than a warning about nothing displaced -- + the identity guard GitHub issue #718 gave the code-keyed + registrars, extended here by GitHub issue #739. ``__output__`` + maps each format to a ``(dumper, ext)`` pair, so the identity + check compares the incumbent *dumper* (index ``0``), not the + pair -- a re-registration that only changes ``ext`` is still + identity-equal on the dumper and stays silent, since the dumper + is what "the same object" means here, not the pair as a whole. + ``__output__`` is also a :class:`collections.defaultdict`, + unlike the other three + registrars this issue touches; :meth:`dict.get` does not invoke + the default factory the way ``cls.__output__[format]`` would, so + it stays non-inserting here as well. + Arguments: format: format name dumper: module descriptor or a :class:`dictdumper.dumper.Dumper` subclass @@ -220,7 +236,9 @@ def register_dumper(cls, format: 'str', dumper: 'ModuleDescriptor[Dumper] | Type dumper = dumper.klass if not issubclass(dumper, Dumper): raise RegistryError(f'dumper must be a Dumper subclass, not {dumper!r}') - if format in cls.__output__: + incumbent_entry = cls.__output__.get(format) + incumbent = incumbent_entry[0] if incumbent_entry is not None else None + if incumbent is not None and incumbent is not dumper: warn(f'dumper {format} already registered, overwriting', RegistryWarning) cls.__output__[format] = (dumper, ext) diff --git a/tests/foundation/test_extraction.py b/tests/foundation/test_extraction.py index 7959b2ec66..97acf93933 100644 --- a/tests/foundation/test_extraction.py +++ b/tests/foundation/test_extraction.py @@ -246,6 +246,204 @@ def submit(self) -> tuple[object, ...]: Extractor.register_traceflow('unit-trace-descriptor', ModuleDescriptor('unit_extraction_trace_mod', 'UnitTraceFlow')) + def test_register_engine_identity_guard(self) -> None: + """GitHub issue #739: re-registering the same engine class is silent. + + ``register_engine`` used a presence-only guard, so the *second* of two + calls registering the exact same class under the exact same name + warned about an overwrite that never happened. This mirrors the + identity guard GitHub issue #718 gave the code-keyed protocol + registrars (see ``test_register_stays_quiet_on_same_object_reregistration`` + in ``tests/protocols/test_protocol_base_unit.py``): the guard now + fires only when the incumbent differs from the replacement, and the + registry write happens either way. + + """ + from pcapkit.foundation.engines.engine import Engine + from pcapkit.foundation.extraction import Extractor + from pcapkit.utilities.warnings import RegistryWarning + + class UnitEngineIdentityA(Engine[str]): + __engine_name__ = 'UnitEngineIdentityA' + __engine_module__ = __name__ + + def run(self) -> None: + pass + + def read_frame(self) -> str: + return 'frame' + + class UnitEngineIdentityB(Engine[str]): + __engine_name__ = 'UnitEngineIdentityB' + __engine_module__ = __name__ + + def run(self) -> None: + pass + + def read_frame(self) -> str: + return 'frame' + + with mock.patch('pcapkit.foundation.extraction.warn'): + Extractor.register_engine('unit-engine-identity', UnitEngineIdentityA) + + # 1. Re-registering the same object is silent. + with mock.patch('pcapkit.foundation.extraction.warn') as warn: + Extractor.register_engine('unit-engine-identity', UnitEngineIdentityA) + warn.assert_not_called() + # 3. The registry write still happens even when the guard stays quiet. + self.assertIs(Extractor.__engine__['unit-engine-identity'], UnitEngineIdentityA) + + # 2. Replacing with a different object still warns, message unchanged. + with mock.patch('pcapkit.foundation.extraction.warn') as warn: + Extractor.register_engine('unit-engine-identity', UnitEngineIdentityB) + warn.assert_called_once_with( + 'engine unit-engine-identity already registered, overwriting', RegistryWarning) + # 3. And the write happens on the warning path too. + self.assertIs(Extractor.__engine__['unit-engine-identity'], UnitEngineIdentityB) + + def test_register_reassembly_identity_guard(self) -> None: + """GitHub issue #739: re-registering the same reassembly class is silent. + + Same defect and fix as ``test_register_engine_identity_guard``, for + ``register_reassembly``. + + """ + from pcapkit.foundation.extraction import Extractor + from pcapkit.foundation.reassembly.reassembly import Reassembly + from pcapkit.utilities.warnings import RegistryWarning + + with mock.patch('pcapkit.foundation.extraction.Extractor.register_reassembly'): + class UnitReassemblyIdentityA(Reassembly[object, object, tuple[str], object], + protocol='unit-reassembly-identity-a'): + def reassembly(self, info: object) -> None: + pass + + def submit(self, buf: object, **kwargs: object) -> list[object]: + return [] + + class UnitReassemblyIdentityB(Reassembly[object, object, tuple[str], object], + protocol='unit-reassembly-identity-b'): + def reassembly(self, info: object) -> None: + pass + + def submit(self, buf: object, **kwargs: object) -> list[object]: + return [] + + with mock.patch('pcapkit.foundation.extraction.warn'): + Extractor.register_reassembly('unit-reassembly-identity', UnitReassemblyIdentityA) + + # 1. Re-registering the same object is silent. + with mock.patch('pcapkit.foundation.extraction.warn') as warn: + Extractor.register_reassembly('unit-reassembly-identity', UnitReassemblyIdentityA) + warn.assert_not_called() + # 3. The registry write still happens even when the guard stays quiet. + self.assertIs(Extractor.__reassembly__['unit-reassembly-identity'], UnitReassemblyIdentityA) + + # 2. Replacing with a different object still warns, message unchanged. + with mock.patch('pcapkit.foundation.extraction.warn') as warn: + Extractor.register_reassembly('unit-reassembly-identity', UnitReassemblyIdentityB) + warn.assert_called_once_with( + 'reassembly unit-reassembly-identity already registered, overwriting', RegistryWarning) + # 3. And the write happens on the warning path too. + self.assertIs(Extractor.__reassembly__['unit-reassembly-identity'], UnitReassemblyIdentityB) + + def test_register_traceflow_identity_guard(self) -> None: + """GitHub issue #739: re-registering the same traceflow class is silent. + + Same defect and fix as ``test_register_engine_identity_guard``, for + ``register_traceflow``. + + """ + from pcapkit.foundation.extraction import Extractor + from pcapkit.foundation.traceflow.traceflow import TraceFlow + from pcapkit.utilities.warnings import RegistryWarning + + with mock.patch('pcapkit.foundation.extraction.Extractor.register_traceflow'): + class UnitTraceFlowIdentityA(TraceFlow[str, object, object, object], + protocol='unit-traceflow-identity-a'): + def dump(self, packet: object) -> None: + pass + + def trace(self, packet: object, *, output: bool = False): + return object() if output else 'trace' + + def submit(self) -> tuple[object, ...]: + return () + + class UnitTraceFlowIdentityB(TraceFlow[str, object, object, object], + protocol='unit-traceflow-identity-b'): + def dump(self, packet: object) -> None: + pass + + def trace(self, packet: object, *, output: bool = False): + return object() if output else 'trace' + + def submit(self) -> tuple[object, ...]: + return () + + with mock.patch('pcapkit.foundation.extraction.warn'): + Extractor.register_traceflow('unit-traceflow-identity', UnitTraceFlowIdentityA) + + # 1. Re-registering the same object is silent. + with mock.patch('pcapkit.foundation.extraction.warn') as warn: + Extractor.register_traceflow('unit-traceflow-identity', UnitTraceFlowIdentityA) + warn.assert_not_called() + # 3. The registry write still happens even when the guard stays quiet. + self.assertIs(Extractor.__traceflow__['unit-traceflow-identity'], UnitTraceFlowIdentityA) + + # 2. Replacing with a different object still warns, message unchanged. + with mock.patch('pcapkit.foundation.extraction.warn') as warn: + Extractor.register_traceflow('unit-traceflow-identity', UnitTraceFlowIdentityB) + warn.assert_called_once_with( + 'traceflow unit-traceflow-identity already registered, overwriting', RegistryWarning) + # 3. And the write happens on the warning path too. + self.assertIs(Extractor.__traceflow__['unit-traceflow-identity'], UnitTraceFlowIdentityB) + + def test_register_dumper_identity_guard(self) -> None: + """GitHub issue #739: re-registering the same dumper class is silent. + + Same defect and fix as ``test_register_engine_identity_guard``, for + ``register_dumper``. Unlike the other three, ``__output__`` maps each + format to a ``(dumper, ext)`` pair and is a + :class:`collections.defaultdict`, so this also pins that the + identity check compares the incumbent *dumper* (not the pair, which + would never compare equal once ``ext`` changes) and that the guard's + ``.get()`` does not trigger the default factory the way a subscript + access would. + + """ + from pcapkit.dumpkit.null import NotImplementedIO + from pcapkit.foundation.extraction import Extractor + from pcapkit.utilities.warnings import RegistryWarning + + class UnitDumperIdentityB(NotImplementedIO): + pass + + with mock.patch('pcapkit.foundation.extraction.warn'): + Extractor.register_dumper('unit-dumper-identity', NotImplementedIO, '.unit-a') + + # 1. Re-registering the same object is silent. + with mock.patch('pcapkit.foundation.extraction.warn') as warn: + Extractor.register_dumper('unit-dumper-identity', NotImplementedIO, '.unit-a') + warn.assert_not_called() + # 3. The registry write still happens even when the guard stays quiet. + self.assertEqual(Extractor.__output__['unit-dumper-identity'], (NotImplementedIO, '.unit-a')) + + # 2. Replacing with a different object still warns, message unchanged. + with mock.patch('pcapkit.foundation.extraction.warn') as warn: + Extractor.register_dumper('unit-dumper-identity', UnitDumperIdentityB, '.unit-b') + warn.assert_called_once_with( + 'dumper unit-dumper-identity already registered, overwriting', RegistryWarning) + # 3. And the write happens on the warning path too. + self.assertEqual(Extractor.__output__['unit-dumper-identity'], (UnitDumperIdentityB, '.unit-b')) + + # Same dumper, only ``ext`` changes: still identity-equal on the + # dumper, so still silent -- but the write still updates ``ext``. + with mock.patch('pcapkit.foundation.extraction.warn') as warn: + Extractor.register_dumper('unit-dumper-identity', UnitDumperIdentityB, '.unit-c') + warn.assert_not_called() + self.assertEqual(Extractor.__output__['unit-dumper-identity'], (UnitDumperIdentityB, '.unit-c')) + def test_import_test_and_make_name_paths(self) -> None: from pcapkit.foundation.extraction import Extractor from pcapkit.utilities.exceptions import FileNotFound, FormatError diff --git a/tests/foundation/traceflow/test_traceflow_base.py b/tests/foundation/traceflow/test_traceflow_base.py index 34ca2afab6..5da9a36286 100644 --- a/tests/foundation/traceflow/test_traceflow_base.py +++ b/tests/foundation/traceflow/test_traceflow_base.py @@ -111,6 +111,45 @@ def submit(self) -> tuple[DummyIndex, ...]: trace(DummyPacket(1)) self.assertEqual(trace.index[0].index, (1,)) + def test_register_dumper_identity_guard(self) -> None: + """GitHub issue #739: re-registering the same dumper class is silent. + + Same defect and fix as ``Extractor.register_dumper`` (see + ``tests/foundation/test_extraction.py``), for + ``TraceFlowBase.register_dumper``. This class keeps its own + ``__output__`` -- a separate :class:`collections.defaultdict` of + ``(dumper, ext)`` pairs from :class:`~pcapkit.foundation.extraction. + Extractor`'s -- so the same identity-on-the-dumper-not-the-pair + nuance applies here independently. + + """ + from pcapkit.dumpkit.null import NotImplementedIO + from pcapkit.foundation.traceflow.traceflow import TraceFlowBase + from pcapkit.utilities.warnings import RegistryWarning + + class UnitTraceDumperIdentityB(NotImplementedIO): + pass + + with mock.patch('pcapkit.foundation.traceflow.traceflow.warn'): + TraceFlowBase.register_dumper('unit-trace-dumper-identity', NotImplementedIO, '.unit-a') + + # 1. Re-registering the same object is silent. + with mock.patch('pcapkit.foundation.traceflow.traceflow.warn') as warn: + TraceFlowBase.register_dumper('unit-trace-dumper-identity', NotImplementedIO, '.unit-a') + warn.assert_not_called() + # 3. The registry write still happens even when the guard stays quiet. + self.assertEqual(TraceFlowBase.__output__['unit-trace-dumper-identity'], + (NotImplementedIO, '.unit-a')) + + # 2. Replacing with a different object still warns, message unchanged. + with mock.patch('pcapkit.foundation.traceflow.traceflow.warn') as warn: + TraceFlowBase.register_dumper('unit-trace-dumper-identity', UnitTraceDumperIdentityB, '.unit-b') + warn.assert_called_once_with( + 'dumper unit-trace-dumper-identity already registered, overwriting', RegistryWarning) + # 3. And the write happens on the warning path too. + self.assertEqual(TraceFlowBase.__output__['unit-trace-dumper-identity'], + (UnitTraceDumperIdentityB, '.unit-b')) + def test_register_callback_and_subclass_registration(self) -> None: from pcapkit.corekit.infoclass import Info, info_final from pcapkit.foundation.traceflow.traceflow import TraceFlow, TraceFlowBase