AspNetCoreOData icon indicating copy to clipboard operation
AspNetCoreOData copied to clipboard

API and OData behavior if there are two methods in the controller

Open Diaz15h opened this issue 3 years ago • 6 comments
trafficstars

Here I have three controllers which inherited from the same controller.

All three controllers in the same configurations.

When I do the following queries; https://localhost:7069/odata/Persons or https://localhost:7069/odata/Employees the method with this signature which is called

public ActionResult<IQueryable<TModel>> Get(ODataQueryOptions<TModel> oDataQueryOptions)

This is the expected behavior

Problem

When I make this request https://localhost:7069/odata/Staffs

It is this method that is called

public async Task<ActionResult<TModel>> Get(int id)

If I remove the Get(int id) method.

The Get(ODataQueryOptions<TModel> oDataQueryOptions) method which is called And everything works

Questions

Where does the problem come from ?

Version OData "Microsoft.AspNetCore.OData" Version="8.0.10"

Setup

Here are the two methods which is in the base controller :

[HttpGet]
[EnableQuery(MaxExpansionDepth = 10, MaxNodeCount = 1000)]
public ActionResult<IQueryable<TModel>> Get(ODataQueryOptions<TModel> oDataQueryOptions)
{
    modelService.ODataQueryOptions = oDataQueryOptions;
    var result = modelService.GetModelsAsync().Result;
    return Ok(result.Value);
}
[HttpGet("{id}")]
public async Task<ActionResult<TModel>> Get(int id)
{
      var result = await modelService.GetModelAsync(id);
     return OK(result);
}

The controllers inherit: they have the same configurations:

[Route("api/[controller]")]
[ApiController]
public class EmployeesController : PersonCollectionControllerBase<EmployeeDto, EmployeesService>
{
    public EmployeesController(IEmployeesService employeesService): base(employeesService)
    {
            
    }
}
[Route("api/[controller]")]
[ApiController]
public class PersonsController : PersonCollectionControllerBase<PersonDto, PersonsService>
{
    public PersonsController(IPersonsService persons): base(persons)
    {
    }
}
[Route("api/[controller]")]
[ApiController]
public class StaffsController : PersonCollectionControllerBase<StaffDto, StaffsService>
{
    public StaffsController(IStaffService modelService) : base(modelService)
    {
    }
}

IServiceCollection configuration:

builder.Services.AddControllers().AddOData(options =>
    {
        options.AddRouteComponents(routePrefix: "odata", GetEdmModel())
            .Select().Count().Filter().Expand().OrderBy().SetMaxTop(maxTopValue: 100);
    });

IEdm configuration :

public static IEdmModel GetEdmModel()
{
    ODataConventionModelBuilder builder = new();
    builder.EntitySet<PersonDto>(name: "Person")
            .EntityType.HasKey(person => person.BusinessEntityId);
    builder.EntitySet<PersonDtoBase>(name: "PersonsBase")
            .EntityType.HasKey(person => person.BusinessEntityId);
    builder.EntitySet<EmployeeDto>(name: "Employees")
            .EntityType.HasKey(employee => employee.BusinessEntityId);
    builder.EntitySet<EmployeeDtoBase>(name: "EmployeesBase")
            .EntityType.HasKey(employee => employee.BusinessEntityId);
    builder.EntitySet<StaffDto>(name: "Staffs")
        .EntityType.HasKey(staff => staff.BusinessEntityId);
    return builder.GetEdmModel();
}

Example Here is the link where I reproduce the bug

Diaz15h avatar Aug 11 '22 23:08 Diaz15h

The [controller] replacement doesn't currently work with OData, so I'm surprised that your other 2 routes work. I assume they must be MVC routes right now.

  • https://github.com/OData/AspNetCoreOData/issues/430

Can you post the results of your $odata endpoint?

Also, can you try just replacing the [controller] with each specific name, like "Staffs", on all controllers?

Lastly and unrelated, I'd not recommend blocking on async code like what you are doing here. This will just cause you headaches in the future and potential performance/concurrency loss. Would recommend changing your code to:

[HttpGet]
[EnableQuery(MaxExpansionDepth = 10, MaxNodeCount = 1000)]
public async Task<ActionResult<IQueryable<TModel>>> Get(ODataQueryOptions<TModel> oDataQueryOptions)
{
    modelService.ODataQueryOptions = oDataQueryOptions;
    var result = await modelService.GetModelsAsync();
    return Ok(result.Value);
}

julealgon avatar Aug 12 '22 14:08 julealgon

Hello, thank you for your answer, Here is the $odata report In this example it is class1 that does not work rapportodata1 rapportodata2

Diaz15h avatar Aug 12 '22 17:08 Diaz15h

@Diaz15h can you make the change I suggested, then check again (both the behavior, and the $odata endpoint result)?

Looks like you'll also want to remove the /api part on your [RouteAttribute]s to match the OData route you defined in the startup.

Optionally, you can also try changing your method to this:

[HttpGet("{key}")]
public async Task<ActionResult<TModel>> Get(int key)
{
      var result = await modelService.GetModelAsync(key);
     return OK(result);
}

Since key is the predefined name OData uses to represent the entity IDs in routes (not id). You can see from your OData routes above that it is registering 2 GET /odata/entity routes, and I suspect this is why: it can't recognize your id parameter and treats it as a querystring parameter not part of the route itself.

The fact that you are hitting the ID endpoint with Staffs could be due to some undefined reflection order during route discovery. Once you change to key it should start to detect 2 distinct GET routes.

Let me know how it goes.

julealgon avatar Aug 12 '22 18:08 julealgon

I renamed id by key and it works, I left attribute [Route("api/[Controller]")] testroute

Diaz15h avatar Aug 12 '22 19:08 Diaz15h

Yeah, figured this would solve it.

Just keep in mind that by keeping those routes desynched, you are just creating confusion for other developers in the team. They will see the "/api/[controller]` routes and expect them to work, but they don't. As you saw, those end up as pure MVC routes with different paths than the OData ones, which you probably don't want.

So because of that, I'd still recommend changing the [Route] attributes to match your actual OData routes and ensure you only have OData endpoints in the $odata view.

Of course, that's up to you to decide. Just keep in mind that you are currently relying on conventional routing, and not on attribute routing.

julealgon avatar Aug 12 '22 19:08 julealgon

Indeed you are right, by putting [Route("api/class1")] I have endpoints for odata and separate api, so I can use odata or standard api. here's the report rap1 rap2 (2)

Thanks for your help

Diaz15h avatar Aug 12 '22 19:08 Diaz15h

@Diaz15h If your issue has been resolved, kindly close this issue.

KenitoInc avatar Aug 16 '22 10:08 KenitoInc