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

Do not crash when converting YAML to JSON fails #4110

Merged
merged 7 commits into from
Aug 12, 2018
Merged

Do not crash when converting YAML to JSON fails #4110

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2b42ec8
Do not crash when converting yaml to json fails
dio Aug 10, 2018
7611631
Clarify test
dio Aug 10, 2018
2162723
Add valid loading
dio Aug 10, 2018
2f20370
Defer the checking at protobuf util
dio Aug 10, 2018
ac89352
Remove superfluous semicolons
dio Aug 10, 2018
16b8691
Remove TODO
dio Aug 10, 2018
c2aff43
review comment: remove "&" and fix comment
dio Aug 11, 2018
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
12 changes: 12 additions & 0 deletions include/envoy/json/json_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,18 @@ class Object {
*/
virtual bool isNull() const PURE;

/**
* Determine if an object has type Object.
* @return bool is the object an Object?
*/
virtual bool isObject() const PURE;

/**
* Determine if an object has type Array.
* @return bool is the object an Array?
*/
virtual bool isArray() const PURE;

/**
* Get an array by name.
* @param name supplies the key name.
Expand Down
4 changes: 2 additions & 2 deletions source/common/json/json_loader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ class Field : public Object {
static FieldSharedPtr createNull() { return FieldSharedPtr{new Field(Type::Null)}; }

bool isNull() const override { return type_ == Type::Null; }
bool isArray() const { return type_ == Type::Array; }
bool isObject() const { return type_ == Type::Object; }
bool isArray() const override { return type_ == Type::Array; }
bool isObject() const override { return type_ == Type::Object; }

// Value factory.
template <typename T> static FieldSharedPtr createValue(T value) {
Expand Down
10 changes: 8 additions & 2 deletions source/common/protobuf/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,14 @@ void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& messa
}

void MessageUtil::loadFromYaml(const std::string& yaml, Protobuf::Message& message) {
const std::string json = Json::Factory::loadFromYamlString(yaml)->asJsonString();
loadFromJson(json, message);
const auto& loaded_object = Json::Factory::loadFromYamlString(yaml);
Copy link
Member

Choose a reason for hiding this comment

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

nit: technically this should not be a reference so I would remove &.

// Load to the message if the loaded object has type Object or Array.
Copy link
Member

Choose a reason for hiding this comment

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

s/to the/the

if (loaded_object->isObject() || loaded_object->isArray()) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess this works because 100% of the time we use loadFromYaml it's for non-trivial compound types.

const std::string json = loaded_object->asJsonString();
loadFromJson(json, message);
return;
}
throw EnvoyException("Unable to convert YAML as JSON: " + yaml);
}

void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& message) {
Expand Down
20 changes: 20 additions & 0 deletions test/common/protobuf/utility_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,26 @@ TEST(UtilityTest, JsonConvertCamelSnake) {
.string_value());
}

TEST(UtilityTest, YamlLoadFromStringFail) {
envoy::config::bootstrap::v2::Bootstrap bootstrap;
// Verify loadFromYaml can parse valid YAML string.
MessageUtil::loadFromYaml("node: { id: node1 }", bootstrap);
// Verify loadFromYaml throws error when the input is an invalid YAML string.
EXPECT_THROW_WITH_MESSAGE(
MessageUtil::loadFromYaml("not_a_yaml_that_can_be_converted_to_json", bootstrap),
EnvoyException, "Unable to convert YAML as JSON: not_a_yaml_that_can_be_converted_to_json");
// When wrongly inputted by a file path, loadFromYaml throws an error.
EXPECT_THROW_WITH_MESSAGE(MessageUtil::loadFromYaml("/home/configs/config.yaml", bootstrap),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this path is available in CI? Is it possible to do a more direct test case of the actual issue potentially not even loading from a file?

Copy link
Member Author

@dio dio Aug 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This simulates what is described in the issue #3990, that gave input to --config-yaml a file path string (while this config option expects a valid yaml string instead). #3990 (comment)

This doesn't try to load a file. The --config-yaml perceives this as a string (in this case the string value is a file path).

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. Got it.

EnvoyException,
"Unable to convert YAML as JSON: /home/configs/config.yaml");
// Verify loadFromYaml throws error when the input leads to an Array. This error message is
// arguably more useful than only "Unable to convert YAML as JSON".
EXPECT_THROW_WITH_MESSAGE(MessageUtil::loadFromYaml("- node: { id: node1 }", bootstrap),
EnvoyException,
"Unable to parse JSON as proto (INVALID_ARGUMENT:: invalid name : Root "
"element must be a message.): [{\"node\":{\"id\":\"node1\"}}]");
}

TEST(DurationUtilTest, OutOfRange) {
{
ProtobufWkt::Duration duration;
Expand Down