Skip to content

Conversation

@tomasr8
Copy link
Member

@tomasr8 tomasr8 commented Oct 20, 2024

Adds a more descriptive error message in case the parameter does not use parameter continuations correctly. For example:

Content-Type: application/x-foo;
    name*0="foo";
    name*="bar"

This is not valid because the second continuation is missing a decimal which defines the order. It should be:

Content-Type: application/x-foo;
    name*0="foo";
-    name*="bar"
+    name*1="bar"

Previously, Message.get_params() would raise a TypeError when trying to sort the list of continuations (which in this case would contain None). This PR simply adds a check before the call to sort and raises a friendlier error message.

@bitdancer
Copy link
Member

It should not be raising an error message at all. The contract of the email module is that it does not raise errors when parsing, instead it registers defects. Now, granted, the old API mostly does this during initial parsing, But see, for example, how get_payload handles base64 decoding defects. (Not that that is great code, but it is the available example for the old API).

As for the defects, here is some code that shows what the new API produces:

import email, email.policy

message = email.message_from_string("""\
Content-Type: application/x-foo;
    name*0="foo";
    name*="bar"

Foo
""", policy=email.policy.default)

print(message['content-type'])
print(message['content-type'].defects)
print(message['content-type'].content_type)
print(message['content-type'].params)
print(message['content-type'].params['name'])

And here is the output it produces on master:

application/x-foo; name="foo"
(InvalidHeaderDefect('Parameter marked as extended but appears to have a quoted string value that is non-encoded'), InvalidHeaderDefect('Missing required charset/lang delimiters'), InvalidHeaderDefect('duplicate parameter name; duplicate(s) ignored'))
application/x-foo
{'name': 'foo'}
foo

Note that these defects are registered during initial parsing. It is better to use the new API.

@bitdancer
Copy link
Member

Thanks for the patch, though. And in fact, since the old API is kind of broken in a lot of ways anyway, I wouldn't actually object to just using your fix if other people think it is OK.

@tomasr8
Copy link
Member Author

tomasr8 commented Oct 21, 2024

Thanks for the explanation! I wasn't aware this was part of the old API. I'll let others weigh in on this, but since it's part of the old API, perhaps it's better to leave it as it is then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants