Final placeholder must work as a catch-all mask

[ig-sinicyn]

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.

[rathga]

Is there a PR to go with this?

I'm certainly interested in fixing the /api/blah/{everything} vs /api/blah{everything} duplication issue

[raman-m]

See my answer

[ig-sinicyn]

Here's a raw code. I did not backport tests yet so there may be silly typos added at backporting time:)

[raman-m]

Please, update your forked repo!

[winromulus]

@ig-sinicyn Would it make sense to mark the optional placeholder as {?name} so we can differentiate between optional and hard requirement?

[ig-sinicyn]

@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:(

[raman-m]

Need more progress? Really? :)

[Woody1193]

Has this been completed? I just ran into the issue as well

[raman-m]

See my answer

[stefan-schweiger]

@TomPallister is there any willingness to accept a potential PR and get this issue resolved?

[raman-m]

Tom lives in the Kingdom far far away! 🤣

[raman-m]

• Upstream route templates that end in a slash are not 'found'. #649 double checked.
  URLs ending (not ending) with slash are supported
• Empty placeholders: Failed to match Route configuration for upstream path #748 by PR #748 Match Route configurations for upstream paths when empty Catch-All placeholders at the end of template #1911
  So, empty placeholders for Catch-All are supported also starting version 23.0.0

Just give me feedback please, what else do you need?