Align wildcard handling with up-spec and up-rust#317
Conversation
|
Code coverage report is ready! 📈
|
PLeVasseur
left a comment
PLeVasseur
left a comment
There was a problem hiding this comment.
Hey @lukas-he -- thanks for taking on updating this! I left a couple of comments to suggest a fix to match spec and an idea to align with up-rust.
src/datamodel/validator/UUri.cpp
Outdated
| bool uses_wildcards(const v1::UUri& uuri) { | ||
| constexpr auto LOWER_8_BIT_MASK = 0xFF; | ||
| bool has_wildcard_authority(const v1::UUri& uuri) { | ||
| return uuri.authority_name().find_first_of('*') != std::string::npos; |
There was a problem hiding this comment.
I think this is not equivalent to the spec, correct?
MAY have its authority_name set to the * (U+002A, Asterisk) character in order to match any (including no) authority.
So this code would call it wildcard authority if there's asterisk at any location in the string, right?
Can we make a simple equality comparison as is done in up-rust?
There was a problem hiding this comment.
Yes, I totally agree. We must consider the methods relying on that, i.e., the checks for valid RPC method/response.
There was a problem hiding this comment.
Just to confirm: this still needs updating, right?
There was a problem hiding this comment.
It depends, if we say that these methods and responses do not allow any wildcards and a wildcard is given if verify_no_wildcards == false, then the corresponding modification is already made. If these methods require the "old" definition of authority wildcard, then we must change something.
There was a problem hiding this comment.
Ah I see where the miscommunication may be creeping in.
In up-spec there's simply the concept of the authority being wildcard ("*") or not. There's no concept of an authority containing a wildcard ("otherstuff1*otherstuff2").
Note that you've updated the test here
uuri.set_authority_name("hello*");
was replaced with
uuri.set_authority_name("*");
which is correct
However, the code which checks for this looks for any instance of * in the authority string. I think we could change this:
return uuri.authority_name().find_first_of('*') != std::string::npos;
to a simple equality check between uuri.authority_name() and *.
In the spec's UUri Pattern Matching section for example:
def: matches_authority(candidate : UUri) : Boolean =
self.authority_name = '*'
or
self.authority_name = candidate.authority_name
Hope that's clear! If not, let's hop on a call tomorrow to talk about it.
src/datamodel/validator/UUri.cpp
Outdated
| return uuri.resource_id() == LOWER_16_BIT_MASK; | ||
| } | ||
|
|
||
| bool uses_wildcards(const v1::UUri& uuri) { |
There was a problem hiding this comment.
This is somewhat the dual of the verify_no_wildcards() function within up-rust.
Perhaps we could remove uses_wildcards() and instead implement verify_no_wildcards() in up-cpp.
My thought process is that up-rust has been used more extensively and they chose this function instead, so aligning with that is probably more sensible.
What do you think?
There was a problem hiding this comment.
I also agree with that. I think that we can benefit if the concepts between up-cpp and up-rust don't differ that much.
| EXPECT_FALSE(uses_wildcards(uuri)); | ||
| } | ||
|
|
||
| { // Change Authority name to "hello*" (Any) |
There was a problem hiding this comment.
Okay... I see why you implemented the wildcard authority check as you did now.
Seems there was a misunderstanding in up-cpp's implementation of the spec.
Please reference the comment I left up above and we can adjust the test code in tandem.
There was a problem hiding this comment.
A first suggestion for the tests in the next commit.
|
Code coverage report is ready! 📈
|
|
Code coverage report is ready! 📈
|
|
Code coverage report is ready! 📈
|
|
Code coverage report is ready! 📈
|
PLeVasseur
left a comment
PLeVasseur
left a comment
There was a problem hiding this comment.
Thanks for contributing this @lukas-he! I found a minor thing in a comment, but I'll just go ahead and apply the change and we can merge.
|
Code coverage report is ready! 📈
|
2 participants
This PR closes #313.