Skip to content

Core 2.1 ODATA with optional parameters #341

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
flieks opened this issue Sep 4, 2018 · 4 comments · Fixed by #355
Closed

Core 2.1 ODATA with optional parameters #341

flieks opened this issue Sep 4, 2018 · 4 comments · Fixed by #355

Comments

@flieks
Copy link

flieks commented Sep 4, 2018

Optional parameters aren't working with OData & latest Api versioning 3.0 beta 1.
i'm using latest of OData api and debugged their source code
But then in mvcRouteHandler it can't find a candidate

public Task RouteAsync(RouteContext context)
        {
            if (context == null)
            {
                throw new ArgumentNullException(nameof(context));
            }

            var candidates = _actionSelector.SelectCandidates(context);

which is strange cause because in NavigationSourceRoutingConvention it could find a proper actionDescriptor. And the optional property was added to controllerContext.RouteData.Add(ODataRouteConstants.OptionalParameters, optional);

return actionDescriptors.Where(
                            c => String.Equals(c.ActionName, actionName, StringComparison.OrdinalIgnoreCase));

maybe related to issue 340

@flieks
Copy link
Author

flieks commented Sep 6, 2018

so SelectCandidates executes code of versioning (which i couldn't properly compile & debug).
And it returns null which is bad. Without versioning enabled it would return a valid route

@bdebaere
Copy link

bdebaere commented Sep 6, 2018

I can confirm that the following does not work with aspnet-api-versioning but works without it. With aspnet-api-versioning calling /odata/Items/RecentlyAwarded routes to the Get(string key) instead with RecentlyAwarded as key parameter! It's not even related to .Optional(). I think it's related to .Collection.

    builder
        .EntityType<Item>()
        .Collection
        .Function(nameof(ItemsController.RecentlyAwarded))
        .ReturnsFromEntitySet<Item>("Items");

    public class ItemsController
    {
        [EnableQuery(MaxExpansionDepth = 10)]
        public new ActionResult Get()
        {
        }

        [EnableQuery(MaxExpansionDepth = 10)]
        public ActionResult Get([FromODataUri] string key)
        {
        }

        [HttpGet]
        [EnableQuery(MaxExpansionDepth = 10)]
        public IActionResult RecentlyAwarded()
        {
        }
    }

@flieks
Copy link
Author

flieks commented Sep 6, 2018

possibly, but if you try without .Collection, it won't call the action properly either..

@commonsensesoftware
Copy link
Collaborator

The excerpts in @flieks post highlights snippets from the OData library and what looks like MVC. The design of the IActionSelector and the various concrete implementations do not allow it to be extended in a way that can support API versioning. This service is completely replaced in MVC and now has a specialized subclass for OData. Looking at the original implementation only helps to compare and contrast because everything had to be reimplemented.

In the case of OData, there are quite a few differences in the implementation because the OData design assumes that you can only get a single match. Fortunately, the AttributeRoutingConvention.SelectAction method affords for multiple results, but this is to support returning matching actions with the same name, but different parameters (ex: Get() and Get(int key)). API versioning needs to consider multiple different controllers and their matching actions.

I also made some other enhancements such that the OData attribute information is actually pre-calculated and attached as actual route information via ActionDescriptor.AttributeRouteInfo. This is on-par with how attribute routing normally works in MVC. The default behavior in OData is simply null because MVC doesn't know how to handle it. This the type of enhancement that the OData team should have done, but instead did more of a mirror of how OData worked in Web API. =/

Ultimately, it looks like this issue is caused by a flaw in the route selection as already noted in #336. The cause appears to be due to not correctly setting the matching action in the route parameters. I should have a fix out this week. I tried the Items example provided by @bdebaere with the forthcoming fix and it appears to work as expected.

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

Successfully merging a pull request may close this issue.

3 participants