diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a49866..6c4d4c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,16 @@ format changes. ### Fixed +- **`(pxf.required)` with `(pxf.default)` on one field is a bind-time + violation** (#50), as draft -01 now requires (protowire#453). + `ValidateFile` reports it as `ViolationKind::kRequiredOption`, over the + import closure, and `Unmarshal` and `UnmarshalFull` refuse such a schema. + Such a schema used to bind here, and which option won depended on the + engine. `(pxf.required) = false` beside a default stays accepted. The + bind-failure `Status` now starts `PXF schema violations:`, as in the Go + reference, instead of `PXF schema reserved-name violations:`, because + it no longer lists only reserved names. + - **`check_decode`'s `Tree` has `branches` and `tags`** (protowire#449). Without them the PB leg skipped both fields as unknown, so the corpus's map-depth rows could not reach the decoder's map path. The decoders diff --git a/include/protowire/pxf/schema.h b/include/protowire/pxf/schema.h index a729958..5137c93 100644 --- a/include/protowire/pxf/schema.h +++ b/include/protowire/pxf/schema.h @@ -35,6 +35,11 @@ enum class ViolationKind : uint8_t { // field that is not a repeated message-typed field, naming a field the // element message lacks, or naming one that is not a singular string. kKeyOption, + // (pxf.required) = true beside (pxf.default) on one field, which draft + // -01 §annotation-extensions ("Required and Default") forbids: the first + // rejects a document where the field is absent, and the second treats it + // as supplied. + kRequiredOption, }; const char* ViolationKindName(ViolationKind k); @@ -46,7 +51,7 @@ struct Violation { std::string element; // fully-qualified protobuf name (e.g. "trades.v1.Side.null") std::string name; // bare reserved identifier, or the (pxf.key) value for kKeyOption ViolationKind kind = ViolationKind::kField; - std::string detail; // human-readable explanation; set for kKeyOption + std::string detail; // human-readable explanation; set for kKeyOption and kRequiredOption // One-line human-readable description, e.g. // "trades.proto: message field \"trades.v1.X.null\" uses PXF-reserved name \"null\" (draft @@ -57,10 +62,12 @@ struct Violation { // ValidateDescriptor walks the file containing `desc` together with its // transitive imports, and returns every bind-time violation in that // closure: reserved-name collisions among messages, oneofs, and enum -// values, and invalid (pxf.key) placements (draft -01 §3.13). The -// returned vector is sorted by declaring file path and then by element -// fully-qualified name, for stable output. An empty vector means the -// schema is conformant. +// values; invalid (pxf.key) placements (draft -01 §3.13); and +// (pxf.required) with (pxf.default) on one field (draft -01 +// §annotation-extensions, "Required and Default"). (pxf.required) = false +// demands nothing, so a default beside it stands. The returned vector is +// sorted by declaring file path and then by element fully-qualified name, +// for stable output. An empty vector means the schema is conformant. // // Scope is the import closure, per draft -01 § Scope of Bind-Time // Checks: a misplaced annotation on a message type declared in an diff --git a/src/pxf/decode_fast.cc b/src/pxf/decode_fast.cc index ae0edc8..c882d21 100644 --- a/src/pxf/decode_fast.cc +++ b/src/pxf/decode_fast.cc @@ -1776,15 +1776,15 @@ Status PostDecode(Message* msg, namespace { -// CheckSchema runs the PXF schema reserved-name check (draft §3.13) on -// the descriptor of `msg`. Returns a Status with the joined violation +// CheckSchema runs the PXF schema's bind-time checks on the descriptor +// of `msg`. Returns a Status with the joined violation // list on conflict, OK otherwise. Skipped when opts.skip_validate is // true. Status CheckSchema(const Message* msg, const UnmarshalOptions& opts) { if (opts.skip_validate) return Status::OK(); auto vs = ValidateDescriptor(msg->GetDescriptor()); if (vs.empty()) return Status::OK(); - std::string msg_str = "PXF schema reserved-name violations:\n "; + std::string msg_str = "PXF schema violations:\n "; for (size_t i = 0; i < vs.size(); ++i) { if (i != 0) msg_str.append("\n "); msg_str.append(vs[i].ToString()); diff --git a/src/pxf/schema.cc b/src/pxf/schema.cc index 45685ad..e1aa23a 100644 --- a/src/pxf/schema.cc +++ b/src/pxf/schema.cc @@ -90,6 +90,15 @@ void WalkMessages(const std::string& path, const pb::Descriptor* md, std::vector if (auto key = KeyFieldName(f); key.has_value()) { CheckKeyOption(path, f, *key, out); } + if (IsRequired(f) && GetDefault(f).has_value()) { + out->push_back(Violation{path, + std::string(f->full_name()), + "", + ViolationKind::kRequiredOption, + "(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"}); + } } // Skip synthetic oneofs (those generated for proto3 optional fields). // libprotobuf exposes a real oneof count and a "real" count; the @@ -123,6 +132,8 @@ const char* ViolationKindName(ViolationKind k) { return "enum value"; case ViolationKind::kKeyOption: return "(pxf.key) placement"; + case ViolationKind::kRequiredOption: + return "(pxf.required) placement"; } return "unknown"; } @@ -132,6 +143,10 @@ std::string Violation::ToString() const { return file + ": field \"" + element + "\" carries (pxf.key) = \"" + name + "\": " + detail + " (draft -01 §3.13)"; } + if (kind == ViolationKind::kRequiredOption) { + return file + ": field \"" + element + "\": invalid (pxf.required): " + detail + + " (draft -01 §annotation-extensions)"; + } return file + ": " + ViolationKindName(kind) + " \"" + element + "\" uses PXF-reserved name \"" + name + "\" (draft §3.13)"; } diff --git a/test/pxf_schema_test.cc b/test/pxf_schema_test.cc index 324f6f9..d0de5fa 100644 --- a/test/pxf_schema_test.cc +++ b/test/pxf_schema_test.cc @@ -278,6 +278,7 @@ TEST_F(PxfSchema, ViolationKindNameRendersExpectedLabels) { EXPECT_STREQ(ViolationKindName(ViolationKind::kField), "message field"); EXPECT_STREQ(ViolationKindName(ViolationKind::kOneof), "oneof"); EXPECT_STREQ(ViolationKindName(ViolationKind::kEnumValue), "enum value"); + EXPECT_STREQ(ViolationKindName(ViolationKind::kRequiredOption), "(pxf.required) placement"); } // ---- Unmarshal-time integration (the decode gate) ------------------------ @@ -298,8 +299,7 @@ message Row { auto st = protowire::pxf::Unmarshal("a = 1\n", msg.get()); ASSERT_FALSE(st.ok()); - EXPECT_NE(std::string(st.message()).find("PXF schema reserved-name violations"), - std::string::npos); + EXPECT_NE(std::string(st.message()).find("PXF schema violations"), std::string::npos); } TEST_F(PxfSchema, UnmarshalSkipsValidateWhenOptionSet) { @@ -342,8 +342,7 @@ message Row { std::unique_ptr msg(factory.GetPrototype(fd->message_type(0))->New()); auto r = protowire::pxf::UnmarshalFull("a = 1\n", msg.get()); ASSERT_FALSE(r.ok()); - EXPECT_NE(std::string(r.status().message()).find("PXF schema reserved-name violations"), - std::string::npos); + EXPECT_NE(std::string(r.status().message()).find("PXF schema violations"), std::string::npos); } // ---- import closure (draft -01 § Scope of Bind-Time Checks; #34) --------- @@ -390,6 +389,99 @@ message Root { Imported inner = 1; } EXPECT_NE(st.message().find("b_closure.proto"), std::string::npos) << st.ToString(); } +// ---- (pxf.required) with (pxf.default) (draft -01 "Required and Default") - + +// The pair is refused when the schema is bound, whatever the document +// says, and the Status names the field. The document here supplies the +// field, so a check deferred to an absent field would miss it. +TEST_F(PxfSchema, RequiredWithDefaultRejected) { + const pb::FileDescriptor* fd = CompileFiles({{"req_def.proto", R"src( +syntax = "proto3"; +package reqdef.v1; +import "pxf/annotations.proto"; +message Row { + string f = 1 [(pxf.required) = true, (pxf.default) = "x"]; + message Inner { + string empty = 1 [(pxf.required) = true, (pxf.default) = ""]; + } +} +)src"}}, + "req_def.proto"); + ASSERT_NE(fd, nullptr) << errors_.last_; + auto vs = ValidateFile(fd); + ASSERT_EQ(vs.size(), 2u); + EXPECT_EQ(vs[0].element, "reqdef.v1.Row.Inner.empty"); + EXPECT_EQ(vs[0].kind, ViolationKind::kRequiredOption); + EXPECT_EQ(vs[1].element, "reqdef.v1.Row.f"); + EXPECT_EQ(vs[1].kind, ViolationKind::kRequiredOption); + EXPECT_NE(vs[1].ToString().find("(pxf.required) and (pxf.default) are exclusive on one field"), + std::string::npos) + << vs[1].ToString(); + + pb::DynamicMessageFactory factory(importer_->pool()); + const pb::Descriptor* row = fd->message_type(0); + std::unique_ptr msg(factory.GetPrototype(row)->New()); + auto st = protowire::pxf::Unmarshal("f = \"given\"\n", msg.get()); + ASSERT_FALSE(st.ok()); + EXPECT_NE(st.message().find("reqdef.v1.Row.f"), std::string::npos) << st.ToString(); + std::unique_ptr full(factory.GetPrototype(row)->New()); + auto r = protowire::pxf::UnmarshalFull("f = \"given\"\n", full.get()); + ASSERT_FALSE(r.ok()); + EXPECT_NE(r.status().message().find("are exclusive on one field"), std::string::npos) + << r.status().ToString(); +} + +// (pxf.required) = false requires nothing, so it does not conflict with a +// default, and the default still applies. +TEST_F(PxfSchema, RequiredFalseBesideDefaultAccepted) { + const pb::FileDescriptor* fd = CompileFiles({{"req_false.proto", R"src( +syntax = "proto3"; +package reqfalse.v1; +import "pxf/annotations.proto"; +message Row { + string f = 1 [(pxf.required) = false, (pxf.default) = "x"]; +} +)src"}}, + "req_false.proto"); + ASSERT_NE(fd, nullptr) << errors_.last_; + EXPECT_TRUE(ValidateFile(fd).empty()); + + pb::DynamicMessageFactory factory(importer_->pool()); + const pb::Descriptor* row = fd->message_type(0); + std::unique_ptr msg(factory.GetPrototype(row)->New()); + auto r = protowire::pxf::UnmarshalFull("", msg.get()); + ASSERT_TRUE(r.ok()) << r.status().ToString(); + EXPECT_EQ(msg->GetReflection()->GetString(*msg, row->FindFieldByName("f")), "x"); +} + +// The pair on a type only an import declares is refused too, even though +// the bound message never refers to it (draft -01 § Scope of Bind-Time +// Checks), and is attributed to the importing file's dependency. +TEST_F(PxfSchema, RequiredWithDefaultInImportReported) { + const char* b = R"src( +syntax = "proto3"; +package reqimp.v1; +import "pxf/annotations.proto"; +message Unreferenced { + string f = 1 [(pxf.required) = true, (pxf.default) = "x"]; +} +)src"; + const char* a = R"src( +syntax = "proto3"; +package reqimp.v1; +import "b_req.proto"; +message Root { string s = 1; } +)src"; + const pb::FileDescriptor* fd = + CompileFiles({{"b_req.proto", b}, {"a_req.proto", a}}, "a_req.proto"); + ASSERT_NE(fd, nullptr) << errors_.last_; + auto vs = ValidateDescriptor(fd->message_type(0)); + ASSERT_EQ(vs.size(), 1u); + EXPECT_EQ(vs[0].file, "b_req.proto"); + EXPECT_EQ(vs[0].element, "reqimp.v1.Unreferenced.f"); + EXPECT_EQ(vs[0].kind, ViolationKind::kRequiredOption); +} + // The diamond — A imports B and C, both importing D — reports D's // violation once, and the closure's violations sort by file, then element. TEST_F(PxfSchema, DiamondImportReportedOnceAndSortedByFile) {