From 2d88311150b2ec19d676d42cf54504e5d9e74a68 Mon Sep 17 00:00:00 2001 From: Gavin Medley <7018964+medley56@users.noreply.github.com> Date: Sun, 13 Sep 2026 05:20:22 +0000 Subject: [PATCH 1/3] Raise UnrecognizedPacketTypeError without assuming a PKT_APID parameter The abstract-container-with-no-valid-inheritors error path built its message by subscripting packet['PKT_APID']. XTCE has no notion of the CCSDS standard, so definitions without that parameter got a KeyError from inside exception construction instead of the documented UnrecognizedPacketTypeError, and partial_data was lost with it. Name the abstract container in the message, report the APID only when the packet actually has one (falling back to the CCSDS header printout when the bytes are CCSDSPacketBytes), and add regression tests for both the non-CCSDS and CCSDS cases. Closes #276 Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 6 +++ space_packet_parser/xtce/definitions.py | 17 +++++-- tests/unit/test_xtce/test_definitions.py | 61 ++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e3add1e..38a3425b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Make `CITATION.cff` conform to CFF 1.2.0 so citation exports work, and update the metadata consistency check to use CFF contacts while keeping package descriptions separate from the citation abstract. [#285](https://github.com/lasp/space_packet_parser/issues/285) +- Raise `UnrecognizedPacketTypeError` (with `partial_data` populated) when an abstract container has + no valid inheritors, even if the definition has no `PKT_APID` parameter. Previously the error + message was built by subscripting `packet['PKT_APID']`, so definitions that do not use CCSDS + naming got a `KeyError` from inside the library instead of the documented exception. The message + still reports the APID when the packet has one. + [#276](https://github.com/lasp/space_packet_parser/issues/276) ## [6.2.0] - 2026-09-13 diff --git a/space_packet_parser/xtce/definitions.py b/space_packet_parser/xtce/definitions.py index bd499d74..aa02e1b6 100644 --- a/space_packet_parser/xtce/definitions.py +++ b/space_packet_parser/xtce/definitions.py @@ -450,12 +450,19 @@ def parse_bytes(self, binary_data: bytes, *, root_container_name: str | None = N if len(valid_inheritors) == 0: if current_container.abstract: - 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, + message = ( + f"Detected an abstract container ({current_container.name}) with no valid inheritors by " + f"restriction criteria. This might mean this packet type is not accounted for in the " + f"provided packet definition." ) + # XTCE has no notion of the CCSDS standard, so a PKT_APID parameter is not guaranteed to exist. + # Report the APID when it is available because it is the most useful diagnostic for CCSDS + # packets, but never let the error report itself fail on a non-CCSDS definition. + if "PKT_APID" in packet: + message += f" APID={packet['PKT_APID']}." + elif isinstance(packet.binary_data, ccsds.CCSDSPacketBytes): + message += f" {packet.binary_data}." + raise UnrecognizedPacketTypeError(message, partial_data=packet) break raise UnrecognizedPacketTypeError( diff --git a/tests/unit/test_xtce/test_definitions.py b/tests/unit/test_xtce/test_definitions.py index a549c38c..1619c19b 100644 --- a/tests/unit/test_xtce/test_definitions.py +++ b/tests/unit/test_xtce/test_definitions.py @@ -8,6 +8,7 @@ import space_packet_parser as spp import space_packet_parser.generators.ccsds import space_packet_parser.xtce.parameter_types +from space_packet_parser.exceptions import UnrecognizedPacketTypeError from space_packet_parser.xtce import comparisons, containers, definitions, encodings, parameters @@ -649,3 +650,63 @@ def test_parse_packet_too_few_bytes(test_data_dir): r"Tried to read 32 bits from position 504 in a packet of length 528 bits.", ): xdef.parse_bytes(too_short_packet_data) + + +def test_parse_bytes_unrecognized_packet_without_pkt_apid(): + """Test that an unrecognized packet raises UnrecognizedPacketTypeError for a definition with no PKT_APID + + XTCE has no notion of the CCSDS standard, so the error path must not assume a parameter named PKT_APID exists. + Regression test for https://github.com/lasp/space_packet_parser/issues/276 + """ + xtce = """ + + + + + + + + + + + + + + + + + + + + + + + + +""" + xdef = definitions.XtcePacketDefinition.from_xtce(io.StringIO(xtce), root_container_name="AbstractRoot") + + # COUNTER=7, so the Child restriction criteria (COUNTER == 99) fail and AbstractRoot has no valid inheritors + with pytest.raises(UnrecognizedPacketTypeError, match=r"abstract container \(AbstractRoot\)") as exc_info: + xdef.parse_bytes(b"\x07") + + # The message must not pretend to know an APID that does not exist in this definition + assert "APID" not in str(exc_info.value) + # The partially parsed packet is the main diagnostic the error exists to carry + assert exc_info.value.partial_data is not None + assert exc_info.value.partial_data["COUNTER"] == 7 + + +def test_parse_bytes_unrecognized_packet_reports_apid(test_data_dir): + """Test that an unrecognized CCSDS packet still reports its APID in the UnrecognizedPacketTypeError message""" + xdef = definitions.XtcePacketDefinition.from_xtce(test_data_dir / "test_xtce.xml") + + # APID 2047 is not defined in test_xtce.xml + unknown_apid_packet = space_packet_parser.generators.ccsds.create_ccsds_packet( + data=bytes(65), apid=2047, sequence_flags=space_packet_parser.generators.ccsds.SequenceFlags.UNSEGMENTED + ) + + with pytest.raises(UnrecognizedPacketTypeError, match="APID=2047") as exc_info: + xdef.parse_bytes(unknown_apid_packet) + + assert exc_info.value.partial_data["PKT_APID"] == 2047 From 53af2bdf54ae3243ac5d92eee45dd88e05fbc0ff Mon Sep 17 00:00:00 2001 From: Gavin Medley <7018964+medley56@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:38:47 +0000 Subject: [PATCH 2/3] Make CCSDSPacketBytes str safe for inputs shorter than a header str(CCSDSPacketBytes) indexed all six primary header bytes, so a shorter input raised IndexError. That made the CCSDS header printout unsafe to use in diagnostic messages, including the new UnrecognizedPacketTypeError fallback and the existing trailing-bits warning. Render the available bytes instead when the input is short. Add a regression test for the CCSDSPacketBytes fallback branch in parse_bytes with a definition that has no PKT_APID parameter, and share the no-CCSDS test definition between the tests that use it. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 4 +- space_packet_parser/generators/ccsds.py | 4 ++ tests/unit/test_generators/test_ccsds.py | 16 +++++ tests/unit/test_xtce/test_definitions.py | 77 +++++++++++++++--------- 4 files changed, 73 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38a3425b..06959b5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 no valid inheritors, even if the definition has no `PKT_APID` parameter. Previously the error message was built by subscripting `packet['PKT_APID']`, so definitions that do not use CCSDS naming got a `KeyError` from inside the library instead of the documented exception. The message - still reports the APID when the packet has one. + still reports the APID when the packet has one. As part of this, `str(CCSDSPacketBytes)` no longer + raises `IndexError` for inputs shorter than a full six-byte primary header and instead renders the + bytes it has, so it is safe to use in diagnostic messages. [#276](https://github.com/lasp/space_packet_parser/issues/276) ## [6.2.0] - 2026-09-13 diff --git a/space_packet_parser/generators/ccsds.py b/space_packet_parser/generators/ccsds.py index 59cdedc5..8a6430a7 100644 --- a/space_packet_parser/generators/ccsds.py +++ b/space_packet_parser/generators/ccsds.py @@ -40,6 +40,10 @@ class CCSDSPacketBytes(bytes): HEADER_LENGTH_BYTES = 6 def __str__(self) -> str: + if len(self) < self.HEADER_LENGTH_BYTES: + # Too short to hold a full primary header. Render what we have rather than raising an IndexError + # from the header properties so this stays usable in diagnostic messages. + return f"CCSDSPacket Header: (incomplete, {len(self)} of {self.HEADER_LENGTH_BYTES} bytes: {self.hex()})" return ( f"CCSDSPacket Header: ({self.version_number=}, {self.type=}, " f"{self.secondary_header_flag=}, {self.apid=}, {self.sequence_flags=}, " diff --git a/tests/unit/test_generators/test_ccsds.py b/tests/unit/test_generators/test_ccsds.py index b587bb1e..ee1ef9f9 100644 --- a/tests/unit/test_generators/test_ccsds.py +++ b/tests/unit/test_generators/test_ccsds.py @@ -337,3 +337,19 @@ def test_ccsds_generator_packet_length_math(data_length_bytes): parsed_p = next(space_packet_parser.generators.ccsds_generator(p)) assert len(parsed_p) == data_length_bytes + 6 assert parsed_p.data_length == data_length_bytes - 1 + + +def test_ccsds_packet_bytes_str_short_input(): + """Test that str(CCSDSPacketBytes) does not raise when there are fewer bytes than a full primary header + + The string form is used in diagnostic messages, so it must never fail on the failure path. + """ + short_packet = ccsds.CCSDSPacketBytes(b"\x07") + rendered = str(short_packet) + assert "incomplete" in rendered + assert "1 of 6 bytes" in rendered + assert "07" in rendered + + # A full header renders the parsed fields as before + full_header = ccsds.create_ccsds_packet(data=b"\x00", apid=11) + assert "apid=11" in str(full_header) diff --git a/tests/unit/test_xtce/test_definitions.py b/tests/unit/test_xtce/test_definitions.py index 1619c19b..e1364c6a 100644 --- a/tests/unit/test_xtce/test_definitions.py +++ b/tests/unit/test_xtce/test_definitions.py @@ -11,6 +11,34 @@ from space_packet_parser.exceptions import UnrecognizedPacketTypeError from space_packet_parser.xtce import comparisons, containers, definitions, encodings, parameters +# Minimal definition with no CCSDS header and no PKT_APID parameter (see issue #276) +NO_CCSDS_XTCE = """ + + + + + + + + + + + + + + + + + + + + + + + + +""" + def test_xtce_definition_from_xtce_inputs(test_data_dir): """Test that we can create an XtcePacketDefinition from various inputs""" @@ -658,33 +686,7 @@ def test_parse_bytes_unrecognized_packet_without_pkt_apid(): XTCE has no notion of the CCSDS standard, so the error path must not assume a parameter named PKT_APID exists. Regression test for https://github.com/lasp/space_packet_parser/issues/276 """ - xtce = """ - - - - - - - - - - - - - - - - - - - - - - - - -""" - xdef = definitions.XtcePacketDefinition.from_xtce(io.StringIO(xtce), root_container_name="AbstractRoot") + xdef = definitions.XtcePacketDefinition.from_xtce(io.StringIO(NO_CCSDS_XTCE), root_container_name="AbstractRoot") # COUNTER=7, so the Child restriction criteria (COUNTER == 99) fail and AbstractRoot has no valid inheritors with pytest.raises(UnrecognizedPacketTypeError, match=r"abstract container \(AbstractRoot\)") as exc_info: @@ -710,3 +712,24 @@ def test_parse_bytes_unrecognized_packet_reports_apid(test_data_dir): xdef.parse_bytes(unknown_apid_packet) assert exc_info.value.partial_data["PKT_APID"] == 2047 + + +def test_parse_bytes_unrecognized_packet_ccsds_bytes_without_pkt_apid(): + """Test that the CCSDS header printout is used when the bytes are CCSDSPacketBytes but no PKT_APID is parsed + + Regression test for the fallback branch added in https://github.com/lasp/space_packet_parser/pull/282 + """ + xdef = definitions.XtcePacketDefinition.from_xtce(io.StringIO(NO_CCSDS_XTCE), root_container_name="AbstractRoot") + + # COUNTER=7 fails the Child restriction criteria. Pad to a full 6-byte header so the printout has real values. + ccsds_bytes = space_packet_parser.generators.ccsds.CCSDSPacketBytes(b"\x07" + bytes(7)) + with pytest.raises(UnrecognizedPacketTypeError, match="CCSDSPacket Header") as exc_info: + xdef.parse_bytes(ccsds_bytes) + + assert "APID=" not in str(exc_info.value) + assert exc_info.value.partial_data["COUNTER"] == 7 + + # Even a CCSDSPacketBytes too short for a full header must not break error reporting + with pytest.raises(UnrecognizedPacketTypeError, match="incomplete, 1 of 6 bytes") as exc_info: + xdef.parse_bytes(space_packet_parser.generators.ccsds.CCSDSPacketBytes(b"\x07")) + assert exc_info.value.partial_data["COUNTER"] == 7 From 14e647b86368bd9f702aa21f0767804ab6b03e01 Mon Sep 17 00:00:00 2001 From: Gavin Medley <7018964+medley56@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:44:48 +0000 Subject: [PATCH 3/3] Remove CCSDS-centric wording from UnrecognizedPacketTypeError docstring The docstring said the error is raised when the packet type cannot be determined "based on the header". The error is raised when no or multiple inheritors match by restriction criteria, which does not depend on a header being present. Co-Authored-By: Claude Fable 5.1 --- space_packet_parser/exceptions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/space_packet_parser/exceptions.py b/space_packet_parser/exceptions.py index 71b06f94..1d41f9e4 100644 --- a/space_packet_parser/exceptions.py +++ b/space_packet_parser/exceptions.py @@ -38,7 +38,7 @@ class InvalidParameterTypeError(Exception): class UnrecognizedPacketTypeError(Exception): - """Error raised when we can't figure out which kind of packet we are dealing with based on the header""" + """Error raised when the packet type cannot be determined from the parsed data and the container restriction criteria""" def __init__(self, *args, partial_data: dict = None): """