litestar-org / starlite-multipart

Toolkit for working with multipart/formdata
https://starlite-api.github.io/starlite-multipart/
MIT License
8 stars 2 forks source link

headers in UploadFile should be a case insensitive multidict #5

Open jtraub opened 2 years ago

jtraub commented 2 years ago

It is super inconvenient that UploadFile uses ordinary dict for headers.

As we discussed in https://github.com/starlite-api/starlite/pull/574 header names are case insensitive so using dict in UploadFile makes user code super complicated because now the user needs to handle different spellings for headers or use lower case everywhere. Also, headers in requests in Starlite are returned in a CI multidict so consistent behavior across the framework is another reason for this change.

Starlite uses starlette.datastructures.Headers for request headers as can be seen here. Internally it just an implementation of case insensitive multidict.

Intuitive solution would be reusing starlette.datastructures.Headers but then it adds Starlette as dependency for starlite-multipart. Doing so while considering departing from Starlette foundation is strange though.

Should we implement our own CI multidict? If yes how should we share the code between this project and the framework?

peterschutt commented 2 years ago

We could create a utility library to share things like this?

Or a 3rd party implementation perhaps? E.g., https://github.com/aio-libs/multidict. Disclaimer: I haven't had a close look at this, just googled.

jtraub commented 2 years ago

In a similar case I've picked up aiolibs/multidict and it worked just fine.

It is an old battle-tested solution. I think aiohttp uses it internally.

peterschutt commented 2 years ago

Seems a good option to me then if it is lightweight.

Goldziher commented 2 years ago

Yes, using multidict is the solution. I looked into it a while back for starlite itself.

jtraub commented 2 years ago

I will make a basic Python implementation of Multidict in the library then.

peterschutt commented 2 years ago

I read @Goldziher's comment as in support of using aiolibs/multidict lib @jtraub - could be wrong though, but let's just clear that up before you get too far into it.

jtraub commented 2 years ago

Sure,

just want to clarify that aiolibs/multidict requires C compiler with their default installation as they use C extension for speed.

Goldziher commented 2 years ago

Sure,

just want to clarify that aiolibs/multidictrequire C compiler with their default installation as they use C extension for speed.

We should assess the impact of this change. Starlette is pure python - we might simply aim for performance parity fir now and consider our options for compiling the code in the future using mypc

peterschutt commented 2 years ago

just want to clarify that aiolibs/multidict requires C compiler with their default installation as they use C extension for speed.

This is no different to orjson though, and there are published wheels for linux, windows and mac - so this would be a non-issue for a big chunk of users and anyone building on Alpine Linux would be used to this kind of thing.

On ubuntu, installing multidict adds less than 500K to the venv size:

image

I'm still in favor of using it.

Goldziher commented 2 years ago

just want to clarify that aiolibs/multidict requires C compiler with their default installation as they use C extension for speed.

This is no different to orjson though, and there are published wheels for linux, windows and mac - so this would be a non-issue for a big chunk of users and anyone building on Alpine Linux would be used to this kind of thing.

On ubuntu, installing multidict adds less than 500K to the venv size:

image

I'm still in favor of using it.

Let's do this. @jtraub can you make an issue?

jtraub commented 2 years ago

Let's do this. @jtraub can you make an issue?

@Goldziher Sorry I don't understand. You meant create issue in the main repo for replacing starlette.datastructures.Headers with multidict?

Goldziher commented 2 years ago

Let's do this. @jtraub can you make an issue?

@Goldziher Sorry I don't understand. You meant create issue in the main repo for replacing starlette.datastructures.Headers with multidict?

yup