diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a3072e..f11db0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ number says nothing about any other port. See the Releases section of - **A decoder rejects `null` on a field with `(pxf.required)` or `@required`** (#131, decided by the owner; trendvidia/protowire#569, protowire-go#268). **This narrows what decodes.** A required field holds a value, so `null` is now rejected as absence is, whatever the field's type and presence. Before, `null` counted as present, so `r = null` on `optional int32 r [(pxf.required) = true]` decoded and left a required field with no value. The error is `null is not allowed on required field "r": a required field holds a value`, in a body, a nested body, a `@dataset` cell and a dotted cell, from `Unmarshal` as well as `UnmarshalFull`. A plain scalar with `(pxf.required)`, which already rejected `null`, now names the field as required instead of suggesting `optional`. A zero value still satisfies the field. A `_null` mask that names an unset required field no longer makes `AppendDatasetRow` write `null` for it: the writer refuses it, as it does without the mask. - **`pxf::Marshal` writes an unset field that has `(pxf.default)` or `(pxf.required)` so that it reads back** (#129, protowire-go#259; draft -01 § Common Schema Layer, "`decode_PXF(encode_PXF(v)) == v`"), as protowire-go does. This changes what Marshal writes for annotated fields, and narrows what encodes. Marshal left out every unset field, so a reader that applies defaults filled it in: `int32 n = 1 [(pxf.default) = "5"]` holding 0 read back as 5, and a required scalar holding 0 made the full decode refuse the document. A proto3 scalar without presence is now written as its zero value, a required list or map as `[]` or `{}`, and a message-typed field with a default, such as a wrapper or a Timestamp, as `null`. An unset `optional` or oneof scalar with a default is still left out and reads back as its default, the owner's choice in protowire-go#267. An unset `optional` field that is `(pxf.required)` is now an error, `pxf: is unset, and the field is (pxf.required)`; with `emit_defaults` it is written as zero, as before. An Any whose body has such a field stays as `type_url` and `value`. Inside a `@dataset` value cell, the same error now names the field by its full name. Unannotated fields are unchanged. +- **`sbe::Codec::New` refuses a field in a oneof, in a message or a composite** (#141), as protowire-go does, with `sbe: oneof field . not supported`, or `composite contains oneof field ` under the caller's prefix. **This narrows what binds:** such a message used to bind with a block slot for every member, and `Marshal` wrote the unset ones as zero. A proto3 `optional` field still binds, and so does a oneof in a group entry, which Go does not check either. - **`sbe` refuses a value its `(sbe.encoding)` primitive cannot hold, in both directions** (#133, protowire-go#252; draft -01 § SBE Annotations, "Emitters MUST reject values that fall outside the narrowed type's range"). This narrows what encodes, decodes and binds. `Marshal` used to keep the low bits: `int32 v [(sbe.encoding) = "int8"]` set to 300 was written as 44, and read back as 44 with no error. It now refuses the value with `sbe: field m.v1.M.v: value 300 is outside the range of int8, [-128, 127]`, for every integer primitive, and a `double` narrowed to `float` past ±3.4028234663852886e+38 (NaN and the infinities still pass). An enum is laid out as `uint8`, so an enum value past 255, or a negative one, is refused too. `Unmarshal` refuses a wire value the field cannot hold, such as `uint32` 2^31 into an `int32` field, with `sbe: field m.v1.M.v: wire value 2147483648 (uint32) does not fit a int32 field, [-2147483648, 2147483647]`; it used to cut it to the field's low bits. A signed primitive is now sign-extended whatever the field, so `int8` 0xFF in a `uint32` field is -1 and refused, where it used to read as 255. `Codec::New` refuses `(sbe.encoding)` on a `string` or `bytes` field, and a primitive that crosses between integers and floating point (`int32` with `"float"`, `double` with `"int32"`). A `double` field under `"float"`, and a `float` field under `"double"`, used to write and read the wrong bits; they now convert the value. - **`sbe::Codec::New` refuses a schema the SBE header cannot describe, and `Marshal` a group it cannot count** (#132, protowire-go#250). This narrows what binds and what encodes. Every SBE header field is a 16-bit number, and the codec kept the low 16 bits of a larger one. `bytes b = 1 [(sbe.length) = 65537]` bound as a 1-byte field. A block longer than 65535 bytes, in a message, a group entry or a composite, bound with a wrapped length and offsets. A `(sbe.template_id)`, `(sbe.schema_id)` or `(sbe.version)` of 65537 was written as 1. `Codec::New` now refuses each of these with protowire-go's text, naming the field, message or file, such as `sbe: field m.v1.M.b: (sbe.length) = 65537 is past 65535, the largest blockLength SBE can state`; 65535 still binds. `Marshal` refuses a repeated field with more than 65535 entries, with `sbe: field m.v1.M.e has 65536 entries, past 65535, the largest numInGroup SBE can state`: 65536 entries used to be written with a count of 0, and the message read back empty with no error. Every other bind error of a template now has protowire-go's text as well, prefixed with the field, group or composite it is about, such as `sbe: field m.v1.M.s: string field requires (sbe.length) annotation` where it used to be `string/bytes field requires (sbe.length)`. - **`sbe` tells two schemas' template ids apart** (#134, protowire-go#253 and #257; draft -01 § SBE Annotations, "sbe.template_id MUST be unique among messages within a schema", and § Header and Block Length). This narrows what binds and what decodes. `Codec::New` refuses two different messages with one `(sbe.template_id)` in one schema, with `sbe: messages A and B share (sbe.template_id) = 7 in schema 1; it must be unique within a schema`; the second used to replace the first, and `NewView` then read one message's bytes as the other's. A file passed twice still binds, and so do two schemas that each use a template id. `NewView` now finds a template by the header's schema id and template id together, and refuses an unknown pair with `sbe: unknown template ID 9 in schema 2`, even if another schema has that template id; it used to say `unknown template id 9`. `Unmarshal` and `UnmarshalDescriptor` refuse a header whose schema id is not the target message's, with `sbe: schema ID mismatch: got 1, want 2 for template 7`; the version is not checked. A caller that builds headers by hand must now set the schema id. diff --git a/src/sbe/template.cc b/src/sbe/template.cc index 3cdca04..dd923d4 100644 --- a/src/sbe/template.cc +++ b/src/sbe/template.cc @@ -134,6 +134,10 @@ StatusOr> BuildCompositeFields(const pb::Descriptor* return Status::Error("composite " + std::string(md->full_name()) + " contains list/map field " + std::string(fd->name())); } + if (fd->real_containing_oneof() != nullptr) { + return Status::Error("composite " + std::string(md->full_name()) + " contains oneof field " + + std::string(fd->name())); + } if (fd->cpp_type() == pb::FieldDescriptor::CPPTYPE_MESSAGE) { uint16_t sub_size = 0; auto subs = BuildCompositeFields(fd->message_type(), &sub_size); @@ -237,6 +241,9 @@ StatusOr BuildTemplate(const pb::Descriptor* md, if (fd->is_map()) { return Status::Error("sbe: map field " + FieldPath(md, fd) + " not supported"); } + if (fd->real_containing_oneof() != nullptr) { + return Status::Error("sbe: oneof field " + FieldPath(md, fd) + " not supported"); + } if (fd->is_repeated() && fd->cpp_type() == pb::FieldDescriptor::CPPTYPE_MESSAGE) { auto gt = BuildGroupTemplate(fd); if (!gt.ok()) return gt.status(); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index b1a50c3..f8bbc4d 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -64,7 +64,8 @@ endif() if(TARGET protowire_sbe) set(_sbe_test_srcs "") foreach(f sbe_codec_test sbe_convert_test sbe_navigation_test sbe_stream_test - sbe_template_id_test sbe_header_bounds_test sbe_encoding_range_test) + sbe_template_id_test sbe_header_bounds_test sbe_encoding_range_test + sbe_oneof_test) if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${f}.cc) list(APPEND _sbe_test_srcs ${f}.cc) endif() diff --git a/test/sbe_oneof_test.cc b/test/sbe_oneof_test.cc new file mode 100644 index 0000000..d8c73ab --- /dev/null +++ b/test/sbe_oneof_test.cc @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 TrendVidia, LLC. +// +// A oneof has no place in an SBE block, so Codec::New refuses a field in a +// real oneof in a message or a composite, as protowire-go's template.go +// does. A proto3 optional field sits in a synthetic oneof and binds. A group +// entry has no such check in Go, so a oneof there binds too (#141). + +#include + +#include +#include + +#include "protowire/sbe.h" +#include "sbe_files.h" + +namespace { + +using protowire::sbe::Codec; +using protowire_test::SbeFiles; + +TEST(SbeOneof, NewCodec) { + struct Case { + const char* name; + std::string body; + std::string reject; // the whole error; empty means it must bind + }; + const std::vector cases = { + {"oneof in message", + "message M { option (sbe.template_id) = 1; uint32 t = 1; oneof o { uint32 a = 2; uint64 " + "b = 3; } }", + "sbe: oneof field oneof.v1.M.a not supported"}, + {"oneof in composite", + "message C { oneof o { uint32 a = 1; uint64 b = 2; } } message M { option " + "(sbe.template_id) = 1; C c = 1; }", + "sbe: composite oneof.v1.M.c: composite oneof.v1.C contains oneof field a"}, + {"oneof in nested composite", + "message D { oneof o { uint32 a = 1; } } message C { D d = 1; } message M { option " + "(sbe.template_id) = 1; C c = 1; }", + "sbe: composite oneof.v1.M.c: composite oneof.v1.D contains oneof field a"}, + {"oneof in composite in group", + "message C { oneof o { uint32 a = 1; } } message E { C c = 1; } message M { option " + "(sbe.template_id) = 1; repeated E e = 1; }", + "sbe: composite in group oneof.v1.E.c: composite oneof.v1.C contains oneof field a"}, + {"oneof in group entry binds", + "message E { oneof o { uint32 a = 1; uint64 b = 2; } } message M { option " + "(sbe.template_id) = 1; repeated E e = 1; }", + ""}, + {"proto3 optional in message binds", + "message M { option (sbe.template_id) = 1; optional uint32 a = 1; }", + ""}, + {"proto3 optional in composite binds", + "message C { optional uint32 a = 1; } message M { option (sbe.template_id) = 1; C c = 1; }", + ""}, + }; + for (const Case& tc : cases) { + SCOPED_TRACE(tc.name); + SbeFiles files; + if (!files.Compile({{"oneof.proto", + "syntax = \"proto3\";\npackage oneof.v1;\nimport " + "\"sbe/annotations.proto\";\noption (sbe.schema_id) = 1;\n" + + tc.body + "\n"}})) { + ADD_FAILURE() << files.error(); + continue; + } + auto codec = Codec::New(files.files()); + if (tc.reject.empty()) { + EXPECT_TRUE(codec.ok()) << codec.status().ToString(); + continue; + } + if (codec.ok()) { + ADD_FAILURE() << "bound; want " << tc.reject; + continue; + } + EXPECT_EQ(codec.status().message(), tc.reject); + } +} + +} // namespace