Repository navigation
pxf: refuse (pxf.required) with (pxf.default) on one field at bind time - #26
Merged
Merged
Conversation
…me (#22) 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A schema with both a required and a default on one field now fails when it is bound, as draft -01 requires (trendvidia/protowire#453). Before, it bound and a document omitting the field had no single meaning.
Fixes #22
Fixes #23
What this means for a port that wraps protowire-cpp
The bind checks live in protowire-cpp, which this port wraps, and its own check (protowire-cpp#50) is not merged or tagged. There were two ways to go:
The check is in
module.ccand uses cpp's ownIsRequiredandGetDefault, so the option numbers stay in one place. It is meant to be temporary, and #25 tracks removing it. If you would rather wait for the cpp tag, drop the second commit and keep the test file.It needed the pin bump first, which is the first commit. The workflows pinned cpp v1.0.0, which reads these options at 50000/50001, so it could not see them on a current schema. That pin also segfaulted on deep PXF (#23). The bump touches
publish.yml, so it changes what the next wheels contain.What it covers
unmarshal,unmarshal_fulland their_bytesforms raiseValueError.validate_descriptorreports a"required_default"violation.skip_validate=Trueskips it, as it skips the other bind checks. That includesbind_row, which defaults to skipping.(pxf.required) = falsebeside a default stays accepted.Bracket options are the only spelling covered, because they are the only one this port reads. protowire-cpp has no reader for the
@required/@defaultcarrier, so a schema written that way gets neither the options nor this check. No test schema in this repo carried the pair, so none changed.The message is the Go wording: "(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".
Evidence
.venv/bin/python -m pytest -qon the head commit, built against sibling protowire-cpp main (8d0867a):108 passed in 0.74s. Before this PR:100 passed.103 passed, 5 skipped. The skips are from running the copy outside the sibling layout. I did not look at each skip reason.FindRequiredWithDefault:4 failed, 104 passed. The four aretest_required_with_default_is_refused_at_bind,test_option_order_does_not_matter,test_nested_message_is_coveredandtest_import_closure_is_covered_even_when_no_field_uses_the_type.annotations.proto,string role = 1 [(pxf.required) = true, (pxf.default) = "viewer"];, is refused. The same field with(pxf.required) = falseis accepted.unmarshal_fullapplied the default and enforced required, so those options were really read and the accept is not a parser that saw nothing.Not proven here: Linux and Windows on the new pin. Those legs run only in this PR's CI.
Also fixed
Violation.kindis"key_option"for a misplaced(pxf.key). On v1.1.0 it would otherwise be an empty string.Filed
bind_row's docstring promises required and default behavior it does not have. Its answer is an open question, so I did not change it.