-
Notifications
You must be signed in to change notification settings - Fork 267
feat: add routeBind support to Giraffe.EndpointRouting #709
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
feat: add routeBind support to Giraffe.EndpointRouting #709
Conversation
|
The documentation mentions that endpoints ending with a trailing |
|
I could not find any explicit mention of this behavior in the documentation, but after reviewing the source code it appears that the request path is split into segments and empty segments are ignored. Based on this, I am going to update the documentation to confirm that no special handling is required for a trailing “/” when using Endpoint Routing. Please let me know if I am missing something. |
64J0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR @AugustoRengel, thanks for this contribution!
It's fine to add this new handler to the EndpointRouting module, I think.
64J0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After taking a better look at the code.
src/Giraffe/EndpointRouting.fs
Outdated
| match ModelParser.tryParse<'T> None routeData with | ||
| | Ok model -> handler model next ctx | ||
| | Error _ -> RequestErrors.BAD_REQUEST "Failed to bind route parameters" next ctx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's better to not return this RequestErrors.BAD_REQUEST "Failed to bind route parameters" message if the parse operation fails.
Instead, we need to just keep going, testing other routes, and eventually using the error handler specified by the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced the error with skipPipeline, which allows the request to flow to the next handler, but it does not switch to another endpoint because the model parsing happens after endpoint selection.
To achieve the desired behavior, the model parsing would need to run before the endpoint selection phase. Based on the ASP.NET Core routing pipeline, this seems only possible by using a custom MatcherPolicy.
If there is any alternative or recommended approach to achieve this, please let me know if I am missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking out loud.
Something we do in other places of the project is to let our clients define a special error handler.
For example: #691.
Perhaps it makes sense to use this approach here.
Anyway, we can add this optional error handler later.
|
Adding @nojaf and @TheAngryByrd for additional review and feedback. |
nojaf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me from afar.
Description
This PR extends the existing
routeBindsupport from theGiraffe.Routingmodule to also work withGiraffe.EndpointRouting, while preserving the same syntax.A new test was added to EndpointRoutingTests.fs to validate the routeBind behavior when using endpoint routing.
How to test
GET /p/John/Doe/32Also, a new test was added to EndpointRoutingTests.fs
Related issues
Fixes #708