Skip to content

ODataQueryOptions parameter on method generates over 1600 parameters in Swagger UI #599

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

Closed
DoctorGamester opened this issue Jan 24, 2020 · 18 comments

Comments

@DoctorGamester
Copy link

If I have a method like this:

        /// <summary>
        /// get/search people
        /// </summary>
        /// <response code="200">Successful get</response>
        /// <response code="401">API key is missing or invalid</response>
        /// <response code="403">You are logged in but do not have the authorization to view this resource.</response>
        /// <response code="500">The request was formed correctly. An internal error is stopping the request from going through.</response>
        /// <response code="0">Error</response>
        [HttpGet]
        [ODataRoute]
        [Authorize(AuthenticationSchemes = BasicAuthenticationHandler.SchemeName)]
        [Produces("application/json")]
        [ProducesResponseType(typeof(ODataValue<IEnumerable<Person>>), 200)]
        [SwaggerResponse(statusCode: 401, type: typeof(Error), description: "API key is missing or invalid")]
        [SwaggerResponse(statusCode: 403, type: typeof(Error), description: "You are logged in but do not have the authroization to view this resource.")]
        [SwaggerResponse(statusCode: 0, type: typeof(OdataError[]), description: "Error")]
        [EnableQuery]
        public IActionResult Get()
        {
            //snip
        }

The moment I change it to have ODataQueryOptions, like this:

public IActionResult Get(ODataQueryOptions<Person> options)

Suddenly over 1600 parameters are generated by the Swagger UI, and a RequestBody that is normally null becomes full of information as well. It also takes an extremely long time to show these parameters when I click on the method in the UI.

I currently get around this by using the following code for a DocumentFilter:

        public void Apply(OpenApiDocument swaggerDoc, DocumentFilterContext context)
        {
            var paths = swaggerDoc.Paths.ToList();
            swaggerDoc.Paths.Clear();
            foreach(var item in paths)
            {
                foreach(var operation in item.Value.Operations)
                {
                    var parameters = operation.Value.Parameters
                        .Where(p => 
                            !p.Name.StartsWith("IfMatch") &&
                            !p.Name.StartsWith("IfNoneMatch") &&
                            !p.Name.StartsWith("Request") &&
                            !p.Name.StartsWith("Context") &&
                            !p.Name.StartsWith("RawValue") &&
                            !p.Name.StartsWith("Select") &&
                            !p.Name.StartsWith("Apply") &&
                            !p.Name.StartsWith("Filter") &&
                            !p.Name.StartsWith("OrderBy") &&
                            !p.Name.StartsWith("Skip") &&
                            !p.Name.StartsWith("Top") &&
                            !p.Name.StartsWith("Count") &&
                            !p.Name.StartsWith("Validator")
                            ).ToList();
                    operation.Value.Parameters.Clear();
                    foreach (var pr in parameters)
                        operation.Value.Parameters.Add(pr);
                    operation.Value.RequestBody = null;
                }
                swaggerDoc.Paths.Add(item.Key, item.Value);
            }

I am using the following packages, and targeting .NET Core 3.1:

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.Mvc.NewtonsoftJson" Version="3.1.1" />
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Versioning" Version="4.1.1" />
    <PackageReference Include="Microsoft.AspNetCore.Mvc.Versioning.ApiExplorer" Version="4.1.1" />
    <PackageReference Include="Microsoft.AspNetCore.OData" Version="7.3.0" />
    <PackageReference Include="Microsoft.AspNetCore.OData.Versioning" Version="4.1.1" />
    <PackageReference Include="Microsoft.AspNetCore.OData.Versioning.ApiExplorer" Version="4.1.1" />
    <PackageReference Include="Microsoft.Data.SqlClient" Version="1.1.0" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="3.1.1" />
    <PackageReference Include="Microsoft.Extensions.Logging.Debug" Version="3.1.0" />
    <PackageReference Include="Microsoft.VisualStudio.Web.CodeGeneration.Design" Version="3.1.0" />
    <PackageReference Include="Swashbuckle.AspNetCore" Version="5.0.0" />
    <PackageReference Include="Swashbuckle.AspNetCore.Annotations" Version="5.0.0" />
    <PackageReference Include="Swashbuckle.AspNetCore.Newtonsoft" Version="5.0.0" />
    <PackageReference Include="Swashbuckle.AspNetCore.Swagger" Version="5.0.0" />
    <PackageReference Include="Swashbuckle.AspNetCore.SwaggerGen" Version="5.0.0" />
    <PackageReference Include="Swashbuckle.AspNetCore.SwaggerUI" Version="5.0.0" />
  </ItemGroup>

Am I doing something wrong? Have I run into a support issue, or more an issue where I am not quite using the library combination properly?

@commonsensesoftware
Copy link
Collaborator

Weird. This has been reported before, but I'm not sure exactly what causes this. I can't seem to find the related issue, but I remember it. The most bizarre thing is that the samples, such as this one, demonstrate using ODataQueryOptions and that doesn't happen.

Do you happen to have the world's simplest repro? There's clearly something different about what some people are doing versus the examples that trigger this behavior. Even more strange is that ODataQueryOptions seems to be treated as a bound model, but it shouldn't because its model binder states that it's special (CancellationToken is another example of this type of behavior). There seems to be more to the story because if that was the only issue, I still wouldn't expect it to yield 1600 parameters! That to me is baffling. Sounds like some type of recursion or something.

@DoctorGamester
Copy link
Author

Hello. First of all allow me to apologize for dropping that on you on a Friday.

I'll see if I can get a small repo whipped up to demonstrate and I'll link it here.

@DoctorGamester
Copy link
Author

I just realized I completely dropped the ball on this. I'll remind myself to get something to you soon.

@commonsensesoftware
Copy link
Collaborator

No problem. I was reviewing the issues and I found the related issue. #529 refers to a similar behavior with 1700+ parameters. ☹️

@zinov
Copy link

zinov commented Jul 6, 2020

does anyone have any update on this question?

In my case, if I don't have the ODataQueryOption but I will need it. If I don't put on top of my controller the following attribute [Produces("application/json")] my swagger generates more outputs

image

I just need to produce actually 2 of them that are related to the same route, in this case

application/vnd.weatherforecast.type1.json
application/vnd.weatherforecast.type2.json

any idea how can I get rid of the others that are automatically added?

@commonsensesoftware
Copy link
Collaborator

@zinov that is somewhat of an unrelated issue. This looks like Swashbuckle. Honestly, this is close to what I would expect to see for OData. There are many variations of output formatters, which map to media types. There are few unrelated to OData that you could probably remove from your configuration (ex: text/plain). The others are valid, regardless of whether you use them or a client requests them. You can also filter down the list in a IOperationFilter.

It would seem that the Swashbuckle library now just assumes this unless you use the ProducesAttribute and ConsumesAttribute, which is wrong. This breaks things for people that version or do anything else with media type. I don't know why that decision was made, but it is also solved by re-adding things via a IOperationFilter.

@commonsensesoftware
Copy link
Collaborator

@DoctorGamester did you happen to have a repro of the problem. That would be immensely useful in trying to track down the problem.

@commonsensesoftware
Copy link
Collaborator

I was able to confirm there was an edge case where things might be explored incorrectly. It seems to be related to IDelta as opposed ODataQueryOptions. Regardless, this is expected to be fixed in 5.0.0-RC.1. The official release will likely occur after 2 weeks of burn-in. Feel free to reopen this issue or open a new one if the problem resurfaces.

@engenb
Copy link

engenb commented Nov 20, 2020

@commonsensesoftware I'll leave it up to you if you want to officially reopen this issue, but I am also experiencing this problem. I've added another sample project to my repo that demonstrates the issue.

https://github.com/engenb/AspNetCore.OData.Issues/tree/main/src/Issue599

start project Issue599 and point your browser at http://localhost:5000/issue599/explorer/index.html then click on the issue599/v1/foos route in Swagger to reproduce.

@commonsensesoftware
Copy link
Collaborator

@engenb it doesn't seem that your sample project has updated its references with the fix. The fix is in 5.0.0. The ultimate cause was a logical error that ended up making ODataQueryOptions sourced from the query string. ASP.NET Core will support this, but by flattening the entire object graph. This is why the parameter count is so high.

If this problem is still happening in 5.0.0, I'm happy to reopen and revisit this issue.

@engenb
Copy link

engenb commented Nov 20, 2020

I can't reproduce with 5.0.0 because my OData route doesn't show up as illustrated in #698

@mlop3s
Copy link

mlop3s commented Mar 13, 2021

Hi @commonsensesoftware,

I'm still experiencing this error. Even on 5.0.0.

This is how my controller is implemented:

        [HttpGet("projects/{projectId}/issues")]
        [Produces("application/json")]
        [ProducesResponseType(typeof(IEnumerable<GeminiIssue>), Status200OK)]
        [ProducesResponseType(Status400BadRequest)]
        public async Task<IActionResult> GetGeminiIssues(long projectId, ODataQueryOptions<GeminiIssue> options)
        {
            var validationSettings = new ODataValidationSettings()
            {
                AllowedQueryOptions = Select | OrderBy | Top | Skip | Count | Filter,
                AllowedArithmeticOperators = AllowedArithmeticOperators.None,
                AllowedFunctions = AllowedFunctions.None,
                AllowedLogicalOperators = AllowedLogicalOperators.All,
                MaxTop = 20,
            };

            try
            {
                options.Validate(validationSettings);
            }
            catch (ODataException d)
            {
                return BadRequest(d.Message);
            }

Endpoints:

            app.UseEndpoints(endpoints =>
            {
                endpoints.MapControllers();                
                endpoints.Select().Filter().Expand().MaxTop(20);
                endpoints.MapODataRoute("odata", "odata", GetEdmModel());
            });

And this is what swagger shows:

image

My packages:
image

This is also strange and only happens when i have odata.
It adds every possible class it finds into the schema:
image

When I send a

http://localhost:45179/odata/projects/184/issues?$top=1

I get first an Exception
Microsoft.OData.UriParser.ODataUnrecognizedPathException: 'Resource not found for the segment 'issues'.'

But it continues and i get the data,

I've prepared a reproduceable project and just added you.
https://github.com/mlop3s/Gemini.API

@engenb Is it working on your side?

Thanks,
Marco

@davidebriscese
Copy link

@mlop3s Were you able to solve the problem? I have the same errors you have.

@Liero
Copy link

Liero commented Apr 20, 2021

Same problem here in 5.0.4

[EnableQuery]
[HttpGet]
public IQueryable<DeliveryNote> Get(ODataQueryOptions<DeliveryNote> options) {}

app.UseEndpoints(endpoints =>
{
    endpoints.MapControllers().RequireAuthorization();
    endpoints.EnableDependencyInjection();
    endpoints.Select().Filter().OrderBy().MaxTop(100).Count();
});
    <PackageReference Include="Microsoft.AspNetCore.OData" Version="7.5.7" />
    <PackageReference Include="Swashbuckle.AspNetCore" Version="6.1.2" />

@davidebriscese
Copy link

I made a minimal repro of the problem: ODataIssue559.
Same problem as 739 currently open.

@mlop3s
Copy link

mlop3s commented Apr 20, 2021

Hi,

on my case i've just removed the projects:

from:
http://localhost:45179/odata/projects/184/issues?$top=1

to:
http://localhost:45179/odata/184/issues?$top=1

and it worked.
It's something with the routing. Hope it helps

Cheers,
Marco

@nathanvj
Copy link

I'm also experiencing this problem on .NET 5 with

<PackageReference Include="Microsoft.AspNetCore.OData" Version="8.0.1" />
<PackageReference Include="Swashbuckle.AspNetCore" Version="6.2.1" />

The OData endpoints work as they should but the ODataQueryOptions are included in the Swagger UI
image

Also a bunch of random classes are included in the schemas.
image

And media types on response schemas seem to be bloated, but I assume it correlates with the issue.
image

Would be very grateful is someone could help me out.

@commonsensesoftware
Copy link
Collaborator

@nathanvj

  1. OData 8.0 is not officially supported - yet (still; it's rewrite 😞)
    a. Anything working is happenstance
  2. The reported media types are correct
    a. OData registers a lot of formatters for the various customer parameter combinations
    b. There are many ways you can change or filter the list, but this is the correctly reported list
    c. Just my option, but Swashbuckle's behavior to completely bypass the API Explorer to check for ConsumesAttribute and ProducesAttribute is wrong

@DoctorGamester, @engenb, @mlop3s (and anyone else I missed)
This issue may be closed, but @davidebriscese 's repro in #739 provided the most context (and it's still open). Thank you all for your repros as well. There definitely was a bug, but that was fixed in the 5.0.0 release. The primary culprit appears to be that you have mixed vanilla controllers and OData controllers. This is allowed and supported; however, I believe your routing configurations are incorrect. You should make sure that your routes not only return a response, but that they return an OData response. A telltale sign is looking for the OData metadata if you ask for application/json;odata.metadata=full. Another simple case is collections. OData will return {"value": [{...}]}, but non-OData will be [{...}].

In the @davidebriscese repro, he is using part of OData; e.g. just the query options. This is actually a supported scenario because it just does some LINQ query expression manipulation without the rest of OData. The problem is that without the rest of the OData services, key metadata parts are missing. The thing missing that will cause this is the OData model binders. Without that registered, ODataQueryOptions looks like any other complex action parameter to ASP.NET Core and the API Explorer. The default behavior for this is to explode the object into a flat set of query parameters, which is supported out of the box. ODataQueryOptions has number of properties that are themselves complex, which is how you end up getting 1600+ parameters - yikes!

Cross-referencing this information is what makes me think your routes are not correctly configured; they are being explored as non-OData routes and ODataQueryOptions has no model binder. I would expect similar behavior for other special OData parameters types such as ODataPath and Delta. OData routing is very picky, even with attributes. When you mix with non-OData controllers, it's pretty easy to get false-positives that things are working from successful responses. I'm willing to bet with some deeper analysis, you'll find the offending APIs and routes are not being picked up as OData routes; resulting in 💥 .

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

8 participants