closes RestComm/jain-slee#111 #149
Conversation
src/gov/nist/core/Debug.java
Outdated
| @@ -34,8 +34,8 @@ | |||
| */ | |||
| public class Debug { | |||
|
|
|||
| public static boolean debug = false; | |||
| public static boolean parserDebug = false; | |||
| public static boolean debug = true; | |||
There was a problem hiding this comment.
do we need these flags set to true? would it be possible you used them for debugging and forget to set them to false again?
There was a problem hiding this comment.
They are back to false
| @@ -736,7 +736,8 @@ public String number() throws ParseException { | |||
|
|
|||
| int startIdx = ptr; | |||
| try { | |||
| if (!isDigit(lookAhead(0))) { | |||
| char c = lookAhead(0); | |||
| if (c != '-' && !isDigit(c)) { | |||
There was a problem hiding this comment.
this logic will affect several parsing scenarios. What is the reasoning to include the condition on "-" char? is it based on any RFC3261 statement? can you include a link to the grammar that supports this rule?
There was a problem hiding this comment.
No, it is based on the following RFC: https://tools.ietf.org/html/rfc4475#section-3.3.9
RFC states:
From a framing perspective, this situation is equivalent to an invalid Content-Length value (or no value at all).
There was a problem hiding this comment.
can you include this link,and brief explanation as comment
There was a problem hiding this comment.
my concern is that this requirement seems to be specific for Content-Length header, but this piece of code we are modifying is applied to all sort of numberic header conversions.., hence im afraid we can break contract for other headers...
There was a problem hiding this comment.
this a brief of main usages for "number" method including different types of headers
There was a problem hiding this comment.
apparently, if I rewrite the test as in (i prefer my version,not based on asserting exception message, but whether exception hs been raised or not)9:
// https://github.com/RestComm/jain-slee/issues/111 public void testNegativeContentLength(){ try { final String header = "Content-Length: -123 \n"; System.out.print(header); createParser(ContentLengthParser.class, header).parse(); fail("previous command should have raise an expection"); } catch (java.text.ParseException ex) { //expected exception thrown } }
Even the original code passes, throwing the parseexception (at different part of the logic).
There was a problem hiding this comment.
All JUnit parser tests (jain-sip/src/test/unit/gov/nist/javax/sip/parser) are OK.
From the headers above the following are covered:
- HostNameParserTest
- TimeStampParserTest
- ContentLengthParserTest
- CSeqParserTest
- MaxForwardsParserTest
- MimeVersionParserTest
- MinExpiresParserTest
- RetryAfterParserTest
TCK regression is also OK.
There was a problem hiding this comment.
what do you think about my claim? "Even the original code passes, throwing the parseexception (at different part of the logic)."
| @@ -43,7 +43,7 @@ | |||
|
|
|||
| protected ParseException createParseException(String exceptionString) { | |||
| return new ParseException( | |||
| lexer.getBuffer() + ":" + exceptionString, | |||
| lexer.getBuffer().trim() + ":" + exceptionString, | |||
There was a problem hiding this comment.
why do we need to "trim" here? messages should be already well formatted..
There was a problem hiding this comment.
if parsing fails, like for example negative number value, buffer will contain new line (\n) at the end, so that's the reason for the trim
| logger.logDebug("Sending automatic 400 Bad Request:"); | ||
| logger.logDebug(badReqRes.toString()); | ||
| } | ||
| sendResponse(badReqRes); |
There was a problem hiding this comment.
should we add a Reason-Phrase header with the exception message?
There was a problem hiding this comment.
Implementation is based on MessageChannel.createBadReqRes, so if we plan to add it, it should be added everywhere
For Reason header, we can use the same exception message that is used in response:
SIPResponse badReqRes = sipRequest.createResponse(Response.BAD_REQUEST, "Bad Request (" + pe.getLocalizedMessage() + ')');
There was a problem hiding this comment.
Implementation for reason header is also ready, it is based on RFC3326.
Should I add it here or in a separate bug report?
There was a problem hiding this comment.
we may introduce that later in a separate PR
| throw new TransactionUnavailableException(ex.getMessage(), ex); | ||
| } catch (ParseException pe) { | ||
| try { | ||
| SIPResponse badReqRes = sipRequest.createResponse(Response.BAD_REQUEST, |
There was a problem hiding this comment.
have you tested this somehow? could you add ad test case for this?
There was a problem hiding this comment.
In the bug report (RestComm/jain-slee#111) there is a SIPP TC which was used to test this correction, but I will additionally create junit TC
No description provided.