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 forwarding with common root at the same level #346

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

0-gravity
Copy link

Incorrect forwarding with common root at the same level
for example next routing will fail
router { forward $"/roots" (text "roots") forwardf "/roots/%O" (sprintf "roots/%O" >> text) }

@0-gravity
Copy link
Author

In my opinion all formatted paths should come before normal ones, which means:
getf/postf/patchesf/putsf/deletef should come before get/post/patches/puts/delete

@Banashek
Copy link

Banashek commented Jul 6, 2022

I believe there are cases where a concrete path would be desired to have precedence.
Say part of your route path is fuzzy, but in certain concrete cases you want it to go to a specific page.

Hypothetical Example:

  • products/featured/catalog would go to a defined path returning featured products
  • products/%s/catalog could go to user-generated tagging categories

One might want the featured products to go to a special page formatted for display, whereas the application might also support user-generated tags for products, which could show on a default display.

While this could also be done within the handler logic itself (which I think would be better in most cases), there may be other complicating logic which determines the route to be the cleanest spot to separate the branches at.

Just thinking out loud here, but how would that be doable with this new routing?

@0-gravity
Copy link
Author

It works just fine, unless I missed some cases.
Here is an example:
`open Saturn
open Giraffe

let apiRouter = router {
get "/products/featured/catalog" (text "/products/featured/catalog - non-parametrized")
getf "/products/%s/catalog" (sprintf "/products/%s/catalog - parametrized" >> text)
}

let appRouter = router {
forward "/api" apiRouter
}

let app =
application {
disable_diagnostics
use_router appRouter
url "http://0.0.0.0:8085/"
}

run app`

I could not come up with an example in which request may fail.
I'd be happy to see one

@Banashek
Copy link

Banashek commented Jul 7, 2022

No you're right that looks to be the correct behavior.
I should have pulled down the branch and tested myself rather than trying to eyeball the code.
Thanks for providing the test!

@64J0
Copy link
Contributor

64J0 commented Sep 16, 2022

In my opinion all formatted paths should come before normal ones, which means:
getf/postf/patchesf/putsf/deletef should come before get/post/patches/puts/delete

I did not get this @0-gravity. Why you make this proposition?

It appears that you want to make generic paths to be evaluated before specific paths. I don't think this is a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants