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

Connexion 2.1.0 doesn't parse the request body using the right content types when more types are present #805

Closed
magnuspaaske opened this issue Nov 30, 2018 · 9 comments
Assignees
Labels

Comments

@magnuspaaske
Copy link

Description

I updated Connexion to version 2.0.2 but none of my routes got any parameters sent. I got the expected behaviour when I accessed the routes with missing params (ie, error), but the params didn't get sent to the controller functions (no params were sent).

Expected behaviour

I expected that the the parameters would be sent to the controller function like before. Instead no arguments are sent when hitting the endpoint.

Actual behaviour

No parameters are sent to the controller function. Not sure if this is new expected behaviour or there's a bug somewhere along the line. Alternatively I would assume there's a global object that holds the parameters?

Steps to reproduce

Just a normal setup – a swagger file to describe the API and a controller script with functions to handle each route. The function for the route expects either named or positional arguments. This is what works in 1.5.3 but not in 2.0.2

Additional info:

Python version 3.7.0


It's very possible there's another way to get the parameters and I would love to hear about that. Right now I've had to downgrade Flask. If I just missed something about the new Connexion I would be happy to hear about it :)

@dtkav
Copy link
Collaborator

dtkav commented Nov 30, 2018

please attach a spec so I can reproduce the problem, otherwise it is impossible to help you.

@dtkav
Copy link
Collaborator

dtkav commented Dec 10, 2018

potentially related to #812

@magnuspaaske
Copy link
Author

magnuspaaske commented Dec 15, 2018

Okay, I had more of a look at it. Here's the relevant code:

Init connexion

connex = connexion.App(__name__, specification_dir='./')
connex.add_api(
    'swagger.yml',
    strict_validation=True,
    validate_responses=True,
)

Swagger file:

openapi:         '3.0.2'
info:
  description:   This is the swagger file
  version:       0.0.1
servers:
  - url: /api/v0

paths:
  /login/email:
    post:
      operationId: project.auth.email_controller.login
      tags:
        - email
        - login
        - user
        - auth
      summary:     Email log in
      description: Allows users to login using email/password
      requestBody:
        description: Login details for user
        content:
          '*/*':
            schema:
              type: object
              required:
                - email
                - password
              properties:
                email:
                  type: string
                  description: Email for user
                password:
                  type: string
                  description: Password for user
      responses:
        202:
          description: Successfully logged in
          content:
            application/json:
              schema:
                properties:
                  access_token:
                    type: string
                    description: jwt_token,
                  refresh_token:
                    type: string
                    description: refresh_token

It seems no parameters get sent to the login function and it doesn't actually provide an error if parameters are missing.

@magnuspaaske
Copy link
Author

Okay, tried to change things. It seems the */* in the content section is what's giving problems. Using application/x-www-form-urlencoded and sending the request with that in the header works. Similar with application/json.

@dtkav
Copy link
Collaborator

dtkav commented Dec 15, 2018

Thanks for reporting back! That's really useful information for a branch I'm working on for adding validation/de-serialization per content-type. (#760)
I hadn't considered the case of */*. Do you mind updating the title to '*/* requestBody content type not handled' or something similar?

@magnuspaaske
Copy link
Author

A couple more things for this:

requestBody:
  description: Login details for user
  content:
  application/json:
    schema:
      $ref: '#/components/schemas/Login'
   application/x-www-form-urlencoded:
      schema:
        $ref: '#/components/schemas/Login'

Adding multiple content types gives all kinds of problems. When I tried this it'll say this operation accepts multiple content types, using application/json in the log and produce an error for JSON and not pass params with form encoding (where it'll also say the same).

For now I can solve this by just using JSON everywhere, but it seems it doesn't see the Content-Type header.

So ...
Expected behavior:
Connexion should use the Content-Type header to decide how to decode the body.

Actual behavior:
Connexion seems to always choose to decode the body with JSON and when multiple content types are present in Swagger it doesn't parse it right.

@magnuspaaske magnuspaaske changed the title No parameters sent to controller function after upgrading 1.5.3 –> 2.0.2 Connexion 2.0.2 doesn't parse the request body using the right content types when more types are present Dec 15, 2018
@magnuspaaske magnuspaaske changed the title Connexion 2.0.2 doesn't parse the request body using the right content types when more types are present Connexion 2.1.0 doesn't parse the request body using the right content types when more types are present Dec 16, 2018
@dtkav
Copy link
Collaborator

dtkav commented Dec 16, 2018

Supporting multiple content types is a known issue, and is not new to connexion 2 ( #655 ).
AFAICT the original developers were targeting json-only APIs.
Please take a look at #760 if you have a chance - I need feedback on that PR as it should fix a lot of issues like this, but is potentially a breaking change.

@magnuspaaske
Copy link
Author

Okay – I can see how your PR could break existing apps if that PR is implemented, so perhaps it makes sense to make a new flag that the content type header should be used? Otherwise it looks good.

As far as I can see the main thing is that most clients may treat json- or form-encoding as substituting each other. For instance when testing an API it's typically easier to use form encoding in Postman than JSON-encoding and ajax clients don't necessarily always specify if they send the request JSON- or form-encoded. This is extra true when making an API for public consumption where the different encodings are often all accepted

@RobbeSneyders
Copy link
Member

Fixed in #1588

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

No branches or pull requests

3 participants