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

Support OData $select for PUT/PATCH/POST #594

Closed
crackalak opened this issue Jan 3, 2020 · 6 comments
Closed

Support OData $select for PUT/PATCH/POST #594

crackalak opened this issue Jan 3, 2020 · 6 comments
Assignees
Milestone

Comments

@crackalak
Copy link

We are using $select on PUT/PATCH/POST requests to filter the properties on the model returned by a successful request. This works well but we cannot get swagger to show $select as an available parameter for these actions.

These are valid operations as per the OData spec http://docs.oasis-open.org/odata/odata/v4.01/cs01/part2-url-conventions/odata-v4.01-cs01-part2-url-conventions.html#sec_SystemQueryOptions

If I change the code below to return true for PUT/PATCH/POST, all works as it should, and it would also support the other rules of the spec.

https://github.com/microsoft/aspnet-api-versioning/blob/6ea139e77b82266ea1a8cc130b55725f8beeaa42/src/Microsoft.AspNet.OData.Versioning.ApiExplorer/AspNet.OData/Builder/ODataValidationSettingsConvention.cs#L146-L154

@commonsensesoftware
Copy link
Collaborator

The filtering on PUT and PATCH is clearly too much - by spec. POST is very tricky depending on what it refers to. This probably requires additional investigation. The goal is to have matching options based on the default conventions. Global query setting configuration make this a challenge because you have to filter options that do not apply for a given context even though they are enabled.

I need to go back and revisit to make it's truly possible, but you should be able to apply explicit conventions. The design intent was to be conservative on making implicit options based on spec, but still allow explicit options which may go off-spec. For example, the spec doesn't allow options in a DELETE, but doesn't mean someone doesn't support them anyway.

Explicit configuration should unblock you in the short-term. Improved implicit matching to the spec will come in a forthcoming patch. Appreciated the link to the spec; it's helpful.

Thanks

@crackalak
Copy link
Author

Thanks for looking at this, I can see that it would require more thought and configuration with the global settings.

I don't think explicit configuration unblocks me in this case, as the IsSupported check is run before any parameter descriptions are applied.

Either way, I have tried the following ways of enabling select on a patch method (using the SwaggerODataSample project) without success:

[EnableQuery(AllowedQueryOptions = AspNet.OData.Query.AllowedQueryOptions.Select)]
public IActionResult Patch( [FromODataUri] int key, Delta<Product> delta )
options.QueryOptions.Controller<V3.ProductsController>()
                                        .Action( c => c.Patch(default, default) )
                                            .Allow( Select );
var product = builder.EntitySet<Product>( "Products" ).EntityType.HasKey( p => p.Id );
            product.Select( AspNet.OData.Query.SelectExpandType.Allowed );
[Select]
public class Product

Am I missing something?

@commonsensesoftware
Copy link
Collaborator

Hmmm... OK. Thanks for the repro steps. I take a deeper look. If there is no immediate workaround, I'll carve out some time to move this up in priority and get a patch out. Thanks for your patience.

@davidyee
Copy link

davidyee commented May 3, 2020

Hi Chris, similar issue here looks like if we use the [HttpPost] attribute the exact same controller endpoint no longer shows the same OData operations it would have had if an [HttpGet] attribute was used instead (as shown in the Swagger output). According to the OData spec (same link as the first post in this issue), it would seem that a POST should support the exact same options as a GET:

For POST requests to an action URL the return type of the action determines the applicable system query options that a service MAY support, following the same rules as GET requests.

POST requests to an entity set follow the same rules as GET requests that return a single entity

Performing the HttpPost request manually in Postman with $expand works as expected so it's just an issue with the parameters being hidden from the Swagger output.

e.g.

// has $expand, $select, etc. parameters shown
[EnableQuery]
[HttpGet]
[ODataRoute("{key}")]
public async Task<IQueryable<MyModel>> Get([FromODataUri] int key)
{
    // ...
}

// no $expand, $select, etc. parameters shown
[EnableQuery]
[HttpPost]
[ODataRoute("{key}")]
public async Task<IQueryable<MyModel>> Post([FromODataUri] int key)
{
    // ...
}

Workaround

Explicit configuration didn’t work for me either. However, a workaround for now is to use an operation filter to force the parameter back similar to the code described here. The only problem is that the default descriptions get lost (e.g. $expand description usually say something like "Indicates the related entities to be represented inline. The maximum depth is 2." but with this approach it's just the specified hard-coded string).

Is this issue still being addressed and if not, is there a better way to have the parameters added with the same default descriptions that would normally be added?

@commonsensesoftware
Copy link
Collaborator

Quick update. Per spec, the feature will be updated with default behavior according to spec (or that's the intent). PUT, PATCH, and POST will return with support out-of-the-box. This may bring more query options than people want or expect, but we'll hash that out as we go. The spec calls out additional rules based on additional semantics in the URL, but I don't have an immediate plan to necessarily honor that (as it's complicated). It likely won't affect much. It's not entirely clearly how the OData libraries realize the spec, but I have found inconsistencies in the past.

@commonsensesoftware commonsensesoftware added this to the 5.0.0 milestone Oct 4, 2020
@commonsensesoftware
Copy link
Collaborator

This is now resolved in 5.0.0-RC.1. The official release will likely occur after 2 weeks of burn-in. If the issue returns, feel free to reopen the issue. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants