Conversation
source/common/router/config_impl.cc
Outdated
| rate_limit_policy_(route), shadow_policy_(route), | ||
| priority_(ConfigUtility::parsePriority(route)) { | ||
| priority_(ConfigUtility::parsePriority(route)), | ||
| opaque_config_(route.getObject("opaque_config")) { |
There was a problem hiding this comment.
I'm not sure that is safe. The main JSON file gets blown away and your are going to point to nothing. This is just a weak pointer. Your test doesn't cover this because you keep the JSON parent alive. I would try switching your test to destroy the loader before you run the tests and it will probably crash.
With that said, I'm not crazy about returning raw JSON here. Can we potentially just do std::unordered_map<std::string, std::string>? Is that general enough for now?
|
For whatever the final solution ends up being you need to add config docs. |
| .. code-block:: json | ||
|
|
||
| [ | ||
| {"name": "...", "value": "..."} |
There was a problem hiding this comment.
Seems weird to do this instead of just a JSON object. Does order matter?
There was a problem hiding this comment.
I originally supported Json objects but matt and I decided to move to this for simplicity.
There was a problem hiding this comment.
I thought Matt's comment was about how the JSON is returned. An unorderd map would be a flat JSON Object where all values are strings. That's a bit different from an array of JSON objects with name and value pairs. This format seems like it's the same structure represented in a more obtuse way.
There was a problem hiding this comment.
I wasn't able to figure out how to get the schema to support what you are describing. @mattklein123 thoughts?
There was a problem hiding this comment.
http://json-schema.org/latest/json-schema-validation.html#rfc.section.5.17 I think you want additionalProperties or patternProperties.
There was a problem hiding this comment.
{"type": "object",
"additionalProperties": {"type": "string"}}^ maybe that?
There was a problem hiding this comment.
If there is a way to do it in the schema, I agree that a JSON dictionary of strings is better. I'm just not sure how to do it. If the above works then that's fine.
|
👍 assuming this config will this be easily accessible in |
4b7cbca to
3fef8c4
Compare
| } | ||
| } | ||
|
|
||
| std::string asString() const { |
There was a problem hiding this comment.
please add dedicated test for this function, including the exception part.
|
👍 looks good to me. |
| if (key == "name1") { | ||
| EXPECT_EQ("value1", value.asString()); | ||
| } else { | ||
| EXPECT_THROW(value.asString(), Exception); |
There was a problem hiding this comment.
This should throw a JsonException
692e19b to
8861f39
Compare
|
🔨 |
RyanMcG
left a comment
There was a problem hiding this comment.
I don't think my opinion matters here anymore, but 👍
|
@wgallagher should probably add "Fix #413" to the description. |
fixes #413