Opened 11 years ago
Last modified 11 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 Triemstra | Branch: |
branches/sip-messageparser-3583
branch-diff, diff-cov, branch-cov, buildbot |
Author: | washort |
Attachments (1)
Change History (16)
comment:1 Changed 11 years ago by
Author: | → washort |
---|---|
Branch: | → branches/sip-messageparser-3583 |
comment:2 Changed 11 years ago by
Keywords: | review added |
---|
comment:3 Changed 11 years ago by
Owner: | Glyph deleted |
---|
comment:4 Changed 11 years ago by
Keywords: | review removed |
---|---|
Owner: | set to washort |
- twisted/protocols/sip.py
MessagesParser.state
is documented as being able to take on the value '"invalid"' but it never does- I'd make
MessagesParser.multiheaders
andMessagesParser.multiAddressHeaders
private. They should be documented as well. MessagesParser.invalidMessage
is untested.- the else case in
MessagesParser.dataDone
is untested and should useraise Foo(...)
instead ofraise Foo, ...
- the exception handling in
MessagesParser.dataReceived
is untested - the exception handling in
MessagesParser.lineReceived
around bothself.processHeaderLine(self.prevline)
lines is untested MessagesParser._splitMultiHeader
:- uses a simple suite, shouldn't
- would probably be clearer with
not
instead of~
- The else case for the
if multi
test inMessagesParser.processHeaderLine
is untested
- twisted/test/test_sip.py
MessageParsingTestCase.testMultiHeaders
is undocumented and misnamedMessageParsingTestCase.testContinuationLines
is undocumented and misnamedParseTestCase.testHeaderSplitting
is misnamed- two blank lines between methods please
comment:5 Changed 11 years ago by
Owner: | washort deleted |
---|
1.
- Fixed. The code for this state was removed in Sine, never changed the docs.
- 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. - 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 11 years ago by
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 11 years ago by
Keywords: | review added |
---|
comment:8 Changed 11 years ago by
Cc: | Thijs Triemstra added |
---|---|
Keywords: | review removed |
Owner: | set to washort |
New public methods should have @since epydoc markup.
comment:9 Changed 11 years ago by
Keywords: | review added |
---|---|
Owner: | washort deleted |
Priority: | normal → highest |
Oops, not supposed to be a public method.
comment:10 Changed 11 years ago by
Keywords: | review removed |
---|---|
Owner: | set to washort |
- The code path through
invalidMessage
which doesraise 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? _multiheaders
and_multiAddressHeaders
are documented as@cvar
, but accessed onself
. 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
.- I believe the idiom for constructor arguments which are also attributes is to document them as
@ivar
s, and leave__init__
undocumented. - 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) - 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 afterself.invalidMessage()
are uncovered, becauseinvalidMessage()
raises an exception.
comment:12 Changed 11 years ago by
Owner: | set to Glyph |
---|---|
Status: | new → assigned |
comment:13 Changed 11 years ago by
Owner: | changed from Glyph to washort |
---|---|
Status: | assigned → new |
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'.- same for
UNSUPPORTED_URI
- same for
messageReceived
is now documented twice, but neither one says what type the argument is nor whether it's expected to return anything.- the
except ValueError:
branch inlineReceived
where thereturn
statement was deleted is still not covered by any test intest_sip
. Please address this. - 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 11 years ago by
Keywords: | review removed |
---|
Changed 9 years ago by
Attachment: | sip-messageparser-3583-patch.txt added |
---|
Replaced response codes with corresponding constants, added missing ones.
comment:15 Changed 9 years ago by
Owner: | washort deleted |
---|
Note: See
TracTickets for help on using
tickets.
(In [25691]) Branching to 'sip-messageparser-3583'