Skip to content

[AVRO] Fix the package handling for the avro generator #4078

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

Merged
merged 3 commits into from
Oct 9, 2019

Conversation

Choobz
Copy link
Contributor

@Choobz Choobz commented Oct 6, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@sgadouar @wing328

Description of the PR

Fix the issue 3987
Also fix an issue with the outputFolder that wasn't overridable through opts

…3987)

Also fix an issue with the outputFolder that wasn't overridable through opts
@auto-labeler auto-labeler bot added the WIP Work in Progress label Oct 6, 2019
@auto-labeler
Copy link

auto-labeler bot commented Oct 6, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

There's an issue with inlined object but it's already present with the previous commit
@Choobz Choobz changed the title WIP: [AVRO] Fix the package handling for the avro generator (#3987) [AVRO] Fix the package handling for the avro generator (#3987) Oct 6, 2019
@Choobz
Copy link
Contributor Author

Choobz commented Oct 6, 2019

There's an issue with the inlined objects while generating the samples (avro-petstore.sh). That bug was already present on master. It doesn't correctly put the model.File in InlineObject1 for some reasons. Is it a known bug ?

Copy link
Contributor

@sgadouar sgadouar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tkanks for this fix

@wing328
Copy link
Member

wing328 commented Oct 7, 2019

That bug was already present on master. It doesn't correctly put the model.File in InlineObject1 for some reasons. Is it a known bug ?

Please open an issue with the details so that I can help take a look.

@@ -11,7 +11,7 @@
},
{
"name": "file",
"type": ["null", "model.File"],
"type": ["null", ],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to remove the comma after "null"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the bug. It's supposed to be "type": ["null", "model.File"] after the generation by avro-petstore.sh.

Since the result is the same with master, it's another modification that broke it between MR 3728 and today.

The petstore.yaml in question :

'/pet/{petId}/uploadImage':
    post:
      tags:
        - pet
      summary: uploads an image
      description: ''
      operationId: uploadFile
      parameters:
        - name: petId
          in: path
          description: ID of pet to update
          required: true
          schema:
            type: integer
            format: int64
      responses:
        '200':
          description: successful operation
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/ApiResponse'
      security:
        - petstore_auth:
            - 'write:pets'
            - 'read:pets'
      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

And for some reasons, that one (which is pretty much the same) works :

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

The only difference that makes sense is the content type and the format field.
It seems like the form-data field has seen some update and for some reason it can't default back to just the basic string type.

I've crossed check with the jax-rs-cxf implementation and they have a specific mustache for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I know where to fix it. Will merge this PR and file another PR to fix it.

@wing328
Copy link
Member

wing328 commented Oct 7, 2019

Also fix an issue with the outputFolder that wasn't overridable through opts

Can you please elaborate on that?

The line you removed seems to be common across all generators.

Copy link
Contributor Author

@Choobz Choobz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would my answer be enough for a bug report ?

@Choobz
Copy link
Contributor Author

Choobz commented Oct 7, 2019

I did some more test on OuputFolder and I was wrong about it, it wasn't my problem with destination path. I reverted that part.

@wing328 wing328 added Issue: Bug Schema: Avro Generation of Avro schemas and removed WIP Work in Progress labels Oct 9, 2019
@wing328 wing328 added this to the 4.2.0 milestone Oct 9, 2019
@wing328 wing328 merged commit 87c7707 into OpenAPITools:master Oct 9, 2019
@wing328 wing328 mentioned this pull request Oct 9, 2019
4 tasks
@wing328 wing328 changed the title [AVRO] Fix the package handling for the avro generator (#3987) [AVRO] Fix the package handling for the avro generator Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Bug Schema: Avro Generation of Avro schemas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants