diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f0062b2..34eaec2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,7 +20,7 @@ env: # PXF v0.72-series feature set (@ / @entry / @table directive # grammar, schema validator, Result accessors, TableReader streaming) # the Python port wraps. Bump in lockstep with cpp release cuts. - PROTOWIRE_CPP_REF: v1.0.0 + PROTOWIRE_CPP_REF: v1.1.0 jobs: # --------------------------------------------------------------------- diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 9bfc974..11b23d9 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -17,7 +17,7 @@ permissions: env: # See ci.yml for the rationale on this pin. - PROTOWIRE_CPP_REF: v1.0.0 + PROTOWIRE_CPP_REF: v1.1.0 jobs: analyze: diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index afe1a03..af12104 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -28,7 +28,7 @@ env: # feature set (@ / @entry / @table grammar, schema validator, # Result accessors, TableReader streaming) that this Python port # wraps. Bump in lockstep with cpp release cuts. - PROTOWIRE_CPP_REF: v1.0.0 + PROTOWIRE_CPP_REF: v1.1.0 jobs: # --------------------------------------------------------------------- diff --git a/CHANGELOG.md b/CHANGELOG.md index 47ad4d5..d38d4b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,21 @@ format changes. ## [Unreleased] +### Changed + +- **Built against protowire-cpp v1.1.0** in CI and the wheels (#23). v1.0.0 + segfaulted on deeply nested PXF and read the PXF and SBE options at their + old numbers. `Violation.kind` gains `"key_option"`, which v1.1.0 reports. + +### Fixed + +- **`(pxf.required)` with `(pxf.default)` on one field is a bind-time + violation** (#22), as draft -01 now requires (protowire#453). Such a schema + used to bind. `unmarshal` and `unmarshal_full` raise `ValueError` unless + `skip_validate` is set, and `validate_descriptor` reports a + `"required_default"` violation, over the file and its imports. + `(pxf.required) = false` beside a default stays accepted. + ## [1.0.0] — 2026-05-13 First major-version cut. Implements the three one-time spec changes diff --git a/src/_protowire/module.cc b/src/_protowire/module.cc index 68c6355..f97dd64 100644 --- a/src/_protowire/module.cc +++ b/src/_protowire/module.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -32,6 +33,7 @@ #include #include "protowire/pxf.h" +#include "protowire/pxf/annotations.h" #include "protowire/sbe.h" namespace nb = nanobind; @@ -129,12 +131,76 @@ nb::object CellToPyTuple(const std::optional& cell) { *cell); } +// --- (pxf.required) with (pxf.default) -------------------------------------- +// +// Draft -01 § Required and Default forbids the pair on one field, and a tool +// binding a descriptor must refuse it over the import closure. The pinned +// protowire-cpp does not check this yet (protowire-cpp#50), so the bridge does, +// using cpp's own option readers: the option numbers stay in one place. Once +// the pin carries the upstream check, this one goes. + +struct RequiredWithDefault { + std::string file; + std::string element; // fully-qualified field name + std::string name; // bare field name +}; + +void WalkRequiredWithDefault(const pbuf::Descriptor* md, + std::vector* out) { + for (int i = 0; i < md->field_count(); ++i) { + const pbuf::FieldDescriptor* f = md->field(i); + if (protowire::pxf::IsRequired(f) && protowire::pxf::GetDefault(f).has_value()) { + out->push_back({std::string(md->file()->name()), std::string(f->full_name()), + std::string(f->name())}); + } + } + for (int i = 0; i < md->nested_type_count(); ++i) { + WalkRequiredWithDefault(md->nested_type(i), out); + } +} + +void WalkRequiredWithDefault(const pbuf::FileDescriptor* fd, + std::set* seen, + std::vector* out) { + if (!seen->insert(fd).second) return; + for (int i = 0; i < fd->message_type_count(); ++i) { + WalkRequiredWithDefault(fd->message_type(i), out); + } + for (int i = 0; i < fd->dependency_count(); ++i) { + WalkRequiredWithDefault(fd->dependency(i), seen, out); + } +} + +std::vector FindRequiredWithDefault(const pbuf::Descriptor* desc) { + std::set seen; + std::vector out; + WalkRequiredWithDefault(desc->file(), &seen, &out); + return out; +} + +// Raises when the schema carries the pair; `op` prefixes the error as the +// decoder's own errors are prefixed. Honors skip_validate through its callers, +// like cpp's CheckSchema. +void CheckRequiredWithDefault(const pbuf::Descriptor* desc, const std::string& op) { + auto found = FindRequiredWithDefault(desc); + if (found.empty()) return; + std::string m = op + ": PXF schema violations:"; + for (const auto& v : found) { + m += "\n " + v.file + ": field \"" + v.element + + "\": (pxf.required) and (pxf.default) are exclusive on one field: the first rejects " + "a document where the field is absent, and the second treats it as supplied " + "(draft -01 §Required and Default)"; + } + throw nb::value_error(m.c_str()); +} + // PXF text -> binary proto bytes. nb::bytes PxfUnmarshal(nb::bytes text, nb::bytes fds_bytes, const std::string& full_name, bool discard_unknown, bool skip_validate) { auto schema = BuildSchema(std::string_view(fds_bytes.c_str(), fds_bytes.size())); const auto* desc = FindDescriptor(schema, full_name); + if (!skip_validate) CheckRequiredWithDefault(desc, "pxf.unmarshal"); std::unique_ptr msg( schema.factory->GetPrototype(desc)->New()); @@ -174,6 +240,7 @@ PxfUnmarshalFull(nb::bytes text, nb::bytes fds_bytes, bool skip_validate) { auto schema = BuildSchema(std::string_view(fds_bytes.c_str(), fds_bytes.size())); const auto* desc = FindDescriptor(schema, full_name); + if (!skip_validate) CheckRequiredWithDefault(desc, "pxf.unmarshal_full"); std::unique_ptr msg( schema.factory->GetPrototype(desc)->New()); @@ -231,7 +298,8 @@ PxfUnmarshalFull(nb::bytes text, nb::bytes fds_bytes, // PXF schema reserved-name check (draft §3.13). Returns a list of // (kind, element, name, file) tuples. Empty list ⇒ conformant schema. -// kind values: "field" / "oneof" / "enum_value". +// kind values: "field" / "oneof" / "enum_value" / "key_option" / +// "required_default". std::vector> PxfValidateDescriptor(nb::bytes fds_bytes, const std::string& full_name) { auto schema = BuildSchema(std::string_view(fds_bytes.c_str(), fds_bytes.size())); @@ -245,9 +313,13 @@ PxfValidateDescriptor(nb::bytes fds_bytes, const std::string& full_name) { case protowire::pxf::ViolationKind::kField: kind = "field"; break; case protowire::pxf::ViolationKind::kOneof: kind = "oneof"; break; case protowire::pxf::ViolationKind::kEnumValue: kind = "enum_value"; break; + case protowire::pxf::ViolationKind::kKeyOption: kind = "key_option"; break; } out.emplace_back(std::move(kind), v.element, v.name, v.file); } + for (const auto& v : FindRequiredWithDefault(desc)) { + out.emplace_back("required_default", v.element, v.name, v.file); + } return out; } diff --git a/src/protowire/pxf.py b/src/protowire/pxf.py index efc7713..f9be99d 100644 --- a/src/protowire/pxf.py +++ b/src/protowire/pxf.py @@ -102,15 +102,17 @@ class ProtoDirective: @dataclass(frozen=True) class Violation: - """A schema reserved-name violation, draft §3.13. + """A schema that must not be bound for PXF, draft -01. - Returned by `validate_descriptor`. `kind` is one of - `"field"` / `"oneof"` / `"enum_value"`. + Returned by `validate_descriptor`. `kind` is one of `"field"` / `"oneof"` + / `"enum_value"` (a reserved name, §3.13), `"key_option"` (a misplaced + `(pxf.key)`) or `"required_default"` (`(pxf.required)` and `(pxf.default)` + on one field). """ - kind: Literal["field", "oneof", "enum_value"] + kind: Literal["field", "oneof", "enum_value", "key_option", "required_default"] element: str # fully-qualified protobuf name, e.g. "trades.v1.Side.null" - name: str # the bare reserved identifier ("null" / "true" / "false") + name: str # the reserved identifier, the (pxf.key) value, or the field's name file: str # .proto file path the element is declared in @@ -147,9 +149,9 @@ def null_fields(self) -> list[str]: def validate_descriptor(msg: Message) -> list[Violation]: - """Return schema reserved-name violations on `msg`'s descriptor. + """Return the bind-time violations of `msg`'s descriptor and its imports. - An empty list means the schema is conformant. The check is + An empty list means the schema is conformant. The reserved-name check is case-sensitive: `NULL` / `True` lex as identifiers and are accepted. """ fds = fds_for_message(msg) diff --git a/tests/test_pxf_required_default.py b/tests/test_pxf_required_default.py new file mode 100644 index 0000000..dbf35b3 --- /dev/null +++ b/tests/test_pxf_required_default.py @@ -0,0 +1,153 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 TrendVidia, LLC. +"""Draft -01 § Required and Default: (pxf.required) and (pxf.default) on one +field must be refused when the descriptor is bound. + +The schemas are built from raw option bytes, at the registered numbers, because +that is what reaches the decoder: this port has no annotations.proto of its own. +""" + +from __future__ import annotations + +import pytest +from google.protobuf import descriptor_pb2, descriptor_pool, message_factory + +from protowire import pxf + +_VARINT, _LEN = 0, 2 +_REQUIRED, _DEFAULT, _KEY = 1314, 1315, 1316 + + +def _varint(n: int) -> bytes: + out = bytearray() + while n > 0x7F: + out.append(n & 0x7F | 0x80) + n >>= 7 + out.append(n) + return bytes(out) + + +def _opt_bool(number: int, value: bool) -> bytes: + return _varint(number << 3 | _VARINT) + _varint(int(value)) + + +def _opt_str(number: int, value: str) -> bytes: + return _varint(number << 3 | _LEN) + _varint(len(value)) + value.encode() + + +REQUIRED = _opt_bool(_REQUIRED, True) +NOT_REQUIRED = _opt_bool(_REQUIRED, False) +DEFAULT = _opt_str(_DEFAULT, "x") + +STRING = descriptor_pb2.FieldDescriptorProto.TYPE_STRING +MESSAGE = descriptor_pb2.FieldDescriptorProto.TYPE_MESSAGE + + +def _add_field(msg, name, number, options=b"", type_=STRING, type_name=""): + f = msg.field.add(name=name, number=number, type=type_) + if type_name: + f.type_name = type_name + if options: + f.options.MergeFromString(options) + + +def _file(name, *deps) -> descriptor_pb2.FileDescriptorProto: + fd = descriptor_pb2.FileDescriptorProto(name=name, package="t", syntax="proto3") + fd.dependency.extend(deps) + return fd + + +def _bind(*files): + """Return the FDS bytes and a class for t.M, the first file's message.""" + fds = descriptor_pb2.FileDescriptorSet() + pool = descriptor_pool.DescriptorPool() + for fd in reversed(files): + fds.file.append(fd) + pool.Add(fd) + desc = pool.FindMessageTypeByName("t.M") + return fds.SerializeToString(), message_factory.GetMessageClass(desc) + + +def _one_field(options: bytes): + fd = _file("one.proto") + _add_field(fd.message_type.add(name="M"), "f", 1, options) + return _bind(fd) + + +def _assert_refused(fds: bytes, cls, element: str, file: str) -> None: + # A document supplying the field: the schema is wrong whatever the input. + doc = 'f = "y"\n' if cls.DESCRIPTOR.fields_by_name.get("f") else "" + for call in ( + lambda: pxf.unmarshal(doc, cls()), + lambda: pxf.unmarshal_full(doc, cls()), + lambda: pxf.unmarshal_bytes(doc, fds, "t.M"), + lambda: pxf.unmarshal_full_bytes(doc, fds, "t.M"), + ): + with pytest.raises(ValueError) as e: + call() + assert "(pxf.required) and (pxf.default) are exclusive on one field" in str(e.value) + assert element in str(e.value) + assert pxf.validate_descriptor(cls()) == [ + pxf.Violation( + kind="required_default", + element=element, + name=element.rsplit(".", 1)[1], + file=file, + ) + ] + + +def test_required_with_default_is_refused_at_bind(): + fds, cls = _one_field(REQUIRED + DEFAULT) + _assert_refused(fds, cls, "t.M.f", "one.proto") + + +def test_option_order_does_not_matter(): + fds, cls = _one_field(DEFAULT + REQUIRED) + _assert_refused(fds, cls, "t.M.f", "one.proto") + + +def test_required_false_beside_default_stays_accepted(): + fds, cls = _one_field(NOT_REQUIRED + DEFAULT) + assert pxf.validate_descriptor(cls()) == [] + msg = cls() + pxf.unmarshal_full("", msg) + assert msg.f == "x" # Proves cpp read the default from these bytes. + + +def test_required_alone_and_default_alone_are_accepted(): + _, required = _one_field(REQUIRED) + assert pxf.validate_descriptor(required()) == [] + with pytest.raises(ValueError, match="required"): # cpp read this one too. + pxf.unmarshal_full("", required()) + _, default = _one_field(DEFAULT) + assert pxf.validate_descriptor(default()) == [] + + +def test_nested_message_is_covered(): + fd = _file("nested.proto") + _add_field(fd.message_type.add(name="M").nested_type.add(name="In"), "f", 1, REQUIRED + DEFAULT) + fds, cls = _bind(fd) + _assert_refused(fds, cls, "t.M.In.f", "nested.proto") + + +def test_import_closure_is_covered_even_when_no_field_uses_the_type(): + dep = _file("dep.proto") + _add_field(dep.message_type.add(name="Other"), "f", 1, REQUIRED + DEFAULT) + root = _file("root.proto", "dep.proto") + root.message_type.add(name="M") + fds, cls = _bind(root, dep) + _assert_refused(fds, cls, "t.Other.f", "dep.proto") + + +def test_skip_validate_skips_this_check_like_the_others(): + fds, cls = _one_field(REQUIRED + DEFAULT) + pxf.unmarshal('f = "y"\n', cls(), skip_validate=True) + pxf.unmarshal_bytes('f = "y"\n', fds, "t.M", skip_validate=True) + + +def test_misplaced_key_option_is_reported_with_its_kind(): + fd = _file("key.proto") + _add_field(fd.message_type.add(name="M"), "f", 1, _opt_str(_KEY, "id")) + _, cls = _bind(fd) + assert [v.kind for v in pxf.validate_descriptor(cls())] == ["key_option"]