From 0961268c1577e95dce013e742a5d28bf47715fcc Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 12 Nov 2021 04:14:43 +0000 Subject: [PATCH 1/6] non-throw version json convert value Signed-off-by: chaoqin-li1123 --- .../formatter/substitution_formatter.cc | 10 ++-- source/common/protobuf/utility.cc | 49 ++++++++++++++----- source/common/protobuf/utility.h | 5 +- test/common/protobuf/utility_test.cc | 11 ++++- 4 files changed, 54 insertions(+), 21 deletions(-) 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..609856a9a6779 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..f3862757408a8 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -252,6 +252,8 @@ class MessageUtil { static void loadFromJson(const std::string& json, Protobuf::Message& message, ProtobufMessage::ValidationVisitor& validation_visitor); + 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 +418,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..4de2a039f5fef 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -1493,7 +1493,7 @@ TEST_F(ProtobufUtilityTest, JsonConvertValueSuccess) { 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) { From a8cd46ac793190b09749423a6501853b1e8b50a9 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 12 Nov 2021 04:44:27 +0000 Subject: [PATCH 2/6] format Signed-off-by: chaoqin-li1123 --- source/common/protobuf/utility.cc | 8 ++++---- source/common/protobuf/utility.h | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 609856a9a6779..006182f0d7ac0 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -251,13 +251,13 @@ void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& messa status.ToString()); } else { // If the error has nothing to do with unknown field. - throw EnvoyException("Unable to parse JSON as proto (" + status.ToString() + - "): " + json); + 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) { +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; diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index f3862757408a8..ebd210debd307 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -252,8 +252,9 @@ class MessageUtil { static void loadFromJson(const std::string& json, Protobuf::Message& message, ProtobufMessage::ValidationVisitor& validation_visitor); - static Protobuf::util::Status loadFromJsonNoThrow(const std::string& json, Protobuf::Message& message, - bool& has_unknown_fileld); + 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); From f58dacddb537a3edc839b96a417b21b597d35ce8 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 12 Nov 2021 21:49:47 +0000 Subject: [PATCH 3/6] add comments Signed-off-by: chaoqin-li1123 --- source/common/protobuf/utility.h | 6 ++++++ test/common/protobuf/utility_test.cc | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index ebd210debd307..4989715659ca8 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -252,6 +252,12 @@ 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 unknownfield) succeeds. + * Return error status for strict conversion and set has_unknow_field to true if relaxed + * conversion(ignore unknow field) succeeds. Return error status relaxed conversion and set + * has_unknow_field to false if relaxed conversion(ignore unknow field) fails. + */ static Protobuf::util::Status loadFromJsonNoThrow(const std::string& json, Protobuf::Message& message, bool& has_unknown_fileld); diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 4de2a039f5fef..4f34718a4f211 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -1486,7 +1486,7 @@ 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; From cdee075d429dc824e42bc065227ef2e56c0da839 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 12 Nov 2021 22:02:09 +0000 Subject: [PATCH 4/6] format Signed-off-by: chaoqin-li1123 --- source/common/protobuf/utility.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 4989715659ca8..585c27e0eedad 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -254,9 +254,10 @@ class MessageUtil { ProtobufMessage::ValidationVisitor& validation_visitor); /** * Return ok only when strict conversion(don't ignore unknownfield) succeeds. - * Return error status for strict conversion and set has_unknow_field to true if relaxed - * conversion(ignore unknow field) succeeds. Return error status relaxed conversion and set - * has_unknow_field to false if relaxed conversion(ignore unknow field) fails. + * Return error status for strict conversion and set has_unknown_field to true if relaxed + * conversion(ignore unknow field) succeeds. + * Return error status for relaxed conversion and set has_unknown_field to false if relaxed + * conversion(ignore unknow field) fails. */ static Protobuf::util::Status loadFromJsonNoThrow(const std::string& json, Protobuf::Message& message, From 52c3855ee2766fad517a07c5d8dbba7480fbd095 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 12 Nov 2021 22:19:02 +0000 Subject: [PATCH 5/6] format Signed-off-by: chaoqin-li1123 --- source/common/protobuf/utility.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 585c27e0eedad..8bd8a1b0f1dc4 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -254,7 +254,7 @@ class MessageUtil { ProtobufMessage::ValidationVisitor& validation_visitor); /** * Return ok only when strict conversion(don't ignore unknownfield) succeeds. - * Return error status for strict conversion and set has_unknown_field to true if relaxed + * Return error status for strict conversion and set has_unknown_field to true if relaxed * conversion(ignore unknow field) succeeds. * Return error status for relaxed conversion and set has_unknown_field to false if relaxed * conversion(ignore unknow field) fails. From 7e409366fd6130573b6ec25e916f99cadc2edc36 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Sat, 13 Nov 2021 04:16:17 +0000 Subject: [PATCH 6/6] fix format_pre Signed-off-by: chaoqin-li1123 --- source/common/protobuf/utility.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 8bd8a1b0f1dc4..f4107c669c285 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -253,11 +253,11 @@ 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 unknownfield) succeeds. + * 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 unknow field) succeeds. + * conversion(ignore unknown field) succeeds. * Return error status for relaxed conversion and set has_unknown_field to false if relaxed - * conversion(ignore unknow field) fails. + * conversion(ignore unknown field) fails. */ static Protobuf::util::Status loadFromJsonNoThrow(const std::string& json, Protobuf::Message& message,