Skip to content

The "more specific route" priority rule is not honored #797

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
gdurandrexel opened this issue Dec 13, 2021 · 5 comments
Closed

The "more specific route" priority rule is not honored #797

gdurandrexel opened this issue Dec 13, 2021 · 5 comments
Assignees

Comments

@gdurandrexel
Copy link

gdurandrexel commented Dec 13, 2021

I have a controller that supports 2 versions (Latest and Obsolete), and this controller contains these routes:

  • sites/{siteId} with [MapToApiVersion(ApiVersions.Latest)]
  • sites/contracts with no version qualifier (should be available for both Latest and Obsolete).
    If I call sites/contracts with version Latest (from header) I expect this route to be called, but it is actually sites/{siteId}.
    I know I can disambiguate with siteId:int but I don't see any reason why the "more specific route" wouldn't apply here.

My config (as is, in case I made some mistake):
Basically all the controllers support Latest and Obsolete versions, and [Obsolete] methods are mapped to the Obsolete version.

services.AddApiVersioning(options =>
{
	options.AssumeDefaultVersionWhenUnspecified = true;
	ApiVersion obsoleteVersion = ApiVersion.Parse(ApiVersions.Obsolete);
	options.DefaultApiVersion = obsoleteVersion;
	options.ApiVersionReader = new HeaderApiVersionReader("X-Api-Version");

	// configure every ApiController that supports versioning
	ApiVersion[] supportedVersions = new[] { obsoleteVersion, ApiVersion.Parse(ApiVersions.Latest) };
	foreach (Type controller in Assembly.GetExecutingAssembly().GetTypes()
		.Where(_ => _.GetCustomAttribute<ApiControllerAttribute>(true) != null && _.GetCustomAttribute<ApiVersionNeutralAttribute>(true) == null))
	{
		IControllerConventionBuilder controllerConventions = options.Conventions.Controller(controller);

		// set the supported versions
		controllerConventions.HasApiVersions(supportedVersions);

		// assign the obsolete version to the obsolete methods
		foreach (MethodInfo method in controller.GetMethods()
			.Where(_ => _.GetCustomAttribute<ObsoleteAttribute>(true) != null))
		{
			controllerConventions.Action(method).MapToApiVersion(obsoleteVersion);
		}
	}
});

And the controller:

[Route("api/prouser/sites")]
public class SiteController : ProUserControllerBase
{
...

[HttpGet("{siteId}")]
[MapToApiVersion(ApiVersions.Latest)]
public async Task<SiteDto> GetSite([FromRoute] SiteId siteId)
...

[HttpGet("contracts")]
public async Task<IEnumerable<ContractsDto>> GetContracts()

Note that setting Latest as the default version (options.DefaultApiVersion = ApiVersion.Parse(ApiVersions.Latest);) produces the same result.

@commonsensesoftware
Copy link
Collaborator

I get the basic gist of what's happening, but it's a bit difficult to debug in my head. Do you have the world's simplest repro that demonstrates the problem? I maybe to be able to help figure out where things when awry.

Sidebar

I would recommend you use a custom convention. That would remove all the nasty Reflection stuff. There are a bunch of other rules for action methods that are not being observed. It might look something like:

public sealed class MyConvention : IControllerConvention
{
    private readonly IReadOnlyList<ApiVersion> supportedVersions;
    private readonly ApiVersion obsoleteVersion;

    public MyConvention(IReadOnlyList<ApiVersion> supportedVersions, ApiVersion obsoleteVersion)
    {
        this.supportedVersions = supportedVersions;
        this.obsoleteVersion = obsoleteVersion;
    }

    public bool Apply(IControllerConventionBuilder controller, ControllerModel controllerModel)
    {
        controller.HasApiVersions(supportedVersions);

        var actions = controller.Actions;

        for (var i = 0; i < actions.Count; i++)
        {
            var action = actions[i];

            if (action.Attributes.OfType<ObsoleteAttribute>().Any())
            {
                controller.Action(action.ActionMethod).MapToApiVersion(obsoleteVersion);
            }
        }
    }
}

@gdurandrexel
Copy link
Author

Hello,
here is a reproducer: ApiVersionMoreSpecificRoute.zip

Thx for the tip on the custom convention. Note that in my case I had to ensure the controller didn't have the ApiVersionNeutral attribute:

public bool Apply(IControllerConventionBuilder controller, ControllerModel controllerModel)
{
	if (!controllerModel.Attributes.OfType<ApiVersionNeutralAttribute>().Any())
	{
		// set the supported versions
		controller.HasApiVersions(_supportedVersions);

		// assign the obsolete version to the obsolete methods
		foreach (ActionModel action in controllerModel.Actions)
		{
			if (action.Attributes.OfType<ObsoleteAttribute>().Any())
			{
				controller.Action(action.ActionMethod).MapToApiVersion(_obsoleteVersion);
			}
		}

		return true;
	}

	return false;
}

@commonsensesoftware
Copy link
Collaborator

I'll take a look at your repro.

Your approach should work here; however, API versioning looks for IApiVersionNeutral and IApiVersionProvider. Using attributes just happens to be one of the ways these can be applied. :)

@commonsensesoftware
Copy link
Collaborator

Thank you for the repro. This is indeed strange. This does have the appearance of a bug, but I'm not sure why it is happening - or rather why it hasn't been discovered before. This a common scenario. There is even an acceptance test for this exact scenario. There is some combination here that is different from that test case. Ultimately, this is happening because sites/contacts has implicitly mapped versions which have a lower priority to the explicitly mapped version of sites/{siteId}, despite sites/contacts have a higher score (e.g. better route match). I believe this may be a flaw in the matching logic where an implicitly mapped version (e.g. from the parent - i.e. controller) can be ranked higher than an explicitly mapped version if the score is higher. Currently, there is no specially handling for scores - only valid/invalid plug the implicit/explicit versions.

There are 2 possible immediate workarounds:

  1. Put a route constraint a la {siteId:int}; however, that might not always make sense
  2. Explicitly map the versions to ~/sites/contracts. This can be done with [MapToApiVersion] or a convention

You should only have to do it for this case. It could apply to any other case where the route templates can overlap.

@gdurandrexel
Copy link
Author

Thanks.
I will use solution 1 as it seems it makes sense here anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants