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

Merged
merged 2 commits into from
Oct 15, 2024

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 captainsafia force-pushed the safia/backport-tryparse-bugfix branch from cb2c856 to bf2014c Compare October 14, 2024 17:56
@captainsafia
Copy link
Member Author

@wtgodbe Done!

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

@wtgodbe wtgodbe merged commit 3bcef1a into release/9.0 Oct 15, 2024
25 checks passed
@wtgodbe wtgodbe deleted the safia/backport-tryparse-bugfix branch October 15, 2024 15:54
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0.0 milestone Oct 15, 2024
@Phalelashvili
Copy link

Phalelashvili commented Dec 14, 2024

could this be the solution for issue where unbound route parameter changes schema type of other action after caching it? in my sample, it has nothing to do with IParsable. I figure JsonNode reference gets modified after it's cached by OpenApiSchemaStore

using System.Text.Json.Nodes;
using Microsoft.AspNetCore.Mvc;

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddControllers();
builder.Services.AddOpenApi();

var app = builder.Build();
app.MapOpenApi();

app.MapControllers();
app.MapDefaultControllerRoute();

app.Run();

[ApiController]
[Route("api/test")]
public class TestController : ControllerBase
{
    /// <summary>
    /// method that's defined properly.
    /// but open api generates parameter value as long (from broken method) instead of string
    /// only AFTER first call to openapi explorer. next time OpenApiSchemaStore returns incorrect cached value
    /// NOTE: if parameter in this method was not a complex object, it would have been fine.
    /// </summary>
    [HttpGet("search")]
    public IActionResult Search([FromQuery] SearchQuery query) => Ok();

    /// <summary>
    /// this is the method that breaks other actions.
    /// it defines a route parameter {id:long} in template but does not have parameter to bind it to.
    /// NOTE: not specifying type of id will not break other actions.
    /// </summary>
    [HttpGet("{id:long}/broken-method")]
    public IActionResult BrokenMethod() => Ok();
}

public class SearchQuery
{
    public string Term { get; init; }
}

@captainsafia
Copy link
Member Author

@Phalelashvili I think what you're seeing is a different issue. I believe there's a difference in behavior between MVC and Minimal APIs where route parameters not associated with a method parameter don't materialize in the ApiExplorer model. Can you file an issue for this?

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.

4 participants