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.

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

Incorrect calculation of placeholders #2212

Closed
fenzad opened this issue Nov 22, 2024 · 3 comments · Fixed by #2213
Closed

Incorrect calculation of placeholders #2212

fenzad opened this issue Nov 22, 2024 · 3 comments · Fixed by #2213
Assignees
Labels
bug Identified as a potential bug highest Highest priority Nov'24 November 2024 release Routing Ocelot feature: Routing
Milestone

Comments

@fenzad
Copy link

fenzad commented Nov 22, 2024

Expected Behavior

Path: /dati-registri/v1.0/operatore/R80QQ5J9600/valida
Template: /dati-registri/{version}/{everything}

Expected:
version = v1.0
everything = operatore/R80QQ5J9600/valida

Actual Behavior

version = v1.0/operatore/R80QQ5J9600
everything = valida

in the previous version, the behavior was as expected

Steps to Reproduce the Problem

Set the configuration in the gateway with an ocelot.settings.json file with an UpstreamPathTemplate as indicated in the first point and create an endpoint in the API with the path indicated in the first point.

Specifications

  • Version: 23.4.0.0
  • Platform: .net 9 win
@raman-m raman-m added the Routing Ocelot feature: Routing label Nov 22, 2024
@ggnaegi
Copy link
Member

ggnaegi commented Nov 22, 2024

@fenzad Thanks for your feedback, ok, I will check that. Again a test case missing on our side.

@raman-m
Copy link
Member

raman-m commented Nov 22, 2024

Yes, as a CatchAll placeholder, it should cover as much path segments as it can.
Moreover, I propose to have a check if placeholders are docked to each other, if yes, then first placeholders should cover data of one path segment, the last placeholder should extract data as much it can but except first docked placeholders.

@ggnaegi
Copy link
Member

ggnaegi commented Nov 22, 2024

The problem is the main regex, it should match only between slashes. But if so, then it can't match the Catch All. So, I must invert the logic here: 1) catch all query, 2) catch all path, remove the catch all part, 3) validate the rest. At the minute we have 1) catch all query, 2) validate placeholders 3) catch all path. And that's wrong, but all tests passed ;-)

@raman-m raman-m added bug Identified as a potential bug highest Highest priority Nov'24 November 2024 release labels Nov 22, 2024
@raman-m raman-m added this to the November'24 milestone Nov 22, 2024
ggnaegi added a commit to ggnaegi/Ocelot that referenced this issue Nov 22, 2024
…ssue and adding unit and acceptance tests verifying the behavior.
ggnaegi added a commit that referenced this issue Nov 22, 2024
…r should be allowed to match several segments (#2213)

* #2212 Incorrect calculation of placeholders, fixing the issue and adding unit and acceptance tests verifying the behavior.

* Code review by @raman-m:
Convert anonimous delegate to local function.

---------

Co-authored-by: Raman Maksimchuk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug highest Highest priority Nov'24 November 2024 release Routing Ocelot feature: Routing
Projects
None yet
3 participants