Skip to content

Catch-all route parameter should be marked as optional in ApiExplorer #60392

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

Open
1 task done
scharnyw opened this issue Feb 14, 2025 · 3 comments
Open
1 task done

Catch-all route parameter should be marked as optional in ApiExplorer #60392

scharnyw opened this issue Feb 14, 2025 · 3 comments
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates good first issue Good for newcomers. help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team
Milestone

Comments

@scharnyw
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Catch-all route parameter is effectively optional because it matches against all values, including whitespace and empty strings. For example, consider the following API:

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddControllers();

var app = builder.Build();

app.MapControllers();

app.Run();

[ApiController]
[Route("")]
public class TestController : ControllerBase
{
    private readonly IApiDescriptionGroupCollectionProvider _apiDescriptionProvider;

    public TestController(IApiDescriptionGroupCollectionProvider apiDescriptionProvider)
    {
        _apiDescriptionProvider = apiDescriptionProvider;
    }

    [HttpGet("{**catchAllParameter}")]
    public IActionResult Get(string catchAllParameter)
    {
        var parameter = _apiDescriptionProvider
            .ApiDescriptionGroups.Items
            .SelectMany(group => group.Items)
            .First(api => api.RelativePath.EndsWith("{catchAllParameter}"))
            .ParameterDescriptions
            .First(parameter => parameter.Name == "catchAllParameter");

        Console.WriteLine(parameter.IsRequired); // true

        return Ok();
    }
}

This API can be reached in any of the following ways:

  1. GET / (with or without trailing slash)
  2. GET /abc

This behavior of catch-all parameter is consistent with the documentation. It is also consistent between Controllers and Minimal API, except that there is currently a regression bug (#58093) in Minimal API in .NET 8 that prevented it from working (it worked fine in .NET 7).

However, in the above code snippet parameter.IsRequired always reports true. This is effectively a lie because the API can be reached without the parameter being set.

While I understand that ApiExplorer is frequently used for OpenAPI generation purposes, and that the OpenAPI specification does not allow for optional path parameters, I think the ASP.NET Core ApiExplorer's behavior should not be driven solely by OpenAPI. Instead, it should faithfully report the actual API's behavior and let some other component be concerned with OpenAPI compatibility when generating the OpenAPI docs.

Expected Behavior

ApiParameterDescription.IsRequired should report false for catch-all route parameters.

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

8.0.10

Anything else?

No response

@martincostello martincostello added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed area-web-frameworks labels Feb 14, 2025
@mikekistler mikekistler added good first issue Good for newcomers. help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team labels Feb 14, 2025
@mikekistler mikekistler added this to the Backlog milestone Feb 14, 2025
@captainsafia
Copy link
Member

Thanks for filing this issue!

While I understand that ApiExplorer is frequently used for OpenAPI generation purposes, and that the OpenAPI specification does not allow for optional path parameters, I think the ASP.NET Core ApiExplorer's behavior should not be driven solely by OpenAPI. Instead, it should faithfully report the actual API's behavior and let some other component be concerned with OpenAPI compatibility when generating the OpenAPI docs.

I agree with this sentiment! It seems like you've done a bit og digging int othe space. Would you be interested in submitting a PR?

You'll want to modify the IApiDescriptionProvider interfaces in the repo to react to this change.

We already do the due dilligence in the OpenAPI package to make sure that path parameters are always required so there shouldn't be a need to harden anything there.

@KetchupCodes
Copy link

KetchupCodes commented Feb 16, 2025

Hi @captainsafia, @martincostello, @mikekistler and @scharnyw ,

I'd like to work on this issue! I've read the description and understand the problem. I'm also grateful for the guidance provided on where to start looking in the codebase.

Would it be possible to assign this issue to me? I'm excited to contribute!

mapedersen added a commit to mapedersen/aspnetcore that referenced this issue Feb 21, 2025
- Added a unit test in DefaultApiDescriptionProviderTest to validate that catch-all route parameters (e.g., "{**catchAllParameter}") are reported as optional.
- Modified ProcessIsRequired in DefaultApiDescriptionProvider to detect catch-all parameters (by checking for the "{**" pattern in the route template) and mark them as not required.
- This change aligns ApiExplorer metadata with the actual runtime behavior, ensuring more accurate API documentation.
- Follows TDD practices by first writing a failing test and then implementing the fix.
@mapedersen
Copy link
Contributor

I have submitted a PR for this issue: #60544

mapedersen added a commit to mapedersen/aspnetcore that referenced this issue Feb 24, 2025
…ers as optional

Previously, ApiExplorer marked all parameters as optional if the route template contained a catch-all marker ("{**"). This update refines the logic by leveraging ApiParameterContext.RouteParameters and the IsCatchAll flag to identify and mark only the catch-all parameter as optional. This change aligns the API metadata with the actual routing semantics and addresses dotnet#60392.
captainsafia pushed a commit that referenced this issue Feb 24, 2025
* Fix: Mark catch-all route parameters as optional (#60392)

- Added a unit test in DefaultApiDescriptionProviderTest to validate that catch-all route parameters (e.g., "{**catchAllParameter}") are reported as optional.
- Modified ProcessIsRequired in DefaultApiDescriptionProvider to detect catch-all parameters (by checking for the "{**" pattern in the route template) and mark them as not required.
- This change aligns ApiExplorer metadata with the actual runtime behavior, ensuring more accurate API documentation.
- Follows TDD practices by first writing a failing test and then implementing the fix.

* Fix: Use route parameter metadata to correctly mark catch-all parameters as optional

Previously, ApiExplorer marked all parameters as optional if the route template contained a catch-all marker ("{**"). This update refines the logic by leveraging ApiParameterContext.RouteParameters and the IsCatchAll flag to identify and mark only the catch-all parameter as optional. This change aligns the API metadata with the actual routing semantics and addresses #60392.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates good first issue Good for newcomers. help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team
Projects
None yet
Development

No branches or pull requests

6 participants