Repository navigation
Reject (pxf.required) with (pxf.default) at bind time - #53
Merged
Merged
Conversation
Draft -01 forbids the pair on one field. A schema with both now fails in ValidateFile, over the import closure, and so in Unmarshal and UnmarshalFull, whether or not a document omits the field. (pxf.required) = false beside a default stays accepted. This port reads only the bracket options; it has no @required / @default carrier reader, so there is one spelling to check. The bind-failure Status now starts "PXF schema violations:", as in Go, because it no longer lists only reserved names. Fixes #50
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
(pxf.required) = trueand(pxf.default)on one field now fails when it is bound, as draft -01 requires. Before, it bound here, and a document omitting the field got an error from this port even though the same schema could mean something else in another engine.Fixes #50
Changes
ValidateFilereports the pair as a newViolationKind::kRequiredOption. The check sits with the other field checks, so it covers the import closure, andUnmarshalandUnmarshalFullrefuse the schema.(pxf.required) = falsebeside a default stays accepted.@required/@defaultcarrier reader, so there is one spelling to check.Also fixed
Statusnow startsPXF schema violations:, matching Go. It used to sayreserved-name violations, which would be wrong for this rule and was already wrong for(pxf.key). Anything matching the old text needs updating.Evidence
ctest --test-dir build --output-on-failure:100% tests passed out of 301, including the three newPxfSchematests. No existing test schema carried the pair, so none needed changing.schema.cc,RequiredWithDefaultRejectedandRequiredWithDefaultInImportReportedfail (Which is: 0, wanted 2 and 1).RequiredFalseBesideDefaultAcceptedpasses either way, as it should.clang-format --dry-run -Werroris clean on the changed sources, using version 23, not the 18 that CI uses.Not covered
BindRowandDatasetReader::Scanskip every bind-time check, so this rule does not reach them, like the reserved-name and(pxf.key)rules. A probe showed it, and #51 tracks it because the fix needs a design choice.scripts/cross_security_check.shis not evidence here: it does not exercise this rule.