Investigate multiple schema case (different mime types) in ModelUtils

jmini commented 6 years ago

In ModelUtils those two methods return a single Schema for a RequestBody or a ApiResponse:

ModelUtils.getSchemaFromRequestBody(RequestBody)
ModelUtils.getSchemaFromResponse(ApiResponse)

This feels wrong because when multiple Mime types are defined (like application/json or application/xml) different Schemas can be returned.

With #74 an additional warning log was added:

[main] WARN org.openapitools.codegen.utils.ModelUtils - Multiple schemas found, returning only the first one

We should investigate on the impacts.

When multiple mime types are defined we might need to compare if the defined schema are different or not.

jimschubert commented 6 years ago

@jmini I've tagged this as general discussion/suggestion. We don't have a tag for infrastructural code that affects all generators, and it wasn't clear to me if you're documenting a thought or looking for additional inputs (I agree that in OAS 3.0, you can end up with different models but it's not as easy in OAS 2.0).

JanMosigItemis commented 5 years ago

Hi there, I recently encountered these two warnings and find them rather annoying. That is: Although my input files are perfectly valid (from what I can tell), my build log contains these warnings, indicating that there is something wrong where afaik there isn't.

Is it possible to suppress those warnings or get rid of them by any other means?

alizeynalli90 commented 4 years ago

Hi I am also getting this warning when i generate Apis from my yaml file. The thing is I am having text/csv or application/json contents for the same endpoint in order to avoid duplicate calls. It works perfectly fine for different response forms. But I find the warning annoying. So suppressing the warnings would be good. Any ideas?

UnleashSpirit commented 4 years ago

Hi, i'm facing the same issue but in request. Although for response it may not be a problem, in request I need the two (or more) possibilities to be generated (typescript-angular) Here I got only one and can't use the second option. I can uderstand that it may be a conception problem from service but legacy doesn't offer many options some times.

ngarg-panw commented 4 years ago

Hi, I am also facing the same issue. Here is my swagger: requestBody: content: application/vnd.api+json: schema: title: convertRequest type: object required: [data] properties: data: type: object properties: templateType: type: string templateVersion: type: string application/octet-stream: schema: title: templateFile type: object properties: templateFile: type: string format: binary application/zip: schema: type: string format: binary

I am getting the following response: > Task :openApiGenerate systemProperties will be renamed to globalProperties in version 5.0 Multiple schemas found in the OAS 'content' section, returning only the first one (application/vnd.api+json) Multiple schemas found in the OAS 'content' section, returning only the first one (application/vnd.api+json) Multiple schemas found in the OAS 'content' section, returning only the first one (application/vnd.api+json) Multiple MediaTypes found, using only the first one

Any help is appreciated. Thanks!

typhoon2k commented 4 years ago

@jmini , @jimschubert, IMO the easiest approach here would be to build CodegenOperation for each combination of possible request body ~~content types~~ schemas and response ~~content types~~ schemas in operation. This will allow to keep existing templates without modifications.

Methods mentioned above will need to have additional contentType parameter. Additionally to support this approach generation of operation IDs will need to be enhanced (to avoid duplicate operation/method/function names in generated code).

jimschubert commented 4 years ago

@typhoon2k would you be able to provide an example of what you mean?

The issues that I see are:

Generators that expect a single response or request body would have to be updated to support multiple. This includes built-in templates, but also affects user customizable templates.
Many built-in generators don't support content negotiation, and because Mustache templates are logic-less, there's a lot of code that would unintentionally iterate over multiple request bodies or responses. For example: At best, those generated outputs would result in erroneous code or broken compilation. At worst, this could introduce attack vectors (DoS, buffer overflow/underrun) in business critical applications where such code doesn't result in compile-time errors.
The last part of your comment assumes we can generate a new operation per request/response body. That's not the case because some languages and frameworks don't support method overloading. Many specifications can define the same schema for request/response but define the transport format (xml, json, etc), and some frameworks such as aspnetcore handle those additional responses via annotation and IoC (see example).

typhoon2k commented 4 years ago

@jimschubert , let's use the following example:

paths:
  /person:
    post:
      requestBody:
        required: true
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/SuperManA"
          application/xml:
            schema:
              $ref: "#/components/schemas/SuperManA"
          application/avro:
            schema:
              $ref: "#/components/schemas/SuperManB"
      operationId: someOperation
      responses:
        '201':
          description: OK
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/SuperManA"
            application/xml:
              schema:
                $ref: "#/components/schemas/SuperManB"
            application/avro:
              schema:
                $ref: "#/components/schemas/SuperManB"

Taking into account current situation (that many generators expect only one schema in request body and response) my idea was that we can generate 4 CodegenOperations:

someOperation1:
- path: /person
- consumes: has "application/json" and "application/xml"
- produces: "application/json"
- bodyParam: SuperManA
- responses: SuperManA
someOperation2:
- path: /person
- consumes: "application/json" and "application/xml"
- produces: "application/xml" and "application/avro"
- bodyParam: SuperManA
- responses: SuperManB
someOperation3:
- path: /person
- consumes: "application/avro"
- produces: "application/json"
- bodyParam: SuperManB
- responses: SuperManA
someOperation4:
- path: /person
- consumes: "application/avro"
- produces: "application/xml" and "application/avro"
- bodyParam: SuperManB
- responses: SuperManB

I.e. expectation to have single request body and response is not broken.

On your example with JSONifying response - I feel that this example works only for specific use cases, where only one response is defined (or maybe generator is reducing list of responses to only one 2xx response). Also it would be useful to include isJson/isXml/isBinary/etc into CodegenResponse to make sure that correct conversion is used.

On your last item - unfortunately I don't have enough knowledge on those languages/frameworks, but if framework permits unique method/function names (generated using unique operationId) with duplicate paths & different content types, then that should work. If duplicate paths are not permitted, then this approach won't work.

I consider proposed approach more as a workaround to reduce amount of necessary changes. Proper solution would be to introduce additional map in CodegenParameter/CodegenResponse to hold schemas (per content type). In transition period all existing (deprecated) schema-related getters in these 2 classes can return properties of first schema in underlying map.

jimschubert commented 4 years ago

@typhoon2k I'm assuming your example expects 1:1 matching with request and response content types? Again, that's not really something we can assume across the board due to content negotiation. For instance, a request can pass xml and accept json, while another expects xml+xml. We'd have to generate each permutation of these in some generators.

What do you think about if we were to move the logic which grabs the first element of each of these lists into DefaultCodegen somewhere, and allow generators to pass all request/responses to templates? It would also allow generators that require it to generate transient operations with the additional parameters.

That way we can phase this support rather than tackling as all-or-nothing. I think if we go that route, we could add a generator feature enum or two, like MultipleContentTypesRequest and MultipleContentTypesResponse. This would also allow people to search for this support on our site.

typhoon2k commented 4 years ago

@jimschubert

I'm assuming your example expects 1:1 matching with request and response content types? No, any schema, with any content type in request and any content type in response. Permutations only based on request schema and response schema.

I was thinking about replacing all CodegenProperty related fields with Map<String, CodegenProperty> in CodegenParameter and CodegenResponse (i.e. map from content type to schema). As I said for backwards compatibility getters of replaced fields can return values of the same fields from first CodegenProperty in map. I will take a look on this (also on your proposal) during next week.

JanMosigItemis commented 4 years ago

Thanks for working on the issue guys 👍

typhoon2k commented 4 years ago

@jimschubert , I've reviewed dependencies that we have on CodegenParameter and CodegenResponse fields and have a feeling that it will be quite challenging to implement my proposal. I agree with you that introducing new fields in CodegenOperation that would return new MultipleContentTypesRequest and MultipleContentTypesResponse and filling them in DefaultCodegen would be a bit easier and would guarantee that nothing is broken.

cshamrick commented 4 years ago

@typhoon2k What's the status of this change? I'm currently developing an API that is versioned via the Accept header, which if documented correctly in our oas definition, would ideally produce separate operations (e.g. someOperationV1, someOperationV2) as you described in your proposal above. Has there been any movement toward this type of setup?

YassinHajaj commented 4 years ago

Hi, any updates on this change?

tomaszwozniak8 commented 3 years ago

I'm also curious about updates on this.

alexrashed commented 3 years ago

In my opinion, content negotiation is a very basic concept of HTTP / REST. We are also facing similar problems and would love to see a solution for that.

nikosmoum commented 3 years ago

Any updates on this? Returning different schema based on accept header is common imo. We are currently forced to workaround this, which is not ideal, so it would be great if this was implemented.

gbormann commented 3 years ago

We're suffering from the same issue.

What's the status of this issue after 2.5years?

exejutable commented 3 years ago

Any status of this?

chutzemischt commented 3 years ago

Are there any plans to implement this in the near future? IMO this is a bug. Why can't the OpenAPI Generator handle multiple content types? Other generators handle this correctly.

I think having different content types in the requestBody is a common use case, e.g.:

/api/item:
    post:
      operationId: addItem
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/Item'
          multipart/form-data:
            schema:
              type: object
              properties:
                item:
                  $ref: '#/components/schemas/Item'
                file:
                  $ref: '#/components/schemas/InputStream'

blaubaer commented 3 years ago

I have also problems to find a way to deal with that except breaking the REST principle by having multiple methods for multiple response content types.

I'm also open for alternatives.

jamesfoster commented 2 years ago

Multiple schemas found, returning only the first one

In my case, at least, the client I want to generate only needs to support a single content type. However, "the first one" is not the one I want. Is there a way to specify which one to use?

generate [...] --preferred-content-type application/json

Thanks for your assistance.

artemptushkin commented 2 years ago

Please start support content negotiation, it's one of the basic REST concepts. Should be on the high demand, imo

MelleD commented 2 years ago

Is there any news here? What about this PR (uh 2019?). Doesn't it solve the problem?

https://github.com/OpenAPITools/openapi-generator/pull/3991

Or this one? https://github.com/OpenAPITools/openapi-generator/pull/10973

spacether commented 2 years ago

Yes #10973 solves this problem; I am using it in this PR: https://github.com/OpenAPITools/openapi-generator/pull/8325 to allow users to send request bodies with different body content types supported. One can see it working here: https://github.com/OpenAPITools/openapi-generator/blob/master/samples/openapi3/client/petstore/python-experimental/petstore_api/api/pet_api_endpoints/add_pet.py#L78

pet_parameter = api_client.RequestBody(
    content={
        'application/json': api_client.MediaType(schema=SchemaForRequestBodyApplicationJson),
        'application/xml': api_client.MediaType(schema=SchemaForRequestBodyApplicationXml),
    },
    required=True,
)

artemptushkin commented 2 years ago

@spacether Just to extend your answer based on what I see. For Java, it's expected to be fixed with https://github.com/OpenAPITools/openapi-generator/issues/6708 in 5.3.1

MelleD commented 2 years ago

@artemptushkin There is an extension for Java and / or Spring? I haven't found a PR yet?

artemptushkin commented 2 years ago

@MelleD I refer to this comment of @spacether https://github.com/OpenAPITools/openapi-generator/issues/6708#issuecomment-998266587 so, I assume only so far. But I'd expect content-negotiation problem will be closed with #6708

tamershahin commented 2 years ago

any news about this?

banool commented 2 years ago

I've tried every typescript generator here but they all don't work for the multiple schema case: https://openapi-generator.tech/docs/generators/.

I'm running this command:

openapi-generator generate -g typescript-fetch -i ../../../api/doc/openapi_v2.yaml -o src/generated/ --enable-post-process-file

I looked at the help message and other docs and can't find any relevant flags to enable this.

Given the issue is closed I would expect this to work, am I missing something?

spacether commented 2 years ago

The investigation is done. One generator has implemented multiple content types. The information is available for any other generators to use.

This is an investigation ticket not an implementation ticket.

OpenAPITools / openapi-generator

Investigate multiple schema case (different mime types) in ModelUtils #144