From 7a3c91769e62e91b5a99b753db87cb6db679d3ef Mon Sep 17 00:00:00 2001 From: "Benedicto Franco Jr." Date: Sun, 1 Nov 2026 22:40:02 -0800 Subject: [PATCH] Report the default's literal as Violation.name for required-with-default protowire-cpp v2.0.0 fills name in for kRequiredOption: the (pxf.default) literal beside a default (protowire-go#211), and the oneof's name for (pxf.required) on a oneof member. The bridge now passes it through instead of deriving the field name from element. A test pins the oneof case. Rebased onto the v2.0.0 migration (#38), which carries the pin; the v1.2.1 tag this used to wait for will not exist. --- CHANGELOG.md | 6 ++++++ src/_protowire/module.cc | 1 - src/protowire/pxf.py | 7 ++++--- tests/test_pxf_required_default.py | 25 +++++++++++++++++++++++-- 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3e45f4..1ba2b45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,12 @@ format changes. target's, and a oneof field. - **Error text** matches protowire-go's. Text a caller matched on changes. +- **`Violation.name` for `(pxf.required)` with `(pxf.default)` on one field is + the default's literal** (#30, trendvidia/protowire-go#211). It was the bare + field name, which `element` already holds. An empty default gives an empty + `name`. This matches the other ports. For `(pxf.required)` on a oneof member, + `name` is the oneof's name, as protowire-cpp reports it. + ### Added - **`Violation.kind` values `"default_option"`, `"retired_number"` and diff --git a/src/_protowire/module.cc b/src/_protowire/module.cc index 39b6ba7..dac88cc 100644 --- a/src/_protowire/module.cc +++ b/src/_protowire/module.cc @@ -253,7 +253,6 @@ PxfValidateDescriptor(nb::bytes fds_bytes, const std::string& full_name) { case protowire::pxf::ViolationKind::kKeyOption: kind = "key_option"; break; case protowire::pxf::ViolationKind::kRequiredOption: kind = "required_default"; - name = v.element.substr(v.element.rfind('.') + 1); // cpp leaves name empty here. break; case protowire::pxf::ViolationKind::kDefaultOption: kind = "default_option"; break; case protowire::pxf::ViolationKind::kRetiredNumber: kind = "retired_number"; break; diff --git a/src/protowire/pxf.py b/src/protowire/pxf.py index 7926a12..e989908 100644 --- a/src/protowire/pxf.py +++ b/src/protowire/pxf.py @@ -125,9 +125,10 @@ class Violation: "well_known_shape", ] element: str # fully-qualified protobuf name, e.g. "trades.v1.Side.null" - # The reserved identifier, the (pxf.key) value, the field's name - # ("required_default"), the (pxf.default) literal ("default_option"), or - # the retired number ("retired_number"); empty for "well_known_shape". + # The reserved identifier, the (pxf.key) value, the (pxf.default) literal + # ("default_option", and "required_default" beside a default), the oneof's + # name ("required_default" on a oneof member), or the retired number + # ("retired_number"); empty for "well_known_shape". name: str file: str # .proto file path the element is declared in diff --git a/tests/test_pxf_required_default.py b/tests/test_pxf_required_default.py index dbf35b3..776789f 100644 --- a/tests/test_pxf_required_default.py +++ b/tests/test_pxf_required_default.py @@ -74,7 +74,7 @@ def _one_field(options: bytes): return _bind(fd) -def _assert_refused(fds: bytes, cls, element: str, file: str) -> None: +def _assert_refused(fds: bytes, cls, element: str, file: str, name: str = "x") -> 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 ( @@ -91,7 +91,7 @@ def _assert_refused(fds: bytes, cls, element: str, file: str) -> None: pxf.Violation( kind="required_default", element=element, - name=element.rsplit(".", 1)[1], + name=name, # The (pxf.default) literal (protowire-go#211). file=file, ) ] @@ -102,6 +102,11 @@ def test_required_with_default_is_refused_at_bind(): _assert_refused(fds, cls, "t.M.f", "one.proto") +def test_empty_default_gives_an_empty_name(): + fds, cls = _one_field(REQUIRED + _opt_str(_DEFAULT, "")) + _assert_refused(fds, cls, "t.M.f", "one.proto", name="") + + def test_option_order_does_not_matter(): fds, cls = _one_field(DEFAULT + REQUIRED) _assert_refused(fds, cls, "t.M.f", "one.proto") @@ -151,3 +156,19 @@ def test_misplaced_key_option_is_reported_with_its_kind(): _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"] + + +def test_required_on_a_oneof_member_names_the_oneof(): + # protowire-cpp v2.0.0 reports this under the same kind, with the oneof's + # name in `name` ("Oneof Members"). + fd = _file("oneof.proto") + m = fd.message_type.add(name="M") + m.oneof_decl.add(name="pick") + _add_field(m, "a", 1, REQUIRED) + _add_field(m, "b", 2) + for f in m.field: + f.oneof_index = 0 + _, cls = _bind(fd) + assert pxf.validate_descriptor(cls()) == [ + pxf.Violation(kind="required_default", element="t.M.a", name="pick", file="oneof.proto") + ]