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

Move back [FromBody] and [Required] to derived controllers #1506

Merged
merged 1 commit into from
Mar 17, 2024

Conversation

bkoelman
Copy link
Member

@bkoelman bkoelman commented Mar 17, 2024

This PR moves back [FromBody] and [Required] to derived controllers, reverting most of the previous PR (#1503). It turns out that ASP.NET ModelState doesn't take attributes on base parameters into account.

As is often the case, there's more to it. The ParameterInfo.GetCustomAttributes method has always had the bug that it doesn't look at attributes on base methods, even with inherit: true. To address that, without taking a breaking change, Attribute.GetCustomAttributes was introduced, which properly takes base classes into account (but not interfaces). Aside from StackOverflow here and here, this isn't mentioned anywhere in the official docs (a remark about it was actually removed). Only a comment in the .NET runtime source code explains it:

// For ParameterInfo's we need to make sure that we chain through all the MethodInfo's in the inheritance chain that
// have this ParameterInfo defined. .We pick up all the CustomAttributes for the starting ParameterInfo. We need to pick up only attributes
// that are marked inherited from the remainder of the MethodInfo's in the inheritance chain.
// For MethodInfo's on an interface we do not do an inheritance walk so the default ParameterInfo attributes are returned.
// For MethodInfo's on a class we walk up the inheritance chain but do not look at the MethodInfo's on the interfaces that the
// class inherits from and return the respective ParameterInfo attributes

However, ASP.NET Core does not use the new method (and neither does Swashbuckle). Tests succeeded in the previous PR because most input validation is handled during JSON:API deserialization, before ModelState kicks in, combined with the fact we only run ModelState validation on POST/PATCH resource endpoints. While we can work around this for Swashbuckle, we can't know what other libraries are out there, possibly using the old broken method. Therefore it seems safest to have the attributes on the derived controller class (as it was). And not having them on the base class, for maximum flexibility for users deriving and tweaking things (same for routes).

QUALITY CHECKLIST

  • Changes implemented in code
  • Complies with our contributing guidelines
  • N/A: Adapted tests
  • N/A: Documentation updated

… most of the previous PR (#1503). It turns out that ASP.NET ModelState doesn't look at attributes on base parameters
Copy link

codecov bot commented Mar 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.78%. Comparing base (6b9d4b8) to head (6bf4a8c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1506      +/-   ##
==========================================
+ Coverage   90.76%   90.78%   +0.01%     
==========================================
  Files         395      346      -49     
  Lines       12970    11044    -1926     
  Branches     2061     1813     -248     
==========================================
- Hits        11772    10026    -1746     
+ Misses        780      669     -111     
+ Partials      418      349      -69     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bkoelman bkoelman marked this pull request as ready for review March 17, 2024 00:19
@bkoelman bkoelman merged commit 7e864a2 into master Mar 17, 2024
16 checks passed
@bkoelman bkoelman deleted the correct-frombody-required-changes branch March 17, 2024 00:19
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