Skip to content

fix: Support multipart requests with type: array #452

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 1 commit into from
Jul 10, 2021

Conversation

csymeonides-mf
Copy link
Contributor

@csymeonides-mf csymeonides-mf commented Jul 8, 2021

Fixes #451 though perhaps a better "fix" would be to disallow multipart request bodies with type: array?

Copy link
Collaborator

@dbanty dbanty left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for fixing this so quick! I'll wait to hear back from the reporters before merging, in case they had something else in mind.

@slamora
Copy link

slamora commented Jul 9, 2021

I'm still having the issue if I try to generate a client from this YAML (I've writted it manually and I'm not an OpenAPI expert so maybe there are conceptual mistakes):
NOTE: there is no type: array on multipart request body.

openapi: 3.0.2
info:
  title: ''
  version: ''
paths:
  /api/media/upload-file/:
    post:
      operationId: uploadFile
      requestBody:
        content:
          multipart/form-data:
            schema:
              properties:
                file:
                  type: string
                  format: binary
      responses:
        '200':
          content:
            application/json:
              schema:
                properties:
                  filename:
                    type: string
                    maxLength: 128
                required:
                - filename
          description: ''

@csymeonides-mf
Copy link
Contributor Author

@slamora Ok, this is a slightly different issue, and a more complicated one!

TLDR; if you add type: object under schema, all will work fine.

Your schema is valid, because type is not a mandatory keyword in JSONschema. This seems a bit strange to me, and I haven't actually found an official source for it, but there's a long discussion here with pros and cons.

As it currently stands, your schema says: "this could be anything, but if it's an object, it must have these properties".

Unfortunately we don't have code to handle this case, so we just interpret this schema as simply "this could be anything" and ignore the properties (see relevant code below from properties/__init__.py). As you can imagine, modifying this behaviour would not be trivial.

    elif data.type == "object" or data.allOf:
        return build_model_property(
            data=data, name=name, schemas=schemas, required=required, parent_name=parent_name, config=config
        )
    elif not data.type:
        return (
            AnyProperty(
                name=name,
                required=required,
                nullable=False,
                default=None,
                python_name=utils.PythonIdentifier(value=name, prefix=config.field_prefix),
            ),
            schemas,
        )

@slamora
Copy link

slamora commented Jul 9, 2021

TLDR; if you add type: object under schema, all will work fine.

Thanks @csymeonides-mf for your tip!

I guess that it's a very uncommon situation so the effort to support it doesn't compensate. As reminder, I've created this YAML by hand to be able to reproduce the issue so it's and edge case that AFAIK doesn't exist on a real environment.

IMHO, you can follow with the PR merge workflow.

@dbanty dbanty merged commit 2cc22a7 into openapi-generators:main Jul 10, 2021
@dbanty
Copy link
Collaborator

dbanty commented Jul 10, 2021

Excellent, sounds like the major issue is solved here then! Thanks again for fixing this so quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception when generating list properties in multipart forms
3 participants