diff --git a/docs/source/pcapkit/corekit/chardet.rst b/docs/source/pcapkit/corekit/chardet.rst new file mode 100644 index 0000000000..a7de78a00b --- /dev/null +++ b/docs/source/pcapkit/corekit/chardet.rst @@ -0,0 +1,18 @@ +Character Set Detection +======================= + +.. module:: pcapkit.corekit.chardet + +:mod:`pcapkit.corekit.chardet` wraps `chardet`_ with a bounded cache, for +turning the bytes of a text field into a :obj:`str`. It is shared by +:meth:`StringField.post_process +` and +:meth:`ProtocolBase.decode `, +which is why it sits directly under :mod:`pcapkit.corekit` -- below both of +them -- rather than in :mod:`pcapkit.corekit.fields`. + +.. _chardet: https://chardet.readthedocs.io + +.. autofunction:: pcapkit.corekit.chardet.detect + +.. autodata:: pcapkit.corekit.chardet.DETECT_CACHE_SIZE diff --git a/docs/source/pcapkit/corekit/index.rst b/docs/source/pcapkit/corekit/index.rst index 6ccbad3b7e..d26a0fefc6 100644 --- a/docs/source/pcapkit/corekit/index.rst +++ b/docs/source/pcapkit/corekit/index.rst @@ -19,6 +19,7 @@ inherits from. :maxdepth: 2 fields/index + chardet context enum infoclass diff --git a/docs/source/pcapkit/utilities/chardet.rst b/docs/source/pcapkit/utilities/chardet.rst deleted file mode 100644 index 86e253b162..0000000000 --- a/docs/source/pcapkit/utilities/chardet.rst +++ /dev/null @@ -1,17 +0,0 @@ -Character Set Detection -======================= - -.. module:: pcapkit.utilities.chardet - -:mod:`pcapkit.utilities.chardet` wraps `chardet`_ with a bounded cache, for -turning the bytes of a text field into a :obj:`str`. It is shared by -:meth:`StringField.post_process -` and -:meth:`ProtocolBase.decode `, -which is why it lives here rather than beside either of them. - -.. _chardet: https://chardet.readthedocs.io - -.. autofunction:: pcapkit.utilities.chardet.detect - -.. autodata:: pcapkit.utilities.chardet.DETECT_CACHE_SIZE diff --git a/docs/source/pcapkit/utilities/index.rst b/docs/source/pcapkit/utilities/index.rst index 7d22240b4d..a5e2426c7f 100644 --- a/docs/source/pcapkit/utilities/index.rst +++ b/docs/source/pcapkit/utilities/index.rst @@ -17,7 +17,6 @@ several user-refined exceptions and warnings. exceptions warnings logging - chardet Version Compatibility ===================== diff --git a/pcapkit/all.py b/pcapkit/all.py index 8575a83ea1..5e67366d5f 100644 --- a/pcapkit/all.py +++ b/pcapkit/all.py @@ -143,7 +143,7 @@ # Scapy engine # pcapkit.utilities -- unlisted: its names stay reachable as attributes, but - # ``import *`` leaves out generic helpers such as ``warn``, ``reset``, - # ``configure`` and ``detect`` + # ``import *`` leaves out generic helpers such as ``warn``, ``reset`` and + # ``configure`` #'beholder_ng', 'seekset_ng', # Decorators ] diff --git a/pcapkit/utilities/chardet.py b/pcapkit/corekit/chardet.py similarity index 95% rename from pcapkit/utilities/chardet.py rename to pcapkit/corekit/chardet.py index a8cd7c67c3..61869d113d 100644 --- a/pcapkit/utilities/chardet.py +++ b/pcapkit/corekit/chardet.py @@ -2,9 +2,9 @@ """Character Set Detection ============================= -.. module:: pcapkit.utilities.chardet +.. module:: pcapkit.corekit.chardet -:mod:`pcapkit.utilities.chardet` wraps `chardet`_ with a bounded cache, for +:mod:`pcapkit.corekit.chardet` wraps `chardet`_ with a bounded cache, for turning the bytes of a text field into a :obj:`str`. .. _chardet: https://chardet.readthedocs.io diff --git a/pcapkit/corekit/fields/strings.py b/pcapkit/corekit/fields/strings.py index b1efac3fac..060836aebb 100644 --- a/pcapkit/corekit/fields/strings.py +++ b/pcapkit/corekit/fields/strings.py @@ -4,8 +4,8 @@ import urllib.parse as urllib_parse from typing import TYPE_CHECKING, Any, Generic, TypeVar +from pcapkit.corekit.chardet import detect from pcapkit.corekit.fields.field import NO_VALUE, Field -from pcapkit.utilities.chardet import detect from pcapkit.utilities.compat import Dict, unquote from pcapkit.utilities.exceptions import FieldValueError diff --git a/pcapkit/protocols/application/ftp.py b/pcapkit/protocols/application/ftp.py index 3cc561c1db..a208d43548 100644 --- a/pcapkit/protocols/application/ftp.py +++ b/pcapkit/protocols/application/ftp.py @@ -19,6 +19,7 @@ from pcapkit.const.ftp.command import Command as Enum_Command from pcapkit.const.ftp.return_code import ReturnCode as Enum_ReturnCode +from pcapkit.corekit.chardet import detect from pcapkit.corekit.enum import EnumLookup from pcapkit.protocols.application.application import Application from pcapkit.protocols.data.application.ftp import FTP as Data_FTP @@ -26,7 +27,6 @@ from pcapkit.protocols.data.application.ftp import Response as Data_Response from pcapkit.protocols.misc.raw import Raw from pcapkit.protocols.schema.application.ftp import FTP as Schema_FTP -from pcapkit.utilities.chardet import detect from pcapkit.utilities.compat import StrEnum, auto from pcapkit.utilities.exceptions import ProtocolError, UnsupportedCall diff --git a/pcapkit/protocols/application/httpv1.py b/pcapkit/protocols/application/httpv1.py index 1eaf70c148..43a52794a0 100644 --- a/pcapkit/protocols/application/httpv1.py +++ b/pcapkit/protocols/application/httpv1.py @@ -32,6 +32,7 @@ from pcapkit.const.http.method import Method as Enum_Method from pcapkit.const.http.status_code import StatusCode as Enum_StatusCode +from pcapkit.corekit.chardet import detect from pcapkit.corekit.enum import EnumLookup from pcapkit.corekit.multidict import OrderedMultiDict from pcapkit.protocols.application.http import (_HTTP2_PREFACE_HEADER, _RE_METHOD, _RE_STATUS, @@ -41,7 +42,6 @@ from pcapkit.protocols.data.application.httpv1 import RequestHeader as Data_RequestHeader from pcapkit.protocols.data.application.httpv1 import ResponseHeader as Data_ResponseHeader from pcapkit.protocols.schema.application.httpv1 import HTTP as Schema_HTTP -from pcapkit.utilities.chardet import detect from pcapkit.utilities.compat import StrEnum, auto from pcapkit.utilities.exceptions import ProtocolError diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index a600d6a5bb..a702680e50 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -33,6 +33,7 @@ import aenum +from pcapkit.corekit.chardet import detect from pcapkit.corekit.context import ContextRegistry from pcapkit.corekit.fields.misc import ConditionalField, SchemaField, SwitchField from pcapkit.corekit.module import ModuleDescriptor @@ -45,7 +46,6 @@ from pcapkit.protocols.data.protocol import Packet as Data_Packet from pcapkit.protocols.schema.misc.raw import Raw as Schema_Raw from pcapkit.protocols.schema.schema import Schema -from pcapkit.utilities.chardet import detect from pcapkit.utilities.compat import cached_property from pcapkit.utilities.decorators import beholder, seekset from pcapkit.utilities.exceptions import (BaseError, ProtocolError, ProtocolNotFound, diff --git a/pcapkit/utilities/__init__.py b/pcapkit/utilities/__init__.py index bd1169d2a2..57c0494123 100644 --- a/pcapkit/utilities/__init__.py +++ b/pcapkit/utilities/__init__.py @@ -12,12 +12,11 @@ several user-refined exceptions and warnings. """ -from pcapkit.utilities.chardet import detect from pcapkit.utilities.decorators import beholder, prepare, seekset from pcapkit.utilities.exceptions import stacklevel from pcapkit.utilities.logging import configure, ensure_output, get_logger, logger, reset from pcapkit.utilities.warnings import warn __all__ = ['logger', 'get_logger', 'configure', 'reset', 'ensure_output', - 'warn', 'stacklevel', 'detect', + 'warn', 'stacklevel', 'beholder', 'prepare', 'seekset'] diff --git a/tests/corekit/test_chardet_housing_1712_unit.py b/tests/corekit/test_chardet_housing_1712_unit.py new file mode 100644 index 0000000000..a05480950a --- /dev/null +++ b/tests/corekit/test_chardet_housing_1712_unit.py @@ -0,0 +1,122 @@ +# -*- coding: utf-8 -*- +"""GitHub issue #1712: the bounded-cache :mod:`chardet` wrapper lives in corekit. + +:func:`~pcapkit.corekit.chardet.detect` and +:data:`~pcapkit.corekit.chardet.DETECT_CACHE_SIZE` moved from +``pcapkit.utilities.chardet`` to :mod:`pcapkit.corekit.chardet`, and the +``pcapkit.utilities.detect`` re-export went with them: the owner's ruling on +#1715, applied to #1712, is that a move updates every import path directly and +leaves no re-export behind. This pins that the old path no longer imports, that +every caller uses the new module, and that the move adds no import cycle -- the +new module imports nothing from :mod:`pcapkit`, and :mod:`pcapkit.utilities` +still imports nothing from :mod:`pcapkit.corekit`. + +""" + +import ast +import importlib +import json +import pathlib +import subprocess # nosec: B404 +import sys +import unittest + +from tests._support import reimport_once_per_class +from tests.corekit._roundtrip import skip_without_runtime + +ROOT = pathlib.Path(__file__).resolve().parents[2] + +NEW_PATH = 'pcapkit.corekit.chardet' + +#: Where the wrapper lived before #1712; removed, not re-exported. +OLD_PATH = 'pcapkit.utilities.chardet' + +#: Every module that calls :func:`~pcapkit.corekit.chardet.detect`. +CALLERS = ( + 'pcapkit.corekit.fields.strings', + 'pcapkit.protocols.protocol', + 'pcapkit.protocols.application.ftp', + 'pcapkit.protocols.application.httpv1', +) + + +def imported_modules(path: 'pathlib.Path') -> 'list[str]': + """Every module ``path`` imports, read with :mod:`ast` rather than imported.""" + names = [] + for node in ast.walk(ast.parse(path.read_text(encoding='utf-8'))): + if isinstance(node, ast.Import): + names.extend(alias.name for alias in node.names) + elif isinstance(node, ast.ImportFrom): + names.append('.' * node.level + (node.module or '')) + return names + + +class ChardetHousingTests(unittest.TestCase): + """The wrapper is at :data:`NEW_PATH` and nowhere else.""" + + def setUp(self) -> None: + skip_without_runtime(self) + reimport_once_per_class(self) + + def test_the_old_import_path_no_longer_imports(self) -> None: + """No re-export is left at ``pcapkit.utilities.chardet``.""" + with self.assertRaises(ModuleNotFoundError) as ctx: + importlib.import_module(OLD_PATH) + self.assertEqual(ctx.exception.name, OLD_PATH) + + def test_detect_is_no_longer_reexported(self) -> None: + """``detect`` is gone from :mod:`pcapkit.utilities`, and so from :mod:`pcapkit.all`.""" + for package in ('pcapkit.utilities', 'pcapkit.all'): + with self.subTest(package=package): + module = importlib.import_module(package) + self.assertNotIn('detect', getattr(module, '__all__', ())) + self.assertFalse(hasattr(module, 'detect')) + + def test_the_wrapper_keeps_its_bound(self) -> None: + """The cache stays bounded at :data:`~pcapkit.corekit.chardet.DETECT_CACHE_SIZE`.""" + module = importlib.import_module(NEW_PATH) + self.assertEqual(module.__all__, ['detect']) + self.assertEqual(module.DETECT_CACHE_SIZE, 1024) + self.assertEqual(module.detect.cache_info().maxsize, module.DETECT_CACHE_SIZE) + + def test_every_caller_uses_the_new_module(self) -> None: + """Each caller holds the one cached function, so they share its cache.""" + detect = importlib.import_module(NEW_PATH).detect + for name in CALLERS: + with self.subTest(module=name): + self.assertIs(importlib.import_module(name).detect, detect) + + +class ChardetLayeringTests(unittest.TestCase): + """The move adds no edge that could close an import cycle.""" + + def test_the_new_module_imports_nothing_from_pcapkit(self) -> None: + """A leaf module cannot close a cycle through its importers.""" + path = ROOT.joinpath(*NEW_PATH.split('.')).with_suffix('.py') + names = imported_modules(path) + self.assertIn('chardet', names) + self.assertEqual([name for name in names if name.startswith(('pcapkit', '.'))], []) + + def test_utilities_imports_nothing_from_corekit(self) -> None: + """:mod:`pcapkit.utilities` sits below :mod:`pcapkit.corekit` and does not reach up.""" + for path in sorted(ROOT.joinpath('pcapkit', 'utilities').glob('*.py')): + with self.subTest(path=path.name): + self.assertEqual([name for name in imported_modules(path) + if name.startswith('pcapkit.corekit')], []) + + def test_the_new_module_imports_first_in_a_fresh_interpreter(self) -> None: + """Imported first, from the tree under test, with nothing loaded at the old path.""" + skip_without_runtime(self) + script = ('import json, sys, pcapkit.corekit.chardet as m; ' + 'print(json.dumps([m.__file__, sorted(n for n in sys.modules' + ' if n.startswith("pcapkit.") and n.endswith(".chardet"))]))') + proc = subprocess.run([sys.executable, '-c', script], cwd=str(ROOT), # nosec: B603 + capture_output=True, text=True, timeout=120, check=False) + self.assertEqual(proc.returncode, 0, proc.stderr) + path, loaded = json.loads(proc.stdout.splitlines()[-1]) + self.assertEqual(pathlib.Path(path).resolve(), ROOT / 'pcapkit' / 'corekit' / 'chardet.py') + self.assertEqual(loaded, [NEW_PATH]) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/corekit/test_stringfield_lossless_unit.py b/tests/corekit/test_stringfield_lossless_unit.py index accf1b89cd..a5f7608c79 100644 --- a/tests/corekit/test_stringfield_lossless_unit.py +++ b/tests/corekit/test_stringfield_lossless_unit.py @@ -3,7 +3,7 @@ GitHub issue #1293: :class:`~pcapkit.corekit.fields.strings.StringField` was lossy three ways. Without an ``encoding`` it decoded with the charset -:func:`~pcapkit.utilities.chardet.detect` guessed and packed the text back as +:func:`~pcapkit.corekit.chardet.detect` guessed and packed the text back as UTF-8; an octet the charset could not decode became U+FFFD and was packed as U+FFFD's own three octets; and a length of ``-1`` was taken from the *character* count, so multi-octet text lost its tail. diff --git a/tests/project/test_all_exports_unit.py b/tests/project/test_all_exports_unit.py index d34a488c62..91d02350b4 100644 --- a/tests/project/test_all_exports_unit.py +++ b/tests/project/test_all_exports_unit.py @@ -24,12 +24,12 @@ #: Names an aggregated package exports and :mod:`pcapkit.all` deliberately does #: not list, keyed by the package. :mod:`pcapkit.utilities` has been unlisted #: since ``769a17c78`` (2022-05-29), which commented its group out: its exports -#: are generic helpers -- ``warn``, ``reset``, ``configure``, ``detect`` -- that +#: are generic helpers -- ``warn``, ``reset``, ``configure`` -- that #: ``import *`` should not drop into the caller's namespace. A name added here is #: an API decision. DELIBERATE_NON_EXPORTS = { 'pcapkit.utilities': ('logger', 'get_logger', 'configure', 'reset', 'ensure_output', - 'warn', 'stacklevel', 'detect', 'beholder', 'prepare', 'seekset'), + 'warn', 'stacklevel', 'beholder', 'prepare', 'seekset'), }