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

[BUG] Problems with model parameters and form-urlencode #6568

Open
4 of 6 tasks
neclimdul opened this issue Jun 5, 2020 · 4 comments · May be fixed by #13566
Open
4 of 6 tasks

[BUG] Problems with model parameters and form-urlencode #6568

neclimdul opened this issue Jun 5, 2020 · 4 comments · May be fixed by #13566

Comments

@neclimdul
Copy link
Contributor

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • What's the version of OpenAPI Generator used?
  • Have you search for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Bounty to sponsor the fix (example)
Description

I'm not sure where the bugs here are but there are some problems the way parameters are handled for form encoded resource. There is at least a bug somewhere in the code so I'll just talk it through it.

First, I'd prefer these not be filtered at all because filtering and collapsing the parameters makes the generated code inconsistent with the Spec/Documentation. While there is a separate set of documentation generated by this project but I think its a pretty big experience problem for developers when it doesn't match. This seems to be a feature added in #74 and #51. I agree with the idea of filtering unused schemas but these schemas are actually used.

Second, it breaks the ability to use API's with dynamic objects. Marketo for example has a schema that can be customized by admin's, adding fields to different models. It is possible to developer the ability for additionalProperties when the model to the method and serialization but it is impossible when the properties are hard-coded into the signature.

Third, the filtering isn't 100%. In the Person2 example we add the reference different and the filtering doesn't catch it.

openapi-generator version

This seems to affect all versions since the fork. The filtering was added before the 3.0.0 release.

OpenAPI declaration file content or url
openapi: 3.0.0
info:
  title: test
  version: 1.0.0
  contact: {}
security: []
servers: []
paths:
  /person1:
    post:
      operationId: createPerson1
      requestBody:
        content:
          application/x-www-form-urlencoded:
            schema:
              $ref: '#/components/schemas/Person1'
        required: true
      responses:
        default:
          description: Unexpected error
      parameters: []
  /person2:
    post:
      operationId: createPerson2
      requestBody:
        $ref: '#/components/requestBodies/Person2'
      responses:
        default:
          description: Unexpected error
      parameters: []
components:
  requestBodies:
    Person2:
      content:
        application/x-www-form-urlencoded:
          schema:
            $ref: '#/components/schemas/Person2'
      required: true
  schemas:
    Person1:
      type: object
      properties:
        email:
          type: string
          format: email
        enabled:
          type: boolean
    Person2:
      type: object
      properties:
        email:
          type: string
          format: email
        enabled:
          type: boolean
        extra:
          type: array
          items:
            type: string
Command line used for generation
docker run --rm -v ${PWD}:/local/ openapitools/openapi-generator-cli:v3.0.0 generate \
   -i test.yaml -g php -o /local/.build
docker run --rm -v ${PWD}:/local/ openapitools/openapi-generator-cli:latest generate \
   -i test.yaml -g php -o /local/.build
Steps to reproduce

Build and review the generated API.

Related issues/PRs

behavior added by #51/#74

Suggest a fix

My preferences would be to go back to swagger's behavior of using models for forms the same as other content types.

@neclimdul
Copy link
Contributor Author

This is I think the only bug blocking me adopting this for my Marketo library and its still unfortunately a show stopper.

If its not clear how broken this is, since these models but are form parameters this logic is filtering them out and inlining the properties of the models directly on the method.

    public function createPerson1($email = null, $enabled = null)

However the OpenAPI spec explicitly supports the Json Schema additional properties. Since there is no object to add properties to they are thrown away breaking the spec. This means you can't add the custom "first name" or "twitter username" or whatever custom properties you have on your Person in the example.

Marketo's API uses Additional Properties extensively to support dynamic user defined fields on its entities and this makes many of its API's unusable as you can't address many of the fields required to make them useful. For example, setting custom fields on a lead or instance specific fields on a form submission.

@wing328
Copy link
Member

wing328 commented Mar 11, 2021

However the OpenAPI spec explicitly supports the Json Schema additional properties.

I don't think the PHP client supports additional properties yet. Generators like Go, PowerShell, Java, C#, Python have better support for additional properties. Please give these a try to see if it's what you're looking for (there's an option to enable better additional properties support, make sure it's switched on)

You may consider sponsoring this enhancement to make it a higher priority among the openapi-generator community.

@neclimdul
Copy link
Contributor Author

You're correct. I've added basic support with custom templates which works fine for both openapi and swagger generators as long as the properties aren't too complex and excluding this bug.

That said, if those generators have full support, I'll try to recreate the problem there and see if it has the same impact. My understanding of the filter behavior in #51/#74 was that this is fundamental to the way openapitools filters models so that would be the case.

@LazyRichard
Copy link

It seems when using x-www-form-data, the array type of model doesn't generate properly.

And I think this behavior happens across all the generators.

The below is openapi.yml for reproduce

openapi: 3.0.0
info:
  title: openapi-gen-_x-www-form-urlencoded
  version: 1.0.0

paths:
  /person1:
    post:
      operationId: createWithFormData
      requestBody:
        content:
          application/x-www-form-urlencoded:
            schema:
              $ref: '#/components/schemas/FormData'
      responses:
        default:
          description: Unexpected error
components:
  schemas:
    FormData:
      type: object
      properties:
        prop:
          type: string
        array_props:
          type: array
          items:
            $ref: "#/components/schemas/Data"
    Data:
      type: object
      properties:
        name:
          type: string
        password:
          type: string

JAVA client

  • With array (using above spec)
    • no model directory was found
  • Without array (comment out array_props on above spec)
    • model directory exists and models are properly generated

Typescript client

  • With aray (using above spec)
    • all.ts file is empty
  • Without array (comment out array_props on above spec)
    • all.ts fils is not empty and Data.ts exists

neclimdul added a commit to neclimdul/openapi-generator that referenced this issue Oct 1, 2022
Remove form parameter schema filtering because some API's actually use this and are broken.

Fixes OpenAPITools#6568
@neclimdul neclimdul linked a pull request Oct 1, 2022 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants