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.

Sign up for GitHub

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

Jump to bottom

Descriptive Error for Improper Use of [AsParameters] #58218

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Descriptive Error for Improper Use of [AsParameters] #58218

wants to merge 3 commits into from

Conversation

smnsht
Copy link
Contributor

@smnsht smnsht commented Oct 3, 2024 •
edited
Loading

Descriptive Error for Improper Use of [AsParameters]

Description

  1. Throw NotSupportedException if [AsParameters] is applied to an enumerable class.

  2. Preserve existing behavior for collections that are also abstract classes, like IFormCollection. In order to do this, a dedicated unit test was added.

Fixes #56114

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 3, 2024
@smnsht smnsht marked this pull request as ready for review October 3, 2024 13:00
@smnsht smnsht marked this pull request as draft October 3, 2024 13:29
@smnsht smnsht marked this pull request as ready for review October 3, 2024 14:15
@smnsht
Copy link
Contributor Author

smnsht commented Oct 3, 2024

Converting draft to PR despite some failing tests in Interop.FunctionalTests.

[QuarantinedTest("https://github.com//issues/41074")]
[QuarantinedTest("https://github.com//issues/39985")]

Justification:

Seems like those tests failed with no regard to my changes. Also, tests are failing for the main branch.
Also, I spotted merged PR's with failed tests in this area.

@amcasey
Copy link
Member

amcasey commented Oct 3, 2024

FYI @captainsafia

@gfoidl gfoidl added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Oct 4, 2024
@smnsht smnsht changed the title Descriptive Error for Improper Use of [AsParameters] #56114 Descriptive Error for Improper Use of [AsParameters] Oct 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 19, 2024
@smnsht
Copy link
Contributor Author

smnsht commented Oct 19, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

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

@captainsafia
Copy link
Member

/azp run

@dotnet-policy-service dotnet-policy-service bot removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 22, 2024
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

captainsafia
captainsafia reviewed Oct 22, 2024
View reviewed changes
src/Http/Http.Extensions/src/RequestDelegateFactory.cs Outdated
@@ -821,6 +821,15 @@ private static Expression CreateArgument(ParameterInfo parameter, RequestDelegat
$"Nested {nameof(AsParametersAttribute)} is not supported and should be used only for handler parameters.");
}

// Parameters like IFormFileCollection should throw "The abstract type ... is not supported" rather than
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we can't throw this exception in the same place where the other exception is thrown?

aspnetcore/src/Shared/ParameterBindingMethodCache.cs

Line 363 in 5f156a2

throw new InvalidOperationException($"The abstract type '{TypeNameHelper.GetTypeDisplayName(type, fullName: false)}' is not supported.");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@captainsafia
Fixed. I moved the exception to ParameterBindingMethodCache.cs, works well.

@captainsafia
Copy link
Member

@smnsht Thanks for changing the code here!

I also realized that similar to the other PR you're working on, we'll need to add checks in the RDG implementation for this. Here is the code that needs to be modified in the source generator. Instead of throwing there, we emit diagnostics when the type is invalid.

@smnsht
Copy link
Contributor Author

smnsht commented Nov 2, 2024

@captainsafia

I also realized that similar to the other PR you're working on, we'll need to add checks in the RDG implementation for this. Here is the code that needs to be modified in the source generator. Instead of throwing there, we emit diagnostics when the type is invalid.

Done

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Nov 9, 2024
@captainsafia
Copy link
Member

/azp run

@dotnet-policy-service dotnet-policy-service bot removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 6, 2025
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

captainsafia
captainsafia reviewed Jan 6, 2025
View reviewed changes
src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointParameter.cs Outdated
@@ -466,6 +467,12 @@ private static bool TryGetAsParametersConstructor(Endpoint endpoint, INamedTypeS
return false;
}

if (type.AllInterfaces.Any(x => x.ToString() == typeof(IEnumerable).FullName))
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing a string-based comparison check, we should do a symbol-based comparison using the extension method that is defined here.

You'll need to add the IEnumerable type to the well-known types cache in this file then resolve it from the WellKnownTypes cache based to this call path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do this in February.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for letting me know! I removed the pr: pending author input label so that the bot doesn't out-close your PR in the meantime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

symbol-based comparison - done.
Appreciate the tip!

@captainsafia captainsafia added the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Jan 6, 2025
@captainsafia captainsafia removed the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Jan 13, 2025
@captainsafia captainsafia requested a review from Copilot January 13, 2025 18:33
Copilot
Copilot AI reviewed Jan 13, 2025
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • src/Http/Http.Extensions/gen/Resources.resx: Language not supported
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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 conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@captainsafia captainsafia captainsafia left review comments

Copilot code review Copilot Copilot left review comments

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@captainsafia captainsafia

Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Descriptive Error for Improper Use of [AsParameters] with IFormFileCollection in .NET 8 Minimal APIs
5 participants
@smnsht @amcasey @captainsafia @adityamandaleeka @gfoidl