From 1b90899440bfee6c43f535caf65be3f5a960d756 Mon Sep 17 00:00:00 2001 From: "Benedicto Franco Jr." Date: Fri, 30 Oct 2026 18:50:19 -0700 Subject: [PATCH 1/2] ci: build against protowire-cpp v1.1.0 (#23) The v1.0.0 pin predates the depth cap and the move of the PXF and SBE options to their registered numbers, so CI and the wheels could not see (pxf.required) on a current schema at all. The bind check for #22 needs the numbers; the depth cap is the rest of #23. Fixes #23 --- .github/workflows/ci.yml | 2 +- .github/workflows/codeql.yml | 2 +- .github/workflows/publish.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) 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: # --------------------------------------------------------------------- From 3f53658e2bea68ab1cf7cad442d5e258640f83ea Mon Sep 17 00:00:00 2001 From: "Benedicto Franco Jr." Date: Fri, 30 Oct 2026 18:50:19 -0700 Subject: [PATCH 2/2] pxf: refuse (pxf.required) with (pxf.default) on one field at bind time (#22) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Draft -01 § Required and Default forbids the pair, over the scope of bind-time checks. The bind checks live in protowire-cpp, whose own check for this (protowire-cpp#50) is unmerged and untagged, so the bridge makes it for now, through cpp's IsRequired and GetDefault so the option numbers stay in one place. It covers the import closure and honors skip_validate, as cpp's checks do, and goes when the pin carries the upstream check. v1.1.0 adds ViolationKind::kKeyOption, which the switch in PxfValidateDescriptor left as an empty kind; it now reads key_option. Fixes #22 --- CHANGELOG.md | 15 +++ src/_protowire/module.cc | 74 +++++++++++++- src/protowire/pxf.py | 16 +-- tests/test_pxf_required_default.py | 153 +++++++++++++++++++++++++++++ 4 files changed, 250 insertions(+), 8 deletions(-) create mode 100644 tests/test_pxf_required_default.py 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"]