Skip to content

Feature ask: Have ability to ignore query options when EnableQueryAttribute use query options to apply queryable. #826

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

Open
SamGuoMsft opened this issue Jan 26, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@SamGuoMsft
Copy link

Assemblies affected
Which assemblies and versions are known to be affected e.g. ASP.NET Core OData 8.x

Describe the bug
@xuzhg , as we discussed, hope we can extend EnableQueryAttribute to support setting ignore query options when apply ODataQueryOptions to queryable during OnActionExecuted since consumer may pass the pagination to underlying backend service and only use in memory filter.

Reproduce steps
The simplest set of steps to reproduce the issue. If possible, reference a commit that demonstrates the issue.

Data Model
Please share your Data model, for example, your C# class.

EDM (CSDL) Model
Please share your Edm model, for example, CSDL file.
You can send $metadata to get a CSDL XML content.

Request/Response
Please share your request Uri, head or the request body
Please share your response head, body.

Expected behavior
A clear and concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Please share your call stack or any error message
Add any other context about the problem here.

@SamGuoMsft SamGuoMsft added the bug Something isn't working label Jan 26, 2023
@julealgon
Copy link
Contributor

I'm confused by this ask. If you want to manually handle parts of the query, you'd pass in the ODataQueryOptions into the controller and then not use [EnableQuery], right?

Are you asking for a variation of this flow that uses both at the same time or something like that?

All in all, this feels really odd to me.

@SamGuoMsft
Copy link
Author

@julealgon, the EnableQuery provides other benefits which we want to leverage (in face this attribute did too many things).

  1. Define different odata options for each action.
  2. Validate the ODataOptions
  3. Nicely integrate with swagger with all odata optoins via ApiVersioning nuget package.

So I would like to use all of them and have ability to configure the in memory querable which I want to use. Yes I can write a base controller to have a function to do the apply and override EnableQueryAttribute ActionExecuted as no ops. But it will be really nice that EnableQueryAttribute provides the ability to support ignore options.

@xuzhg , would like to hear your thoughts also here.

@julealgon
Copy link
Contributor

@julealgon, the EnableQuery provides other benefits which we want to leverage (in face this attribute did too many things).

  1. Define different odata options for each action.

This to me feels like it should either be a different attribute, or configured some other way.

  1. Validate the ODataOptions

I've used FluentValidator to validate ODataQueryOptions before.

Though I understand what you are asking here, I don't think keeping [EnableQuery] because of it is the cleanest approach.

  1. Nicely integrate with swagger with all odata optoins via ApiVersioning nuget package.

If just ODataQueryOptions<T> itself doesn't generate correct swagger documentation but [EnableQuery] does, an issue should be opened in the ApiVersioning github IMHO. This is a clear gap and shouldn't be taken into consideration for changes in OData itself.

So I would like to use all of them and have ability to configure the in memory querable which I want to use. Yes I can write a base controller to have a function to do the apply and override EnableQueryAttribute ActionExecuted as no ops. But it will be really nice that EnableQueryAttribute provides the ability to support ignore options.

It just seems to me like the proposed solution is not good. I understand the underlying reason for the request, but I'd propose we think more thoroughly about how this would be achieved.

@SamGuoMsft
Copy link
Author

SamGuoMsft commented Jan 26, 2023

@julealgon ,
#2 I understand we can use fluent validation, but seems we are doing redundant work that enable query attribute also did. But I do understand you propose this since as I mentioned earlier EnableQueryAttribute did so many things :).
#3 If just ODataQueryOptions, the swagger will generate all the odata query paratmers, but each action may only expose certain limited odata query options by its own, people use EnableQueryAttribute to control it and today AspVersioning library is consuming this attribute to generate swagger parameters.

This proposal is just based on the scenario which I am facing, it may not be idea, if you have any thoughts and idea, how we should acheive this, we can have discussion.

@brucezlata
Copy link

@julealgon , maybe the request is liking this:
[HttpGet] [EnableQuery(PageSize = 100)] public async Task<ActionResult<IEnumerable<User>>> Get(ODataQueryOptions<User> options)
I want to handle query by myself but hope query Attribute can handle page size.

Is that possible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants