diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index e69a95fad5..fd0bf6aa36 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -450,15 +450,15 @@ def __new__(cls, *args: '_VT', **kwargs: '_VT') -> 'Self': # pylint: disable=un ``__dict__`` without :attr:`~pcapkit.corekit.infoclass.FinalisedState.FINAL`. - Warning: - Out of scope, deliberately -- see - :meth:`pcapkit.corekit.infoclass.Info.__new__`, which documents the - identical gap: a ``@final`` class descending from a + Note: + Whether ``cls`` still needs finalising is read from its *own* + ``__dict__``, never inherited (GitHub issue :issue:`1498`). Each + class is finalised on its first construction, whatever its + ancestors' state, so a subclass of a :attr:`~pcapkit.corekit.infoclass.FinalisedState.BASE` ancestor - inherits ``BASE`` rather than ``NONE`` and so never reaches the - branch below at all, regardless of its own marker. No ``BASE``-state - schema in the tree is marked ``@final`` by hand, so the escape is - theoretical. + still gets its own ``__builtin__``, ``__excluded__`` and generated + ``__init__``, and a ``@final`` class under a ``BASE`` ancestor is + refused like any other. """ # NOTE: first instantiation is the earliest point a bare ``@final`` can @@ -466,10 +466,21 @@ def __new__(cls, *args: '_VT', **kwargs: '_VT') -> 'Self': # pylint: disable=un # :meth:`__init_subclass__` has already returned. Nested inside the NONE # branch, which is already one-shot per class, so a finalised schema pays # nothing for it. See :meth:`pcapkit.corekit.infoclass.Info.__new__`, - # which carries the full reasoning and the measurement; on this side the - # missing generated ``__init__`` is the one built from - # :attr:`__fields__`. - if cls.__finalised__ == FinalisedState.NONE: + # which carries the full reasoning; on this side the missing generated + # ``__init__`` is the one built from :attr:`__fields__`. + # + # Both lookups read ``cls.__dict__`` rather than ``getattr``, because + # both markers are ordinary, inherited class attributes. The outer one: + # :func:`schema_final` writes ``BASE`` onto the class it finalises, and a + # subclass reading that state through inheritance was never finalised + # itself -- it would keep the ``__excluded__`` it copied at declaration, + # without the names :func:`schema_final` adds, and leak ``__map__``, + # ``__map_reverse__``, ``__buffer__`` and ``__updated__`` from + # :meth:`to_dict` (GitHub issue #1498). The inner one: an ancestor + # hand-marked bare ``@final`` passes its marker to every subclass + # declared before it landed, and ``getattr`` would blame such a subclass + # for a mismarking that is its ancestor's. + if cls.__dict__.get('__finalised__', FinalisedState.NONE) == FinalisedState.NONE: if cls.__dict__.get('__final__'): raise SchemaError(f'{cls.__name__}: marked final but never finalised, so it has no generated ' '__init__; apply schema_final, which applies final itself, not final alone') diff --git a/tests/protocols/schema/test_schema_child_finalisation_1498_unit.py b/tests/protocols/schema/test_schema_child_finalisation_1498_unit.py new file mode 100644 index 0000000000..8769f831f2 --- /dev/null +++ b/tests/protocols/schema/test_schema_child_finalisation_1498_unit.py @@ -0,0 +1,146 @@ +# -*- coding: utf-8 -*- +"""GitHub issue #1498: each non-final :class:`~pcapkit.protocols.schema.schema.Schema` +subclass is finalised on its own state, whatever was built before it. + +:meth:`Schema.__new__ ` used to +read ``__finalised__`` through inheritance, so once a non-final parent had been +built its non-final children read the parent's ``BASE`` and were never +finalised. They kept the ``__excluded__`` copied at declaration, and +``to_dict()`` then included ``__map__``, ``__map_reverse__``, ``__buffer__`` and +``__updated__``. The Quick-Start option schemas of HOPOPT, IPv6-Opts and IPv4 +are the shipped instances: each is a non-final subclass of a non-final +``Option``. + +The defect depends on process-wide state -- which class was built first -- so +every order runs in a fresh interpreter, started from the repository root so it +imports the tree under test; the child reports ``pcapkit.__file__`` and that is +asserted too. + +""" + +import json +import os +import subprocess # nosec: B404 +import sys +import unittest + +ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))) + +#: Parent, child, and the octets each is unpacked from; ``synthetic`` is declared +#: in the script. The shipped octets are a Quick-Start option, which both the +#: generic ``Option`` and the Quick-Start schema can read. +MODELS = { + 'synthetic': ('', 'Parent', 'Child', '01', '0102'), + 'hopopt.QuickStartOption': ('pcapkit.protocols.schema.internet.hopopt', 'Option', 'QuickStartOption', + '2606000000000000', '2606000000000000'), + 'ipv6_opts.QuickStartOption': ('pcapkit.protocols.schema.internet.ipv6_opts', 'Option', 'QuickStartOption', + '2606000000000000', '2606000000000000'), + 'ipv4.QSOption': ('pcapkit.protocols.schema.internet.ipv4', 'Option', 'QSOption', + '1906000000000000', '1906000000000000'), +} + +SCRIPT = r''' +import importlib, json, sys +import pcapkit +from pcapkit.corekit.fields.numbers import UInt8Field +from pcapkit.protocols.schema.schema import Schema + +module, parent, child, pdata, cdata, order = sys.argv[1:] +if module: + ns = vars(importlib.import_module(module)) +else: + class Parent(Schema): + a: 'int' = UInt8Field() + class Child(Parent): + b: 'int' = UInt8Field() + ns = {'Parent': Parent, 'Child': Child} + +data = {parent: bytes.fromhex(pdata), child: bytes.fromhex(cdata)} +result = {'file': pcapkit.__file__} +for name in ((parent, child) if order == 'parent-first' else (child, parent)): + cls = ns[name] + schema = cls.unpack(data[name]) + result[name] = {'expected': list(cls.__fields__), 'to_dict': list(schema.to_dict()), + 'len': len(schema), 'iter': list(schema)} +print(json.dumps(result)) +''' + + +def build(model: 'str', order: 'str') -> 'dict': + """Build ``model``'s parent and child in ``order``, in a fresh interpreter.""" + module, parent, child, pdata, cdata = MODELS[model] + proc = subprocess.run([sys.executable, '-c', SCRIPT, module, parent, child, pdata, cdata, order], # nosec: B603 + cwd=ROOT, capture_output=True, text=True, timeout=120, check=False) + if proc.returncode: + raise AssertionError(proc.stderr) + return json.loads(proc.stdout.splitlines()[-1]) + + +class SchemaChildFinalisationTests(unittest.TestCase): + """``to_dict()``, ``len()`` and iteration are right in both construction orders.""" + + def test_both_orders(self) -> None: + for model, (_, parent, child, _, _) in MODELS.items(): + for order in ('parent-first', 'child-first'): + with self.subTest(model=model, order=order): + result = build(model, order) + self.assertEqual(os.path.dirname(os.path.dirname(result['file'])), ROOT) + for name in (parent, child): + got = result[name] + self.assertEqual(got['to_dict'], got['expected'], name) + self.assertEqual(got['iter'], got['expected'], name) + self.assertEqual(got['len'], len(got['expected']), name) + + def test_a_child_declared_after_its_parent_was_built(self) -> None: + """The other side of the inheritance: declared *after* the parent's ``BASE``.""" + from pcapkit.corekit.fields.numbers import UInt8Field + from pcapkit.corekit.infoclass import FinalisedState + from pcapkit.protocols.schema.schema import Schema + + class Parent(Schema): + a: 'int' = UInt8Field() + + Parent(a=1) + self.assertEqual(Parent.__dict__.get('__finalised__'), FinalisedState.BASE) + + class Child(Parent): + b: 'int' = UInt8Field() + + def method(self) -> 'None': + """A name of its own, which ``__builtin__`` must also cover.""" + + child = Child(1, 2) + self.assertEqual(Child.__dict__.get('__finalised__'), FinalisedState.BASE) + self.assertIsNot(Child.__init__, Parent.__init__) + self.assertEqual(child.to_dict(), {'a': 1, 'b': 2}) + self.assertEqual((len(child), list(child)), (2, ['a', 'b'])) + self.assertIn('method', Child.__builtin__) + + def test_a_final_schema_under_a_base_state_ancestor_is_refused(self) -> None: + """A bare ``@final`` no longer escapes the guard by reading ``BASE`` off its parent.""" + from pcapkit.corekit.fields.numbers import UInt8Field + from pcapkit.corekit.infoclass import FinalisedState + from pcapkit.protocols.schema.schema import Schema + from pcapkit.utilities.compat import final + from pcapkit.utilities.exceptions import SchemaError + + class Parent(Schema): + a: 'int' = UInt8Field() + + Parent(a=1) + self.assertEqual(Parent.__dict__.get('__finalised__'), FinalisedState.BASE) + + @final + class Mismarked(Parent): + pass + + self.assertIn('__final__', Mismarked.__dict__) + self.assertNotIn('__finalised__', Mismarked.__dict__) + + with self.assertRaises(SchemaError) as caught: + Mismarked(a=1) + self.assertIn('Mismarked', str(caught.exception)) + + +if __name__ == '__main__': + unittest.main()