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

[release/9.0] Fix ModelMetadata for TryParse-parameters in ApiExplorer #58372

Open
wants to merge 1 commit into
base: release/9.0
Choose a base branch
from

Conversation

captainsafia
Copy link
Member

Description

This PR fixes the handling for schemas associated with types that implement a custom TryParse and are consumed from either the route or query string.

Fixes #58318

Customer Impact

Without this change, APIs that bind parameters from the query string or URL will produce invalid schemas if the type being bound to implements a TryParse method or the IParsable interface.

There are workarounds for this scenario (using schema transformers) but it requires users add a fair bit of code in their own applications and this fix is small enough to warrant improving the QoL for this.

Example of impacted scenario

app.MapGet("/student/{student}", (Student student) => $"Hi {student.Name}");

public record Student(string Name)
{
    public static bool TryParse(string value, out Student? result)
    {
        if (value is null)
        {
            result = null;
            return false;
        }

        result = new Student(value);
        return true;
    }
}

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

Change localized to OpenAPI support + TryParsable parameters in the route query.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@captainsafia captainsafia requested a review from a team as a code owner October 11, 2024 20:16
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Oct 11, 2024
@captainsafia captainsafia added Servicing-consider Shiproom approval is required for the issue area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Oct 11, 2024
@captainsafia captainsafia changed the title [release/9.0] Fix ModelMetadata for TryParse-parameters in ApiExplorer (#58350) [release/9.0] Fix ModelMetadata for TryParse-parameters in ApiExplorer Oct 11, 2024
@captainsafia
Copy link
Member Author

Approved via email.

@captainsafia captainsafia added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Oct 14, 2024
@wtgodbe
Copy link
Member

wtgodbe commented Oct 14, 2024

@captainsafia can you resolve the merge conflict?

* Fix ModelMetadata for TryParse-parameters in ApiExplorer

* Add tests and update code comments

* Update src/OpenApi/src/Services/OpenApiDocumentService.cs
@captainsafia
Copy link
Member Author

@wtgodbe Done!

@BrennanConroy Can you take a look at this backport? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants