Routing inconsistency

[karayanni]

Hi Team,

I have a simple controller - using Odata8.0.8 in .Net6

public class StudentsController : BaseExternalController
    {
        [HttpGet("Students/{key}/GetStudentGrades")]
        public async Task<IActionResult> GetStudentGrades(string key)
        {
            business logic...

            return Ok(grades);
        }
    }

I defined the method in my EDM:

    var student= builder.EntitySet<Student>("students").EntityType;
    student.HasKey(student => student.Id);

    builder.AddComplexType(typeof(Grades)).Namespace = builder.Namespace;
    var assets = student.Action(nameof(StudentsController.GetStudentGrades)).ReturnsCollection<Grades>();
    assets.Namespace = builder.Namespace;

when trying to call the api/students/someId/GetStudentGrades I can't reach the endpoint (api is the Odata route prefix when constructing the route template.)

Important notes:

• changing only the http method attribute from

       [HttpGet("Students/{key}/GetStudentGrades")]

to

        [HttpPost("Students/{key}/GetStudentGrades")] 

will solve the issue (will reach the endpoint as expected.)

• The AspNetCore routing Works, Calling the method directly without the Odata prefix reaches the controller (~~api~~/students/someId/GetStudentGrades)

• The same EDM / controller structure worked in OData in .net Framework 4.7.2 with Microsoft.AspNet.OData.7.5.12

[julealgon]

There is nothing magical in the routing system that will "add" your prefix automatically to the routes you define. If you use attribute routing, you have to be explicit about each route you add.

Change this:

[HttpGet("Students/{key}/GetStudentGrades")]

to this:

[HttpGet("api/Students/{key}/GetStudentGrades")]

And test again.

Either that, or remove the "api" prefix when configuring OData.

Important notes:

• changing only the http method attribute from

       [HttpGet("Students/{key}/GetStudentGrades")]

to

        [HttpPost("Students/{key}/GetStudentGrades")] 

will solve the issue (will reach the endpoint as expected.)

This is probably because you give up on explicit attribute routing when you do this and fallback to convention routing for bound actions. I.e its not that it "fixes" it, you just changed the routing mechanism from explicit to implicit. The implicit routing mechanism then implies an HttpGet on that route and matches it with your EDM, completely ignoring your HttpPost since actions should.

Having said that, aren't OData actions always supposed to run using the POST verb due to (potential) side effects while OData functions use the GET verb?

Why did you have a GET action?

[KenitoInc]

@karayanni Actions have "side effects" so are invoked by POST method. Functions have no "side effects" so are invoked by GET method.

[KenitoInc]

@karayanni Did the information above help resolve your issue?

[karayanni]

Thank you for the information, indeed i have a GET method by design and it should be a function not an action.

I fixed the EDM, Yet still, after defining the EDM with an action and not a funciton - the method is not found by the implicit routing.

Can you please provide the way to achieve said endpoint definition? if what I did is the best practice I'll be glad to provide any additional info/schedule a quick call to understand what is the issue and is this a BUG

also sorry for the late responses - I'm OOF this week and the next one

[karayanni]

After a call with Clement +@habbes we came to few conclusions: if i add () at the end of the uri Students/{key}/GetStudentGrades() the implicit routing will work

this is most likely due to functions routing expecting parameters, although that is not always the case (parameterless functions should work with no parentheses what so ever).

[KenitoInc]

There is also a config that allows parameterless functions to work with no parantheses. Try the code below

builder.Services.AddControllers().AddOData(opt =>
{
    opt.AddRouteComponents("odata", EdmModel.GetEdmModel()).Count().OrderBy().Filter().Select().Expand();
    opt.RouteOptions.EnableNonParenthesisForEmptyParameterFunction = true;
});

[julealgon]

@KenitoInc , shouldn't we consider just always allowing that to simplify the API/improve usability? Is there a specific reason to have this as a configurable option instead of just defaulting the behavior to true always?

[KenitoInc]

@KenitoInc , shouldn't we consider just always allowing that to simplify the API/improve usability? Is there a specific reason to have this as a configurable option instead of just defaulting the behavior to true always?

@xuzhg Any comments?

[xuzhg]

@julealgon @KenitoInc Typically, a qualified function without parameters can call without parenthesis. However, we also support the unqualified function call. In this case, it could be a problem without parenthesis, because the function name could be considered a "property" name.

For example: [HttpPost("Students/{key}/GetStudentGrades")] we can not distinguish from a function call or a property named "GetStudentGrades". In order to make it clear, an empty parenthese is better for ODL to guess what it is.

You may ask "Why can't we search the model, if we find it's a defined function name, treat it as a function, otherwise, treat it as a property"? Yes, we can, however, what about if "Student" does have a property named "GetStudentGrades", meanwhile, we have a function named "GetStudentGrades" for the "Student" type?

So, by default, we generate the parentheses for the empty parameter, and also provide a way/configuration to get rid of the parenthese. any thoughts?

[julealgon]

So, by default, we generate the parentheses for the empty parameter, and also provide a way/configuration to get rid of the parenthese. any thoughts?

I just wonder if we are making the API less intuitive overall because of a very specific corner case. At this point, I'd probably just consider throwing some ambiguity exception if we cannot decide between property/function/etc and have that as something that is just not supported.

Alternatively, only in that scenario, we should require the parenthesis for disambiguation: have it work by default on normal cases with or without it, but require it when there are ambiguity issues (say this in the exception message).

Having such an exception that then forces the use of parenthesis would also reduce the amount of logic in OData itself: since it wouldn't need to "deduce" anything, and leave the decision to the consumer.

To me that would be a better approach to this than having it disabled by default as it would probably make things simpler/more intuitive.