swagger-api / swagger-codegen-generators

Apache License 2.0
284 stars 420 forks source link

[ALL] Form params - individual or as container? #315

Open skrysmanski opened 5 years ago

skrysmanski commented 5 years ago

While writing my own code generator I noticed something weird: codegen would generate models called Body, Body1, ... that the generated code didn't use.

After some digging I found out that this is a result of the change of how form parameters are handled in OAS2 (Swagger) vs. OAS3 (OpenAPI),

In OAS2 form parameters were defined like this:

parameters:
  - in: formData
    name: name
    type: string
    description: A person's name.
  - in: formData
    name: fav_number
    type: number
    description: A person's favorite number.

The "natural" form for the generated api methods here is to have individual parameters (name, fav_number).

In OAS3 the form has changed to this:

requestBody:
  content:
    multipart/form-data:
      schema:
        type: object
        properties:
          additionalMetadata:
            description: Additional data to pass to server
            type: string
          file:
            description: file to upload
            type: string
            format: binary

The "natural" form for this would be to pass all parameters as an object (as opposed to individual parameters).

In fact, if the content type is switched from multipart/form-data to application/json, all/most/some code generators will switch from individual parameters to container data types.

The problem with the redundant files I mentioned earlier stems from a design decision in the swagger parser: inline schemas are automatically "externalized", i.e. they're moved to the global components/schema section and replaced with a $ref. So, the example above basically becomes:

requestBody:
  content:
    multipart/form-data:
      schema:
        $ref: '#/components/schemas/body'

components:
  schemas:
    body:
      type: object
      properties:
        additionalMetadata:
          description: Additional data to pass to server
          type: string
        file:
          description: file to upload
          type: string
          format: binary

So, the code generators would see a model called body and thus generate a file for it - while the operations would use the formParams collection and create individual parameters.

The question now is: Which way will be the future?

The current implementation points to "container models" but this would be breaking change for existing api users. There are also two todos in the code that may suggest there is some plan for this. Unfortunately, they don't have an issue number so I'm not sure where their progress is tracked.

On the other hand we could stay with individual parameters but would then require a way to figure out whether a model file should be generated or not. (I'm currently trying this way but it's rather difficult because you need to find all schema usages in an OpenAPI instance - and there seems to be no straight forward way of doing this.)

HugoMario commented 5 years ago

Hello @skrysmanski , sorry for delay response. Current behavior identifies a requestBody with a "form" content type and the generated code will "split" the request body properties as individuals parameters.

See this example on swagger-codegen (3.0.0 branch)

from command at: https://github.com/swagger-api/swagger-codegen/blob/3.0.0/bin/java-petstore-feign.sh#L28-L34

using pet-store test definition :

    post:
      tags:
      - pet
      summary: Updates a pet in the store with form data
      operationId: updatePetWithForm
      parameters:
      - name: petId
        in: path
        description: ID of pet that needs to be updated
        required: true
        schema:
          type: integer
          format: int64
      requestBody:
        content:
          application/x-www-form-urlencoded:
            schema:
              properties:
                name:
                  type: string
                  description: Updated name of the pet
                status:
                  type: string
                  description: Updated status of the pet

you'll get this output:

 void updatePetWithForm(@Param("petId") Long petId, @Param("name") String name, @Param("status") String status);

from https://github.com/swagger-api/swagger-codegen/blob/3.0.0/samples/client/petstore/java/feign/src/main/java/io/swagger/client/api/PetApi.java#L160

Hope this help to clarify

skrysmanski commented 5 years ago

@HugoMario Sorry, if I wasn't really clear on what I meant. I know how the current implementation looks like and what it does.

This "issue" was more meant as a discussion on whether the currently implementation will and/or should stay that way - given that with OpenAPI 3.0 the "interpretation" of form params shifted from individual params to a request body.