Skip to content
This repository has been archived by the owner on Dec 9, 2022. It is now read-only.

headers in UploadFile should be a case insensitive multidict #5

Open
jtraub opened this issue Oct 19, 2022 · 12 comments
Open

headers in UploadFile should be a case insensitive multidict #5

jtraub opened this issue Oct 19, 2022 · 12 comments
Assignees
Labels
enhancement New feature or request help wanted Help from external contributors is welcome

Comments

@jtraub
Copy link
Contributor

jtraub commented Oct 19, 2022

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

As we discussed in litestar-org/litestar#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
Copy link

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
Copy link
Contributor Author

jtraub commented Oct 19, 2022

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
Copy link

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

@peterschutt peterschutt added enhancement New feature or request help wanted Help from external contributors is welcome labels Oct 19, 2022
@Goldziher
Copy link
Contributor

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

@jtraub jtraub self-assigned this Oct 20, 2022
@jtraub
Copy link
Contributor Author

jtraub commented Oct 20, 2022

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

@peterschutt
Copy link

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
Copy link
Contributor Author

jtraub commented Oct 20, 2022

Sure,

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

@Goldziher
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor Author

jtraub commented Oct 21, 2022

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
Copy link
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request help wanted Help from external contributors is welcome
Projects
None yet
Development

No branches or pull requests

4 participants