small bug in SIP Via header generation

[Antoine Pitrou]

There is a small bug in the Via.toString() method. Perhaps it can only occur in theoretical conditions (I don't know enough about the SIP protocol to know for sure).

However, here it is:

>>> from twisted.protocols import sip
>>> via = sip.Via("localhost", ttl=2)
>>> via.toString()
'SIP/2.0/UDP localhost:5060;ttl=2'
>>> via = sip.Via("localhost", ttl=1)
>>> via.toString()
'SIP/2.0/UDP localhost:5060;ttl'

As you see, if a value is exactly 1, it isn't output. This is because of the test "if value == True" at line 207. Replacing it with "if value is True" fixes the bug.

[Antoine Pitrou]

And here is a test to be added in test_sip:ViaTestCase.

    def testOneIsNotTrue(self):
        s = "SIP/2.0/UDP example.com:5060;ttl=1"
        v = sip.parseViaHeader(s)
        self.assertEquals(v.toString().rsplit(';')[1], "ttl=1")
        self.checkRoundtrip(v)

[therve]

AFAIK, 1 is the default value for ttl.

[Antoine Pitrou]

The bug can happen with any parameter, I just took "ttl" as an example. Also, RFC 3261 says (section 18.2.2):

If the address is a multicast address, the response SHOULD be sent using the TTL indicated in the "ttl" parameter, or with a TTL of 1 if that parameter is not present.

It does not say there is a default value if the parameter is present but does not specify a value.

[washort]

(In [25676]) Branching to 'via-parameters-2194'

[Jean-Paul Calderone]

1. twisted/protocols/sip.py
   1. In the class docstring for Via several attributes are documented as Optional or Required. I see that this is information from the spec, but I'm not sure what it means for the attribute. This should be converted into information which can more easily be used by a developer likely to interact with this class (for example, perhaps the optional attributes should be documented as possibly having a None value if the information isn't present).
   2. The types of most of the attributes are not documented but should be.
   3. The enormous try/except in parseViaHeader doesn't seem ideal to me. One specific problem it has is that some of the parsing code raises SIPError with detailed information about how the parsing failed (the wrong version number case), but the except suite squashes everything into a generic SIPError. Another specific problem is the log.err call in the except suite; low-level parsing functions shouldn't log errors, they should raise exceptions indicating how parsing failed. Another problem is that there seems to be no test coverage for the exception handling in this function; it's hard to really decide if the implementation strategy is appropriate without tests. :)
2. The test suite hangs at testAmoralRPort for me. A parse error shows up in the logs. This may indicate that the Via rport handling has changed incompatibly...
3. Why was testExtraWhitespace deleted?

[washort]

(In [26100]) Branching to 'via-parameters-2194-2'

[washort]

Fixed. The try/except turned out to be useless since nothing catches any exceptions at the call sites for parseViaHeader.

[Jean-Paul Calderone]

1. It seems that there's still an incompatibility in the rport change. Old code passing True will get inexpected behavior (rport=True, I think). Can you deprecate this use of the Via.__init__ rport parameter and restore the old behavior for that case?
2. Before testExtraWhitespace was deleted. Now it's duplicated. Er?
3. I don't think _Absent.__nonzero__ quite does the trick. Even if the deprecated code isn't being edited, its behavior is potentially being changed. That's fine, if it has tests, but it doesn't appear to have tests. So it probably needs to have tests added. It also looks like some of it is broken by this change - for example, Base._fixupNAT compares the value of rport to True. That case never happens anymore, right? It's now the None case.

[washort]

OK, new scheme for compatibility added (and other stuff fixed).

[Glyph]

1. flakes: Not your fault, but in test_sip, twisted.cred.checkers and twisted.cred.portal are apparently unused. Also, twisted.cred.error is imported in a weird way.
2. getWarningMethod() should really be deprecated. It's no longer necessary. just use warnings.warn.
3. BAD_REQUEST is apparently never used. Neither is a "400" used anywhere in the diff. I guess it should just be deleted?
4. I think it would be clearer to call the getter _getrport, rather than the ambiguous rport. (Also the setter should be _setrport, since I find the case difference is a bit confusing.)
   1. also please two blank lines after the rport method, when you change its name.
5. Given that rportRequested is a boolean, the "== True" crud is just noise. Delete it.
6. Docstrings should use L{} more often. For example, instead of using a sentence fragment, Via's docstring could begin with: "A L{Via} is a SIP 'Via' header...". parseViaHeader should say "Parse a 'via' header, returning a L{Via}." Also it could say "@return" and "@rtype".

This is all pretty cosmetic, so please address and then merge.

[washort]

(In [26236]) Merge via-parameters-2194-2. Fix a defect in SIP Via header parameter generation.

Author: washort Reviewers: exarkun, glyph Closes #2194.