Add TryBindJsonAsync<'T> extension method and tryBindJson<'T> HTTP handler#733
Add TryBindJsonAsync<'T> extension method and tryBindJson<'T> HTTP handler#733esbenbjerre wants to merge 3 commits into
TryBindJsonAsync<'T> extension method and tryBindJson<'T> HTTP handler#733Conversation
There was a problem hiding this comment.
Pull request overview
Adds a non-throwing JSON model-binding option to Giraffe to address issue #323 by introducing a TryBindJsonAsync<'T> HttpContext extension and a tryBindJson<'T> HttpHandler wrapper, alongside documentation, release notes, and tests.
Changes:
- Add
HttpContext.TryBindJsonAsync<'T>() : Task<Result<'T, string>>for safe JSON deserialization. - Add
Core.tryBindJson<'T>HTTP handler which dispatches to success/error handlers based on parse result. - Add tests and documentation covering the new binding options, plus release note entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Giraffe.Tests/JsonTests.fs | Adds endpoint-routing tests validating tryBindJson success and error paths. |
| src/Giraffe/HttpContextExtensions.fs | Introduces TryBindJsonAsync<'T> extension method. |
| src/Giraffe/Core.fs | Introduces tryBindJson<'T> HTTP handler built on TryBindJsonAsync. |
| RELEASE_NOTES.md | Adds a release note entry for the new API. |
| DOCUMENTATION.md | Documents TryBindJsonAsync<'T> and tryBindJson<'T> with examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with exn -> | ||
| return Error exn.Message |
There was a problem hiding this comment.
I'm agnostic to this, letting maintainers decide.
There was a problem hiding this comment.
This approach of extracting the exception message and returning it as an Error seems inadequate. It loses part of the information that can be useful for the error handler.
Does it make sense to transform parsingErrorHandler into a function that receives the exn and handles it?
Moreover, ideally this function would return the incorrect/missing fields as a string, without dealing with exceptions, which can be triggered by other system components, as Copilot suggested. This behavior is consistent with the Giraffe.ModelParser.parseModel function.
What do you think?
There was a problem hiding this comment.
I don't normally endorse converting exceptions to monadic style. My intention was to keep the signature of parsingErrorHandler consistent with the one passed to tryBindForm, and then let clients implement any error handling (reporting missing fields, etc.) in the Json.ISerializer instance.
I hadn't looked too closely at Giraffe.ModelParser.parseModel, but I can certainly look into extending the PR to cover some sensible defaults consistent with it.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| with exn -> | ||
| return Error exn.Message |
There was a problem hiding this comment.
This approach of extracting the exception message and returning it as an Error seems inadequate. It loses part of the information that can be useful for the error handler.
Does it make sense to transform parsingErrorHandler into a function that receives the exn and handles it?
Moreover, ideally this function would return the incorrect/missing fields as a string, without dealing with exceptions, which can be triggered by other system components, as Copilot suggested. This behavior is consistent with the Giraffe.ModelParser.parseModel function.
What do you think?
Implementation of #323.