Skip to content

fix(registry): ModuleDescriptor.klass leaks a bare AttributeError, so all nine register_* call sites fail a bad class name with a stdlib error #832

Closed

Description

@JarryShaw

ModuleDescriptor.klass raises a bare builtins.AttributeError when the class name does not exist in the module, so every register_* function in pcapkit/foundation/registry/protocols.py fails a bad class name with a stdlib exception rather than a pcapkit one.

Measured on ee51366a6 (PR #815's head) and reproducible on main:

register_apptype(port, 'pcapkit.protocols.misc.raw', 'tcp', TransportProtocol.tcp)
  -> AttributeError: module 'pcapkit.protocols.misc.raw' has no attribute 'tcp'

register_apptype(AppType.http, 'pcapkit.protocols.misc.raw')      # class_ omitted
  -> AttributeError: module 'pcapkit.protocols.misc.raw' has no attribute '(null)'

The second is the more telling one: the NULL sentinel '(null)' reaches getattr and is reported as if the caller had asked for a class of that name.

Scope: protocols.py builds ModuleDescriptor(module, class_) in nine places, so this is the whole register_* family, not one function. The exception originates at pcapkit/corekit/module.py:37 — re-derive that line before changing it.

This is the same family as #805/#811 (struct.error escaping FieldBase.length), #828/#829 (ValueError escaping NumberField.__call__) and #831 (the second shift): a caller cannot catch these as pcapkit errors. The fix belongs in ModuleDescriptor.klass, raising ProtocolError from pcapkit.utilities.exceptions and naming both the module and the missing class, so all nine call sites gain it at once.

One coupling to know before changing it. #815 added a test asserting assertRaises(AttributeError) with assertIn("'tcp'", …) — deliberately, to prove that a str third argument is resolved as a class name and never sniffed as a transport. That assertion will need updating in the same change; the property it protects should survive as a ProtocolError whose message still contains the attempted class name.

Also worth folding in: a missing class_ should not reach getattr at all. When module is a str and class_ is still the NULL sentinel, the caller has omitted a required argument, and saying so is more useful than reporting '(null)' as an absent attribute.

Found by #815's cross-review while judging whether that AttributeError was the right failure for the residual. The conclusion there was to leave it — fixing it in one function would make register_apptype inconsistent with its eight siblings — which is what makes this a family-wide issue rather than part of that PR.

Activity

  1. added
    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)
    fixPull requests that fix a defect (fix: subject prefix)
    on Sep 26, 2026
  2. JarryShaw commented on Sep 26, 2026

    @JarryShaw
    OwnerAuthor

    Checkable blocker: #815 merged.

    gh pr view 815 -R JarryShaw/PyPCAPKit --json state,mergedAt
    

    Labelled blocked rather than dispatched, because fixing this would red #815's test suite. Verified on that PR's head ee51366a6:

    tests/foundation/registry/test_protocols.py:708   with self.assertRaises(AttributeError) as caught:
                                              :709       registry.register_apptype(65204, raw_module[0], 'tcp', TransportProtocol.udp)
                                              :710   self.assertIn("'tcp'", str(caught.exception))
    

    main's copy of that file asserts AttributeError 0 times, so the coupling arrives with #815 and did not exist before.

    That assertion is deliberate and load-bearing — it proves a str third argument is resolved as a class name and never sniffed as a transport, which is the residual of the maintainer's type(module) ruling. So this issue's fix has to land after #815 and update it in the same change: the property must survive as a ProtocolError whose message still contains the attempted class name, so assertIn("'tcp'", …) keeps working while assertRaises changes type.

    Doing it in the other order would either break #815's CI or force that PR to weaken an assertion it added on purpose. Ordering matters here, not scope.

  3. added
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    wipWork in flight - a covering PR is open or an agent is actively on it
    and removed
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    on Sep 26, 2026
  4. removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Sep 26, 2026
  5. added this to the 1.5 milestone on Oct 6, 2026
  6. moved this to Done in PyPCAPKiton Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)fixPull requests that fix a defect (fix: subject prefix)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions