fix #1525: add missing sub MultipartWriter headers

[terencehonles]

When appending MultipartWriter objects to another MultipartWriter use
the subwriter's headers in the constructed BodyPartWriter

fixes issue #1525

What do these changes do?

Fixes issue #1525 by allowing the wrapping BodyPartWriter to use the containing MultipartWriter's headers. This fixes the issue with the missing headers and also allows additional header changes until .serialize() is called.

Are there changes in behavior for the user?

Documentation is now correct, and if the user previously explicitly passed the subwriter's headers when appending the object the headers will already be in the subwriter's headers object so it is effectively a no-op replacing existing values with their same values.

Related issue number

#1525

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• Add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new entry to CHANGES.rst
  • Choose any open position to avoid merge conflicts with other PRs.
  • Add a link to the issue you are fixing (if any) using #issue_number format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.

[codecov-io]

Current coverage is 98.85% (diff: 100%)

Merging #1530 into 1.2 will increase coverage by <.01%

@@                1.2      #1530   diff @@
==========================================
  Files            30         30          
  Lines          6984       6988     +4   
  Methods           0          0          
  Messages          0          0          
  Branches       1163       1165     +2   
==========================================
+ Hits           6904       6908     +4   
  Misses           40         40          
  Partials         40         40          

Powered by Codecov. Last update 63d1ad5...7144268

[terencehonles]

@kxepal not sure if you're the multipart owner, but this is related to the other multipart issue I raised. Ty

[kxepal]

@terencehonles eventually (:
But why not to let MultipartWriter have others MultipartWriters as a part? Having twisted mix of MultipartWriters that includes BodyPartWriters which holds else MultpartsWriters and so on and so deeper looks and feels quite strange. Assuming Multpart message is a tree, MultipartWriters are nodes while BodyPartWriters are the leaves.

[terencehonles]

@kxepal but that's not what is currently written and written that way would actually make it more complicated in code. If multipart messages were nodes in the tree then they would have to know if they are the root node or not. If they were not the root node they would serialize their headers and body as the BodyPartWriter.serialize() does. If they were at the root they would omit that information and let whatever was containing it write the headers as needed.

By keeping all serialization code in the BodyPartWriter it simplifies how the encoding and serialization is done (and it doesn't change how the code is currently organized). Which leaves MultipartWriter as a sort of tree builder (via .append(...)) and a root to call .serialize() on.

If you think it is awkward that the user can find out that a BodyPartWriter is being used, then we could special case the append to allow both BodyPartWriter or MultipartWriter in the parts list and when serializing wrap the MultipartWriter in a BodyPartWriter and serialize that, but the downside is we are doing more work during serialization which means there are more chances for exceptions to be raised.

The nice thing about how the code is currently written is MultipartWriter.serialize() only cares that it emits boundaries between all its parts. The part handles how it needs to be serialized and it does so. If the part is also a MultipartWriter it allows it to handle the body serialization, but it still handles everything else. The way it is written is really seems like how it should be written.

[kxepal]

@terencehonles
Good points. I'm indeed missed few details there. Ok, let's have it so. No lightweight alternatives comes to mind anyway.

Please, add few more tests about this change to let us remember about this use case.

[terencehonles]

@kxepal sounds good 👍

[terencehonles]

@kxepal I'm not sure what needs to be done to satisfy those checks that failed

[kxepal]

@terencehonles
It's about patch code coverage. Check codecov report.
The yellow line is a partial covered branch. This means that there are two possible cases for this line: when condition is True and you fall into the if case and when it's False and you don't enter there. It seems that the False-case wasn't satisfied. Probably, worth to add a case when you append a MultipartWriter and not pass custom headers.

[terencehonles]

@kxepal how do you even know what is covering it or not? This is not actually the case you are talking about because MultipartWriter.append(...) always passes an object for headers and it is never None. If there was something covering it before it was part of another test case (which goes back to my original question)

[kxepal]

@terencehonles

how do you even know what is covering it or not?

The codecov report says that with colors (;

This is not actually the case you are talking about because MultipartWriter.append(...) always passes an object for headers and it is never None.

I'm about headers argument in BodyPartWriter.__init__ - you have a condition there. We're talking about the same codeline, right?

[terencehonles]

@kxepal you're answering a slightly different question. I asked what is covering a line you're answering if a line is covered.

We're talking about the same code path, but the only way to exercise that code path is to create a BodyPartWriter with a multipart object during construction. None of the tests currently explicitly check construction of the BodyPartWriter but rely on the one created in the setUp method. (This is why I was asking the first question). I added a test case which covers instantiation, but I'd be willing to bet our code coverage report is lying to us.

[kxepal]

@terencehonles
Sorry, I didn't understand your question (actually, I can't even find it) .

But anyway, yes, it seems the only way to cover those branch is to test BodyPartWriter constructor explicitly - that's not a problem, I think (: There were no else such tests because setup test suite and else tests did that job well. Not very unit approach, but quite dry. So that one should fill the blanks.

Coverage report here doesn't lies, just tells which branches were not covered by test run.

[terencehonles]

how do you even know what is covering it [a line] or not?
Was the question.

Unless you know what is covering a line and what data it was using there's a possibility it is not being meaningfully covered. The colors only show that some line in the test suite exercised the code not that it completely exercised it with the correct data (that's why I was wondering if there was a way to backtrack). I don't have enough time right now to look at the tests to figure out if this is currently a problem, but it's great that we even have as many tests as we do.

[kxepal]

@terencehonles
Well, if you put there those condition, you had some reasons for it. But every condition causes branches and that's what coverage ask you to do: cover them all e.g. make sure each to be executed somehow. It's not and never about the data or any meaning. You can remove those condition and code coverage will be full, but not the data, so there be a bug. But that's about another kind of coverage and it need different tools to use.

The current failure seems to be not your fault, but some flaky test in webserver about serving static content. I triggered it retry.

[terencehonles]

@kxepal it seems like we're not understanding each other, but it's not really something that needs to be continued on this PR.

My point was we need to make sure we write tests to test the code not just to make the coverage tool show green. The tool helps us see what we might have missed, but I'm arguing it might be more generous than it should be when saying a line is "covered". Anyways, I'm fine leaving the discussion wherever it is :)

[kxepal]

@terencehonles
No, I finally get your point, but it's still about different. Coverage tool checks only code coverage, so we need at least such set of tests that covers all the code statements (not even lines). But this doesn't means that this set of tests is enough to say that we tested our code right and complete. There could be more tests about edge cases. Suddenly, there is no tools to show how better you catch all such cases, only to help you reach them.

Anyway, for current PR it all looks fine. +1

@asvetlov, any notes before merge?

[fafhrd91]

lgtm

[lock]

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.