aspnet / Mvc

[Archived] ASP.NET Core MVC is a model view controller framework for building dynamic web sites with clean separation of concerns, including the merged MVC, Web API, and Web Pages w/ Razor. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
5.61k stars 2.14k forks source link

Action selection based on parameters #4915

Closed tugberkugurlu closed 8 years ago

tugberkugurlu commented 8 years ago

sorry for not providing code samples. This is an issue I am seeing on a closed sourced project and trying to solve it now. I can get some reproducible steps for this if needed.

I have two controller actions which have the same route pattern but have different non-optional, simple type (e.g. string, int, etc.) parameters on the method signature. However, I am getting AmbiguousActionException.

Don't MVC do parameter based routing as Web API is doing?

rynowak commented 8 years ago

No, the WebAPI compat shim package has a backwards compatible version of this as an IActionConstraint, but all parameters in MVC are optional.

Anything that we did in the future here will have to be opt-in. Personally, I like the idea of making query string patterns part of attribute routing. It's opt in, expressive, and lights up existing features like constraints, and view expanders. It also gives you a clear way to indicate "required" query string data.

I have two controller actions which have the same route pattern but have different non-optional, simple type (e.g. string, int, etc.)

This only works if they have different names in the query string in WebAPI 1-3x. WebAPI never had overloading based on type.

tugberkugurlu commented 8 years ago

thanks @rynowak! So, the preferred way to do this is to do an inner routing inside the action itself based on the query strings to make it perform different operations based on query string values which are being passed in?

That's will end up being a bit dirty but not entirely bad. I assume I can extend action selection to employ this logic?

rynowak commented 8 years ago

There's a few options here, I'll give some examples and you can decide what you like :+1:

1. Just let parameters be optional

If your actions represent distinct logical operations, then make sure they have a different route/path. The routing system will help you out here and you don't have to build any infrastructure to support it. All parameters in MVC are optional. If you want to treat some of them as required then validate them inside the action code and return BadRequest(..).

If you're doing this, you probably have a pretty clear separation between "this thing doesn't exist" (404) and "you didn't provide all the required data" (400).

Obviously you can't always do this because you might have compatibility requirements with the URL space. You also may not want to do this because you don't think it's the right thing to do, but this is basically what the framework provides out of the box.

2. Manually dispatch to different actions

If you want to write separate methods to handle distinct logical operations, but want them all to be routed to the same action, then you need to write some ugly dispatching code inside that action.

If you want to try and bind different parameters based on that "logical operation" that you're handling, the framework won't do anything to help you with this. This is a little painful, but depending on the scale you're operating at, might be an acceptable level of boilerplate.

The difference between 1 and 2 here is that in 1, your separation of logical actions is in your business layer, not in the controller layer.

3. Extend action selection

We actually planned for this, we just didn't want to put it in the box.

There's a new interface IActionConstraint which is a much more powerful version of ActionMethodSelectorAttribute from ye olde MVC. An IActionConstraint get to vote yes/no for a particular action, and having an IActionConstraint make an action "better" then a method without. This is how filtering based on HTTP verb works in MVC.

Here's where I'd start to implement this:

public interface IRequiredParameterMetadata
{
    // Just a marker
}

public class RequiredQueryStringAttribute  : Attribute, IBindingSourceProvider, IRequiredParameterMetadata
{
    public BindingSource BindingSource => BindingSource.Query;
}

public class RequiredQueryParametersConstraintConvention : IActionModelConvention
{
    public void Apply(ActionModel action)
    {
        var required = action.Parameters
            .Where(p => p.Attributes.OfType<IRequiredParameterMetadata>().Any())
            .Select(p => p.BindingInfo?.BinderModelName ?? p.ParameterName)
            .ToArray();
        if (required.Count > 0)
        {
            action.ActionConstraints.Add(new RequiredQueryParametersActionConstraint(required));
        }
    }
}

public class RequiredQueryParametersActionConstraint : IActionConstraint
{
    public RequiredQueryParametersActionConstraint(string[] required)
    {
        Required = required;
    }

    public string[] Required { get; }

    public int Order => 1000; // applied after HTTP methods

    public bool Accept(ActionConstraintContext context)
    {
        for (var i = 0; i < Required.Length; i++)
        {
            var request = context.RouteContext.HttpContext.Request;
            if (!request.Query.ContainsKey(Required[i]))
            {
                return false;
            }
        }

        return true;
    }
}

public class MyController
{
    public IActionResult Action1([RequiredQueryString] int required1, [RequiredQueryString] int required2, string optional)
    {
    }

    public IActionResult Action2([RequiredQueryString] int otherRequired, string optional)
    {
    }
}

Note that this won't disambiguate when multiple actions both have their requirements satisfied. How you want to resolve that is up to you. You could use another attribute to set the priority (and enforce it by setting Order in the action constraint) or you could resolve it based on number of required parameters, which is what webapicompatshim does

tugberkugurlu commented 8 years ago

great explanation @rynowak :+1: thanks a lot!

rynowak commented 8 years ago

Always a pleasure @tugberkugurlu - let us if know if you end up building it, it might be something that others ask for.