Skip to content

Fail on missing required non-nullable value type in request body #1254

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

Closed
wants to merge 1 commit into from

Conversation

bkoelman
Copy link
Member

@bkoelman bkoelman commented Feb 5, 2023

This proof-of-concept PR fails deserialization of an incoming request body at a POST resource endpoint when the value is omitted for a non-nullable value type property decorated with [Attr] [Required].

This works around the limitation in ASP.NET ModelState validation described at https://www.jsonapi.net/usage/resources/nullability.html#value-types.

However, I wonder if we should actually take this approach. It feels like a dirty hack that mixes model binding with validation, which are distinct steps in ASP.NET. Also, it prevents any other ModelState violations from being returned in the response. A better approach would be to still detect it during deserialization, but instead of producing an error, somehow make it result in a ModelState validation error via request-scoped shared state. This would only work properly when ModelState validation is turned on. That approach would potentially apply to various other errors that currently cause a deserialization failure, such as "missing ID", "property is read-only" etc.

And this is just one approach to dealing with the situation. Alternatives:

  • Document the behavior and advise users not to do this. This is a fundamental limitation in ASP.NET, not our concern.
  • Scan for this situation when building the resource graph and produce a hard error that prevents application startup, or log a warning. The former is a breaking change, the latter means it'll still occur and we may want to handle it.

QUALITY CHECKLIST

@bkoelman bkoelman changed the base branch from master to data-types February 5, 2023 14:50
@codecov
Copy link

codecov bot commented Feb 5, 2023

Codecov Report

Merging #1254 (18407ce) into data-types (78d753c) will decrease coverage by 0.15%.
The diff coverage is 50.00%.

@@              Coverage Diff               @@
##           data-types    #1254      +/-   ##
==============================================
- Coverage       92.68%   92.53%   -0.15%     
==============================================
  Files             243      244       +1     
  Lines            7804     7827      +23     
==============================================
+ Hits             7233     7243      +10     
- Misses            571      584      +13     
Impacted Files Coverage Δ
...zation/Request/JsonMissingRequiredAttributeInfo.cs 0.00% <0.00%> (ø)
...lization/JsonConverters/ResourceObjectConverter.cs 94.05% <66.66%> (-2.68%) ⬇️
...lization/Request/Adapters/ResourceObjectAdapter.cs 97.29% <77.77%> (-2.71%) ⬇️
...ore/Controllers/BaseJsonApiOperationsController.cs 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bkoelman
Copy link
Member Author

bkoelman commented Feb 5, 2023

Closing due to #1245 (comment).

@bkoelman bkoelman closed this Feb 5, 2023
@bkoelman bkoelman deleted the fail-on-required-value-type branch February 7, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant