Skip to content

Commit

Permalink
Implement special handling for enum maps
Browse files Browse the repository at this point in the history
  • Loading branch information
@antongrbin
antongrbin committed Jul 29, 2024
1 parent 4d16d95 commit 724acf1
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 26 deletions.
8 changes: 0 additions & 8 deletions conformance/failure_list_cpp.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ Recommended.Editions_Proto2.JsonInput.FieldNameDuplicateDifferentCasing1
Recommended.Editions_Proto2.JsonInput.FieldNameDuplicateDifferentCasing2 # Should have failed to parse, but didn't.
Recommended.Editions_Proto2.JsonInput.FieldNameExtension.Validator # Expected JSON payload but got type 1
Recommended.Editions_Proto2.JsonInput.FieldNameNotQuoted # Should have failed to parse, but didn't.
Recommended.Editions_Proto2.JsonInput.IgnoreUnknownEnumStringValueInMapPart.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key2]: FOO
Recommended.Editions_Proto2.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key]: FOO
Recommended.Editions_Proto2.JsonInput.MapFieldValueIsNull # Should have failed to parse, but didn't.
Recommended.Editions_Proto2.JsonInput.RepeatedFieldMessageElementIsNull # Should have failed to parse, but didn't.
Recommended.Editions_Proto2.JsonInput.RepeatedFieldPrimitiveElementIsNull # Should have failed to parse, but didn't.
Expand All @@ -41,8 +39,6 @@ Recommended.Editions_Proto3.JsonInput.FieldNameDuplicate
Recommended.Editions_Proto3.JsonInput.FieldNameDuplicateDifferentCasing1 # Should have failed to parse, but didn't.
Recommended.Editions_Proto3.JsonInput.FieldNameDuplicateDifferentCasing2 # Should have failed to parse, but didn't.
Recommended.Editions_Proto3.JsonInput.FieldNameNotQuoted # Should have failed to parse, but didn't.
Recommended.Editions_Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapPart.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key2]: FOO
Recommended.Editions_Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key]: FOO
Recommended.Editions_Proto3.JsonInput.MapFieldValueIsNull # Should have failed to parse, but didn't.
Recommended.Editions_Proto3.JsonInput.RepeatedFieldMessageElementIsNull # Should have failed to parse, but didn't.
Recommended.Editions_Proto3.JsonInput.RepeatedFieldPrimitiveElementIsNull # Should have failed to parse, but didn't.
Expand All @@ -65,8 +61,6 @@ Recommended.Proto2.JsonInput.FieldNameDuplicateDifferentCasing1
Recommended.Proto2.JsonInput.FieldNameDuplicateDifferentCasing2 # Should have failed to parse, but didn't.
Recommended.Proto2.JsonInput.FieldNameExtension.Validator # Expected JSON payload but got type 1
Recommended.Proto2.JsonInput.FieldNameNotQuoted # Should have failed to parse, but didn't.
Recommended.Proto2.JsonInput.IgnoreUnknownEnumStringValueInMapPart.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key2]: FOO
Recommended.Proto2.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key]: FOO
Recommended.Proto2.JsonInput.MapFieldValueIsNull # Should have failed to parse, but didn't.
Recommended.Proto2.JsonInput.RepeatedFieldMessageElementIsNull # Should have failed to parse, but didn't.
Recommended.Proto2.JsonInput.RepeatedFieldPrimitiveElementIsNull # Should have failed to parse, but didn't.
Expand All @@ -92,8 +86,6 @@ Recommended.Proto3.JsonInput.FieldNameDuplicate
Recommended.Proto3.JsonInput.FieldNameDuplicateDifferentCasing1 # Should have failed to parse, but didn't.
Recommended.Proto3.JsonInput.FieldNameDuplicateDifferentCasing2 # Should have failed to parse, but didn't.
Recommended.Proto3.JsonInput.FieldNameNotQuoted # Should have failed to parse, but didn't.
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapPart.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key2]: FOO
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput # Output was not equivalent to reference message: added: map_string_nested_enum[key]: FOO
Recommended.Proto3.JsonInput.MapFieldValueIsNull # Should have failed to parse, but didn't.
Recommended.Proto3.JsonInput.RepeatedFieldMessageElementIsNull # Should have failed to parse, but didn't.
Recommended.Proto3.JsonInput.RepeatedFieldPrimitiveElementIsNull # Should have failed to parse, but didn't.
Expand Down
93 changes: 93 additions & 0 deletions src/google/protobuf/json/internal/parser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -691,12 +691,105 @@ absl::Status ParseMapKey(const Desc<Traits>& type, Msg<Traits>& entry,
return absl::OkStatus();
}

// Parses map entry when value is an enum name (string) and ignoring unknown
// fields flag is set.
// Specification says that in this case, if the enum name is unknown, we should
// skip the map entry.
// For example {"key1": "UNKNOWN_ENUM_VALUE"} should parse into an empty map if
// the ignore_unknown_fields is set.
//
// The default implementation in ParseMapEntry will first allocate a new map
// entry message (by calling Traits::NewMsg) and then consume the value from
// the lexer.
//
// In this special case, we must do it the other way around:
// - first consume the value from the lexer,
// - check that this is a known enum value,
// - and only then proceed with allocating a new map entry message.
//
// Note that in both cases the key is already consumed and passed in through
// 'LocationWith<MaybeOwnedString>& key' param.
template <typename Traits>
absl::Status ParseMapEntryWithEnumStringValueWhenIgnoringUnknownFields(
JsonLexer& lex, Field<Traits> map_field, Msg<Traits>& parent_msg,
LocationWith<MaybeOwnedString>& key) {
// Parse the enum value from string, advancing the lexer.
absl::StatusOr<absl::optional<int32_t>> enum_value;
(void)Traits::WithFieldType(
map_field, [&lex, &enum_value](const Desc<Traits>& map_entry_desc) {
enum_value = ParseEnum<Traits>(lex, Traits::ValueField(map_entry_desc));
return absl::OkStatus();
});
RETURN_IF_ERROR(enum_value.status());

// If enum value is unknown, we'll stop here and avoid allocating a new map
// entry message.
if (!enum_value->has_value()) {
return absl::OkStatus();
}

// If the enum value is known, output the map entry with already parsed
// value.
return Traits::NewMsg(
map_field, parent_msg,
[&](const Desc<Traits>& type, Msg<Traits>& entry) -> absl::Status {
RETURN_IF_ERROR(ParseMapKey<Traits>(lex.options(), type, entry, key));

// This is different from the default implementation in 'ParseMapEntry'
// since we already parsed this value (we must not attempt to advance
// the lexer to parse the value here again).
Traits::SetEnum(Traits::ValueField(type), entry,
enum_value->value_or(0));

return absl::OkStatus();
});
}

// ParseMapEntry implementation has a special case if:
// * 'map_field' is a map of enums, and
// * value that follows in 'lex' is a string (enum name), and
// * and ignore_unknown_fields is set.
// This function does not advance the lexer.
template <typename Traits>
absl::StatusOr<bool> MapEntryNeedsEnumStringValueSpecialCase(
JsonLexer& lex, Field<Traits> map_field) {
// Check if the map_field is a map of enums.
bool is_map_of_enums = false;
(void)Traits::WithFieldType(
map_field, [&is_map_of_enums](const Desc<Traits>& desc) {
is_map_of_enums = Traits::FieldType(Traits::ValueField(desc)) ==
FieldDescriptor::TYPE_ENUM;
return absl::OkStatus();
});
if (!is_map_of_enums) {
return false;
}

// Check that the value is a string (instead of a number).
absl::StatusOr<JsonLexer::Kind> kind = lex.PeekKind();
RETURN_IF_ERROR(kind.status());
if (*kind != JsonLexer::kStr) {
return false;
}

// We need the special case only if we are ignoring unknown fields.
return lex.options().ignore_unknown_fields;
}

// Parses one map entry for 'map_field' in 'parent_msg' with already consumed
// 'key'.
template <typename Traits>
absl::Status ParseMapEntry(JsonLexer& lex, Field<Traits> map_field,
Msg<Traits>& parent_msg,
LocationWith<MaybeOwnedString>& key) {
auto needs_special_case =
MapEntryNeedsEnumStringValueSpecialCase<Traits>(lex, map_field);
RETURN_IF_ERROR(needs_special_case.status());
if (*needs_special_case) {
return ParseMapEntryWithEnumStringValueWhenIgnoringUnknownFields<Traits>(
lex, map_field, parent_msg, key);
}

return Traits::NewMsg(
map_field, parent_msg,
[&](const Desc<Traits>& type, Msg<Traits>& entry) -> absl::Status {
Expand Down
24 changes: 6 additions & 18 deletions src/google/protobuf/json/json_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -562,15 +562,9 @@ TEST_P(JsonTest, ParseMapWithEnumValuesProto2WithUnknownFields) {
})json";

ASSERT_OK(ToProto(message, input_json, options));
// With ignore_unknown_fields set, the unknown enum string value is accepted
// but coerced to 0-th enum value. This behavior fails the conformance test
// 'IgnoreUnknownEnumStringValueInMap' and it will be fixed in
// https://github.com/protocolbuffers/protobuf/pull/16479.
EXPECT_EQ(message.enum_map().size(), 5);
EXPECT_EQ(message.enum_map().contains("key2"), true);
EXPECT_EQ(message.enum_map().contains("key4"), true);
EXPECT_EQ(message.enum_map().at("key2"), 0);
EXPECT_EQ(message.enum_map().at("key4"), 0);
EXPECT_EQ(message.enum_map().size(), 3);
EXPECT_EQ(message.enum_map().contains("key2"), false);
EXPECT_EQ(message.enum_map().contains("key4"), false);
}

TEST_P(JsonTest, ParseMapWithEnumValuesProto3WithUnknownFields) {
Expand All @@ -588,15 +582,9 @@ TEST_P(JsonTest, ParseMapWithEnumValuesProto3WithUnknownFields) {
})json";

ASSERT_OK(ToProto(message, input_json, options));
// With ignore_unknown_fields set, the unknown enum string value is accepted
// but coerced to 0-th enum value. This behavior fails the conformance test
// 'IgnoreUnknownEnumStringValueInMap' and it will be fixed in
// https://github.com/protocolbuffers/protobuf/pull/16479.
EXPECT_EQ(message.map().size(), 5);
EXPECT_EQ(message.map().contains("key2"), true);
EXPECT_EQ(message.map().contains("key4"), true);
EXPECT_EQ(message.map().at("key2"), 0);
EXPECT_EQ(message.map().at("key4"), 0);
EXPECT_EQ(message.map().size(), 3);
EXPECT_EQ(message.map().contains("key2"), false);
EXPECT_EQ(message.map().contains("key4"), false);
}

TEST_P(JsonTest, RepeatedMapKey) {
Expand Down

0 comments on commit 724acf1

Please sign in to comment.