RFR: 8185898: setRequestProperty(key, null) results in HTTP header without colon in request

[Julia Boes]

Hi,

Please find an alternative fix below. Chris and Daniel suggested to add 
a new method MessageHeader::isRequestline rather than a separate data 
field for the requestline. A whitebox test was added with the method 
testMessageHeader.

Updated webrev: http://cr.openjdk.java.net/~michaelm/jboes/8185898/webrev.4/

Regards,

Julia

On 06/08/2019 11:03, Julia Boes wrote:
> Hi Michael and Daniel,
>
> Thanks for your comments. I made the following changes to B8185898:
>
> - Added checks for URLConnection::getRequestProperties, 
> ::getHeaderField(0), ::getHeaderFieldKey(0)
>
> - Included a test of MessageHeader::print and ::toString. Added a null 
> check for the requestLine in MessageHeader::toString (Thanks Jaikiran!)
>
> Updated webrev: 
> http://cr.openjdk.java.net/~michaelm/jboes/8185898/webrev.2/
>
> Best,
>
> Julia
>
>
> On 31/07/2019 15:34, Daniel Fuchs wrote:
>> Hi Michael,
>>
>> On 31/07/2019 14:47, Michael McMahon wrote:
>>> Daniel
>>>
>>> I don't think the change affects the usage of response headers,
>>> but it might be useful to include in the test a call to 
>>> getHeaderField(0)
>>> to verify that.
>>
>> Oh - right - but it will/might affect the toString() of the
>> response headers, if that is used for logging, isn't it?
>> Wouldn't the status line now be printed with an additional colon
>> at the end?
>>
>> best,
>>
>> -- daniel
>>
>>>
>>> Michael.
>>>
>>> On 31/07/2019, 12:30, Daniel Fuchs wrote:
>>>> Hi Julia,
>>>>
>>>> Could you verify that `HttpURLConnection::getHeaderField(0)` and
>>>> `HttpURLConnection::getHeaderFieldKey(0)` return the same thing
>>>> before and after the fix, for all implementations of
>>>> `HttpURLConnection` in the JDK?
>>>>
>>>> I believe your test is missing that.
>>>>
>>>> More specifically, I'm referring to this:
>>>> https://download.java.net/java/early_access/jdk13/docs/api/java.base/java/net/HttpURLConnection.html#getHeaderFieldKey(int) 
>>>>
>>>>
>>>> "Some implementations may treat the 0th header field as special,
>>>> i.e. as the status line returned by the HTTP server. In this case,
>>>> getHeaderField(0) returns the status line, but getHeaderFieldKey(0)
>>>> returns null."
>>>>
>>>> I wonder if the JDK itself was one of those implementations before
>>>> the fix. We do not want to change the behaviour of these methods.
>>>>
>>>> best regards,
>>>>
>>>> -- daniel
>>>>
>>>> On 31/07/2019 11:56, Julia Boes wrote:
>>>>> Hi,
>>>>>
>>>>> Please find below a patch for:
>>>>>
>>>>> 8185898: setRequestProperty(key, null) results in HTTP header 
>>>>> without colon in request
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8185898
>>>>>
>>>>> According to RFC 2616 
>>>>> <https://tools.ietf.org/html/rfc2616#section-4>, message headers 
>>>>> of a HTTP message must adhere to the format:
>>>>>
>>>>> field-name ":" [ field-value ]
>>>>>
>>>>> Previously, the request line was handled like a message header and 
>>>>> MessageHeader::print would omit ":" for message headers with value 
>>>>> == null to account for the request line. However, this can result 
>>>>> in a regular message header missing the colon if its value is 
>>>>> null. To fix this, MessageHeader.requestLine was added, which is 
>>>>> used only for the status line of a request. Any use of 
>>>>> MessageHeader.set(0) and MessageHeader.prepend() in 
>>>>> HttpURLConnection was replaced by MessageHeader::setRequestLine.
>>>>>
>>>>> Webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~michaelm/jboes/8185898/webrev.1/
>>>>>
>>>>>
>>>>> Cheers,
>>>>>
>>>>> Julia
>>>>>
>>>>
>>