Skip to content

Commit

Permalink
Add wire format parsing unit tests for implicit presence fields.
Browse files Browse the repository at this point in the history
This gist of the new test case coverage can be summarized as:
- The wire does not distinguish between explicit and implicit fields.
- For implicit presence fields, zeros on the wire can overwrite nonzero values
  (i.e. they are treated as a 'clear' operation).

It's TBD whether we want to accept this behaviour going forward.
Right now we are leaning towards keeping this behaviour, because:
- If we receive zeros on the wire for implicit-presence fields, the protobuf
  wire format is "wrong" anyway. Well-behaved code should never generate zeros
  on the wire for implicit presence fields or serialize the same field multiple
  times.
- There might be some value to enforce that "anything on the wire is accepted".
  This can make handling of wire format simpler.

There are some drawbacks with this approach:
- It might be somewhat surprising for users that zeros on the wire are always
  read, even for implicit-presence fields.
- It might make the transition from implicit-presence to explicit-presence
  harder (or more unsafe) if user wants to migrate.
- It leads to an inconsistency between what it means to "Merge".
  - Merging from a constructed object, with implicit presence and with field
    set to zero, will not overwrite.
  - Merging from the wire, with implicit presence and with zero explicitly
    present on the wire, WILL overwrite.

I still need to add conformance tests to ensure that this is a consistent
behavior across all languages, but for now let's at least add some coverage in
C++ to ensure that this is a tested behaviour.

PiperOrigin-RevId: 657724599
  • Loading branch information
tonyliaoss authored and copybara-github committed Jul 30, 2024
1 parent 6ab302d commit 06a520c
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 1 deletion.
2 changes: 2 additions & 0 deletions src/google/protobuf/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -1657,6 +1657,8 @@ cc_test(
deps = [
":cc_test_protos",
":protobuf",
"@com_google_absl//absl/log:absl_check",
"@com_google_absl//absl/strings:string_view",
"@com_google_googletest//:gtest",
"@com_google_googletest//:gtest_main",
],
Expand Down
73 changes: 73 additions & 0 deletions src/google/protobuf/no_field_presence_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
#include <string>

#include "google/protobuf/descriptor.pb.h"
#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include "absl/log/absl_check.h"
#include "absl/strings/string_view.h"
#include "google/protobuf/descriptor.h"
#include "google/protobuf/unittest.pb.h"
#include "google/protobuf/unittest_no_field_presence.pb.h"
Expand All @@ -17,6 +20,8 @@ namespace google {
namespace protobuf {
namespace {

using ::testing::StrEq;

// Helper: checks that all fields have default (zero/empty) values.
void CheckDefaultValues(
const proto2_nofieldpresence_unittest::TestAllTypes& m) {
Expand Down Expand Up @@ -457,6 +462,74 @@ TEST(NoFieldPresenceTest, MergeFromIfNonzeroTest) {
EXPECT_EQ("test2", dest.optional_string());
}

TEST(NoFieldPresenceTest, ExtraZeroesInWireParseTest) {
// check extra serialized zeroes on the wire are parsed into the object.
proto2_nofieldpresence_unittest::ForeignMessage dest;
dest.set_c(42);
ASSERT_EQ(42, dest.c());

// ExplicitForeignMessage has the same fields as ForeignMessage, but with
// explicit presence instead of implicit presence.
proto2_nofieldpresence_unittest::ExplicitForeignMessage source;
source.set_c(0);
std::string wire = source.SerializeAsString();
ASSERT_THAT(wire, StrEq(absl::string_view{"\x08\x00", 2}));

// The "parse" operation clears all fields before merging from wire.
ASSERT_TRUE(dest.ParseFromString(wire));
EXPECT_EQ(0, dest.c());
std::string dest_data;
EXPECT_TRUE(dest.SerializeToString(&dest_data));
EXPECT_TRUE(dest_data.empty());
}

TEST(NoFieldPresenceTest, ExtraZeroesInWireMergeTest) {
// check explicit zeros on the wire are merged into an implicit one.
proto2_nofieldpresence_unittest::ForeignMessage dest;
dest.set_c(42);
ASSERT_EQ(42, dest.c());

// ExplicitForeignMessage has the same fields as ForeignMessage, but with
// explicit presence instead of implicit presence.
proto2_nofieldpresence_unittest::ExplicitForeignMessage source;
source.set_c(0);
std::string wire = source.SerializeAsString();
ASSERT_THAT(wire, StrEq(absl::string_view{"\x08\x00", 2}));

// TODO: b/356132170 -- Add conformance tests to ensure this behaviour is
// well-defined.
// As implemented, the C++ "merge" operation does not distinguish between
// implicit and explicit fields when reading from the wire.
ASSERT_TRUE(dest.MergeFromString(wire));
// If zero is present on the wire, the original value is overwritten, even
// though this is specified as an "implicit presence" field.
EXPECT_EQ(0, dest.c());
std::string dest_data;
EXPECT_TRUE(dest.SerializeToString(&dest_data));
EXPECT_TRUE(dest_data.empty());
}

TEST(NoFieldPresenceTest, ExtraZeroesInWireLastWins) {
// check that, when the same field is present multiple times on the wire, we
// always take the last one -- even if it is a zero.

absl::string_view wire{"\x08\x01\x08\x00", /*len=*/4}; // note the null-byte.
proto2_nofieldpresence_unittest::ForeignMessage dest;

// TODO: b/356132170 -- Add conformance tests to ensure this behaviour is
// well-defined.
// As implemented, the C++ "merge" operation does not distinguish between
// implicit and explicit fields when reading from the wire.
ASSERT_TRUE(dest.MergeFromString(wire));
// If the same field is present multiple times on the wire, "last one wins".
// i.e. -- the last seen field content will always overwrite, even if it's
// zero and the field is implicit presence.
EXPECT_EQ(0, dest.c());
std::string dest_data;
EXPECT_TRUE(dest.SerializeToString(&dest_data));
EXPECT_TRUE(dest_data.empty());
}

TEST(NoFieldPresenceTest, IsInitializedTest) {
// Check that IsInitialized works properly.
proto2_nofieldpresence_unittest::TestProto2Required message;
Expand Down
8 changes: 7 additions & 1 deletion src/google/protobuf/unittest_no_field_presence.proto
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ message TestAllTypes {
repeated string repeated_string_piece = 54 [ctype = STRING_PIECE];
repeated string repeated_cord = 55 [ctype = CORD];

repeated NestedMessage repeated_lazy_message = 57 ;
repeated NestedMessage repeated_lazy_message = 57;

oneof oneof_field {
uint32 oneof_uint32 = 111;
Expand All @@ -112,6 +112,12 @@ message ForeignMessage {
int32 c = 1;
}

// Same as ForeignMessage, but all fields have explicit presence.
// It can be useful for testing explicit-implicit presence interop behaviour.
message ExplicitForeignMessage {
int32 c = 1 [features.field_presence = EXPLICIT];
}

enum ForeignEnum {
FOREIGN_FOO = 0;
FOREIGN_BAR = 1;
Expand Down

0 comments on commit 06a520c

Please sign in to comment.