diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 51da65e89e362..ddd3e68dc9598 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -1335,14 +1335,10 @@ ProtobufWkt::Value FilterStateFormatter::formatValue(const Http::RequestHeaderMa } ProtobufWkt::Value val; - // TODO(chaoqin-li1123): make this conversion return an error status instead of throwing. - // Access logger conversion from protobufs occurs via json intermediate state, which can throw - // when converting that to a structure. - TRY_NEEDS_AUDIT { MessageUtil::jsonConvertValue(*proto, val); } - catch (EnvoyException& ex) { - return unspecifiedValue(); + if (MessageUtil::jsonConvertValue(*proto, val)) { + return val; } - return val; + return unspecifiedValue(); } // Given a token, extract the command string between parenthesis if it exists. diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 7e5d60d0752b7..006182f0d7ac0 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -240,6 +240,25 @@ size_t MessageUtil::hash(const Protobuf::Message& message) { void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message, ProtobufMessage::ValidationVisitor& validation_visitor) { + bool has_unknown_field; + auto status = loadFromJsonNoThrow(json, message, has_unknown_field); + if (status.ok()) { + return; + } + if (has_unknown_field) { + // If the parsing failure is caused by the unknown fields. + validation_visitor.onUnknownField("type " + message.GetTypeName() + " reason " + + status.ToString()); + } else { + // If the error has nothing to do with unknown field. + throw EnvoyException("Unable to parse JSON as proto (" + status.ToString() + "): " + json); + } +} + +Protobuf::util::Status MessageUtil::loadFromJsonNoThrow(const std::string& json, + Protobuf::Message& message, + bool& has_unknown_fileld) { + has_unknown_fileld = false; Protobuf::util::JsonParseOptions options; options.case_insensitive_enum_parsing = true; // Let's first try and get a clean parse when checking for unknown fields; @@ -248,7 +267,7 @@ void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& messa const auto strict_status = Protobuf::util::JsonStringToMessage(json, &message, options); if (strict_status.ok()) { // Success, no need to do any extra work. - return; + return strict_status; } // If we fail, we see if we get a clean parse when allowing unknown fields. // This is essentially a workaround @@ -259,15 +278,11 @@ void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& messa const auto relaxed_status = Protobuf::util::JsonStringToMessage(json, &message, options); // If we still fail with relaxed unknown field checking, the error has nothing // to do with unknown fields. - if (!relaxed_status.ok()) { - throw EnvoyException("Unable to parse JSON as proto (" + relaxed_status.ToString() + - "): " + json); + if (relaxed_status.ok()) { + has_unknown_fileld = true; + return strict_status; } - // We know it's an unknown field at this point. If we're at the latest - // version, then it's definitely an unknown field, otherwise we try to - // load again at a later version. - validation_visitor.onUnknownField("type " + message.GetTypeName() + " reason " + - strict_status.ToString()); + return relaxed_status; } void MessageUtil::loadFromJson(const std::string& json, ProtobufWkt::Struct& message) { @@ -526,8 +541,20 @@ void MessageUtil::jsonConvert(const ProtobufWkt::Struct& source, jsonConvertInternal(source, validation_visitor, dest); } -void MessageUtil::jsonConvertValue(const Protobuf::Message& source, ProtobufWkt::Value& dest) { - jsonConvertInternal(source, ProtobufMessage::getNullValidationVisitor(), dest); +bool MessageUtil::jsonConvertValue(const Protobuf::Message& source, ProtobufWkt::Value& dest) { + Protobuf::util::JsonPrintOptions json_options; + json_options.preserve_proto_field_names = true; + std::string json; + auto status = Protobuf::util::MessageToJsonString(source, &json, json_options); + if (!status.ok()) { + return false; + } + bool has_unknow_field; + status = MessageUtil::loadFromJsonNoThrow(json, dest, has_unknow_field); + if (status.ok() || has_unknow_field) { + return true; + } + return false; } ProtobufWkt::Struct MessageUtil::keyValueStruct(const std::string& key, const std::string& value) { diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index e8f4a61cd90b6..f4107c669c285 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -252,6 +252,16 @@ class MessageUtil { static void loadFromJson(const std::string& json, Protobuf::Message& message, ProtobufMessage::ValidationVisitor& validation_visitor); + /** + * Return ok only when strict conversion(don't ignore unknown field) succeeds. + * Return error status for strict conversion and set has_unknown_field to true if relaxed + * conversion(ignore unknown field) succeeds. + * Return error status for relaxed conversion and set has_unknown_field to false if relaxed + * conversion(ignore unknown field) fails. + */ + static Protobuf::util::Status loadFromJsonNoThrow(const std::string& json, + Protobuf::Message& message, + bool& has_unknown_fileld); static void loadFromJson(const std::string& json, ProtobufWkt::Struct& message); static void loadFromYaml(const std::string& yaml, Protobuf::Message& message, ProtobufMessage::ValidationVisitor& validation_visitor); @@ -416,7 +426,8 @@ class MessageUtil { static void jsonConvert(const ProtobufWkt::Struct& source, ProtobufMessage::ValidationVisitor& validation_visitor, Protobuf::Message& dest); - static void jsonConvertValue(const Protobuf::Message& source, ProtobufWkt::Value& dest); + // Convert a message to a ProtobufWkt::Value, return false upon failure. + static bool jsonConvertValue(const Protobuf::Message& source, ProtobufWkt::Value& dest); /** * Extract YAML as string from a google.protobuf.Message. diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index c89d4f78c5003..4f34718a4f211 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -1486,14 +1486,14 @@ TEST_F(ProtobufUtilityTest, JsonConvertCamelSnake) { .string_value()); } -// Test the jsonConvertValue happy path. Failure modes are converted by jsonConvert tests. +// Test the jsonConvertValue in both success and failure modes. TEST_F(ProtobufUtilityTest, JsonConvertValueSuccess) { { envoy::config::bootstrap::v3::Bootstrap source; source.set_flags_path("foo"); ProtobufWkt::Value tmp; envoy::config::bootstrap::v3::Bootstrap dest; - MessageUtil::jsonConvertValue(source, tmp); + EXPECT_TRUE(MessageUtil::jsonConvertValue(source, tmp)); TestUtility::jsonConvert(tmp, dest); EXPECT_EQ("foo", dest.flags_path()); } @@ -1502,12 +1502,19 @@ TEST_F(ProtobufUtilityTest, JsonConvertValueSuccess) { ProtobufWkt::StringValue source; source.set_value("foo"); ProtobufWkt::Value dest; - MessageUtil::jsonConvertValue(source, dest); + EXPECT_TRUE(MessageUtil::jsonConvertValue(source, dest)); ProtobufWkt::Value expected; expected.set_string_value("foo"); EXPECT_THAT(dest, ProtoEq(expected)); } + + { + ProtobufWkt::Duration source; + source.set_seconds(-281474976710656); + ProtobufWkt::Value dest; + EXPECT_FALSE(MessageUtil::jsonConvertValue(source, dest)); + } } TEST_F(ProtobufUtilityTest, YamlLoadFromStringFail) {