Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Conformance test: JSON parsing of unknown enum string values #9534

Closed
wants to merge 1 commit into from
Closed

Conformance test: JSON parsing of unknown enum string values #9534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions conformance/binary_json_conformance_suite.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1766,6 +1766,7 @@ void BinaryAndJsonConformanceSuite::RunJsonTests() {
RunJsonTestsForStruct();
RunJsonTestsForValue();
RunJsonTestsForAny();
RunJsonTestsForUnknownEnumStringValues();

RunValidJsonIgnoreUnknownTest("IgnoreUnknownJsonNumber", REQUIRED,
R"({
Expand Down Expand Up @@ -1801,6 +1802,43 @@ void BinaryAndJsonConformanceSuite::RunJsonTests() {
ExpectParseFailureForJson("RejectTopLevelNull", REQUIRED, "null");
}

void BinaryAndJsonConformanceSuite::RunJsonTestsForUnknownEnumStringValues() {
// Tests the handling of unknown enum values when encoded as string labels.
// The expected behavior depends on whether unknown fields are ignored:
// * when ignored, the parser should ignore the unknown enum string value.
// * when not ignored, the parser should fail.
struct TestCase {
// Used in the test name.
string enum_location;
// JSON input which will contain the unknown field.
string input_json;
};
const std::vector<TestCase> test_cases = {
{"InOptionalField", R"json({
"optional_nested_enum": "UNKNOWN_ENUM_VALUE"
})json"},
{"InRepeatedField", R"json({
"repeated_nested_enum": ["UNKNOWN_ENUM_VALUE"]
})json"},
{"InMapValue", R"json({
"map_string_nested_enum": {"key": "UNKNOWN_ENUM_VALUE"}
})json"},
};
for (const TestCase& test_case : test_cases) {
// Unknown enum string value is a parse failure when not ignoring unknown fields.
ExpectParseFailureForJson(
absl::StrCat("RejectUnknownEnumStringValue", test_case.enum_location),
RECOMMENDED,
test_case.input_json);
// Unknown enum string value is ignored when ignoring unknown fields.
RunValidJsonIgnoreUnknownTest(
absl::StrCat("IgnoreUnknownEnumStringValue", test_case.enum_location),
RECOMMENDED,
test_case.input_json,
"");
}
}

void BinaryAndJsonConformanceSuite::RunJsonTestsForFieldNameConvention() {
RunValidJsonTest(
"FieldNameInSnakeCase", REQUIRED,
Expand Down
1 change: 1 addition & 0 deletions conformance/binary_json_conformance_suite.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ class BinaryAndJsonConformanceSuite : public ConformanceTestSuite {
void RunJsonTestsForStruct();
void RunJsonTestsForValue();
void RunJsonTestsForAny();
void RunJsonTestsForUnknownEnumStringValues();
void RunValidJsonTest(const std::string& test_name, ConformanceLevel level,
const std::string& input_json,
const std::string& equivalent_text_format);
Expand Down
2 changes: 2 additions & 0 deletions conformance/failure_list_cpp.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ Recommended.Proto3.JsonInput.FieldNameDuplicate
Recommended.Proto3.JsonInput.FieldNameDuplicateDifferentCasing1
Recommended.Proto3.JsonInput.FieldNameDuplicateDifferentCasing2
Recommended.Proto3.JsonInput.FieldNameNotQuoted
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInRepeatedField.ProtobufOutput
Comment on lines +20 to +21
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were these added as failures? What's the expected behavior here? From the PR description it sounds like C++ and Java were implemented "correctly", so it's a little strange that two of the added tests are failing for C++

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was committed already and is causing presubmits to fail since these indeed shouldn't be in the failure list: e.g. https://github.com/protocolbuffers/upb/actions/runs/3473202608/jobs/5804957297

Copy link
Member

@mkruskal-google mkruskal-google Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was, I'm trying to retroactively understand the goal here. We can roll it back if it's causing issues, but it looks like you solved it by bumping upb's protobuf version

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry, i was actually wrong above -- the presubmit failure was for upb and wasn't from the cpp failure list)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the breakage!

Initially I was considering unknown enum string value in optional fields. That case works for C++ as expected, the unknown value is ignored.

When the unknown enum string value is encountered in a repeated field or a map, the C++ shows unexpected behavior too. It fails parsing the whole message although ignoreUnknownFields is set. This is inconsistent with Java, csharp, objc, but more importantly it breaks forward compatibility for enum evolution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhangskz, based on your latest message I suspect you already executed the following steps to debug upb failure, but to over-communicate adding it here.

I can reproduce the failure you linked above link locally. The error message is:

These tests were listed in the failure list, but they don't exist:

  Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput
  Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInRepeatedField.ProtobufOutput

It looks like the conformance test runner and the failure list fell out of sync:

There is an open PR that is supposed to fix this: protocolbuffers/upb#938. That PR is held back due to a python test failure that looks unrelated to the conformance test that we added.

Recommended.Proto3.JsonInput.MapFieldValueIsNull
Recommended.Proto3.JsonInput.RepeatedFieldMessageElementIsNull
Recommended.Proto3.JsonInput.RepeatedFieldPrimitiveElementIsNull
Expand Down
2 changes: 2 additions & 0 deletions conformance/failure_list_php.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ Recommended.Proto2.JsonInput.FieldNameExtension.Validator
Recommended.Proto3.JsonInput.BytesFieldBase64Url.JsonOutput
Recommended.Proto3.JsonInput.BytesFieldBase64Url.ProtobufOutput
Recommended.Proto3.JsonInput.FieldMaskInvalidCharacter
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInRepeatedField.ProtobufOutput
Recommended.Proto3.ProtobufInput.ValidDataOneofBinary.MESSAGE.Merge.ProtobufOutput
Required.Proto2.JsonInput.StoresDefaultPrimitive.Validator
Required.Proto3.JsonInput.DoubleFieldTooSmall
Expand Down
2 changes: 2 additions & 0 deletions conformance/failure_list_php_c.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
Recommended.Proto2.JsonInput.FieldNameExtension.Validator
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInRepeatedField.ProtobufOutput
Required.Proto2.JsonInput.StoresDefaultPrimitive.Validator
3 changes: 3 additions & 0 deletions conformance/failure_list_python.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInOptionalField.ProtobufOutput
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInRepeatedField.ProtobufOutput
3 changes: 3 additions & 0 deletions conformance/failure_list_python_cpp.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@
#
# TODO(haberman): insert links to corresponding bugs tracking the issue.
# Should we use GitHub issues or the Google-internal bug tracker?
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInOptionalField.ProtobufOutput
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInRepeatedField.ProtobufOutput
2 changes: 2 additions & 0 deletions conformance/failure_list_ruby.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ Recommended.Proto2.ProtobufInput.ValidDataRepeated.UINT32.PackedInput.PackedOutp
Recommended.Proto2.ProtobufInput.ValidDataRepeated.UINT32.UnpackedInput.PackedOutput.ProtobufOutput
Recommended.Proto2.ProtobufInput.ValidDataRepeated.UINT64.PackedInput.PackedOutput.ProtobufOutput
Recommended.Proto2.ProtobufInput.ValidDataRepeated.UINT64.UnpackedInput.PackedOutput.ProtobufOutput
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput
Recommended.Proto3.JsonInput.IgnoreUnknownEnumStringValueInRepeatedField.ProtobufOutput
Recommended.Proto3.ProtobufInput.ValidDataOneofBinary.MESSAGE.Merge.ProtobufOutput
Recommended.Proto3.ProtobufInput.ValidDataRepeated.BOOL.PackedInput.UnpackedOutput.ProtobufOutput
Recommended.Proto3.ProtobufInput.ValidDataRepeated.BOOL.UnpackedInput.UnpackedOutput.ProtobufOutput
Expand Down