Skip to content

UnrecognizedPacketTypeError message assumes a PKT_APID parameter, raising KeyError for non-CCSDS definitions #276

Description

@medley56

Context

parse_bytes builds its UnrecognizedPacketTypeError message by interpolating packet['PKT_APID']:

raise UnrecognizedPacketTypeError(
f"Detected an abstract container with no valid inheritors by restriction criteria. "
f"This might mean this packet type is not accounted for in the provided packet definition. "
f"APID={packet['PKT_APID']}.",
partial_data=packet,
)
break

That subscript assumes the packet has a parameter literally named PKT_APID. XTCE has no notion of the CCSDS standard, and any container in a definition may be used as the root, so this assumption does not hold in general. When it fails, the KeyError is raised while constructing the exception, so the intended UnrecognizedPacketTypeError is never raised at all.

Noticed while correcting the root_container_name docstrings in #273, which had claimed a root container "must begin with the definition of a CCSDS header in order to parse correctly". That claim is false — but this line is the one place in the parse path where it is effectively assumed to be true.

Reproduction

A definition with no CCSDS header, an abstract root container, and an inheritor whose restriction criteria do not match:

import io
import struct

from space_packet_parser import load_xtce
from space_packet_parser.exceptions import UnrecognizedPacketTypeError

XTCE = """<?xml version="1.0" encoding="UTF-8"?>
<xtce:SpaceSystem name="NoCcsds" xmlns:xtce="http://www.omg.org/spec/XTCE/20180204">
  <xtce:TelemetryMetaData>
    <xtce:ParameterTypeSet>
      <xtce:IntegerParameterType name="U8_Type" signed="false">
        <xtce:IntegerDataEncoding sizeInBits="8" encoding="unsigned"/>
      </xtce:IntegerParameterType>
    </xtce:ParameterTypeSet>
    <xtce:ParameterSet>
      <xtce:Parameter name="COUNTER" parameterTypeRef="U8_Type"/>
    </xtce:ParameterSet>
    <xtce:ContainerSet>
      <xtce:SequenceContainer name="AbstractRoot" abstract="true">
        <xtce:EntryList><xtce:ParameterRefEntry parameterRef="COUNTER"/></xtce:EntryList>
      </xtce:SequenceContainer>
      <xtce:SequenceContainer name="Child">
        <xtce:EntryList/>
        <xtce:BaseContainer containerRef="AbstractRoot">
          <xtce:RestrictionCriteria>
            <xtce:Comparison parameterRef="COUNTER" value="99" useCalibratedValue="false"/>
          </xtce:RestrictionCriteria>
        </xtce:BaseContainer>
      </xtce:SequenceContainer>
    </xtce:ContainerSet>
  </xtce:TelemetryMetaData>
</xtce:SpaceSystem>
"""

definition = load_xtce(io.BytesIO(XTCE.encode()))
data = struct.pack(">B", 7)  # COUNTER=7, so Child's criteria (== 99) fails

try:
    definition.parse_bytes(data, root_container_name="AbstractRoot")
except UnrecognizedPacketTypeError as e:
    print("intended error:", e)

Expected: UnrecognizedPacketTypeError, caught by the except clause, carrying partial_data.

Actual: the except clause does not fire, and the KeyError propagates:

  File "space_packet_parser/xtce/definitions.py", line 454, in parse_bytes
    f"APID={packet['PKT_APID']}.",
            ~~~~~~^^^^^^^^^^^^
KeyError: 'PKT_APID'

Driving Requirements

A packet that cannot be identified should raise UnrecognizedPacketTypeError regardless of whether the definition happens to use CCSDS naming. Error reporting on the failure path should not itself be able to fail.

Implementation Requirements

  • parse_bytes raises UnrecognizedPacketTypeError for the abstract-container-with-no-valid-inheritors case even when the packet has no PKT_APID parameter.
  • partial_data is populated in that case, since it is the main diagnostic the error exists to carry.
  • The message still reports the APID when one is available, since that is genuinely useful for the common CCSDS case.
  • Regression test covering a definition without a PKT_APID parameter.

A minimal fix is to stop subscripting on the failure path. SpacePacket subclasses dict, so .get is available:

f"APID={packet.get('PKT_APID', 'unknown')}."

Considerations

  • Severity is low but the failure mode is unpleasant. It is invisible for ordinary CCSDS definitions, where PKT_APID is parsed from the header before inheritors are evaluated. It surfaces only once someone uses a non-CCSDS definition or different parameter naming — at which point the documented try/except UnrecognizedPacketTypeError pattern silently stops working and callers get a KeyError from inside the library.
  • partial_data is lost. Because the failure happens during exception construction, the caller gets neither the intended exception type nor the partial packet, which is exactly the information needed to debug a definition that fails this way.
  • Naming, not just the CCSDS header. Even a definition that does describe CCSDS packets hits this if it names the field something other than PKT_APID (APID, ApID, etc.). Nothing in XTCE mandates that spelling.
  • Worth a scan for sibling assumptions. This is the only PKT_APID subscript in definitions.py, but other CCSDS-specific naming assumptions may exist on other error paths.
  • No API change and no behavior change for existing CCSDS users — only the failure path is affected.

Activity

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

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions