Skip to content

Conversation

mitchdenny
Copy link
Member

This PR addresses #45382.

Detect when two or more [FromBody] attributes are used

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

This PR ads another diagnostic for when two [FromBody] attributes are used on a single handler function. It ads a diagnostic for each parameter which is decorated with [FromBody].

@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 7, 2023
@mitchdenny mitchdenny added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework labels Feb 7, 2023
@ghost
Copy link

ghost commented Feb 15, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please remove the pending ci rerun label to kick off a new CI run.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 15, 2023
@mitchdenny
Copy link
Member Author

@halter73 waiting on your review. Based on @martincostello's feedback I also added support for scenarios where [AsParameters] is used. Because of this I wonder whether we should revisit the diagnostic message to make it cover that scenario as well. I would probably leave the title message alone, but expand the message with a bit more detail ... for example.

  <data name="Analyzer_MultipleFromBody_Message" xml:space="preserve">
    <value>Route handler has multiple parameters with the [FromBody] attribute or a parameter with an [AsParameters] attribute where the parameter type contains multiple members with [FromBody] attributes. Only one parameter can have a [FromBody] attribute.</value>
  </data>

@mitchdenny mitchdenny removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 21, 2023
@ghost
Copy link

ghost commented Feb 21, 2023

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 46494 in repo dotnet/aspnetcore

@mitchdenny
Copy link
Member Author

/azp run aspnetcore-ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mitchdenny
Copy link
Member Author

/azp run aspnetcore-components-e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mitchdenny
Copy link
Member Author

/azp run aspnetcore-quarantined-pr

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mitchdenny mitchdenny force-pushed the greater-than-1-frombody-attribute-analyzer branch from 5de0b4a to fc414d7 Compare February 21, 2023 02:24
@mitchdenny mitchdenny enabled auto-merge (squash) February 21, 2023 22:09

if (fromBodyMetadataInterfaceParameters.Count() >= 2)
{
ReportDiagnostics(context, fromBodyMetadataInterfaceParameters);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't fail if there's a single [FromBody] parameter and then another [AsParameter] parameter with a single [FromBody] property, right? Or multiple [AsParameter] parameters with a single [FromBody] property each? All of these should fail. Can we add test cases for these?

@halter73
Copy link
Member

Because of this I wonder whether we should revisit the diagnostic message to make it cover that scenario as well. I would probably leave the title message alone, but expand the message with a bit more detail ... for example.

  <data name="Analyzer_MultipleFromBody_Message" xml:space="preserve">
    <value>Route handler has multiple parameters with the [FromBody] attribute or a parameter with an [AsParameters] attribute where the parameter type contains multiple members with [FromBody] attributes. Only one parameter can have a [FromBody] attribute.</value>
  </data>

I think the new message is good. I think the old message was good enough too. Hopefully it's obvious to the people using the feature that [AsParameters] properties count as parameters, but it doesn't hurt to be extra descriptive.

@mitchdenny mitchdenny merged commit 86e3a4b into dotnet:main Feb 21, 2023
@ghost ghost added this to the 8.0-preview2 milestone Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants