[Kde-pim] Review Request: Fix/improve handling of Headers and Contents in KMime (long)
Constantin Berzan
exit3219 at gmail.com
Wed Jul 29 10:45:25 BST 2009
> On 2009-07-29 09:27:02, Andras Mantia wrote:
> > /trunk/KDE/kdepimlibs/kmime/kmime_content.cpp, lines 448-495
> > <http://reviewboard.kde.org/r/1151/diff/1/?file=9206#file9206line448>
> >
> > What happened with this code? Are you 100% sure it is not needed?
Re-encoding uuencoded and yenc-encoded parts into valid MIME parts is now done in parseUuencode() and parseYenc().
> On 2009-07-29 09:27:02, Andras Mantia wrote:
> > /trunk/KDE/kdepimlibs/kmime/kmime_content.cpp, lines 386-403
> > <http://reviewboard.kde.org/r/1151/diff/1/?file=9206#file9206line386>
> >
> > That's the part I can really comment: this changes the behavior back, so calling assemble() breaks the message integrity. In Akonadi assemble() is used to retrieve the message from the payload, the reason was I think that theoretically you could store a message constructed from parts (and not with setContent) in the database. If you don't call assemble when retrieving, those messages are not "complete". Calling always assemble() is a way to go, but that breaks the message integrity without the extra checks I added.
> > An idea would be to have a dirty flag (set if a header was modified/added/removed programatically) and than we can check in payloadData() if there is a need to call assemble() or not.
> > As it is in your patch, I think it is not good.
What is the reason for calling assemble() upon retrieving the message? Shouldn't the message be properly assembled before it is given to Akonadi for storage? As I understand, the serializer uses the string-representation of the message anyway, so it seems like a reasonable thing to require that messages be assembled before storage.
I do not think the job of keeping message integrity belongs in a function like assemble() -- it sounds like it is *supposed* to create / change the content. I can understand wanting to keep message integrity after it was assembled (e.g. for signed parts), and that's what setFrozen() is for.
- Constantin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1151/#review1831
-----------------------------------------------------------
On 2009-07-28 13:07:54, Constantin Berzan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1151/
> -----------------------------------------------------------
>
> (Updated 2009-07-28 13:07:54)
>
>
> Review request for KDE PIM, Thomas McGuire, Andras Mantia, and Tom Albers.
>
>
> Summary
> -------
>
> This is a rather huge patch attempting to fix a number of problems in KMime. (You will also find small unrelated changes, some refactoring, and some TODO questions.)
>
> (1) The problem with headers:
> --------------------------------
> Before: Headers were handled in an inconsistent and broken manner.
> Examples:
> * Content-ID and a bunch of others were not supported at all (assemble() didn't know about them).
> * Headers were not parsed from the head in parse(), but only when a particular header was asked for. This meant that after removeHeader(some_header), headerByType(type_of_header_removed) still returned the header which was supposed to be removed.
> * There was a lot of duplication and unnecessary complexity with separate handling of headers in KMime::Content, KMime::Message and KMime::NewsArticle.
> * There wasn't any way of having more than one header of a particular type (e.g. Resent-From:), other than manually playing with the head QByteArray.
>
> After: Headers are handled uniformly in KMime::Content, and no special handling is necessary in Message or NewsArticle. MIME headers (Content-*), RFC5322 headers (From: etc.), and custom headers (X-*) all work. Each header class has a virtual clone() method for making a copy of the given header. A HeaderFactory is used when parsing, so that headers of appropriate type are created. appendHeader() and prependHeader() Content methods allow setting multiple headers of a given type.
>
> (2) The problem with singlepart<->multipart Content conversion:
> ------------------------------------------------------------------
> Before: Consequences of this automatic conversion led to bugs.
> Examples:
> * Say c is single-part. Headers::Base *h = c->contentType(); c->addContent(bla); Now the h pointer no longer belongs to c, but to c->contents().first(), because c was converted to multipart.
> * Say c is multipart/mixed, with 0 sub-contents. c->addContent(alpha); c->addContent(beta); c->removeContent(beta); alpha has just been deleted, because c has been converted to singple-part.
>
> After: In all cases that I can think of, the developer can create a single-part or a multipart content, depending on what he or she wants. The automatic conversion is a source of bugs and confusion, with very little benefit / convenience. But we cannot change this behaviour and stay backwards-compatible, right? :-(
> All I did was put big warnings in the API docs.
>
> (3) The problem with signatures and corrupted content:
> ---------------------------------------------------------
> Before: Calling assemble() on a Message changed it in slight ways (header order, where header folding occurs, Content-[Tt]ype capitalization, etc). This broke signature checking. András has added a fix for this, where assembleHeaders() would not change the head of the Content unless some header has actually been modified. IMHO, this is not the best possible solution. Problems with this are:
> * It's slow: Every header has to be checked in assembleHeaders().
> * It's easy to break: For example, calling c->contentDescription() will silently create an empty Content-Description header in c. It is difficult to handle such empty headers correctly in assembleHeaders(), getHeaderByType(), setHeader(), as well as when the headers have to be moved around in addContent()/removeContent().
> * Ideally, assemble() should not bother with message integrity. Assembling a message means creating it from scratch.
>
> After: I started with this clear picture of KMime::Content: There is a string representation (a big QByteArray holding the head and body of the content), and there is a broken-down representation (a list of headers and a list of subcontents). parse() updates the broken-down representation from the string representation. assemble() updates the string representation from the broken-down representation. This distinction has been inconsistent before (true for subcontents but not true for headers). Making parse() and assemble() do what they are supposed to do may actually break some applications, but I think fixing this will make everything clearer. In particular, before one could do:
> Content *c = new Content; c->setContent(something); c->getSomeHeader();
> Whereas now the header will not be available until parse() is called.
> For signature checking, I have added setFrozen() / isFrozen() methods to KMime::Content. Freezing a content guarantees that parse() and assemble() will never change the string representation returned by encodedContent().
>
> ---
> Thanks for reading. I appreciate any comments or suggestions.
>
>
> Diffs
> -----
>
> /trunk/KDE/kdepimlibs/kmime/CMakeLists.txt 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_codecs.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_content.h 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_content.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_content_p.h 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_header_parsing.h 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_header_parsing.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_headerfactory.h PRE-CREATION
> /trunk/KDE/kdepimlibs/kmime/kmime_headerfactory.cpp PRE-CREATION
> /trunk/KDE/kdepimlibs/kmime/kmime_headers.h 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_headers.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_message.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_message_p.h 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_newsarticle.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_util.h 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_util.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_util_p.h 1003373
> /trunk/KDE/kdepimlibs/kmime/tests/CMakeLists.txt 1003373
> /trunk/KDE/kdepimlibs/kmime/tests/headertest.h 1003373
> /trunk/KDE/kdepimlibs/kmime/tests/headertest.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/tests/kmime_content_test.h 1003373
> /trunk/KDE/kdepimlibs/kmime/tests/kmime_content_test.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/tests/kmime_headerfactorytest.h PRE-CREATION
> /trunk/KDE/kdepimlibs/kmime/tests/kmime_headerfactorytest.cpp PRE-CREATION
>
> Diff: http://reviewboard.kde.org/r/1151/diff
>
>
> Testing
> -------
>
> I added some unit tests, and the message composer in kdepimlibs seems to have no problems.
>
> It would be great if big users of KMime such as the mailreader and Mailody could try this and point out any potential problems. That is the reason I left plenty of kDebug()s in the code.
>
>
> Thanks,
>
> Constantin
>
>
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/
More information about the kde-pim
mailing list