Skip to content

Commit 8c24163

Browse files
protobuf-github-botcopybara-github
protobuf-github-bot
authored and
copybara-github
committed
Compare explicit zeroes from prototext in partially.
Right now, zeros are not allowed in explicit prototext comparison, frequently leading to very awkward manual checking. This tracks such explicit zeros, and ensures they remain 0. PiperOrigin-RevId: 580729313
1 parent 5987c89 commit 8c24163

8 files changed

Lines changed: 518 additions & 42 deletions

‎src/google/protobuf/message.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,7 @@ class MapValueConstRef;
135135
class MapValueRef;
136136
class MapIterator;
137137
class MapReflectionTester;
138+
class TextFormat;
138139

139140
namespace internal {
140141
struct FuzzPeer;
@@ -1010,6 +1011,7 @@ class PROTOBUF_EXPORT Reflection final {
10101011
return schema_.IsSplit(field);
10111012
}
10121013

1014+
friend class google::protobuf::TextFormat;
10131015
friend class FastReflectionBase;
10141016
friend class FastReflectionMessageMutator;
10151017
friend bool internal::IsDescendant(Message& root, const Message& message);

‎src/google/protobuf/text_format.cc‎

Lines changed: 29 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,14 @@ const Descriptor* DefaultFinderFindAnyType(const Message& message,
295295
}
296296
} // namespace
297297

298+
const void* TextFormat::Parser::UnsetFieldsMetadata::GetUnsetFieldAddress(
299+
const Message& message, const Reflection& reflection,
300+
const FieldDescriptor& fd) {
301+
// reflection->GetRaw() is a simple cast for any non-repeated type, so for
302+
// simplicity we just pass in char as the template argument.
303+
return &reflection.GetRaw<char>(message, &fd);
304+
}
305+
298306
// ===========================================================================
299307
// Internal class for parsing an ASCII representation of a Protocol Message.
300308
// This class makes use of the Protocol Message compiler's tokenizer found
@@ -331,7 +339,7 @@ class TextFormat::Parser::ParserImpl {
331339
bool allow_unknown_extension, bool allow_unknown_enum,
332340
bool allow_field_number, bool allow_relaxed_whitespace,
333341
bool allow_partial, int recursion_limit,
334-
bool error_on_no_op_fields)
342+
UnsetFieldsMetadata* no_op_fields)
335343
: error_collector_(error_collector),
336344
finder_(finder),
337345
parse_info_tree_(parse_info_tree),
@@ -349,7 +357,7 @@ class TextFormat::Parser::ParserImpl {
349357
recursion_limit_(recursion_limit),
350358
had_silent_marker_(false),
351359
had_errors_(false),
352-
error_on_no_op_fields_(error_on_no_op_fields) {
360+
no_op_fields_(no_op_fields) {
353361
// For backwards-compatibility with proto1, we need to allow the 'f' suffix
354362
// for floats.
355363
tokenizer_.set_allow_f_after_float(true);
@@ -834,19 +842,21 @@ class TextFormat::Parser::ParserImpl {
834842
// When checking for no-op operations, We verify that both the existing value in
835843
// the message and the new value are the default. If the existing field value is
836844
// not the default, setting it to the default should not be treated as a no-op.
837-
#define SET_FIELD(CPPTYPE, CPPTYPELCASE, VALUE) \
838-
if (field->is_repeated()) { \
839-
reflection->Add##CPPTYPE(message, field, VALUE); \
840-
} else { \
841-
if (error_on_no_op_fields_ && !field->has_presence() && \
842-
field->default_value_##CPPTYPELCASE() == \
843-
reflection->Get##CPPTYPE(*message, field) && \
844-
field->default_value_##CPPTYPELCASE() == VALUE) { \
845-
ReportError("Input field " + field->full_name() + \
846-
" did not change resulting proto."); \
847-
} else { \
848-
reflection->Set##CPPTYPE(message, field, std::move(VALUE)); \
849-
} \
845+
// The pointer of this is kept in no_op_fields_ for bookkeeping.
846+
#define SET_FIELD(CPPTYPE, CPPTYPELCASE, VALUE) \
847+
if (field->is_repeated()) { \
848+
reflection->Add##CPPTYPE(message, field, VALUE); \
849+
} else { \
850+
if (no_op_fields_ && !field->has_presence() && \
851+
field->default_value_##CPPTYPELCASE() == \
852+
reflection->Get##CPPTYPE(*message, field) && \
853+
field->default_value_##CPPTYPELCASE() == VALUE) { \
854+
no_op_fields_->addresses_.insert( \
855+
UnsetFieldsMetadata::GetUnsetFieldAddress(*message, *reflection, \
856+
*field)); \
857+
} else { \
858+
reflection->Set##CPPTYPE(message, field, std::move(VALUE)); \
859+
} \
850860
}
851861

852862
switch (field->cpp_type()) {
@@ -1429,7 +1439,7 @@ class TextFormat::Parser::ParserImpl {
14291439
int recursion_limit_;
14301440
bool had_silent_marker_;
14311441
bool had_errors_;
1432-
bool error_on_no_op_fields_;
1442+
UnsetFieldsMetadata* no_op_fields_{};
14331443

14341444
};
14351445

@@ -1727,7 +1737,7 @@ bool TextFormat::Parser::Parse(io::ZeroCopyInputStream* input,
17271737
allow_case_insensitive_field_, allow_unknown_field_,
17281738
allow_unknown_extension_, allow_unknown_enum_,
17291739
allow_field_number_, allow_relaxed_whitespace_,
1730-
allow_partial_, recursion_limit_, error_on_no_op_fields_);
1740+
allow_partial_, recursion_limit_, no_op_fields_);
17311741
return MergeUsingImpl(input, output, &parser);
17321742
}
17331743

@@ -1752,7 +1762,7 @@ bool TextFormat::Parser::Merge(io::ZeroCopyInputStream* input,
17521762
allow_case_insensitive_field_, allow_unknown_field_,
17531763
allow_unknown_extension_, allow_unknown_enum_,
17541764
allow_field_number_, allow_relaxed_whitespace_,
1755-
allow_partial_, recursion_limit_, error_on_no_op_fields_);
1765+
allow_partial_, recursion_limit_, no_op_fields_);
17561766
return MergeUsingImpl(input, output, &parser);
17571767
}
17581768

@@ -1788,7 +1798,7 @@ bool TextFormat::Parser::ParseFieldValueFromString(absl::string_view input,
17881798
allow_case_insensitive_field_, allow_unknown_field_,
17891799
allow_unknown_extension_, allow_unknown_enum_,
17901800
allow_field_number_, allow_relaxed_whitespace_,
1791-
allow_partial_, recursion_limit_, error_on_no_op_fields_);
1801+
allow_partial_, recursion_limit_, no_op_fields_);
17921802
return parser.ParseField(field, output);
17931803
}
17941804

‎src/google/protobuf/text_format.h‎

Lines changed: 38 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <vector>
2222

2323
#include "absl/container/flat_hash_map.h"
24+
#include "absl/container/flat_hash_set.h"
2425
#include "absl/strings/cord.h"
2526
#include "absl/strings/string_view.h"
2627
#include "google/protobuf/descriptor.h"
@@ -81,6 +82,9 @@ PROTOBUF_EXPORT enum class Option;
8182
// Converts a protobuf message to a string with redaction enabled.
8283
PROTOBUF_EXPORT std::string StringifyMessage(const Message& message,
8384
Option option);
85+
86+
class UnsetFieldsMetadataTextFormatTestUtil;
87+
class UnsetFieldsMetadataMessageDifferencerTestUtil;
8488
} // namespace internal
8589

8690
// This class implements protocol buffer text format, colloquially known as text
@@ -719,11 +723,40 @@ class PROTOBUF_EXPORT TextFormat {
719723
// the maximum allowed nesting of proto messages.
720724
void SetRecursionLimit(int limit) { recursion_limit_ = limit; }
721725

722-
// If called, the parser will report an error if a parsed field had no
726+
// Uniquely addresses fields in a message that was explicitly unset in
727+
// textproto. Example:
728+
// "some_int_field: 0"
729+
// where some_int_field is non-optional.
730+
//
731+
// This class should only be used to pass data between the text_format
732+
// parser and the MessageDifferencer.
733+
class UnsetFieldsMetadata {
734+
public:
735+
UnsetFieldsMetadata() = default;
736+
737+
private:
738+
// Return a pointer to the unset field in the given message.
739+
static const void* GetUnsetFieldAddress(const Message& message,
740+
const Reflection& reflection,
741+
const FieldDescriptor& fd);
742+
743+
// List of addresses of explicitly unset proto fields.
744+
absl::flat_hash_set<const void*> addresses_;
745+
746+
friend class ::google::protobuf::internal::
747+
UnsetFieldsMetadataMessageDifferencerTestUtil;
748+
friend class ::google::protobuf::internal::UnsetFieldsMetadataTextFormatTestUtil;
749+
friend class ::google::protobuf::util::MessageDifferencer;
750+
friend class ::google::protobuf::TextFormat::Parser;
751+
};
752+
753+
// If called, the parser will report the parsed fields that had no
723754
// effect on the resulting proto (for example, fields with no presence that
724-
// were set to their default value).
725-
void ErrorOnNoOpFields(bool return_error) {
726-
error_on_no_op_fields_ = return_error;
755+
// were set to their default value). These can be passed to the Partially()
756+
// matcher as an indicator to explicitly check these fields are missing
757+
// in the actual.
758+
void OutputNoOpFields(UnsetFieldsMetadata* no_op_fields) {
759+
no_op_fields_ = no_op_fields;
727760
}
728761

729762
private:
@@ -748,7 +781,7 @@ class PROTOBUF_EXPORT TextFormat {
748781
bool allow_relaxed_whitespace_;
749782
bool allow_singular_overwrites_;
750783
int recursion_limit_;
751-
bool error_on_no_op_fields_ = false;
784+
UnsetFieldsMetadata* no_op_fields_ = nullptr;
752785
};
753786

754787

0 commit comments

Comments
 (0)