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.
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.
After taking a better look at the code.
| 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.
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.
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.
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.
Looks good to me from afar.
…p#709) * feat: add routeBind support to Giraffe.EndpointRouting * docs: update routeBind use * Apply suggestions from code review * format: run fantomas on EnpointRoutingTests.fs
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