Skip to content

Throw for invalid TryParse and BindAsync methods #36523

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
BrennanConroy opened this issue Sep 15, 2021 · 2 comments · Fixed by #36628
Closed

Throw for invalid TryParse and BindAsync methods #36523

BrennanConroy opened this issue Sep 15, 2021 · 2 comments · Fixed by #36628
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-actions Controller-like actions for endpoint routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Milestone

Comments

@BrennanConroy
Copy link
Member

BrennanConroy commented Sep 15, 2021

If your TryParse method has a nullable out parameter and is a struct it fails to find the TryParse method and will throw:
System.InvalidOperationException : No public static bool MyBindAsyncRecord.TryParse(string, out MyBindAsyncRecord) method found for myBindAsyncRecord.

private record struct MyBindAsyncRecord(Uri Uri)
{
    public static bool TryParse(string? value, out MyBindAsyncRecord? result) =>
        throw new NotImplementedException();
}

This is because the method parameter type is Nullable`1[[MyBindAsyncRecord]]& and we pass MyBindAsyncRecord&

methodInfo = type.GetMethod("TryParse", BindingFlags.Public | BindingFlags.Static, new[] { typeof(string), typeof(IFormatProvider), type.MakeByRefType() });

@BrennanConroy BrennanConroy added feature-minimal-actions Controller-like actions for endpoint routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Sep 15, 2021
@halter73
Copy link
Member

I'm not sure we should support this because it creates weird edge cases. For example, what should we do if a route handler takes in a non-nullable MyBindAsyncRecord and TryParse returns true but produces a null result?

I guess we could log an error similar to when TryParse return false, but what's wrong with just having the TryParse implementer return a default result?

@BrennanConroy BrennanConroy added this to the 6.0-rc2 milestone Sep 16, 2021
@BrennanConroy
Copy link
Member Author

Triage: We should instead throw for any TryParse methods that aren't conforming, and we would check for null struct as well.
And we should do this for BindAsync as well.

@BrennanConroy BrennanConroy changed the title Support nullable structs with TryParse in RequestDelegateFactory Throw for invalid TryParse and BindAsync methods Sep 16, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-actions Controller-like actions for endpoint routing 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 a pull request may close this issue.

3 participants