From 711141b3af98216949720ec42f37977da9c8d2dd Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sat, 31 Oct 2026 17:04:21 -0400 Subject: [PATCH] fix(internet): IGMPv1 narrows id()/alias to the RFC canonical name Follow-up to the owner's ruling on this fix: IGMPv1 (RFC 1112) should take the same shape as ICMPv4 (PR #2446) -- its RFC's own name carries no version, so id() and alias both drop it, rather than narrowing to a versioned 'IGMPv1' name that RFC 1112 never uses. - IGMPv1.id() -> ('IGMP',), IGMPv1.alias -> 'IGMP'. - IGMPv2.id() -> ('IGMPv2',) and IGMPv2.alias -> 'IGMPv2', unchanged: RFC 2236 names IGMPv2 with its own version, so it keeps the shape from the first round of this fix. - IGMP.id() (the family base) -> ('IGMP', 'IGMPv2'), the family-inclusive shape, since IGMPv1's canonical name and the family's own name are now the same string -- the same shape ICMP.id() takes, ('ICMP', 'ICMPv6'). - igmp.py's shared read() error message kept the version-specific 'is not an IGMPv1/IGMPv2 message' wording explicit, decoupled from alias, so the error stays as informative as before despite the alias change. Selector matching for the bare 'IGMP' and 'IGMPv1' strings is unaffected -- 'IGMPv1' still has its own registry key independent of id(), the same mechanism ICMPv4 relies on for 'ICMPv4'. Promoted the ICMPv4 doc text from 'a proposal, not a ruling' to a settled ruling (PR #2446 shipped exactly this shape), and recorded IGMPv1's matching case alongside it. Verified: tests/protocols/internet/test_igmp_unit.py (39 tests), test_dispatch_registry_unit.py, test_conventions_doc_claims.py and test_public_api.py all pass clean (91 tests, 1163 subtests). --- .../conventions/protocol-family-grouping.rst | 27 ++++++++++---- pcapkit/protocols/internet/igmp.py | 18 ++++++---- pcapkit/protocols/internet/igmpv1.py | 16 +++++++-- pcapkit/protocols/internet/igmpv2.py | 13 +++++-- tests/protocols/internet/test_igmp_unit.py | 35 ++++++++++++------- 5 files changed, 78 insertions(+), 31 deletions(-) diff --git a/docs/source/contributing/conventions/protocol-family-grouping.rst b/docs/source/contributing/conventions/protocol-family-grouping.rst index 0f82014671..d9efa64218 100644 --- a/docs/source/contributing/conventions/protocol-family-grouping.rst +++ b/docs/source/contributing/conventions/protocol-family-grouping.rst @@ -65,7 +65,11 @@ Member Shapes * - Narrow to self - the member's own name only - :class:`~pcapkit.protocols.internet.ah.AH` ``('AH',)``, - :class:`~pcapkit.protocols.internet.esp.ESP` ``('ESP',)`` + :class:`~pcapkit.protocols.internet.esp.ESP` ``('ESP',)``; + :class:`IGMPv2 ` ``('IGMPv2',)`` -- + :class:`IGMPv1 ` narrows the same way, + but to the RFC canonical name rather than a versioned one + (:ref:`protocol-family-canonical-names`) * - Family-inclusive - the member's label plus the family name - :class:`~pcapkit.protocols.link.c_tag.C_Tag` ``('C_Tag', 'VLAN')``, @@ -108,11 +112,22 @@ preference on :pr:`2444`, not a settled rule. ``name`` of their own, so they report ``Hypertext Transfer Protocol``, which carries no version. -**ICMPv4 is the case where the RFC's own name carries no version.** The owner's -reply on :pr:`2446` proposes that ``ICMPv4`` keeps its versioned class and module name, -that its ``id()`` should probably drop the version for the RFC canonical name, and -that its ``name`` carries no "version 4", following RFC 792. It is a proposal, not a -ruling. +**ICMPv4 is the case where the RFC's own name carries no version.** The ruling on +:pr:`2446`: ``ICMPv4`` keeps its versioned class and module name, but its ``id()`` +drops the version for the RFC canonical name (:rfc:`792` names it plain ``ICMP``), +and its ``name`` carries no "version 4". ``ICMPv6`` keeps its own version, since +:rfc:`4443` names it with one. + +**IGMPv1 follows the same shape, by the same reasoning** (:issue:`2439`, :pr:`2506`). +:rfc:`1112` names IGMPv1's protocol plain ``IGMP``, so +:meth:`IGMPv1.id() ` is ``('IGMP',)`` and +its ``alias`` is ``'IGMP'``, while +:meth:`IGMPv2.id() ` keeps its own version, +``('IGMPv2',)``, since :rfc:`2236` names it with one. The family base's ``id()`` +follows: :meth:`IGMP.id() ` is +``('IGMP', 'IGMPv2')``, the family-inclusive shape, because IGMPv1's canonical name +and the family's own name are the same string -- the same shape +:meth:`ICMP.id() ` takes, ``('ICMP', 'ICMPv6')``. .. _protocol-family-selectors: diff --git a/pcapkit/protocols/internet/igmp.py b/pcapkit/protocols/internet/igmp.py index 06ff670af5..8607853e4f 100644 --- a/pcapkit/protocols/internet/igmp.py +++ b/pcapkit/protocols/internet/igmp.py @@ -68,9 +68,9 @@ def version(self) -> 'Literal[1, 2]': """IGMP version this class reads: ``1`` or ``2``.""" @property + @abc.abstractmethod def alias(self) -> 'str': - """Acronym of corresponding protocol, ``'IGMPv1'`` or ``'IGMPv2'``.""" - return f'IGMPv{self.version}' + """Acronym of corresponding protocol, ``'IGMP'`` or ``'IGMPv2'``.""" @property def length(self) -> 'Literal[8]': @@ -82,15 +82,19 @@ def length(self) -> 'Literal[8]': ########################################################################## @classmethod - def id(cls) -> 'tuple[Literal["IGMP"], Literal["IGMPv1"], Literal["IGMPv2"]]': + def id(cls) -> 'tuple[Literal["IGMP"], Literal["IGMPv2"]]': """Index ID of the protocol. + :rfc:`1112` names the protocol ``IGMP``, without a version, the same + shape :class:`~pcapkit.protocols.internet.icmp.ICMP` takes for + :rfc:`792` -- so the family base's own name doubles as IGMPv1's + canonical name. :rfc:`2236` names IGMPv2 with its own version. + Returns: - Index ID of the protocol, the family name and both versions; the - concrete IGMPv1 and IGMPv2 classes narrow it to their own version. + Index ID of the protocol. """ - return ('IGMP', 'IGMPv1', 'IGMPv2') + return ('IGMP', 'IGMPv2') def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> '_PT': """Read Internet Group Management Protocol. @@ -112,7 +116,7 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> '_PT': schema = cast('Schema_IGMP', self.__header__) if self._classify(schema.type, schema.max_resp_time, length) != self.version: - raise ProtocolError(f'{self.alias}: {schema.type!r} is not an {self.alias} ' + raise ProtocolError(f'{self.alias}: {schema.type!r} is not an IGMPv{self.version} ' f'message ({length} octets)') igmp = Data_IGMP( diff --git a/pcapkit/protocols/internet/igmpv1.py b/pcapkit/protocols/internet/igmpv1.py index 0ed7630223..86c33391d2 100644 --- a/pcapkit/protocols/internet/igmpv1.py +++ b/pcapkit/protocols/internet/igmpv1.py @@ -38,16 +38,26 @@ def version(self) -> 'Literal[1]': """IGMP version this class reads: ``1``.""" return 1 + @property + def alias(self) -> 'Literal["IGMP"]': + """Acronym of corresponding protocol.""" + return 'IGMP' + ########################################################################## # Methods. ########################################################################## @classmethod - def id(cls) -> 'tuple[Literal["IGMP"], Literal["IGMPv1"]]': # type: ignore[override] + def id(cls) -> 'tuple[Literal["IGMP"]]': # type: ignore[override] """Index ID of the protocol. + :rfc:`1112` names the protocol ``IGMP``, without a version. + + String ``==`` compares against ``(cls, *id())``, so ``== 'IGMPv1'`` is + :data:`False`, while ``'IGMPv1' in`` matches by class. + Returns: - Index ID of the protocol, the family name and the version. + Index ID of the protocol. """ - return ('IGMP', 'IGMPv1') + return ('IGMP',) diff --git a/pcapkit/protocols/internet/igmpv2.py b/pcapkit/protocols/internet/igmpv2.py index 5a07e5cfdb..e8ec7658c3 100644 --- a/pcapkit/protocols/internet/igmpv2.py +++ b/pcapkit/protocols/internet/igmpv2.py @@ -38,16 +38,23 @@ def version(self) -> 'Literal[2]': """IGMP version this class reads: ``2``.""" return 2 + @property + def alias(self) -> 'Literal["IGMPv2"]': + """Acronym of corresponding protocol.""" + return 'IGMPv2' + ########################################################################## # Methods. ########################################################################## @classmethod - def id(cls) -> 'tuple[Literal["IGMP"], Literal["IGMPv2"]]': # type: ignore[override] + def id(cls) -> 'tuple[Literal["IGMPv2"]]': # type: ignore[override] """Index ID of the protocol. + :rfc:`2236` names the protocol ``IGMPv2``, carrying its own version. + Returns: - Index ID of the protocol, the family name and the version. + Index ID of the protocol. """ - return ('IGMP', 'IGMPv2') + return ('IGMPv2',) diff --git a/tests/protocols/internet/test_igmp_unit.py b/tests/protocols/internet/test_igmp_unit.py index 391a59d98d..fedc9ecff0 100644 --- a/tests/protocols/internet/test_igmp_unit.py +++ b/tests/protocols/internet/test_igmp_unit.py @@ -60,6 +60,16 @@ def igmp_class(version: 'str'): return igmpv1.IGMP if version == 'IGMPv1' else igmpv2.IGMP +def expected_alias(version: 'str') -> 'str': + """The alias a parsed layer reports for ``version``. + + :rfc:`1112` names the protocol ``IGMP``, without a version, so IGMPv1's + alias is the bare family name; :rfc:`2236` names IGMPv2 with its own + version, so IGMPv2's alias carries it. + """ + return 'IGMP' if version == 'IGMPv1' else version + + #: One fixture per message type IGMPv1 and IGMPv2 define, each a correctly #: checksummed 8-octet message, with the version it is read as. Type #: octets per :rfc:`1112#page-11` (``0x11``/``0x12``, v1) and @@ -268,7 +278,7 @@ def test_alias_names_the_version_read(self) -> None: with self.subTest(packet=name): parsed = igmp_class(version)(io.BytesIO(packet), len(packet)) self.assertIsInstance(parsed, IGMPBase) - self.assertEqual(parsed.alias, version) + self.assertEqual(parsed.alias, expected_alias(version)) def test_refusals_name_the_version_refused(self) -> None: from pcapkit.protocols.internet.igmpv1 import IGMP as IGMPv1 @@ -276,7 +286,7 @@ def test_refusals_name_the_version_refused(self) -> None: from pcapkit.utilities.exceptions import ProtocolError v1_report, v2_report = IGMP_PACKETS['v1-report'][0], IGMP_PACKETS['v2-report'][0] - with self.assertRaisesRegex(ProtocolError, r'^IGMPv1: .+ is not an IGMPv1 message \(8 octets\)'): + with self.assertRaisesRegex(ProtocolError, r'^IGMP: .+ is not an IGMPv1 message \(8 octets\)'): IGMPv1(io.BytesIO(v2_report), len(v2_report)) with self.assertRaisesRegex(ProtocolError, r'^IGMPv2: .+ is not an IGMPv2 message \(8 octets\)'): IGMPv2(io.BytesIO(v1_report), len(v1_report)) @@ -286,7 +296,7 @@ def test_membership_query_max_resp_time_zero_is_v1(self) -> None: packet = igmp_packet(0x11, 0, '0.0.0.0') parsed = IGMP(io.BytesIO(packet), len(packet)) - self.assertEqual(parsed.alias, 'IGMPv1') + self.assertEqual(parsed.alias, 'IGMP') def test_membership_query_max_resp_time_nonzero_is_v2(self) -> None: from pcapkit.protocols.internet.igmpv2 import IGMP @@ -340,21 +350,22 @@ def test_id_and_version(self) -> None: from pcapkit.protocols.internet.igmpv1 import IGMP as IGMPv1 from pcapkit.protocols.internet.igmpv2 import IGMP as IGMPv2 - self.assertEqual(IGMPv1.id(), ('IGMP', 'IGMPv1')) - self.assertEqual(IGMPv2.id(), ('IGMP', 'IGMPv2')) + self.assertEqual(IGMPv1.id(), ('IGMP',)) + self.assertEqual(IGMPv2.id(), ('IGMPv2',)) self.assertEqual(object.__new__(IGMPv1).version, 1) self.assertEqual(object.__new__(IGMPv2).version, 2) def test_base_id_covers_both_versions(self) -> None: from pcapkit.protocols.internet.igmp import IGMP - self.assertEqual(IGMP.id(), ('IGMP', 'IGMPv1', 'IGMPv2')) + self.assertEqual(IGMP.id(), ('IGMP', 'IGMPv2')) def test_each_version_narrows_the_base_id(self) -> None: v1, v2 = igmp_class('IGMPv1'), igmp_class('IGMPv2') - self.assertEqual(v1.id(), ('IGMP', 'IGMPv1')) - self.assertEqual(v2.id(), ('IGMP', 'IGMPv2')) + self.assertEqual(v1.id(), ('IGMP',)) + self.assertEqual(v2.id(), ('IGMPv2',)) self.assertNotIn('IGMPv2', v1.id()) + self.assertNotIn('IGMPv1', v1.id()) self.assertNotIn('IGMPv1', v2.id()) def test_igmpv1_parses_its_own_messages_directly(self) -> None: @@ -364,7 +375,7 @@ def test_igmpv1_parses_its_own_messages_directly(self) -> None: packet, _version = IGMP_PACKETS[name] with self.subTest(packet=name): parsed = IGMPv1(io.BytesIO(packet), len(packet)) - self.assertEqual(parsed.alias, 'IGMPv1') + self.assertEqual(parsed.alias, 'IGMP') def test_igmpv1_rejects_a_v2_only_message(self) -> None: from pcapkit.protocols.internet.igmpv1 import IGMP as IGMPv1 @@ -448,7 +459,7 @@ def test_versions_take_the_length_from_the_data_when_none_is_given(self) -> None packet, version = IGMP_PACKETS[name] with self.subTest(version=version): parsed = cls(packet) - self.assertEqual(parsed.alias, version) + self.assertEqual(parsed.alias, expected_alias(version)) self.assertEqual(len(parsed), 8) with warnings.catch_warnings(): @@ -556,7 +567,7 @@ def test_max_resp_time_bounds_round_trip(self) -> None: parsed = cls(io.BytesIO(packet), len(packet)) self.assertEqual(packet[1], value) self.assertEqual(parsed.info.max_resp_time, value) - self.assertEqual(parsed.alias, version) + self.assertEqual(parsed.alias, expected_alias(version)) self.assertEqual(cls.from_data(parsed.info).data, packet) @@ -607,7 +618,7 @@ def test_ipv4_dispatches_igmp_to_each_version(self) -> None: parsed = IPv4(io.BytesIO(outer), len(outer)) expected = IGMPv1 if version == 'IGMPv1' else IGMPv2 self.assertIsInstance(parsed.payload, expected) - self.assertEqual(parsed.payload.alias, version) + self.assertEqual(parsed.payload.alias, expected_alias(version)) if __name__ == '__main__':