Ocelot
Ocelot copied to clipboard
ThreeMammals
[Proposal] Final placeholder must work as a catch-all mask
Good day here:)
I have meet multiple issues with route templates that seems to be widespread (#1264, #1067, #649 etc).
As a workaround I've implemented custom logic for route template handling. As far as I can see from reading the code and documentation there is almost no changes in logic. The only difference is, last placeholder in the template path part is treated as .*, not .+.If you're interested in it I'will prepare a PR with code to discuss.
How it's implemented
To be honest, code is simpler than the description:)
UPD I've changed tokenization logic a little to match current ocelot behavior. As it is for now all tests are green except the
should_return_response_200_favouring_forward_slash and should_return_not_found routing tests. These two are failing as the match behavior for /{url} and /products/{productId} has been changed.
1 Tokenizer
There is a TemplateTokenizer, a class that splits route template into a sequence of template tokens (actually, a ReadOnlyMemory<char> span with token kind attached).
I've introduced minimal set of token kinds but it can be changed to better match desired behavior. Current set is
- normal text
- inner uri placeholder
- catch all placeholder of a path part
- catch all placeholder of a path part (prev char is separator)
- placeholder in a query part.
I've preferred to make token kinds a bit verbose to simplify rest of the code. Outside of the tokenizer, there is no need to check whether there is a / before last path placeholder, or whether the placeholder is located at query or path part.
As example,
/a/{sample}/route/{rest}?someArg=1&{queryRest} will be splitted to:
/a/ # normal
{sample} # inner uri placeholder
/route/ # normal
{rest} # catch all path part placeholder, prev char is '/'
?someArg=1& # normal
{queryRest} # query part placeholder
2 Regex generation helper
The helper produces a regex from a sequence of the tokens, placeholders are represented as a named groups. For the sample above output will be
^(?in)/a/(?'sample'[^/?]+)/route(|/?(?'rest'.*))\?someArg=1&(?'queryRest'[^&]+).*$
or, expanded
^ # begin of string
(?in) # case-insensitive, named groups only
/a/ # normal text
(?'sample'[^/?]+) # named group 'Sample', matches everything up to '/' or '?' char
/route # normal text, note that trailing slash is trimmed
(|/?(?'rest'.*)) # named group 'rest', matches empty string -or- slash followed by any number of characters
\?someArg=1& # normal text, the '?' is escaped
(?'queryRest'[^&]+) # named group 'queryRest', matches everything up to '&' char
.* # catch-all stub to match rest of the query part
$ # endof string
3 Integrate regex generator/tokenizer into ocelot stack.
The rest of the code is pretty straightforward. I've replaced
IUpstreamTemplatePatternCreator, IPlaceholderNameAndValueFinder, IDownstreamPathPlaceholderReplacer and it just works:)
4 Set of tests
Set of test with cases that proofs that every service is working as desired.
Final thoughts
Actually, all above was not a big deal due to the good architecture. Almost every part of the Ocelot is a DI service and implementation is replaceable, nice thing to see:)
Also, I prefer proposed code over existing even without the fix. It is much more compact and readable and have no code duplication. If you need to change template parsing logic, all you have to do is to change list of token kinds, modify the tokenizer (hard part, but it is covered with tests) and modify rest of the pattern-related code (easy part and also is covered with tests);
Performance-related: we saw no difference in our loadtests. There are some optimizations to do in the future (as example, value finder may reuse match results from the template matcher), but they are definitely out of the scope of current proposal.
Coverage: 85.102% (-0.07%) from 85.175% when pulling a6d54e84d731519df948ee5b822b48030f240907 on jsv2 into c939dd1e9a3748cda61f58e325b326b1e27eec18 on main.
Is there a PR to go with this?
I'm certainly interested in fixing the /api/blah/{everything} vs /api/blah{everything} duplication issue
I have working implementation but I do not want to spend time on a PR if it is of no interest.
Here's a raw code. I did not backport tests yet so there may be silly typos added at backporting time:)
@ig-sinicyn - I'm also interested in seeing this as a PR. Currently hit this exact issue.
@ig-sinicyn Would it make sense to mark the optional placeholder as {?name} so we can differentiate between optional and hard requirement?
@winromulus such decisions have to be made by ocelot team. There are no new commits or merged PRs since end of May so I do not expect any progress here:(
@TomPallister is there any willingness to accept a potential PR and get this issue resolved?
