OData / AspNetCoreOData

ASP.NET Core OData: A server library built upon ODataLib and ASP.NET Core
Other
457 stars 158 forks source link

Duplicate results shown in OData route debug view for conventional and attribute routed routes #428

Open julealgon opened 2 years ago

julealgon commented 2 years ago

The OData route debug view currently show matches for all conventions, regardless if they are duplicates. Let's take the default Weather Forecast sample for this.

If we have:

[Route("Forecasts")]
public class ForecastsController : ODataController
{
    [HttpGet]
    public IQueryable<WeatherForecast> Whatever()
    {
        return this.repository.GetForecasts();
    }

Then the debug view correctly lists GET Forecasts only once: image

However, if I change the method name from Whatever to Get, it will now show the exact same route twice (due to now matching both via convention, and attribute routing):

[Route("Forecasts")]
public class ForecastsController : ODataController
{
    [HttpGet]
    public IQueryable<WeatherForecast> Get()
    {
        return this.repository.GetForecasts();
    }

image

The application works correctly in both cases, resolving the GET action to the same method. The way the routes are displayed in the debug view, however, is fairly confusing. IMHO, this should be fixed in one of 2 ways:

  1. Remove duplicate entries from the table and only show the first matches for each route (like what the convention priority itself does normally, which will hit the attribute route and skip other conventions)
  2. Add a third column to the table, to indicate which convention resulted in the route's inclusion.

1 seems simpler to me, but 2 might be of help for debugging purposes.

FYI, the exact same behavior is present for all other route types and verbs: as long as they are attribute routed and match the naming convention at the same time, they appear twice. I used the GET entityset here as an example only.

xuzhg commented 2 years ago

@julealgon Typically, don't mix them together. Any reason why did you do like this?

I know it's a problem. But, it seems no matter how I implement could not meet all the requirements.

julealgon commented 2 years ago

@julealgon Typically, don't mix them together. Any reason why did you do like this?

I'm not completely sure what you mean with "why did you do like this". As you can see from the example, all I have is a single Get method annotated with a [HttpGet] attribute. This of course triggers attribute routing as expected, but I don't have control over the conventional routing. It is triggering automatically based on the pattern. You seem to imply that one can opt into only attribute routing. Unless I'm mistaken, there is no way to "deactivate" the OData conventional routing mechanism, or is there? I believe it is safe to assume that one would expect this to generate a single route for the entity set, yet 2 identical routes are presented in the table. I know why 2 are showing because I understand the underlying mechanisms, but new people coming into the framework for the first time would have no idea whatsoever about what is going on here: they will see the duplicate entries and won't be able to match them to their code which "only has a single route".

As for context/scenario, I had a v7 sample project and was migrating it to v8 to see what the new features were. Additionally, I was also doing an experiment on migrating a normal MVC API (using the default API project template) to OData and seeing what the potential pain points would be.

I think it's very important for the framework to be consistent and predictable in its behavior. IMHO, we are violating the Principle of Least Astonishment right now with the current implementation and this is bad for consumers: it creates confusion, causes people to ask the wrong questions, etc.

If the table represents "routes", then it makes zero sense to have multiple entries for the same route: they are semantically the "same route" anyways. From this perspective, it doesn't really matter "who discovered the route", only that "the route was discovered".

I know it's a problem. But, it seems no matter how I implement could not meet all the requirements.

I don't know the internals enough to understand why you think it can't be implemented in a way that meets all requirements, so if you could clarify on the challenges that would be great. Having said that, this issue in particular is in regards to the presentation of the matches, and not necessarily about the matches themselves.

The main problem here is that routes are presented in an apparently "duplicated" manner. Seeing 100% duplicated data in a table like this is very off-putting. This is why I proposed a couple of ways to fix this presentation problem: either remove the duplicated entry altogether (a .Distinct, or changing the collection to ISet if you will), or add more columns to the table so that the duplication goes away. The approach to take would depend on the intent of the view in the first place. Can you elaborate why none of these options would be a good fit?

Sure, if the discovery mechanism itself guaranteed non-duplicate routes, that would be even better (and I do think that would be the way to go). However, I think this is fixable even without that in place.