Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#748 Match Route configurations for upstream paths when empty Catch-All placeholders at the end of template #1911

Merged
merged 33 commits into from
Jan 18, 2024
Merged

#748 Match Route configurations for upstream paths when empty Catch-All placeholders at the end of template #1911

merged 33 commits into from
Jan 18, 2024

Conversation

AlyHKafoury
Copy link
Contributor

@AlyHKafoury AlyHKafoury commented Jan 8, 2024 •
edited by raman-m
Loading

@AlyHKafoury AlyHKafoury marked this pull request as ready for review January 8, 2024 16:00
@AlyHKafoury AlyHKafoury marked this pull request as draft January 9, 2024 15:38
@AlyHKafoury AlyHKafoury marked this pull request as ready for review January 9, 2024 22:30
@AlyHKafoury
Copy link
Contributor Author

@raman-m I believe the PR is ready for review please give me your thoughts about this, and if there is anything I need to improve

raman-m
raman-m requested changes Jan 10, 2024
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make it better.

src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs Outdated Show resolved Hide resolved
src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs Outdated Show resolved Hide resolved
src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs Outdated Show resolved Hide resolved
test/Ocelot.AcceptanceTests/RoutingTests.cs Outdated Show resolved Hide resolved
test/Ocelot.AcceptanceTests/RoutingTests.cs Outdated Show resolved Hide resolved
test/Ocelot.UnitTests/Configuration/UpstreamTemplatePatternCreatorTests.cs Show resolved Hide resolved
src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs Outdated Show resolved Hide resolved
@raman-m raman-m mentioned this pull request Jan 10, 2024
Closed
@raman-m raman-m changed the title #748 Fix Empty placeholder: Failed to match ReRoute configuration for upstream path #748 Empty placeholders: Failed to match Route configuration for upstream path Jan 10, 2024
@raman-m raman-m added bug Identified as a potential bug Jan'24 January 2024 release labels Jan 10, 2024
@raman-m raman-m added this to the January'24 milestone Jan 10, 2024
@AlyHKafoury
Copy link
Contributor Author

@raman-m I addressed most of your concerns, and Looking forward to your new comments

@raman-m
Copy link
Member

raman-m commented Jan 11, 2024

Thanks for working on code review issues!
Too busy!... Sorry... Going to ask another Ocelot team members to review..

FYI I've added this PR to Jan'24 milestone...
If you are on fire having desire to work more, please pick up open tickets from Annual'23 backlog!... But seems there are no free tickets to assign because all Annual tickets have their PRs... Well... We can find something else...

@raman-m
Copy link
Member

raman-m commented Jan 17, 2024 •
edited
Loading

It's mostly ready, but not quite... See below 👇 commit 1365f50

@raman-m raman-m added Nov'23 November 2023 release and removed Jan'24 January 2024 release labels Jan 17, 2024
@raman-m raman-m modified the milestones: January'24, Nov-December'23 Jan 17, 2024
@raman-m
Copy link
Member

raman-m commented Jan 17, 2024 •
edited
Loading

@AlyHKafoury I've added new these Routing tests and they failed.
Sorry, we have to fix them!... 😞
This theory should check possible breaking changes for the Catch All Query String feature. Seems this feature is broken now, or tests should be fixed.

I'm going to debug today... It will be nice if you help me too!..

@AlyHKafoury
Copy link
Contributor Author

@raman-m Please leave the debugging to me thanks for adding the failings tests and add more failing tests if they are needed and I can debug them to insure I understand all ocelot functionality

@raman-m
Copy link
Member

raman-m commented Jan 17, 2024

Thank you!
We've made aggressive refactoring in route matching logic...
So, Better to double check current Routing sub-features.

@raman-m
Copy link
Member

raman-m commented Jan 17, 2024

What's the news, Aly?

@AlyHKafoury
Copy link
Contributor Author

@raman-m fixing the test cases now will submit commit today

@AlyHKafoury
Copy link
Contributor Author

@raman-m I added fixes for the unit tests, please review the added commits

raman-m
raman-m reviewed Jan 18, 2024
View reviewed changes
test/Ocelot.AcceptanceTests/RoutingTests.cs
@@ -1171,7 +1171,7 @@ private void GivenThereIsAServiceRunningOn(string baseUrl, string basePath, Http
{
_serviceHandler.GivenThereIsAServiceRunningOn(baseUrl, basePath, async context =>
{
_downstreamPath = !string.IsNullOrEmpty(context.Request.PathBase.Value) ? context.Request.PathBase.Value : context.Request.Path.Value;
_downstreamPath = !string.IsNullOrEmpty(context.Request.PathBase.Value) ? context.Request.PathBase.Value + context.Request.Path.Value : context.Request.Path.Value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot get it! Was it a real issue?
Seems this change is not needed...

raman-m
raman-m approved these changes Jan 18, 2024
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for delivery! ✔️

Thanks, Aly!
Good job! 💪

@raman-m raman-m merged commit f4803c2 into ThreeMammals:develop Jan 18, 2024
2 checks passed
This was referenced Feb 11, 2024
Merged
Merged
@raman-m raman-m added the Routing Ocelot feature: Routing label Mar 17, 2024
@raman-m raman-m mentioned this pull request Mar 17, 2024
Closed
This was referenced Mar 27, 2024
Closed
Closed
@Burgyn Burgyn mentioned this pull request May 10, 2024
Closed
@LueNC LueNC mentioned this pull request May 16, 2024
Closed
This was referenced Jun 15, 2024
Closed
Closed
@suchmememanyskill suchmememanyskill mentioned this pull request Aug 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raman-m raman-m raman-m approved these changes

@RaynaldM RaynaldM RaynaldM approved these changes

@binarymash binarymash Awaiting requested review from binarymash

@jps jps Awaiting requested review from jps

@geffzhang geffzhang Awaiting requested review from geffzhang

@philproctor philproctor Awaiting requested review from philproctor

@anktsrkr anktsrkr Awaiting requested review from anktsrkr

@sharpn sharpn Awaiting requested review from sharpn

@abooraja abooraja Awaiting requested review from abooraja

@wast wast Awaiting requested review from wast

@yangzhongke yangzhongke Awaiting requested review from yangzhongke

@thiagoloureiro thiagoloureiro Awaiting requested review from thiagoloureiro

Assignees

@AlyHKafoury AlyHKafoury

Labels
bug Identified as a potential bug Nov'23 November 2023 release Routing Ocelot feature: Routing
Projects
None yet
Milestone
Nov-December'23
Development

Successfully merging this pull request may close these issues.

Empty placeholders: Failed to match Route configuration for upstream path
3 participants
@AlyHKafoury @raman-m @RaynaldM