#2199 Adding support of placeholder matching between slashes👍 #2200

ggnaegi · 2024-11-12T22:06:04Z

Fixes #2199

Ocelot can't match placeholders between slashes #2199

Proposed Changes

Reworked UrlPathPlaceholderNameAndValueFinder.cs supporting now embedded placeholders

Pattern matching with Regexes

Regex regex = new($@"\{PlaceHolderLeft}(.*?)\{PlaceHolderRight}", RegexOptions.None, TimeSpan.FromSeconds(1));
template = EscapeExceptBraces(template);
string regexPattern = $"^{regex.Replace(template, match => $"(?<{match.Groups[1].Value}>[^&]*)")}";

Some tweaks:

If the path template contains a catch-all query, then return the catch-all placeholder with an empty value

private static (bool IsMatch, string Placeholder) IsCatchAllQuery(string template)
    {
        Regex catchAllQueryRegex = new($@"^[^{{}}]*\?\{PlaceHolderLeft}(.*?)\{PlaceHolderRight}$", RegexOptions.None, TimeSpan.FromMilliseconds(300));
        Match catchAllMatch = catchAllQueryRegex.Match(template);

        return (catchAllMatch.Success, catchAllMatch.Success ? catchAllMatch.Groups[1].Value : string.Empty);
    }

If the path template contains a catch all path, then return the catch-all placeholder with an empty value
Skipping the query evaluation if not present in the path template

Added some unit tests with complex path templates.
Added code comments

…oduct/products/categories/ since the last slash is part of the template and not the catch-all placeholder

ggnaegi · 2024-11-12T22:19:10Z

@raman-m yeaaaah, the checks have passed ;-)

raman-m · 2024-11-13T09:24:38Z

Gui, how quickly do you need this delivered? Do you require the officially released NuGet package, or can you wait for the next v24.0?

raman-m · 2024-11-13T09:26:56Z

yeaaaah, the checks have passed ;-)

Congratulations! 🎉 It appears that the old functionality remains intact.

ggnaegi · 2024-11-13T09:32:45Z

Gui, how quickly do you need this delivered? Do you require the officially released NuGet package, or can you wait for the next v24.0?

It's urgent, but, when it is foreseen to release v24.0?

raman-m

My suggestions are below 👇

src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs

...elot.UnitTests/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinderTests.cs

raman-m · 2024-11-13T11:06:30Z

It's urgent, but, when it is foreseen to release v24.0?

@ggnaegi, I'm OK to release minor version 23.4.0 with the already merged 2 features + this feat. Will you prepare the release PR for urgent delivery? Will you write the release notes, etc.? Or will our next minor release be without release notes? 😄

when it is foreseen to release v24.0?

First days, 1st week of December, I guess...

ggnaegi · 2024-11-13T11:17:06Z

It's urgent, but, when it is foreseen to release v24.0?

@ggnaegi, I'm OK to release minor version 23.4.0 with the already merged 2 features + this feat. Will you prepare the release PR for urgent delivery? Will you write the release notes, etc.? Or will our next minor release be without release notes? 😄

when it is foreseen to release v24.0?

First days, 1st week of December, I guess...

Ok let's do this, minor version 23.4.0

raman-m · 2024-11-13T11:26:33Z

Ok let's do this, minor version 23.4.0

OK! Your destiny, Gui, is to write the documentation for the feature. It seems we have to review all Routing documentation to inform the community that we support placeholders in the middle.
As you know, the documentation now includes all placeholder examples "between slashes".

raman-m · 2024-11-14T14:49:15Z

@ggnaegi Are you going to write acceptance tests and update docs?

...elot.UnitTests/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinderTests.cs

ggnaegi · 2024-11-14T15:27:36Z

@ggnaegi Are you going to write acceptance tests and update docs?

Yes!

…aceholderNameAndValueFinderTests.cs Recover old test for feat 89. Remove BDDfy from new unit tests. Apply AAA-pattern.

raman-m · 2024-11-14T15:47:14Z

The build failed because of unsupported can_match_down_stream_url_with_downstream_template_with_place_holder_to_final_url_path test❗
So, the old feat #89 is broken after refactroring❗
We must support old functionality, at least try to support...

I've recovered the old unit test

ggnaegi · 2024-11-14T15:55:44Z

The build failed because of unsupported can_match_down_stream_url_with_downstream_template_with_place_holder_to_final_url_path test❗ So, the old feat #89 is broken after refactroring❗ We must support old functionality, at least try to support...

I've recovered the old unit test

Yes, but the old unit test, is totally crap! I'm wondering why it should pass! It's stupid

raman-m · 2024-11-14T15:57:06Z

Yes, but the old unit test, is totally crap! I'm wondering why it should pass! It's stupid

Do you think we had a stupid contributor in #89 ? 😄

ggnaegi · 2024-11-14T16:04:58Z

Yes, but the old unit test, is totally crap! I'm wondering why it should pass! It's stupid

Do you think we had a stupid contributor in #89 ? 😄

this.Given(x => x.GivenIHaveAUpstreamPath("api/product/products/categories/"))
                 .And(x => x.GivenIHaveAnUpstreamUrlTemplate("api/{finalUrlPath}/")) // Gui, don't remove final slash! We should not break old functionality
                 .When(x => x.WhenIFindTheUrlVariableNamesAndValues())
                 .And(x => x.ThenTheTemplatesVariablesAre(expectedTemplates))
                 .BDDfy();
        }

Why on earth {finalUrlPath} should match /product/products/categories/ if the slash is not part of it?????

Besides, the acceptance test is passing...
should_return_response_200_with_placeholder_for_final_url_path()

@raman-m

raman-m

Sorry, I require the following missing artifacts from the development process:

Acceptance test(s) with the same data as the new unit tests
Updated documentation for the Routing feature, at least draft with ideas to review

P.S. Regarding the #89 failed test

I saw that the unit test fails, but the corresponding #89 acceptance test doesn't, which is strange. Logically, both tests should fail together or they should both pass. I recommend carefully reviewing the #89 and syncing the test data, at least for a final decision.

raman-m · 2024-11-15T16:13:43Z

Upon reviewing the old #89 tests, I noticed that the author made a typo error in the unit test, specifically an extra / on line 152, in the first commit that only included the unit test. Subsequently, Tom requested an acceptance test, which lacked a final slash in the test data. To the best of my knowledge, the acceptance test aligns with the requirements described in #89, but the unit test does not. It appears to be a mistyping by the author.

The tests have been reviewed and are now successfully passing. Apologies for any inconvenience over the past two days!
FYI, the author implemented a fix in the code, PR line 47, which reads urlPathTemplate.Trim('/').EndsWith(variableName). Currently, this fix for the unit test is located at this line:

Ocelot/src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs

Line 124 in da9d6fa

    
           if (positionOfNextSlash == -1 || (urlPathTemplate.Trim(delimiter).EndsWith(variableName) && string.IsNullOrEmpty(query)))

, where the slash trimming is still presented as Trim(delimiter). This is why the old typo and the corrected unit test coexist in the codebase. However, this does not align with the actual requirements of feature #89.

If you don't mind, I will refactor both files along with the tests to adhere to our development conventions.
New tests won't change...

raman-m · 2024-11-16T19:53:37Z

@ggnaegi Are you going to write acceptance tests and update docs?

Yes!

When?

ggnaegi · 2024-11-16T19:54:52Z

@raman-m 🤣

ggnaegi · 2024-11-16T23:04:06Z

@raman-m ready

FindGroups method is private, better to return exact type. Regex optimizations across net6, 7, 8. XML dev-docs markup review. Rename 'curly bracket' to 'brace'.

raman-m

Ready for Delivery ✅

Feel free to press the magic Squash button! 🎉
If you merge today, I will prepare the release v23.4.0 for tomorrow.

raman-m · 2024-11-17T13:28:13Z

docs/features/routing.rst

+Embedded Placeholders [#f1]_
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^


It's okay now. Thank you for writing this! I will review the section again during the release.

raman-m · 2024-11-17T13:44:17Z

test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs

+    [Theory]
+    [Trait("Bug", "2199")]
+    [Trait("Feat", "2200")]
+    [InlineData("/api/invoices/{url0}-{url1}-{url2}", "/api/invoices_{url0}/{url1}-{url2}_abcd/{url3}?urlId={url4}", 
+        "/api/invoices_abc/def-ghi_abcd/xyz?urlId=bla", "/api/invoices/abc-def-ghi", "?urlId=bla")]
+    [InlineData("/api/products/{category}-{subcategory}/{filter}", "/api/products_{category}/{subcategory}_details/{itemId}?filter={filter}", 
+        "/api/products_electronics/computers_details/123?filter=active", "/api/products/electronics-computers/active", "")]
+    [InlineData("/api/users/{userId}/posts/{postId}/{lang}", "/api/users/{userId}/{postId}_content/{timestamp}?lang={lang}", 
+        "/api/users/101/2022_content/2024?lang=en", "/api/users/101/posts/2022/en", "")]
+    [InlineData("/api/categories/{cat}-{subcat}?sort={sort}", "/api/categories_{cat}/{subcat}_items/{itemId}?sort={sort}", 
+        "/api/categories_home/furniture_items/789?sort=asc", "/api/categories/home-furniture", "?sort=asc")]
+    [InlineData("/api/orders/{order}-{detail}?status={status}", "/api/orders_{order}/{detail}_invoice/{ref}?status={status}", 
+        "/api/orders_987/abc_invoice/123?status=shipped", "/api/orders/987-abc", "?status=shipped")]
+    [InlineData("/api/transactions/{type}-{region}", "/api/transactions_{type}/{region}_summary/{year}?q={query}", 
+        "/api/transactions_sales/NA_summary/2024?q=forecast", "/api/transactions/sales-NA", "?q=forecast")]
+    [InlineData("/api/resources/{resource}-{subresource}", "/api/resources_{resource}/{subresource}_data/{id}?key={apikey}", 
+        "/api/resources_images/photos_data/555?key=xyz123", "/api/resources/images-photos", "?key=xyz123")]
+    [InlineData("/api/accounts/{account}-{detail}", "/api/accounts_{account}/{detail}_info/{id}?opt={option}", 
+        "/api/accounts_admin/settings_info/101?opt=true", "/api/accounts/admin-settings", "?opt=true")]
+    public void ShouldMatchComplexQueriesWithEmbeddedPlaceholders(string downstream, string upstream, string requestUrl, string downstreamPath, string queryString)


Thank you for adding these tests! I have a question.
Does the feat support embedded placeholders within a single query parameter and its value?
For example →

/api/invoices_{url0}/{url1}-{url2}_abcd/{url3}?Id1={id1}&filter=A{fid1}B&text=quick fox jumped over lazy {animal2} and&etc

I assume the answer is NO...

@raman-m It should work yes, it focuses on the left and right brackets for the matching

Adding support for placeholder matching between slashes.

9a25546

ggnaegi added the Routing Ocelot feature: Routing label Nov 12, 2024

ggnaegi requested review from RaynaldM and raman-m November 12, 2024 22:06

ggnaegi self-assigned this Nov 12, 2024

now the behavior is right, it can't be that {finalUrlPath} matches pr…

e3fd7ea

…oduct/products/categories/ since the last slash is part of the template and not the catch-all placeholder

ggnaegi mentioned this pull request Nov 13, 2024

Bug with catch-all routing #2169

Closed

Performance improvements for EscapeExceptBraces

fb81e5a

raman-m reviewed Nov 13, 2024

View reviewed changes

raman-m mentioned this pull request Nov 13, 2024

Ocelot can't match placeholders between slashes #2199

Closed

raman-m changed the title ~~Adding support for placeholder matching between slashes.~~ #2199 Adding support for placeholder matching between slashes, in the middle of URL path segment Nov 13, 2024

raman-m added bug Identified as a potential bug proposal Proposal for a new functionality in Ocelot high High priority labels Nov 13, 2024

raman-m added this to the Autumn'24 milestone Nov 13, 2024

raman-m reviewed Nov 14, 2024

View reviewed changes

...elot.UnitTests/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinderTests.cs Show resolved Hide resolved

EOL: test/Ocelot.UnitTests/DownstreamRouteFinder/UrlMatcher/UrlPathPl…

ee0011c

…aceholderNameAndValueFinderTests.cs Recover old test for feat 89. Remove BDDfy from new unit tests. Apply AAA-pattern.

raman-m requested changes Nov 14, 2024

View reviewed changes

ThreeMammals deleted a comment from ggnaegi Nov 15, 2024

Review user scenarios of feat ThreeMammals#89 tests

a8d7ae1

raman-m added 3 commits November 15, 2024 21:15

Refactor unit tests

5bb9a4e

Refactor acceptance tests

1d6249d

Merge branch 'develop' into features/should_match_between_slashes

19139a3

ggnaegi added 4 commits November 16, 2024 22:59

Adding documentation for embedded placeholders

0426a1b

Adding acceptance tests

2a9c722

Adding suggestions

749f175

again...

30e7873

ggnaegi changed the title ~~#2199 Adding support for placeholder matching between slashes, in the middle of URL path segment~~ #2199 Adding support of placeholder matching between slashes. Nov 16, 2024

raman-m changed the title ~~#2199 Adding support of placeholder matching between slashes.~~ #2199 Adding support of placeholder matching between slashes Nov 17, 2024

ggnaegi changed the title ~~#2199 Adding support of placeholder matching between slashes~~ #2199 Adding support of placeholder matching between slashes; Nov 17, 2024

Final code review by @raman-m.

709d37b

FindGroups method is private, better to return exact type. Regex optimizations across net6, 7, 8. XML dev-docs markup review. Rename 'curly bracket' to 'brace'.

raman-m approved these changes Nov 17, 2024

View reviewed changes

raman-m changed the title ~~#2199 Adding support of placeholder matching between slashes;~~ #2199 Adding support of placeholder matching between slashes👍 Nov 17, 2024

raman-m added the Dec'24 December 2024 release label Nov 17, 2024

ggnaegi merged commit 27d3df2 into ThreeMammals:develop Nov 17, 2024
1 check passed

raman-m mentioned this pull request Nov 18, 2024

Release 23.4 | Routing Update | McDonald's release | +semver: minor #2206

Merged

raman-m added Nov'24 November 2024 release and removed Dec'24 December 2024 release labels Nov 20, 2024

raman-m modified the milestones: Autumn'24, November'24 Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2199 Adding support of placeholder matching between slashes👍 #2200

#2199 Adding support of placeholder matching between slashes👍 #2200

ggnaegi commented Nov 12, 2024 •

edited by raman-m

Loading

ggnaegi commented Nov 12, 2024

raman-m commented Nov 13, 2024

raman-m commented Nov 13, 2024

ggnaegi commented Nov 13, 2024

raman-m left a comment

raman-m commented Nov 13, 2024 •

edited

Loading

ggnaegi commented Nov 13, 2024

raman-m commented Nov 13, 2024 •

edited

Loading

raman-m commented Nov 14, 2024

ggnaegi commented Nov 14, 2024

raman-m commented Nov 14, 2024 •

edited by ggnaegi

Loading

ggnaegi commented Nov 14, 2024

I've recovered the old unit test

raman-m commented Nov 14, 2024 •

edited by ggnaegi

Loading

ggnaegi commented Nov 14, 2024 •

edited

Loading

raman-m left a comment •

edited

Loading

raman-m commented Nov 15, 2024 •

edited

Loading

raman-m commented Nov 16, 2024

ggnaegi commented Nov 16, 2024

ggnaegi commented Nov 16, 2024

raman-m left a comment •

edited

Loading

raman-m Nov 17, 2024

raman-m Nov 17, 2024 •

edited

Loading

ggnaegi Nov 17, 2024 •

edited

Loading

		Embedded Placeholders [#f1]_
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^

#2199 Adding support of placeholder matching between slashes👍 #2200

#2199 Adding support of placeholder matching between slashes👍 #2200

Conversation

ggnaegi commented Nov 12, 2024 • edited by raman-m Loading

Fixes #2199

Proposed Changes

ggnaegi commented Nov 12, 2024

raman-m commented Nov 13, 2024

raman-m commented Nov 13, 2024

ggnaegi commented Nov 13, 2024

raman-m left a comment

Choose a reason for hiding this comment

raman-m commented Nov 13, 2024 • edited Loading

ggnaegi commented Nov 13, 2024

raman-m commented Nov 13, 2024 • edited Loading

raman-m commented Nov 14, 2024

ggnaegi commented Nov 14, 2024

raman-m commented Nov 14, 2024 • edited by ggnaegi Loading

I've recovered the old unit test

ggnaegi commented Nov 14, 2024

I've recovered the old unit test

raman-m commented Nov 14, 2024 • edited by ggnaegi Loading

ggnaegi commented Nov 14, 2024 • edited Loading

raman-m left a comment • edited Loading

Choose a reason for hiding this comment

P.S. Regarding the #89 failed test

raman-m commented Nov 15, 2024 • edited Loading

raman-m commented Nov 16, 2024

ggnaegi commented Nov 16, 2024

ggnaegi commented Nov 16, 2024

raman-m left a comment • edited Loading

Choose a reason for hiding this comment

Ready for Delivery ✅

raman-m Nov 17, 2024

Choose a reason for hiding this comment

raman-m Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

ggnaegi Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

ggnaegi commented Nov 12, 2024 •

edited by raman-m

Loading

raman-m commented Nov 13, 2024 •

edited

Loading

raman-m commented Nov 13, 2024 •

edited

Loading

raman-m commented Nov 14, 2024 •

edited by ggnaegi

Loading

raman-m commented Nov 14, 2024 •

edited by ggnaegi

Loading

ggnaegi commented Nov 14, 2024 •

edited

Loading

raman-m left a comment •

edited

Loading

raman-m commented Nov 15, 2024 •

edited

Loading

raman-m left a comment •

edited

Loading

raman-m Nov 17, 2024 •

edited

Loading

ggnaegi Nov 17, 2024 •

edited

Loading