Fix dispatching of webhooks with non-ASCII content by sending only binary strings

Review Request #8560 — Created Dec. 4, 2016 and submitted

Information

Review Board
master
f026695...

Reviewers

When dispatching a webhook that contained non-ASCII content, the urlopen call raised an exception
that reported an encoding error, complaining that some characters are not encodable with the ascii
encoding.

This issue occurs because the urlopen implementation concatenates the given URL, headers, and body
using string concatenation, and, if the resulting string is a unicode string, encodes it implicitly
as ASCII before sending it to the server.

This change encodes all data passed to urlopen as binary strings, so that the result of string
concatenation is also a binary string, thus necessitating no implicit encoding. While writing tests
for this issue, an issue with silently swallowed assertions in the test _urlopen was fixed.

Added 4 unit tests for non-ASCII custom, JSON, XML and Form-Data content.
Checked locally that payloads are sent without issue.
Added check that all data sent to _test_dispatch is made of binary strings.

Description From Last Updated

Col: 17 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

Col: 80 E501 line too long (90 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Can we switch these lines (to keep it in alphabetical order)?

daviddavid

I know the other methods in here have "Test ..." but our general convention for unit test docstrings is "Testing …

daviddavid

Same here.

daviddavid

Same here.

daviddavid

Same here.

daviddavid

Please add a blank line between these two.

daviddavid

Please add a blank line between these two.

daviddavid

Should have two blank lines between top-level classes.

daviddavid

We should get six from django.utils, rather than the standalone package.

daviddavid

Please add a blank line between these two.

daviddavid

This kind of comment (narrating the code) doesn't really help too much with understanding the code. The call to .encode() …

daviddavid

Again, this comment is kind of narrative instead of explanatory. Perhaps an even better solution than doing this test would …

daviddavid

Instead of having this comment, can we have a unit test that calls this while spying on urlopen/Request and verifies …

daviddavid

Again, this comment doesn't really add to the understanding of the code.

daviddavid

'six' imported but unused

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
  2. reviewboard/notifications/webhooks.py (Diff revision 1)
     
     
    Show all issues
    Col: 17
     E131 continuation line unaligned for hanging indent
    
  3. reviewboard/notifications/webhooks.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (90 > 79 characters)
    
  4. reviewboard/notifications/webhooks.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  5. 
      
JH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
  2. 
      
JH
david
  1. 
      
  2. reviewboard/notifications/tests.py (Diff revision 2)
     
     
     
    Show all issues

    Can we switch these lines (to keep it in alphabetical order)?

  3. reviewboard/notifications/tests.py (Diff revision 2)
     
     
    Show all issues

    I know the other methods in here have "Test ..." but our general convention for unit test docstrings is "Testing ..."

  4. reviewboard/notifications/tests.py (Diff revision 2)
     
     
    Show all issues

    Same here.

  5. reviewboard/notifications/tests.py (Diff revision 2)
     
     
    Show all issues

    Same here.

  6. reviewboard/notifications/tests.py (Diff revision 2)
     
     
    Show all issues

    Same here.

  7. reviewboard/notifications/tests.py (Diff revision 2)
     
     
     
    Show all issues

    Please add a blank line between these two.

  8. reviewboard/notifications/tests.py (Diff revision 2)
     
     
     
    Show all issues

    Please add a blank line between these two.

  9. reviewboard/notifications/tests.py (Diff revision 2)
     
     
    Show all issues

    Should have two blank lines between top-level classes.

  10. reviewboard/notifications/webhooks.py (Diff revision 2)
     
     
    Show all issues

    We should get six from django.utils, rather than the standalone package.

  11. reviewboard/notifications/webhooks.py (Diff revision 2)
     
     
     
    Show all issues

    Please add a blank line between these two.

  12. 
      
JH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/notifications/webhooks.py (Diff revision 3)
     
     
     
     
     
    Show all issues

    This kind of comment (narrating the code) doesn't really help too much with understanding the code. The call to .encode() is pretty self-explanatory.

  3. reviewboard/notifications/webhooks.py (Diff revision 3)
     
     
     
     
    Show all issues

    Again, this comment is kind of narrative instead of explanatory. Perhaps an even better solution than doing this test would be to put bytes = bytes.encode('utf-8') into each of the cases above where it's needed.

  4. reviewboard/notifications/webhooks.py (Diff revision 3)
     
     
     
    Show all issues

    Instead of having this comment, can we have a unit test that calls this while spying on urlopen/Request and verifies that all headers are bytes? That way we won't accidentally regress this.

    1. Test assertions to check that all arguments passed to urlopen are binary strings have always been part of this review request. (With a fix to make test assertions made in the _urlopen replacement actually fire).

  5. reviewboard/notifications/webhooks.py (Diff revision 3)
     
     
    Show all issues

    Again, this comment doesn't really add to the understanding of the code.

  6. 
      
JH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
  2. reviewboard/notifications/webhooks.py (Diff revision 4)
     
     
    Show all issues
     'six' imported but unused
    
  3. 
      
JH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
  2. 
      
chipx86
  1. We have a specific format we like for the review request descriptions, which are used for the commit message. We like to clearly communicate what the problem was before and how it was fixed. Would you mind updating the review request for that? We have details and examples up at https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea

  2. 
      
JH
david
  1. Ship It!
  2. 
      
JH
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (975b8e5)
Loading...