Opened 6 years ago

Last modified 5 years ago

#3583 enhancement new

Include SIP message-parsing changes from Sine

Reported by: washort Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: thijs Branch: branches/sip-messageparser-3583
(diff, github, buildbot, log)
Author: washort Launchpad Bug:

Description

Sine's implementation of MessageParser improves on Twisted's some. It should be included in twisted.protocols.sip.

Among the improvements are fixes for #2197 and #2198.

Attachments (1)

sip-messageparser-3583-patch.txt (55.2 KB) - added by wooshie 4 years ago.
Replaced response codes with corresponding constants, added missing ones.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by washort

  • Author set to washort
  • Branch set to branches/sip-messageparser-3583

(In [25691]) Branching to 'sip-messageparser-3583'

comment:2 Changed 6 years ago by washort

  • Keywords review added

comment:3 Changed 6 years ago by washort

  • Owner glyph deleted

comment:4 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner set to washort
  1. twisted/protocols/sip.py
    1. MessagesParser.state is documented as being able to take on the value '"invalid"' but it never does
    2. I'd make MessagesParser.multiheaders and MessagesParser.multiAddressHeaders private. They should be documented as well.
    3. MessagesParser.invalidMessage is untested.
    4. the else case in MessagesParser.dataDone is untested and should use raise Foo(...) instead of raise Foo, ...
    5. the exception handling in MessagesParser.dataReceived is untested
    6. the exception handling in MessagesParser.lineReceived around both self.processHeaderLine(self.prevline) lines is untested
    7. MessagesParser._splitMultiHeader:
      1. uses a simple suite, shouldn't
      2. would probably be clearer with not instead of ~
    8. The else case for the if multi test in MessagesParser.processHeaderLine is untested
  2. twisted/test/test_sip.py
    1. MessageParsingTestCase.testMultiHeaders is undocumented and misnamed
    2. MessageParsingTestCase.testContinuationLines is undocumented and misnamed
    3. ParseTestCase.testHeaderSplitting is misnamed
    4. two blank lines between methods please

comment:5 Changed 6 years ago by washort

  • Owner washort deleted

1.

  1. Fixed. The code for this state was removed in Sine, never changed the docs.
  2. I've removed this case, since apparently it never worked. In fact, twisted.test.test_sip.MessageParsingTestCase.testGarbage asserts that messages with data past the Content-Length parse correctly and ignore trailing garbage.
  3. After inspecting the code, I can't find anything that can reasonably trigger this; lineReceived handles all exceptions raised by parsing problems. Hence, the entire method has been removed.

Everything else fixed.

comment:6 Changed 6 years ago by washort

Made a further change: the RFC says that requests with incomplete bodies should be replied to with error 400, and that responses with incomplete bodies should be ignored.

comment:7 Changed 6 years ago by washort

  • Keywords review added

comment:8 Changed 6 years ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to washort

New public methods should have @since epydoc markup.

comment:9 Changed 5 years ago by washort

  • Keywords review added
  • Owner washort deleted
  • Priority changed from normal to highest

Oops, not supposed to be a public method.

comment:10 Changed 5 years ago by glyph

  • Keywords review removed
  • Owner set to washort
  1. The code path through invalidMessage which does raise exc is not covered by any tests. However, nothing actually invokes that code path. Why did you add the argument, if it's never passed?
  2. _multiheaders and _multiAddressHeaders are documented as @cvar, but accessed on self. The fact that these are initialized on the class is an implementation detail; they can be replaced on instances and the values will be honored. This is important (c.f. for testing) so please document them as @ivar.
  3. I believe the idiom for constructor arguments which are also attributes is to document them as @ivars, and leave __init__ undocumented.
  4. Despite its new documentation, lineLengthExceeded is untested. Also, can we be a little more specific than "ridiculous"? (maybe it should say that raising the exception in that context will close the connection)
  5. There are a bunch of lines in lineReceived that have no test coverage. Some of them aren't this branch's fault, but many of them are. trial --coverage twisted.test.test_sip is your friend. Specifically, all of the 'return' statements after self.invalidMessage() are uncovered, because invalidMessage() raises an exception.

comment:11 Changed 5 years ago by washort

  • Keywords review added
  • Owner washort deleted

comment:12 Changed 5 years ago by glyph

  • Owner set to glyph
  • Status changed from new to assigned

comment:13 Changed 5 years ago by glyph

  • Owner changed from glyph to washort
  • Status changed from assigned to new
  1. BAD_REQUEST is defined but there are still new lines of code that have an explicit '400' on them, in a context whree I assume they mean 'BAD_REQUEST'.
    1. same for UNSUPPORTED_URI
  2. messageReceived is now documented twice, but neither one says what type the argument is nor whether it's expected to return anything.
  3. the except ValueError: branch in lineReceived where the return statement was deleted is still not covered by any test in test_sip. Please address this.
  4. the comment that says # CRLF, we now have... probably should say # blank line, we now have... - or at the very least, CRLFCRLF. presumably non-blank lines end with CRLF too.

comment:14 Changed 5 years ago by glyph

  • Keywords review removed

Changed 4 years ago by wooshie

Replaced response codes with corresponding constants, added missing ones.

comment:15 Changed 3 years ago by <automation>

  • Owner washort deleted
Note: See TracTickets for help on using tickets.