2011-04-04 Hooking Up Email6 Policy: First Steps

(Note: given how long this post has turned out to be, I think I may need to increase the frequency of these posts.)

Defects

For whatever reason, I decided to start with the defect support. Many moons ago the email package had two modes: strict and lenient. In lenient mode it operated more or less as it does now; in strict mode it would raise errors when the message was out of RFC compliance. Although you usually want to parse a message successfully whether or not it is compliant, there are times when you want to know right away that the message is bogus. So part of the email6 design re-introduces a strict mode.

The way we do this is fairly slick. The parser has to detect non-compliance whether it yells about it right away or does its best to recover. (This isn’t entirely true, but I won’t be dealing with the edge cases where it isn’t true until I start enhancing the parser). What the parser currently does when it detects non-compliance is to register a MessageDefect by creating an instance of one of the defect classes and appending it to the ‘defects’ property of the Message instance it is currently building.

To implement a strict mode, all we need to do is to raise the defect instead of registering it. In some ways, we’ve added this feature (back) simply because it is so easy to implement. But the Policy defect handling machinery is flexible enough that it can be re-purposed for other jobs. I’ll come back to that thought.

The first thing I thought about when starting the hookup was: how well tested is the relevant code? So I set up the coverage tester according to the instructions in the dev guide and ran it on the email package. Overall we’re doing pretty good: 92% coverage. I’m expecting to have that up to 100% before I’m done.

The particular lines of code of interest in this case (that generate defects) showed as covered. Looking through the test suite for the tests specifically about defects, there are several that make sure the various defects are registered in the appropriate circumstances.

Good. So all I needed to do was add additional tests for the new functionality, and then make them pass.

What I did was to generalize the defect tests, creating a base test class that ran the tests that generated the defects, and two subclasses, one that used the hooked test method to see if the defect got appended to the message object, and one that tested to see if a custom defect handler defined on the policy got called. It didn’t really make sense to use the same framework for testing the strict case, though, since the existing tests sometimes check for more than one defect, while strict will raise on the first defect found. So I also added a specific test class to check for the strict cases, modeled on the existing tests.

The custom defect handler in the new tests can serve as a partial model of what it is possible to do. In the test case all I cared about was making sure the subclass method got called with the relevant defect, so I just appended it to a list stored on the policy instance itself. But the same basic mechanism could be used to, for example, accumulate all the defects encountered during parsing of a multipart message into a single master list, without having to walk the object tree afterward to collect it.

Improving coverage by deleting code

In the process of adding the policy keyword to the Parser class, I noticed that there was some crufty code in Parser designed for backward compatibility with the old strict argument. At first I was just going to refactor it, but when I checked the coverage map I found that those lines weren’t even tested. So rather than write tests before making changes, I decided to just rip out the backward compatibility code. After all, the option was deprecated in Python 2.4, so I think it should be safe to remove it now.

It’s a nice feeling to remove unneeded code.

Conforming to PEP 8

There was something that I noticed while writing the new tests and code, though. My fingers kept typing policy.default and policy.strict instead of policy.Default and policy.Strict. I think my original thought was that Default and Strict looked better because we also have SMTP and HTTP. But in fact PEP 8 calls for instance names to be all lower case, and it seems that my fingers know that better than my concious mind does.

So I changed the spelling. I also found that I prefer importing the policy module and referring to the policy using the policy. prefix. The names are too generic for clarity otherwise. But that, clearly, is a matter of taste.

The process of making the tests pass was straightforward: change the test case in the base class to use the generic setup and test calls, watch them fail, go to the failing bit of code, and add the calls to the policy hooks. Iterate a few dozen times, and defect management is all hooked up.

Getting the strict tests to pass after that was a piece of cake. The only change needed was to make MessageDefect a subclass of Exception so that the defects could be raised as errors. Since they already otherwise look an awful lot like Exceptions, I don’t think there will be any backward compatibility issues from this change.

From here I moved on to the linesep parameter of policy, and did the same exercise again, except this time it was generator that got the policy keyword additions.

Then I went back and updated the documentation for to reflect all these changes.

ASCII-only Binary Email Transformation

Next I decided to tackle use_raw_data_if_possible.

Now, that is a really awkwardly named attribute, and it is only named that because it expressed the problem I was trying to solve when I adopted it (keeping signed messages from having their headers messed up by Generator). Giving it a little thought from a broader perspective, I decided that the essential concept here was whether or not the email package was to be allowed to rewrap headers that already had wrapping information in them. This is as opposed to wrapping headers provided by the application, where there is no pre-existing wrapping information.

So first I renamed the parameter rewrap, adjusted the policy docs and tests, and then I started implementing it. I created a couple tests, and started adjusting the code....and realized that there’s a problem here.

At this stage of email6 development, the generator does not have enough information to know whether or not a given header is the way it is because it came from a pre-existing message, or if it was provided that way by an application program, without wrapping. We can assume that if there is pre-existing wrapping then we should preserve it (which is what the current email package does), but if there is no pre-existing wrapping then we can’t know. If the line fits within the maxlinelen then there’s no problem, but if it doesn’t...was it because the source email had a long line, or because the header value was set by the application program expecting the email package to do the wrapping?

The email6 design calls for making each header an object, and when that is true we’ll be able to record the source of the header and thus know. At that point, a policy parameter controlling re-wrapping will make sense. But with the model as it exists at this stage, we just don’t have enough information.

So I ripped the rewrap parameter out of policy for now, and will put it back when I can actually implement it.

The next parameter was more fun, because it proved to be a straightforward extension of work I’d already done for email5.1. The must_be_7bit parameter is intended to allow the transformation of an email from a format where characters with the 8th bit set are allowed, to a form where only ASCII is allowed. There are still some mail gateways out there that don’t handle 8bit, and this is the kind of transformation that an MTA is expected to do when talking to such a gateway.

Fortunately I already had to solve this problem in order to make email5 handle such bytes. That’s because unicode can’t handle bytes with the 8th bit set. To turn such bytes into unicode you have to decode them into some unicode character by using knowledge of what code set those bytes are encoded in. But in email, sometimes we are dealing with real binary data, and there is no associated encoding.

So to represent such a message as a unicode string in Python3, we have to use the normal email mechanisms to encode the binary data as ASCII characters. Therefore I already had all the code pieces available to do the transformation from 8bit to ASCII, all I needed to do was hook them up inside the BytesGenerator class and trigger them when must_be_7bit was true.

So on to another round of test writing and making tests pass. Getting this to work involved some interesting refactorings in the BytesGenerator code. The end result makes the code more consistent and simpler, I think.

assertBytesEqual

At this point I took a little detour into the test suite infrastructure. During my development the tests would of course fail. While testing BytesGenerator, the test failures were mismatched byte strings. Now, when unittest string comparison tests fail, you get a nice line-by-line diff. But when byte comparisons fail, you just get the two bytestrings. I’ve solved this problem before, and even tried to submit it as a feature to unittest, but we ultimately decided that it is a specialized case and is simple enough to solve that it should be done only by the particular test suites that need it.

I decided it was time to bake it in to the email test suite test infrastructure. Currently the test infrastructure (the TestEmailBase class) has a specialized ndiffAssertEqual that predates the existence of the nice diff support in unittest. For the moment I haven’t tackled converting that to the new style, but I’ll do that eventually. What I did was to add a new assertBytesEqual method, and then provide an __init__ method for TestEmailBase that hooks it in to assertEqual using the new unittest equality-testing infrastructure. So now a test author doesn’t have to think about the fact that the results are bytes, all that matters is that they are supposed to be equal. This also means I can use assertEqual in tests where I’m using a common base class to run the same tests on on both string output and bytes output.

As the development process moves along I plan to refactor the existing 4400 lines of tests from a single test file into multiple, more logically organized test files. So as long as I was mucking about with TestEmailBase, I decided to refactor it over in to the __init__.py file for the test_email test package.

In the process of doing this move, I refactored the way the test suite uses its custom openfile method and the test.support.findfile() function. Which is to say, I removed all calls to the latter. Only some of the tests were using findfile, yet there have been no reports of the test suite being unable to find the test files, so I concluded that openfile was succeeding in locating the data files all by itself, without need for findfile. We’ll see if this remains true when I repackage email6 for pre-release on PyPI; but if it doesn’t the place I’ll put findfile support back in is right in the openfile method.

This set of changes I committed into the cpython repository rather than the email6 feature branch.

Public Review

I feel like there are enough changes now to make a public review worthwhile, so I created issue 11731 (closed) and posted the link to the feature branch there. Unfortunately there seems to be some problem with the automated generation of the patch. Whether for that reason or because everybody just figures I’m doing the right there, there haven’t been any review comments so far.

I’ve just manually uploaded the current diff with default, and I will post the tracker link to the email-sig mailing list, and we’ll see if any comments are forthcoming.