Opened 9 years ago

Last modified 6 years ago

#3582 enhancement assigned

Improve SIP URI parsing/formatting

Reported by: washort Owned by: Rami C
Priority: highest Milestone:
Component: core Keywords:
Cc: Branch: branches/sip-uri-3582-2
branch-diff, diff-cov, branch-cov, buildbot
Author: washort, acapnotic

Description

Among other things, the current address and URI parsing code handles escaping incorrectly.

Attachments (3)

twisted3582.patch (72.4 KB) - added by Rami C 6 years ago.
Updated patch against trunk with several fixes
3582.bugfix (82 bytes) - added by Rami C 6 years ago.
twisted3582_against_branch.patch (1010 bytes) - added by Rami C 6 years ago.
Patch against the branch correcting issues raised

Download all attachments as: .zip

Change History (20)

comment:1 Changed 9 years ago by washort

Author: washort
Branch: branches/sip-uri-3582

(In [25684]) Branching to 'sip-uri-3582'

comment:2 Changed 9 years ago by washort

Owner: changed from Glyph to washort

comment:3 Changed 9 years ago by washort

Keywords: review added
Owner: washort deleted
Priority: normalhighest

comment:4 Changed 9 years ago by therve

Keywords: review removed
Owner: set to dash

Here we go!

  • There are some conflicts with trunk
  • In headerCapitalize, the lower() call is not tested
  • Why is the dashCapitalize deprecation commented?
  • Can you add a blank lines between fields documentation, and indent the subsequent lines? Like this for example:
        @ivar host: A hostname or IP address.
        @type host: C{str}
    
        @ivar username: The identifier of a particular resource at the host being
            addressed.
        @type username: C{str}, or None.
    
  • the removal of tag in URL is a backward incompatible change.
  • URI.__ne__ is not tested.
  • URI.__hash__ has few test coverage, and I suspect is a bit wrong. Objects that are equals must have the same hash, but the current implementation is missing several attributes.
  • the first lower() call in parseURL is not tested.
  • +        return "", parseURL(address, host=host, port=port), params
    

I suspect it should be u"".

  • +    return name.decode('utf8','replace'), url, params
    

The usage of replace is not tested. Please add a space after the comma, too.

Thanks!

Changed 6 years ago by Rami C

Attachment: twisted3582.patch added

Updated patch against trunk with several fixes

comment:5 Changed 6 years ago by Rami C

Keywords: review added
Owner: dash deleted

Patch attached against trunk, addressing issues raised in the last review:

  • Merge conflicts fixed against trunk
  • headerCapitalize now has a test case
  • dashCapitalize is deprecated again
  • fields documentation has been reformatted
  • tag in URI remains out, but it's been confirmed that tag is actually an attribute of the surrounding header and not of the URI itself, so shouldn't have been there originally
  • URI.__eq__ (and by extension URI.__ne__, which calls not self.__eq__()) have more tests
  • URI.__hash__ has better tests and a corrected implementation that passes them
  • parseURL has a test case for how it treats capitalization
  • parseURL also now returns a Unicode string
  • parseURL has a test case for its use of string.decode('utf8', 'replace')

There are also some formatting / coding style updates, and the tests have been moved from twisted/test/test_sip.py to twisted/protocol/test/test_sip.py

comment:6 Changed 6 years ago by acapnotic

Author: washortwashort, acapnotic
Branch: branches/sip-uri-3582branches/sip-uri-3582-2

(In [33795]) Branching to 'sip-uri-3582-2'

comment:7 Changed 6 years ago by acapnotic

Owner: set to acapnotic
Status: newassigned

Changed 6 years ago by Rami C

Attachment: 3582.bugfix added

comment:8 Changed 6 years ago by acapnotic

Keywords: review removed

I appreciate your attention to detail on the tests here. I believe you've addressed all the previous reviewer's comments, and I have nothing to add. It's always great to get old patches out of limbo, thanks for bringing some new energy to this!

+1, will merge.

comment:9 Changed 6 years ago by acapnotic

Resolution: fixed
Status: assignedclosed

(In [33801]) Merge sip-uri-3582-2: Better escaping and equality comparisons for SIP URLs.URLs.

Authors: washort, necaris Reviewers: acapnotic, therve Fixes: #3582

comment:10 Changed 6 years ago by Thijs Triemstra

A new deprecation was introduced by this ticket, but it's version nr is incorrect;

@deprecated(Version("Twisted", 8, 2, 0)) 

There's also not a test for this deprecation.

comment:11 Changed 6 years ago by Thijs Triemstra

The test case name is also incorrect. I'll open a new ticket for these issues instead of reverting this..

comment:12 Changed 6 years ago by Jean-Paul Calderone

Resolution: fixed
Status: closedreopened

(In [33815]) Revert r33801 - test suite regression

Reopens: #3582

The twisted.protocols.sip module docstring has formatting errors.

Also a test fails, at least on Python 2.5:

[FAIL]
Traceback (most recent call last):
  File "/var/lib/buildbot/bot-glyph-3/hardy32-python2.5-select/Twisted/twisted/protocols/test/test_sip.py", line 584, in testParseAddress
    self.assertEqual(name, gname)
  File "/var/lib/buildbot/bot-glyph-3/hardy32-python2.5-select/Twisted/twisted/trial/unittest.py", line 270, in assertEqual
    % (msg, pformat(first), pformat(second)))
twisted.trial.unittest.FailTest: not equal:
a = u'Invalid \ufffd('
b = u'Invalid \ufffd'

twisted.protocols.test.test_sip.ParseTestCase.testParseAddress

Changed 6 years ago by Rami C

Patch against the branch correcting issues raised

comment:13 Changed 6 years ago by Rami C

Keywords: review added
Owner: acapnotic deleted
Status: reopenednew

Replying to exarkun:

(In [33815]) Revert r33801 - test suite regression

Reopens: #3582

The twisted.protocols.sip module docstring has formatting errors.

Oops, sorry about that.

Also a test fails, at least on Python 2.5:

[FAIL]
Traceback (most recent call last):
  File "/var/lib/buildbot/bot-glyph-3/hardy32-python2.5-select/Twisted/twisted/protocols/test/test_sip.py", line 584, in testParseAddress
    self.assertEqual(name, gname)
  File "/var/lib/buildbot/bot-glyph-3/hardy32-python2.5-select/Twisted/twisted/trial/unittest.py", line 270, in assertEqual
    % (msg, pformat(first), pformat(second)))
twisted.trial.unittest.FailTest: not equal:
a = u'Invalid \ufffd('
b = u'Invalid \ufffd'

twisted.protocols.test.test_sip.ParseTestCase.testParseAddress

Argh -- I believe this is a change in Python's Unicode handling between 2.5 and 2.6 :-( I've changed the test to look at individual invalid characters rather than composing characters, which tests the relevant functionality but is less dependent on this particular behavior of Python 2.6+

I've attached a patch against the branch which addresses both these issues.

comment:14 Changed 6 years ago by Corbin Simpson

(In [33845]) t.p.test.test_sip: Apply latest patch from #3582.

Refs #3582

comment:15 Changed 6 years ago by Corbin Simpson

Keywords: review removed
Owner: set to Jean-Paul Calderone

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/sip-uri-3582-2 has builds for this branch.

After applying the provided patch (and committing it to the branch), I am not able to reproduce the test failure. Additionally, I think that the patch does adequately address the most recent review issues. However, I want exarkun to sign off on this, since he was the reopener, and I want to wait for the builds to finish so that we can be sure there aren't more failures.

comment:16 Changed 6 years ago by Thijs Triemstra

Owner: changed from Jean-Paul Calderone to Corbin Simpson
  • URI needs an @since: 12.1
  • Many classes and methods still use """Allow looking up physical address for logical URL.""" but should use:
    """
    Allow looking up physical address for logical URL.
    """ 
    
  • dashCapitalize is deprecated but this doesn't have a test. It also uses the incorrect version nr (should be 12.1)
    @deprecated(Version("Twisted", 8, 2, 0))
    

comment:17 Changed 6 years ago by Rami C

Owner: changed from Corbin Simpson to Rami C
Status: newassigned

I'm on it.

Note: See TracTickets for help on using tickets.