IMAP4Server bodystructure body parameter parenthesized list too deeply parenthesized

[biny]

The third item in bodystructure is body parameter parenthesized list, RFC 3501 says:

body parameter parenthesized list

A parenthesized list of attribute/value pairs [e.g., ("foo" "bar" "baz" "rag") where "bar" is the value of "foo", and "rag" is the value of "baz"] as defined in [MIME-IMB].

twisted.mail.test.test_imap.NewFetchTestCase.testFetchBodyStructure expects

[['name', 'thing'], ['key', 'value']]

and this is clearly wrong.

[biny]

In some other tests it expects an empty list and this caused problems for me. None has worked much better. In other words, if the constructed list is empty, return None.

[biny]

This patch

• Fixes the too deep parenthesis for the parameters
• Replaces empty parenthesis with None
• Adds test for multipart bodystructure
• Fixes the multipart itself to not be included as first subpart
• Fixes subparts to be generated extended too, if the multipart is extended
• Unifies content-disposition parsing with content-type parsing
• Fixes also ticket #3950, tested by the multipart test

[therve]

Thanks a lot for your contribution. Some comments:

• you're removing several 'lower' calls. There were not tested, but I suspect they may be useful. e'd rather be conservative here.
• you've changed from an empty list to None, saying that 'it worked much better". I'm not sure I get this argument. Can you explain why you changed this? I suspect this is a backward incompatible change.
• the test method should be named "test_fetchMultipartBodyStructure", and have a docstring.
• _splitParameters should probably have a docstring has well.
• I actually have some trouble understanding the original problem. You're referring to the RFC, and then show some Python structure: aren't those unrelated? It's the current format easier to parse?

Thanks!

[biny]

Replying to therve:

Thanks a lot for your contribution. Some comments:

* you're removing several 'lower' calls. There were not tested, but I suspect they may be useful. e'd rather be conservative here.

There is now a 'lower' call in _splitParameters.

* you've changed from an empty list to None, saying that 'it worked much better". I'm not sure I get this argument. Can you explain why you changed this? I suspect this is a backward incompatible change.

• It didn't work with my email client.
• Dovecot didn't output empty list there.
• Like this it is more consistent with other parts of the bodystructure.

* I actually have some trouble understanding the original problem. You're referring to the RFC, and then show some Python structure: aren't those unrelated? It's the current format easier to parse?

The returned list is converted to a part of a protocol message. This patch makes it match the standard so that it at least works with my email client.

I would argue that the code hadn't been really used, it has been so broken. With this patch I am able to use it.

[exarkun]

The issue in the ticket summary may now be resolved. I don't know if all of the issues the patch tried to address are resolved. It would be great if someone could:

1. Visit each unit test for BODYSTRUCTURE and check the depth of the parameter lists to make sure they are the way the ticket description says they should be.
2. Visit each code site that generates parameter lists (there are several, as several different headers have parameters) and verify that they are covered by the unit tests.
3. Consider the other issues the attached patch is supposed to address and file new tickets for them, if the code still handles them improperly or if there is missing test coverage for them.

[argonemyth]

Replying to exarkun:

1. Visit each unit test for BODYSTRUCTURE and check the depth of the parameter lists to make sure they are the way the ticket description says they should be.

I added two extended parameters to test_multiPartExtended to illustrate that "Fixes subparts to be generated extended too, if the multipart is extended". All other tests are good.

2. Visit each code site that generates parameter lists (there are several, as several different headers have parameters) and verify that they are covered by the unit tests.

I only found three places that generate parameter lists, let me know if I missed any.

1) _getContentType

I modified the docstring of this function. The original one is outdated. I also added a test called test_GetContentType in IMAP4HelperTestCase.

2) _disposition -> I will add more tests in ticket http://twistedmatrix.com/trac/ticket/5799

3) _produce in MessageProducer -> already covered by MessageProducerTestCase

3. Consider the other issues the attached patch is supposed to address and file new tickets for them, if the code still handles them improperly or if there is missing test coverage for them.

There is one issue still remains: "Fixes also ticket #3950, tested by the multipart test".

_getContentType still throws the same error, but I think it's better to fix it in ticket #3950 (http://twistedmatrix.com/trac/ticket/3950). My patch for the ticket is old, I will submit a new one. And I think the best way to fix the bug is to remove any trailing ';'.

While checking the code, I did spot some issues, but my observation might be wrong:

1. Content-Type Default

According to  http://tools.ietf.org/html/rfc2045#section-5, "messages without a MIME Content-Type header are taken by this protocol to be plain text in the US-ASCII character set." Should we set the default content-type instead of returning None? If yes, I will open a ticket for it.

2. Some Content-Type Header parameter values should also be quoted  http://www.ietf.org/rfc/rfc2045.txt section 5.1

Let's all for now, please review the patch, thanks!

[glyph]

Hey argonemyth, sorry for the latency on this.

The patch looks pretty good, but... bears almost no resemblance to this ticket. I had to read the whole ticket history to figure out why it was even doing what it was doing :). I think that if you've verified that the functions behave correctly, you should just mark this ticket as fixed and move the patch to a new ticket and branch.

1. It needs a NEWS file that says what it does; part and parcel with the describing the issue it actually fixes on a new ticket.
2. Since the content-language and content-location tags are somewhat distant from the test that requires them, there should be a comment explaining what they're there for.
3. As per the coding standard, test_GetContentType really ought to be test_getContentType.
4. A minor grammar nit: it should be "an" IMessagePart provider, since the 'I' at the front of the word isn't silent :).

Thanks again! Since you're a committer now, remember when you make the new ticket to commit to a branch and force builds so the next review can be quick and just OK the change without having to run the builds.

[exarkun]

This issue really does appear to have been resolved as part of #5763, which was partially a duplicate of this ticket (but also had a partially wider scope).