udpa: UDPA URL encoding/decoding utils. #11805
Conversation
This patch introduces support for encoding/decoding udpa::core::v1::ResourceLocator, in addition to the existing support for udpa::core::v1::ResourceName. Some refactoring and attention to the URI/URN/URL distinction (as per RFC3986) is introduced. Risk level: Low (not used) Testing: Additional unit tests for URLs added. Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
| UdpaResourceIdentifier::decodeUrl(EscapedUrlWithManyQueryParamsAndDirectives); | ||
| EXPECT_EQ("f123%/?#o", resource_locator.authority()); | ||
| EXPECT_EQ("envoy.config.listener.v3.Listener", resource_locator.resource_type()); | ||
| EXPECT_EQ(3, resource_locator.id().size()); |
There was a problem hiding this comment.
I think this would be better written as follows, both for readability and for more useful output when the test fails:
EXPECT_THAT(resource_locator.id(), ::testing::ElementsAre("b%:/?#[]ar", "", "baz"));
Same thing for all repeated fields throughout this file.
There was a problem hiding this comment.
Done, but not for directives, since when working with proto types you don't get for free equality and nice debug string output. It's possible to do this with some extra work, but not worth it in this case IMHO.
Signed-off-by: Harvey Tuch <[email protected]>
|
|
||
| #define EXPECT_CONTEXT_PARAMS(context_params, ...) \ | ||
| { \ | ||
| std::map<std::string, std::string> param_map((context_params).begin(), \ |
There was a problem hiding this comment.
Why is it necessary to convert to std::map<> here? Doesn't the protobuf map field accessor provide the same interface as std::map<>, such that it works directly with UnorderedElementsAre()?
I would think that you could just say EXPECT_THAT(resource_name.context().params(), UnorderedElementsAre(...)).
There was a problem hiding this comment.
That doesn't work, since what you get is a comparison on the proto MapPair object, which does not pretty-print on failure, i.e. you see stuff like:
Expected: has 2 elements and there exists some permutation of elements such that:
- element #0 has a first field that is equal to "%#[]&=", and has a second field that is equal to "bar", and
- element #1 has a first field that is equal to "foo", and has a second field that is equal to "%#[]&="
Actual: { 48-byte object <06-66 6F-6F 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 0C-25 23-5B 5D-26 3D-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00>, 48-byte object <10-25 23-5B 5D-26 3D-61 62-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 14-63 64-65 25-23 5B-5D 26-3D 66-00 00-00 00-00 00-00 00-00 00-00 00-00>, 48-byte object <0C-25 23-5B 5D-26 3D-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 06-62 61-72 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00> }, which has 3 elements
There was a problem hiding this comment.
Ah, okay. That's too bad.
Might be worth adding a comment about why this conversion is necessary.
Signed-off-by: Harvey Tuch <[email protected]>
|
|
||
| #define EXPECT_CONTEXT_PARAMS(context_params, ...) \ | ||
| { \ | ||
| std::map<std::string, std::string> param_map((context_params).begin(), \ |
There was a problem hiding this comment.
Ah, okay. That's too bad.
Might be worth adding a comment about why this conversion is necessary.
Signed-off-by: Harvey Tuch <[email protected]>
This patch introduces support for encoding/decoding
udpa::core::v1::ResourceLocator, in addition to the existing support for
udpa::core::v1::ResourceName. Some refactoring and attention to the
URI/URN/URL distinction (as per RFC3986) is introduced.
Part of #11264
Risk level: Low (not used)
Testing: Additional unit tests for URLs added.
Signed-off-by: Harvey Tuch [email protected]