connexion icon indicating copy to clipboard operation
connexion copied to clipboard

Documentation should reflect that file inputs must always be Optional [Or, Connexion should handle it differently]

Open danielbprice opened this issue 2 years ago • 2 comments

Description

[This is a narrowed version of #1811, but reflects my experiences after the fixes mentioned therein were made. I still think there are problems here and that is what I am trying to capture in this bug report].

Hi again. I agree that the fixes in 3.0.2 have helped, but I think the documentation still needs clarification or the behavior needs an update. It seems that you must always declare parameters that could be of type "file" as =None, because the user could always pass a non-file type.

I retested this with 3.0.2 and the behavior remains the same. It seems that you must declare anything which handles a file as:

async def foobar(image: Optional[UploadFile] = None)

Here is a demonstration using the spec from the docs.

import connexion

def foo_get(file):  # <--- as per documentation
    assert isinstance(file, starlette.UploadFile)

app = connexion.AsyncApp(__name__, specification_dir="spec")
app.add_api("foo.yaml")

if __name__ == "__main__":
    app.run(f"{Path(__file__).stem}:app", port=8080)
openapi: "3.0.1"

info:
  title: Form Data
  version: "1.0"
servers:
  - url: /

paths:
  /path:
    post:
      operationId: foo.foo_get
      requestBody:
        content:
          multipart/form-data:
            schema:
              type: object
              properties:
                file:
                  type: string
                  format: binary
      responses:
        200:
          description: OK
$ curl -F file=sometext http://localhost:8080/path

Expected behaviour

Something that isn't HTTP 500. It seems like either:

  • Connexion should infer from the OpenAPI and the function parameters that the developer is expecting an UploadFile object, and therefore this request should return HTTP 400 and fail validation
  • OR Connexion should be agnostic about the type, and pass what the user gave, either a string (or bytes or whatever) OR the UploadFile, and let the developer sort it out.
  • OR punt and tell everyone that all file arguments should be Optional[UploadFile], and tell me that I need to do my own not-None checking.

Actual behaviour

Traceback:

INFO:     Application startup complete.
TypeError("foo_get() missing 1 required positional argument: 'file'")
Traceback (most recent call last):
  File "/home/dp/venv/lib/python3.10/site-packages/starlette/_exception_handler.py", line 44, in wrapped_app
    await app(scope, receive, sender)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/middleware/swagger_ui.py", line 220, in __call__
    await self.router(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/starlette/routing.py", line 746, in __call__
    await route.handle(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/starlette/routing.py", line 459, in handle
    await self.app(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/starlette/routing.py", line 775, in __call__
    await self.default(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/middleware/swagger_ui.py", line 233, in default_fn
    await self.app(original_scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/middleware/routing.py", line 153, in __call__
    await self.router(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/starlette/routing.py", line 746, in __call__
    await route.handle(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/starlette/routing.py", line 459, in handle
    await self.app(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/starlette/routing.py", line 746, in __call__
    await route.handle(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/starlette/routing.py", line 288, in handle
    await self.app(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/middleware/routing.py", line 47, in __call__
    await self.next_app(original_scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/middleware/abstract.py", line 268, in __call__
    return await operation(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/middleware/security.py", line 95, in __call__
    await self.next_app(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/middleware/abstract.py", line 268, in __call__
    return await operation(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/middleware/request_validation.py", line 142, in __call__
    await self.next_app(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/middleware/abstract.py", line 268, in __call__
    return await operation(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/middleware/lifespan.py", line 26, in __call__
    await self.next_app(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/middleware/abstract.py", line 268, in __call__
    return await operation(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/middleware/context.py", line 25, in __call__
    await self.next_app(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/middleware/abstract.py", line 268, in __call__
    return await operation(scope, receive, send)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/apps/asynchronous.py", line 69, in __call__
    response = await self.fn()
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/decorators/main.py", line 188, in wrapper
    return await decorated_function(request)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/decorators/response.py", line 191, in wrapper
    handler_response = await function(*args, **kwargs)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/decorators/parameter.py", line 115, in wrapper
    return await function(**kwargs)
  File "/home/dp/venv/lib/python3.10/site-packages/connexion/decorators/main.py", line 175, in wrapper
    return await run_in_threadpool(function, *args, **kwargs)
  File "/home/dp/venv/lib/python3.10/site-packages/starlette/concurrency.py", line 35, in run_in_threadpool
    return await anyio.to_thread.run_sync(func, *args)
  File "/home/dp/venv/lib/python3.10/site-packages/anyio/to_thread.py", line 33, in run_sync
    return await get_async_backend().run_sync_in_worker_thread(
  File "/home/dp/venv/lib/python3.10/site-packages/anyio/_backends/_asyncio.py", line 2106, in run_sync_in_worker_thread
    return await future
  File "/home/dp/venv/lib/python3.10/site-packages/anyio/_backends/_asyncio.py", line 833, in run
    result = context.run(func, *args)
TypeError: foo_get() missing 1 required positional argument: 'file'

Steps to reproduce

Additional info:

Output of the commands:

  • python --version 3.10.13
  • pip show connexion | grep "^Version\:". Version: 3.0.2

danielbprice avatar Nov 20 '23 18:11 danielbprice

I should say that in my example above, I left out:

              required:
                - file

Which was a mistake. However, the behavior remains the same with and without it in my testing. This also, I think, helps to really show off why this is confusing. ("I marked it required but I'm getting this weird traceback?!!!")

danielbprice avatar Nov 20 '23 19:11 danielbprice

Thanks @danielbprice.

I agree that the behavior should be updated to match the first expected behavior in your list:

Connexion should infer from the OpenAPI and the function parameters that the developer is expecting an UploadFile object, and therefore this request should return HTTP 400 and fail validation

In the validator, we should validate that we receive a file object when the schema defines it. Shouldn't be too hard if we develop against your example as a test case.

RobbeSneyders avatar Nov 20 '23 21:11 RobbeSneyders