Add MIME content type info to File by jhnstrk · Pull Request #143 · Kludex/python-multipart

jhnstrk · 2024-04-30T20:57:08Z

Adds MIME type information as File.content_type as requested in issue #58.

Tests have been updated and a case-sensitive header issue was fixed too.

The part headers are also exposed as a dict, with keys as strings, lower-cased as File.headers.

multipart/multipart.py

Makes all headers lower case, fixing case sensitivity issues. Exposes jheaders property in Files and Fields.

Kludex · 2024-12-11T16:38:45Z

python_multipart/multipart.py


    class FieldProtocol(_FormProtocol, Protocol):
-        def __init__(self, name: bytes | None) -> None: ...
+        def __init__(self, name: bytes, content_type: str | None = None) -> None: ...


Is it needed on Field?

Yes, but not frequently used. Examples:

Passing json and specifying "application/json"

Embedding "multipart/mixed" inside ( https://www.ietf.org/rfc/rfc2388.txt ).

Specifying a charset for a field

Specifying a charset for a field

This is enough to convince me. Thanks.

python_multipart/multipart.py

Kludex · 2024-12-16T07:04:13Z

python_multipart/multipart.py


                # Get the field and filename.
-                field_name = options.get(b"name")
+                field_name = options.get(b"name", b"")


Is this needed? Isn't the field_name supposed to always be present?

I added this for typing. Without it, we'll need to raise or assert it's not None. Which makes sense actually.
Will probably need a test to cover it. too.

Removed the default value and added a check for None.

Kludex · 2024-12-16T07:05:11Z

python_multipart/multipart.py


            def on_header_end() -> None:
-                headers[b"".join(header_name)] = b"".join(header_value)
+                headers[b"".join(header_name).decode().lower()] = b"".join(header_value)


Do we need to decode?

I think the headers dict is easier to use if the keys are strings rather than bytes, so I used decode to do the transformation. The string is guaranteed to be valid since it can only contain a subset of ASCII characters (it's filtered as it's built). Can be bytes if you prefer, in which case decode isn't needed.

Does it need to be in this PR?

Indeed it's not needed here. I'll create another PR for that. The change made the internal headers consistent with the interface of create_form_parser , but isn't needed to achieve the aims of this PR.

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Kludex · 2024-12-25T09:22:58Z

python_multipart/multipart.py

    """

-    def __init__(self, name: bytes | None) -> None:
+    def __init__(self, name: bytes, content_type: str | None = None) -> None:


Why did the name type changed here? Now we have a mismatch between this and the File.field_name. 🤔

Can we revert this bit?

This can be reverted indeed. Done.

Kludex · 2024-12-25T09:23:14Z

python_multipart/multipart.py

            def on_start() -> None:
                nonlocal file
-                file = FileClass(file_name, None, config=cast("FileConfig", self.config))
+                file = FileClass(file_name, b"", config=cast("FileConfig", self.config))


Suggested change

file = FileClass(file_name, b"", config=cast("FileConfig", self.config))

file = FileClass(file_name, config=cast("FileConfig", self.config))

This suggestion can't be done without a significant change in the function and Protocol signature because it would place an optional argument before a positional argument. We can keep the None as it was though.

Kludex · 2024-12-25T09:24:54Z

python_multipart/multipart.py


            def on_header_end() -> None:
-                headers[b"".join(header_name)] = b"".join(header_value)
+                headers[b"".join(header_name).decode().lower()] = b"".join(header_value)


Does it need to be in this PR?

jhnstrk

Updated the code in response to suggestions. Additional comments inline.

jhnstrk · 2025-01-21T20:07:36Z

python_multipart/multipart.py

    """

-    def __init__(self, name: bytes | None) -> None:
+    def __init__(self, name: bytes, content_type: str | None = None) -> None:


This can be reverted indeed. Done.

jhnstrk · 2025-01-21T20:09:21Z

python_multipart/multipart.py

            def on_start() -> None:
                nonlocal file
-                file = FileClass(file_name, None, config=cast("FileConfig", self.config))
+                file = FileClass(file_name, b"", config=cast("FileConfig", self.config))


This suggestion can't be done without a significant change in the function and Protocol signature because it would place an optional argument before a positional argument. We can keep the None as it was though.

jhnstrk · 2025-01-21T20:18:20Z

python_multipart/multipart.py


            def on_header_end() -> None:
-                headers[b"".join(header_name)] = b"".join(header_value)
+                headers[b"".join(header_name).decode().lower()] = b"".join(header_value)


Indeed it's not needed here. I'll create another PR for that. The change made the internal headers consistent with the interface of create_form_parser , but isn't needed to achieve the aims of this PR.

Kludex requested changes Dec 6, 2024

View reviewed changes

jhnstrk added 4 commits December 8, 2024 11:16

Adding headers

cb75e06

Add trailer test

d46a9c6

Add content_type property to File

12b3f26

Makes all headers lower case, fixing case sensitivity issues. Exposes jheaders property in Files and Fields.

Use content_type in Fields not header

f1e28d6

jhnstrk force-pushed the file_content_type branch from 4304002 to f1e28d6 Compare December 8, 2024 12:43

Kludex reviewed Dec 11, 2024

View reviewed changes

jhnstrk force-pushed the file_content_type branch from 0fc5e2d to f1e28d6 Compare December 11, 2024 20:30

Kludex reviewed Dec 16, 2024

View reviewed changes

jhnstrk and others added 4 commits December 16, 2024 19:13

Update python_multipart/multipart.py

37f4c53

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Update python_multipart/multipart.py

afbf23f

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Update python_multipart/multipart.py

b1c2d3b

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Check for field name and raise if not found

954df08

Kludex requested changes Dec 25, 2024

View reviewed changes

jhnstrk added 3 commits January 21, 2025 20:54

Formatting fix

0957d48

Ruff formatting

dc2ae31

Revert headers to Dict[bytes,bytes]

eaa3b3c

jhnstrk commented Jan 21, 2025

View reviewed changes

	file = FileClass(file_name, b"", config=cast("FileConfig", self.config))
	file = FileClass(file_name, config=cast("FileConfig", self.config))

Uh oh!

Conversation

jhnstrk commented Apr 30, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhnstrk Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhnstrk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jhnstrk Dec 11, 2024 •

edited

Loading