Chiebot-Mirror
/
protobuf
mirror of https://github.com/protocolbuffers/protobuf.git
8
0
Fork 0
Code Issues Projects Releases Wiki Activity

Update proto parsing to keep track of parsed fields that have no presence and were parsed to their default value, hence had no effect on the result proto.

Browse Source 
Update the Partially proto matcher to cause proto parsing to check fail if the above condition was triggered.
This is to address a known issue where test are silently passing when proto3 protos are the arguments to Partially(EqualsProto) and have some of the fields ignored by the EqualsProto matcher since they were set to their default value in the expected proto.

PiperOrigin-RevId: 518262030
pull/12295/head
Protobuf Team Bot 2 years ago committed by Copybara-Service
parent 35ffb665e2
commit bf1ae222c9
3 changed files with 134 additions and 27 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 70
      src/google/protobuf/text_format.cc
  2. 8
      src/google/protobuf/text_format.h
  3. 83
      src/google/protobuf/text_format_unittest.cc

70
src/google/protobuf/text_format.cc
Unescape View File

@ -320,7 +320,8 @@ class TextFormat::Parser::ParserImpl {
bool allow_case_insensitive_field, bool allow_unknown_field,
bool allow_unknown_extension, bool allow_unknown_enum,
bool allow_field_number, bool allow_relaxed_whitespace,
bool allow_partial, int recursion_limit)
bool allow_partial, int recursion_limit,
bool error_on_no_op_fields)
: error_collector_(error_collector),
finder_(finder),
parse_info_tree_(parse_info_tree),
@ -337,7 +338,8 @@ class TextFormat::Parser::ParserImpl {
initial_recursion_limit_(recursion_limit),
recursion_limit_(recursion_limit),
had_silent_marker_(false),
had_errors_(false) {
had_errors_(false),
error_on_no_op_fields_(error_on_no_op_fields) {
// For backwards-compatibility with proto1, we need to allow the 'f' suffix
// for floats.
tokenizer_.set_allow_f_after_float(true);
@ -817,60 +819,71 @@ class TextFormat::Parser::ParserImpl {
// Define an easy to use macro for setting fields. This macro checks
// to see if the field is repeated (in which case we need to use the Add
// methods or not (in which case we need to use the Set methods).
#define SET_FIELD(CPPTYPE, VALUE) \
if (field->is_repeated()) { \
reflection->Add##CPPTYPE(message, field, VALUE); \
} else { \
reflection->Set##CPPTYPE(message, field, VALUE); \
// When checking for no-op operations, We verify that both the existing value in
// the message and the new value are the default. If the existing field value is
// not the default, setting it to the default should not be treated as a no-op.
#define SET_FIELD(CPPTYPE, CPPTYPELCASE, VALUE) \
if (field->is_repeated()) { \
reflection->Add##CPPTYPE(message, field, VALUE); \
} else { \
if (error_on_no_op_fields_ && !field->has_presence() && \
field->default_value_##CPPTYPELCASE() == \
reflection->Get##CPPTYPE(*message, field) && \
field->default_value_##CPPTYPELCASE() == VALUE) { \
ReportError("Input field " + field->full_name() + \
" did not change resulting proto."); \
} else { \
reflection->Set##CPPTYPE(message, field, std::move(VALUE)); \
} \
}
switch (field->cpp_type()) {
case FieldDescriptor::CPPTYPE_INT32: {
int64_t value;
DO(ConsumeSignedInteger(&value, kint32max));
SET_FIELD(Int32, static_cast<int32_t>(value));
SET_FIELD(Int32, int32, static_cast<int32_t>(value));
break;
}
case FieldDescriptor::CPPTYPE_UINT32: {
uint64_t value;
DO(ConsumeUnsignedInteger(&value, kuint32max));
SET_FIELD(UInt32, static_cast<uint32_t>(value));
SET_FIELD(UInt32, uint32, static_cast<uint32_t>(value));
break;
}
case FieldDescriptor::CPPTYPE_INT64: {
int64_t value;
DO(ConsumeSignedInteger(&value, kint64max));
SET_FIELD(Int64, value);
SET_FIELD(Int64, int64, value);
break;
}
case FieldDescriptor::CPPTYPE_UINT64: {
uint64_t value;
DO(ConsumeUnsignedInteger(&value, kuint64max));
SET_FIELD(UInt64, value);
SET_FIELD(UInt64, uint64, value);
break;
}
case FieldDescriptor::CPPTYPE_FLOAT: {
double value;
DO(ConsumeDouble(&value));
SET_FIELD(Float, io::SafeDoubleToFloat(value));
SET_FIELD(Float, float, io::SafeDoubleToFloat(value));
break;
}
case FieldDescriptor::CPPTYPE_DOUBLE: {
double value;
DO(ConsumeDouble(&value));
SET_FIELD(Double, value);
SET_FIELD(Double, double, value);
break;
}
case FieldDescriptor::CPPTYPE_STRING: {
std::string value;
DO(ConsumeString(&value));
SET_FIELD(String, std::move(value));
SET_FIELD(String, string, value);
break;
}
@ -878,14 +891,14 @@ class TextFormat::Parser::ParserImpl {
if (LookingAtType(io::Tokenizer::TYPE_INTEGER)) {
uint64_t value;
DO(ConsumeUnsignedInteger(&value, 1));
SET_FIELD(Bool, value);
SET_FIELD(Bool, bool, value);
} else {
std::string value;
DO(ConsumeIdentifier(&value));
if (value == "true" || value == "True" || value == "t") {
SET_FIELD(Bool, true);
SET_FIELD(Bool, bool, true);
} else if (value == "false" || value == "False" || value == "f") {
SET_FIELD(Bool, false);
SET_FIELD(Bool, bool, false);
} else {
ReportError(absl::StrCat("Invalid value for boolean field \"",
field->name(), "\". Value: \"", value,
@ -921,7 +934,7 @@ class TextFormat::Parser::ParserImpl {
if (enum_value == nullptr) {
if (int_value != kint64max &&
!field->legacy_enum_field_treated_as_closed()) {
SET_FIELD(EnumValue, int_value);
SET_FIELD(EnumValue, int64, int_value);
return true;
} else if (!allow_unknown_enum_) {
ReportError(absl::StrCat("Unknown enumeration value of \"", value,
@ -935,7 +948,7 @@ class TextFormat::Parser::ParserImpl {
}
}
SET_FIELD(Enum, enum_value);
SET_FIELD(Enum, enum, enum_value);
break;
}
@ -1404,6 +1417,8 @@ class TextFormat::Parser::ParserImpl {
int recursion_limit_;
bool had_silent_marker_;
bool had_errors_;
bool error_on_no_op_fields_;
};
// ===========================================================================
@ -1700,7 +1715,7 @@ bool TextFormat::Parser::Parse(io::ZeroCopyInputStream* input,
allow_case_insensitive_field_, allow_unknown_field_,
allow_unknown_extension_, allow_unknown_enum_,
allow_field_number_, allow_relaxed_whitespace_,
allow_partial_, recursion_limit_);
allow_partial_, recursion_limit_, error_on_no_op_fields_);
return MergeUsingImpl(input, output, &parser);
}
@ -1725,7 +1740,7 @@ bool TextFormat::Parser::Merge(io::ZeroCopyInputStream* input,
allow_case_insensitive_field_, allow_unknown_field_,
allow_unknown_extension_, allow_unknown_enum_,
allow_field_number_, allow_relaxed_whitespace_,
allow_partial_, recursion_limit_);
allow_partial_, recursion_limit_, error_on_no_op_fields_);
return MergeUsingImpl(input, output, &parser);
}
@ -1755,12 +1770,13 @@ bool TextFormat::Parser::ParseFieldValueFromString(const std::string& input,
const FieldDescriptor* field,
Message* output) {
io::ArrayInputStream input_stream(input.data(), input.size());
ParserImpl parser(
output->GetDescriptor(), &input_stream, error_collector_, finder_,
parse_info_tree_, ParserImpl::ALLOW_SINGULAR_OVERWRITES,
allow_case_insensitive_field_, allow_unknown_field_,
allow_unknown_extension_, allow_unknown_enum_, allow_field_number_,
allow_relaxed_whitespace_, allow_partial_, recursion_limit_);
ParserImpl parser(output->GetDescriptor(), &input_stream, error_collector_,
finder_, parse_info_tree_,
ParserImpl::ALLOW_SINGULAR_OVERWRITES,
allow_case_insensitive_field_, allow_unknown_field_,
allow_unknown_extension_, allow_unknown_enum_,
allow_field_number_, allow_relaxed_whitespace_,
allow_partial_, recursion_limit_, error_on_no_op_fields_);
return parser.ParseField(field, output);
}

8
src/google/protobuf/text_format.h
Unescape View File

@ -694,6 +694,13 @@ class PROTOBUF_EXPORT TextFormat {
// the maximum allowed nesting of proto messages.
void SetRecursionLimit(int limit) { recursion_limit_ = limit; }
// If called, the parser will report an error if a parsed field had no
// effect on the resulting proto (for example, fields with no presence that
// were set to their default value).
void ErrorOnNoOpFields(bool return_error) {
error_on_no_op_fields_ = return_error;
}
private:
// Forward declaration of an internal class used to parse text
// representations (see text_format.cc for implementation).
@ -716,6 +723,7 @@ class PROTOBUF_EXPORT TextFormat {
bool allow_relaxed_whitespace_;
bool allow_singular_overwrites_;
int recursion_limit_;
bool error_on_no_op_fields_ = false;
};

83
src/google/protobuf/text_format_unittest.cc
Unescape View File

@ -901,6 +901,89 @@ TEST_F(TextFormatTest, ParseUnknownEnumFieldProto3) {
EXPECT_EQ(-2147483648, proto.repeated_nested_enum(3));
}
TEST_F(TextFormatTest, ErrorOnNoOpFieldsProto3) {
proto3_unittest::TestAllTypes proto;
TextFormat::Parser parser;
parser.ErrorOnNoOpFields(true);
{
const absl::string_view singular_int_parse_string = "optional_int32: 0";
EXPECT_TRUE(TextFormat::ParseFromString(singular_int_parse_string, &proto));
EXPECT_FALSE(parser.ParseFromString(singular_int_parse_string, &proto));
}
{
const absl::string_view singular_bool_parse_string = "optional_bool: false";
EXPECT_TRUE(
TextFormat::ParseFromString(singular_bool_parse_string, &proto));
EXPECT_FALSE(parser.ParseFromString(singular_bool_parse_string, &proto));
}
{
const absl::string_view singular_string_parse_string =
"optional_string: ''";
EXPECT_TRUE(
TextFormat::ParseFromString(singular_string_parse_string, &proto));
EXPECT_FALSE(parser.ParseFromString(singular_string_parse_string, &proto));
}
{
const absl::string_view nested_message_parse_string =
"optional_nested_message { bb: 0 } ";
EXPECT_TRUE(
TextFormat::ParseFromString(nested_message_parse_string, &proto));
EXPECT_FALSE(parser.ParseFromString(nested_message_parse_string, &proto));
}
{
const absl::string_view foreign_message_parse_string =
"optional_foreign_message { c: 0 } ";
EXPECT_TRUE(
TextFormat::ParseFromString(foreign_message_parse_string, &proto));
EXPECT_FALSE(parser.ParseFromString(foreign_message_parse_string, &proto));
}
{
const absl::string_view nested_enum_parse_string =
"optional_nested_enum: ZERO ";
EXPECT_TRUE(TextFormat::ParseFromString(nested_enum_parse_string, &proto));
EXPECT_FALSE(parser.ParseFromString(nested_enum_parse_string, &proto));
}
{
const absl::string_view foreign_enum_parse_string =
"optional_foreign_enum: FOREIGN_ZERO ";
EXPECT_TRUE(TextFormat::ParseFromString(foreign_enum_parse_string, &proto));
EXPECT_FALSE(parser.ParseFromString(foreign_enum_parse_string, &proto));
}
{
const absl::string_view string_piece_parse_string =
"optional_string_piece: '' ";
EXPECT_TRUE(TextFormat::ParseFromString(string_piece_parse_string, &proto));
EXPECT_FALSE(parser.ParseFromString(string_piece_parse_string, &proto));
}
{
const absl::string_view cord_parse_string = "optional_cord: '' ";
EXPECT_TRUE(TextFormat::ParseFromString(cord_parse_string, &proto));
EXPECT_FALSE(parser.ParseFromString(cord_parse_string, &proto));
}
{
// Sanity check that repeated fields work the same.
const absl::string_view repeated_int32_parse_string = "repeated_int32: 0 ";
EXPECT_TRUE(
TextFormat::ParseFromString(repeated_int32_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(repeated_int32_parse_string, &proto));
}
{
const absl::string_view repeated_bool_parse_string =
"repeated_bool: false ";
EXPECT_TRUE(
TextFormat::ParseFromString(repeated_bool_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(repeated_bool_parse_string, &proto));
}
{
const absl::string_view repeated_string_parse_string =
"repeated_string: '' ";
EXPECT_TRUE(
TextFormat::ParseFromString(repeated_string_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(repeated_string_parse_string, &proto));
}
}
TEST_F(TextFormatTest, ParseStringEscape) {
// Create a parse string with escaped characters in it.
std::string parse_string =

Write Preview
Loading…
Cancel
Save