search

ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.31k stars 1.63k forks source link

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

Closed AlyHKafoury closed 7 months ago

AlyHKafoury commented 8 months ago

Fixes #748

AlyHKafoury commented 8 months ago

@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

AlyHKafoury commented 8 months ago

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

raman-m commented 8 months ago

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 commented 8 months ago

@ggnaegi @RaynaldM Please review!

AlyHKafoury commented 8 months ago

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...

Thanks alot, I am gonna lookup for More tickets to work on I am really excited to contribute more

raman-m commented 8 months ago

@AlyHKafoury commented on Jan 11

Great! We need to work on issues & PRs with lowest numbers... Checking all related issues with higher numbers... Please choose from the last page because I've reviewed them marking with labels. And write me from the new ticket plz.

AlyHKafoury commented 8 months ago

@raman-m my daily submission, I addressed most of the comments regarding styling and clean code. waiting for your review

raman-m commented 8 months ago

@AlyHKafoury Did you check all user scenarios from #748 by the tests?

For example these ones:

satano commented on Jan 15, 2019

AlyHKafoury commented 8 months ago

@AlyHKafoury Did you check all user scenarios from #748 by the tests?

Yes these are all the requests on the issue and extra ones for other cases I found that needed to be verified

raman-m commented 8 months ago

OK I'm going to check code coverage and do quick final code review...

AlyHKafoury commented 8 months ago

@raman-m if you have any changes you can request I am looking forward to contribute more in a lot of tickets

raman-m commented 8 months ago

AlyHKafoury commented on Jan 15:

Yes these are all the requests on the issue and extra ones for other cases I found that needed to be verified

Unfortunately, we didn't write all required unit tests for original user scenarios from bug's description. See my code review results above! ☝️ I believe if "all required unit tests will be written" then this will be the end of development. Tomorrow I will update docs to help you and letting you finish unit tests. We need to inform community that we support "empty placeholders" now. 😃 And that info will go to Release Notes partially.

AlyHKafoury commented 8 months ago

@raman-m working on the unit tests now will deliver today

AlyHKafoury commented 8 months ago

@raman-m is there a way I can run test coverage myself ?

raman-m commented 8 months ago

Tomorrow... Have a good evening!

AlyHKafoury commented 8 months ago

@raman-m unit tests are added for the code, but I cannot check coverage yet

raman-m commented 8 months ago

@AlyHKafoury commented on Jan 15:

Is there a way I can run test coverage myself?

Sure, there is! We use coverlet.collector package (see NuGet) to run code coverage by Coverlet tool. This is standard tool which included in xUnit Test Project template by Visual Studio. For NUnit template I'm not sure.

To get coverage results you need to

Sure your TestResults folder can be located anywhere. Hope it helps!

raman-m commented 8 months ago

Sorry for failed build status by commit https://github.com/ThreeMammals/Ocelot/pull/1911/commits/9f7e640ae8efdb3ff1e2325ce5e3c52c8105fa82 😇

AlyHKafoury commented 8 months ago

I've added test cases for query strings and in middle replacements @raman-m

AlyHKafoury commented 8 months ago

@raman-m should I fix https://github.com/ThreeMammals/Ocelot/commit/9f7e640ae8efdb3ff1e2325ce5e3c52c8105fa82

raman-m commented 8 months ago

Regarding https://github.com/ThreeMammals/Ocelot/commit/9f7e640ae8efdb3ff1e2325ce5e3c52c8105fa82 ... I've debugged a little bit, seems the problem is here In feature branch: lines 60-64

AlyHKafoury commented 8 months ago

@raman-m something is broken with Circle-CI ?

AlyHKafoury commented 8 months ago

@raman-m all tests are passing locally now, please retry the Circle-CI test and check if you need more changes or unit tests

raman-m commented 8 months ago

@raman-m something is broken with Circle-CI ?

Why do you think so?

AlyHKafoury commented 8 months ago

@raman-m the tests fail because being unable to pull branch

AlyHKafoury commented 8 months ago

@raman-m something is broken with Circle-CI ?

Why do you think so?

https://app.circleci.com/pipelines/github/ThreeMammals/Ocelot/1643/workflows/62a87415-82a9-4d2b-906e-ecd00e97eb19/jobs/3130

raman-m commented 8 months ago

Wow! Maybe CircleCI netwotking issue or GitHub deployments...

raman-m commented 8 months ago

Everything is fine with your GitHub repo... I've re-cloned successfully... Let me run the CircleCI build once again...

raman-m commented 8 months ago

🆗 Let's wait the build 10 minutes... 😃 It was environment issue on CircleCI side. Are tests green locally?

AlyHKafoury commented 8 months ago

Yes they are

AlyHKafoury commented 8 months ago

@raman-m tests have passed on Circle-CI, please do inform me if there are more needed changes

raman-m commented 8 months ago

https://github.com/ThreeMammals/Ocelot/pull/1911/commits/0360b12d841b1af3c97deed4b064879681d7a591 is very nice fix! 🤣 Thank you! I'll review carefully today...

raman-m commented 8 months ago

Well, Aly... I've added commit https://github.com/ThreeMammals/Ocelot/pull/1911/commits/2f614565564b68b68557399208ef7bf64ef06e11 which has failed build 3134 because of Routing Tests when string downstreamURL is null while checking by these lines 851-856. So, I added them to check query string case.

Probably it is better to move query constants to InlineData as extra parameter, because case (string downstreamURL is null) is not actual Catch-All placeholder one and we haven't to forward query string at all, because matcher returns null.

Please, fix and this will be the end of development. 😋

AlyHKafoury commented 7 months ago

@raman-m I fixed the unit tests, and they also revealed a sneaky bug which I fixed also in the same commit, please review carefully the changes and inform me if there are anymore changes to do

AlyHKafoury commented 7 months ago

@raman-m I believe the branch is ready for merge my side, waiting for your confirmation or change requests

raman-m commented 7 months ago

It's mostly ready, but not quite... See below 👇 commit https://github.com/ThreeMammals/Ocelot/pull/1911/commits/1365f50248a84718f07c729d20d4fa7b55968cbe

raman-m commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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

raman-m commented 7 months ago

What's the news, Aly?

AlyHKafoury commented 7 months ago

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

AlyHKafoury commented 7 months ago

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