Roundup Tracker - Issues

Issue 2550870

classification
Title: Still using the rfc822 module, shoud be migrated to the email module
Type: Severity: normal
Components: Mail interface Versions: devel, 1.5
process
Status: closed Resolution:
Dependencies: Remove the rfc2822 module
View: 2550874
Superseder:
Assigned To: Nosy List: ber, jerrykan, r.david.murray
Priority: Keywords: Effort-Medium, patch

Created on 2015-02-25 21:07 by ber, last changed 2015-03-20 14:17 by jerrykan.

Files
File name Uploaded Description Edit Remove
test_mailgw-20150225.patch ber, 2015-02-25 22:53
issue2550870-1.patch jerrykan, 2015-02-27 12:58
Messages
msg5223 Author: [hidden] (ber) Date: 2015-02-25 21:07
Roundup as in 1.5.0 or devel rev:2c3cc4ccd024 still uses the rfc822 module.
https://docs.python.org/2.7/library/rfc822.html reads

"Deprecated since version 2.3: The email package should be used in preference to the 
rfc822 module. This module is present only to maintain backward compatibility, and has 
been removed in Python 3."

I'm not quite sure what deprecated means, but as we still have a roundup/rfc2822.py
which says "Some rfc822 functions taken from the new (python2.3) "email" module.",
there is some code cleanup and possibly there are defects fixed in the newer versions
of the email module that have not been backported to the rfc822 module.
msg5224 Author: [hidden] (r.david.murray) Date: 2015-02-25 21:20
Deprecated in this case means (as it says) gone in Python3, but it also
means unmaintained for longer than that.  And yes, there have been a
number of fixes to header handling.
msg5225 Author: [hidden] (ber) Date: 2015-02-25 22:53
Attaching test_mailgw-20150225.patch which is an INCOMPLETE start at 
replacing the rfc822 module with the email module in this single file. Needs more work.
msg5230 Author: [hidden] (jerrykan) Date: 2015-02-27 12:58
I've attached a patch that migrates from the rfc822 module to the email
module for files that import it:

 - roundup/cgi/client.py
 - roundup/mailgw.py
 - test/test_mailgw.py

The 'roundup/mailgw.py' stuff is pretty straight forward and the tests pass

The the section of code in 'roundup/cgi/client.py' that uses the rfc822
module are not covered by the tests, so it may be worth someone double
checking these changes.


The 'test/test_mailgw.py' is mostly converted but there are a few tests
that are failing so I have included skip() decorators for the time
being[1]. I've spent a bit of time looking into why the failures are
occurring and I think there are a few things contributing to the errors...

A lot of the message strings in the tests seem to have headers defined
twice (ie. 'FROM:' & 'From:', 'TO:' & 'To:'), and the behaviour for
parsing these seems to be different between the rfc822 and email
modules. I'm not sure if the duplicate headers were originally
intentional - to comply with how the rfc822 module works - or not.

The second thing is that the 'roundup/mailgw.py' seems to use the
mimetypes module to generate outgoing emails. This is another deprecated
module that should probably be replaced with the email module. It also
seems to generate messages with duplicate headers. Replacing the
mimetypes module would also seem to be a reasonable sized undertaking.

So that is about as far as I got with this. The next step would seem to
be to try and resolve the issues around the tests that are currently
being skipped, so if anyone can shed any light on the duplicate headers
things or have any other advice on how to proceed that would be helpful.

--

[1] note that the 'skip()' decorators are only available in python 2.7,
but that should be OK for the time being as they should be removed in
the final patch that fixes the reaming issues.
msg5231 Author: [hidden] (r.david.murray) Date: 2015-02-27 14:08
The mimetypes module is not deprecated.

There is a bug in either this tracker or the python.org metatracker
(bugs about the python bug tracker) about roundup emitting messages with
duplicate headers, and some mailers rejecting the emails because of this.
msg5232 Author: [hidden] (jerrykan) Date: 2015-02-27 14:13
On 28/02/15 01:08, R David Murray wrote:
> The mimetypes module is not deprecated.

Sorry, that should have been mimetools:

   https://docs.python.org/2/library/mimetools.html

mimetools.Message is a subclass of rfc822.Message, and 
roundup.mailgw.Message is a subclass of mimetools.Message which I think 
may be one source of the problems.
msg5233 Author: [hidden] (ber) Date: 2015-02-27 14:22
> I've attached a patch that migrates from the rfc822 module to the email
> module for files that import it:

Cool John!

> A lot of the message strings in the tests seem to have headers defined
> twice (ie. 'FROM:' & 'From:', 'TO:' & 'To:'), and the behaviour for
> parsing these seems to be different between the rfc822 and email
> modules. I'm not sure if the duplicate headers were originally
> intentional - to comply with how the rfc822 module works - or not.

see issue2550869 it seems this is unwanted behaviour, the rfc822 module
was used in a way that the duplicate headers were not discovered in
test_mailgw.py.

> The second thing is that the 'roundup/mailgw.py' seems to use the
> mimetypes module to generate outgoing emails. This is another deprecated
> module that should probably be replaced with the email module.

Yes, I also saw that mimetypes (und mimetools) should do away.
Thanks for the progress!
msg5239 Author: [hidden] (ber) Date: 2015-03-02 16:11
Thanks John for the get_body() function, here is my first commit.
This leads to 119 failures, but I believe this is the right next
step as the test itself was to lax, missing problems which the test
authors probably did not understand.

changeset:   4965:a850f8bae536
tag:         tip
user:        Bernhard Reiter <bernhard@intevation.de>
date:        Mon Mar 02 17:08:40 2015 +0100
files:       test/test_mailgw.py
description:
Moved test_mailgw to email module and make message comparison sharper to
flag differences in headers with the same name.
msg5241 Author: [hidden] (ber) Date: 2015-03-02 16:23
Okay, mimetools still to do, what about the rfc2822 module (that is
internal to roundup, could possibly go).
msg5243 Author: [hidden] (jerrykan) Date: 2015-03-02 23:58
I have a patch floating around to remove the rfc2822 module. I'll dig it
up a bit later and create a new issue for people to review it before
committing it.

On 03/03/15 03:23, Bernhard Reiter wrote:
> 
> Bernhard Reiter added the comment:
> 
> Okay, mimetools still to do, what about the rfc2822 module (that is
> internal to roundup, could possibly go).
> 
> ----------
> title: Still using the rfc822 module, shoud be migrated to the email module -> Still using the rfc822 module, should be migrated to the email module
> 
> ________________________________________________
> Roundup tracker <issues@roundup-tracker.org>
> <http://issues.roundup-tracker.org/issue2550870>
> ________________________________________________
>
msg5246 Author: [hidden] (jerrykan) Date: 2015-03-03 01:33
Patch for removing rfc2822.py can be found in issue2550874
msg5247 Author: [hidden] (ber) Date: 2015-03-03 08:46
On Tuesday 03 March 2015 at 00:58:52, John Kristensen wrote:
> I have a patch floating around to remove the rfc2822 module. I'll dig it
> up a bit later

Nice!
It would be okay to do it as part of this issue as well.  ;)
msg5261 Author: [hidden] (jerrykan) Date: 2015-03-06 07:55
On 03/03/15 03:11, Bernhard Reiter wrote:
> Thanks John for the get_body() function, here is my first commit.
> This leads to 119 failures, but I believe this is the right next
> step as the test itself was to lax, missing problems which the test
> authors probably did not understand.
> 
> changeset:   4965:a850f8bae536
> tag:         tip
> user:        Bernhard Reiter <bernhard@intevation.de>
> date:        Mon Mar 02 17:08:40 2015 +0100
> files:       test/test_mailgw.py
> description:
> Moved test_mailgw to email module and make message comparison sharper to
> flag differences in headers with the same name.

As you mentioned this commit is breaking the tests which makes it
difficult to determine if the test are failing because of these known
broken tests or some new change.

Are there any objections to reverting this commit until it is more
fleshed-out and not breaking the tests?
msg5262 Author: [hidden] (ber) Date: 2015-03-06 08:41
On Friday 06 March 2015 at 08:55:54, John Kristensen wrote:
> Are there any objections to reverting this commit until it is more
> fleshed-out and not breaking the tests?

Yes, the tests in mailgw show bad behaviour, so they should stay in.
Just because we swtich off the tests do not make the behaviour good,
so there is no additional knowledge that you gain from running (partly) broken 
tests.

(If it helps you, you could temporarily revert the patch in a  working copy, 
but please do not commit..)
msg5287 Author: [hidden] (ber) Date: 2015-03-17 08:23
As you may have seen, the tests pass now
and anypy has a small "patch" to the email.Header class
so it behaves consistently over python 2.5, 2.6 and 2.7.

Just wanted to add that if we move to the email module as part
of this issue, we also want to remove some part of anypy.email_.
The ones that are not needed anymore (FeedParser and decode_header).
msg5293 Author: [hidden] (jerrykan) Date: 2015-03-19 12:23
I have applied the patch to remove the rest of the rfc822 imports
(rev13f8f88ad984):


  http://hg.code.sf.net/p/roundup/code/rev/13f8f88ad984
msg5296 Author: [hidden] (ber) Date: 2015-03-19 14:54
John, thanks for adding the patch.
What else is missing to make this migration?
What about the anypy.email_ functions?
At least FeedParser and decode_header could possibly be removed.
msg5297 Author: [hidden] (jerrykan) Date: 2015-03-20 07:54
On 20/03/15 01:54, Bernhard Reiter wrote:
> 
> Bernhard Reiter added the comment:
> 
> John, thanks for adding the patch.
> What else is missing to make this migration?
> What about the anypy.email_ functions?
> At least FeedParser and decode_header could possibly be removed.
> 

As far as I can tell all the rfc822 (and rfc2822) imports have now been
removed, so we can probably close this issue.

I do have a patch almost reading to commit that will remove most of the
roundup.anypy.email_ imports, but there seems to be some sticking points
before we could look at removing roundup/anypy/email_.py, which might be
best discussed in a new issue.
msg5298 Author: [hidden] (ber) Date: 2015-03-20 08:08
On Friday 20 March 2015 at 08:54:24, John Kristensen wrote:
> As far as I can tell all the rfc822 (and rfc2822) imports have now been
> removed, so we can probably close this issue.

Cool! Please also write a nice CHANGES.txt entry. 

Can you state what the problem is with 
removing anypy.email_   decode_header and FeedParser?
The ws_continuation "patch" have to stay, so have the imports,
but the code of  decode_header and FeedParser should be able to go.
msg5299 Author: [hidden] (jerrykan) Date: 2015-03-20 13:36
On 20/03/15 19:08, Bernhard Reiter wrote:
>
> Bernhard Reiter added the comment:
>
> On Friday 20 March 2015 at 08:54:24, John Kristensen wrote:
>> As far as I can tell all the rfc822 (and rfc2822) imports have now been
>> removed, so we can probably close this issue.
>
> Cool! Please also write a nice CHANGES.txt entry.

Sure. I'll commit a note and close this issue.

>
> Can you state what the problem is with
> removing anypy.email_   decode_header and FeedParser?
> The ws_continuation "patch" have to stay, so have the imports,
> but the code of  decode_header and FeedParser should be able to go.

Removing the FeedParser stuff if fairly straight forward, and I'll 
commit a patch doing exactly that shortly.

Given that this issue is essentially resolved, I'll open a new issue 
outlining to problem with roundup/anypy/email_.py
msg5302 Author: [hidden] (jerrykan) Date: 2015-03-20 14:17
CHANGES.txt note committed:

  http://hg.code.sf.net/p/roundup/code/rev/20786c5152e4

Discussion of roundup.anypy.email_ can continue in issue2550879
History
Date User Action Args
2015-03-20 14:17:07jerrykansetstatus: new -> closed
messages: + msg5302
2015-03-20 13:36:49jerrykansetmessages: + msg5299
2015-03-20 08:08:45bersetmessages: + msg5298
2015-03-20 07:54:24jerrykansetmessages: + msg5297
2015-03-19 14:54:19bersetmessages: + msg5296
2015-03-19 12:23:00jerrykansetmessages: + msg5293
2015-03-17 08:23:16bersetmessages: + msg5287
2015-03-06 08:41:18bersetmessages: + msg5262
2015-03-06 07:55:54jerrykansetmessages: + msg5261
title: Still using the rfc822 module, should be migrated to the email module -> Still using the rfc822 module, shoud be migrated to the email module
2015-03-03 08:46:31bersetmessages: + msg5247
2015-03-03 01:33:15jerrykansetdependencies: + Remove the rfc2822 module
messages: + msg5246
2015-03-02 23:58:52jerrykansetmessages: + msg5243
2015-03-02 16:23:31bersetmessages: + msg5241
title: Still using the rfc822 module, shoud be migrated to the email module -> Still using the rfc822 module, should be migrated to the email module
2015-03-02 16:11:10bersetmessages: + msg5239
2015-02-27 14:22:58bersetmessages: + msg5233
2015-02-27 14:13:38jerrykansetmessages: + msg5232
2015-02-27 14:08:24r.david.murraysetmessages: + msg5231
2015-02-27 12:58:19jerrykansetfiles: + issue2550870-1.patch
messages: + msg5230
2015-02-26 01:46:04jerrykansetnosy: + jerrykan
2015-02-25 22:54:00bersetfiles: + test_mailgw-20150225.patch
keywords: + patch
messages: + msg5225
2015-02-25 21:20:06r.david.murraysetnosy: + r.david.murray
messages: + msg5224
2015-02-25 21:07:45bercreate