Skip to content
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

OData OpenAPI AllowedQueryOptions is not generated correctly #853

Closed
dnperfors opened this issue Aug 17, 2022 · 5 comments · Fixed by #879
Closed

OData OpenAPI AllowedQueryOptions is not generated correctly #853

dnperfors opened this issue Aug 17, 2022 · 5 comments · Fixed by #879

Comments

@dnperfors
Copy link

Using .NET 6, OData 8 and the preview version of the library, I have some problems with generating correct swagger documentation for the AllowedQueryOptions.
All the code is based on the ODataOpenApiExample in the main branch.

Given the following:
options in Program.cs:

options.Count().Select().OrderBy();

On top of the OrdersController Get method:

[EnableQuery( MaxTop = 100, AllowedQueryOptions = Select | Top | Skip | Count )]

I get not 4, but 5 fields in the swagger documentation:
image
I wouldn't expect the "OrderBy" field and when I use it I get a 400 Bad Request with the message: " Query option 'OrderBy' is not allowed".

When I add:

[EnableQuery( MaxTop = 100, AllowedQueryOptions = Select | Top | Skip | Count | Filter )]

I do get the filter field, but I can't use it because I get the message: "The property 'customer' cannot be used in the $filter query option." (Although this might be more of a OData issue)

@commonsensesoftware
Copy link
Collaborator

Matching the OData query options via the various ways you can configure them has been a bit of a moving target and the rules don't feel to be very documented IMHO.

I can tell you that in the first case, the reason that you get $orderby is because it is defined by the .OrderBy() extension method in Program.cs. This is supposed to set/enable the query option for everything in the application. To my knowledge, you cannot disable this at a specific point. It sounds like I'm either wrong or something has changed. If we can confirm the behavior, I'm happy to change it.

Your second case highlights exactly what I'm talking about. You've allowed the Filter query option, but when you try to use $filter, it says it's not allowed. I suspect this is an issue with the OData implementation.

Right or wrong, my understanding of the query option rules are:

  1. All query options are [now] disabled by default
  2. Setting a global query option enables (or, oddly, disables) it for the whole application
  3. Applying [EnableQuery] enables query options for a specific route
  4. Applying Model Bound attributes or annotations in the EDM enables/disables query options for that entity or action
  5. All configurations are mutually inclusive, except for 4, which can explicitly disable a query option

Hopefully, you see the crux of the problem. If $orderby is globally enabled, then it should appear, even if a specific route doesn't define it because it's enabled for everything and can't be disabled by [EnableQuery]. Conversely, if $filter is not globally enabled, then [EnableQuery] on a specific route should enable it for that route.

It's been really hard to verify my interpretation of the rules versus misconfiguration of OData. It's easy to get in a situation where all the query options show up as expected, but it doesn't work because something else is misconfigured for OData. This makes it difficult to tell the difference between an actual bug and simple configuration. The OData documentation mentions how to use each method of configuration, but doesn't discuss what happens when you use multiple methods together.

This issue will likely require some more research to make sure the corrective action goes in the right direction. Thanks for reporting it.

@dnperfors
Copy link
Author

Right or wrong, my understanding of the query option rules are:

  1. All query options are [now] disabled by default
  2. Setting a global query option enables (or, oddly, disables) it for the whole application
  3. Applying [EnableQuery] enables query options for a specific route
  4. Applying Model Bound attributes or annotations in the EDM enables/disables query options for that entity or action
  5. All configurations are mutually inclusive, except for 4, which can explicitly disable a query option

I think you are mostly right here, but I think there are a few nuances in how you should think about it:"

  1. All query options are [now] disabled by default
  2. The global query options enable the possible and default options for the whole application
  3. Applying [EnableQuery] enables the default options, or you can override those with specific AllowedQueryOptions (default is All, but by providing a specific set, you can disable specific ones.)
  4. Applying Model Bound attributes or annotations in the EDM enables/disables query options for that entity or action

@commonsensesoftware
Copy link
Collaborator

Do you have links to documentation, code, or tests that highlight this? What I can remember seeing documented or from looking at code outlines each method. I don't recall a clear call out of mixing and matching options nor their precedence. Dialing that in is critical to making sure any change does what people expect.

@commonsensesoftware
Copy link
Collaborator

I can't find any specific rules, code, or tests that documents what the precedence of operation is. There are plenty of tests for individual scenarios, but I can't find anything that combines multiple scenarios together. As an example, I found that if you enable $filter via [EnableQuery] it still doesn't work unless you also enable Filter() via default or model bound settings. It's really hard to tell or understand what might be a bug in OData versus how it supposed to work.

I'm going to proceed with the discussion from here. [EnableQuery] will supersede other settings from a documentation perspective. If that doesn't jive with OData's implementation, that is a configuration that developers can fix in their own code (like the $filter example). This just means the documentation will be a little more lax and might indicate a client can perform a query option that isn't properly configured on the server.

#702 calls out a strange, but related case. I can't find any Model Bound specific way to enable $top or $skip. It seems that any query on a collection can allow it. Calling Page() will set either no limit or the maximum allowable $top value and page size. I'll move forward with enabling $top and $skip for any scenario where MaxTop is set (likely via Page()) for a collection (e.g. entity set). They can still be explicitly configured via [EnableQuery] or the API Explorer query options conventions.

My preliminary results show this seems to be what you (and others) are asking for. I didn't uncover any regressions in the test suite - yet, so I think things are good to move forward. These changes will be in 6.1, which is likely only days away now. 😄

@dnperfors
Copy link
Author

Great to hear 😀 I am looking forward to try it out as soon as possible 😀

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.

2 participants