RFR 4906983: java.net.URL constructors throw MalformedURLException in undocumented way

Wed Sep 16 14:27:50 UTC 2015

Hi,

here is the updated webrev:

http://cr.openjdk.java.net/~dbuck/4906983.3/

Hope the comments are fine now.

What would be the normal procedure in net-dev for accepting a change (a
2 group-member thumbs-up like in core-libs)?

-- Sebastian

Am 14.09.2015 um 10:53 schrieb Chris Hegarty:
> On 13/09/15 20:18, Sebastian Sickelmann wrote:
>> Am 13.09.2015 um 16:25 schrieb Chris Hegarty:
>>>
>>>> On 13 Sep 2015, at 14:07, Mark Sheppard
>>>> <<mailto:mark.sheppard at oracle.com>mark.sheppard at oracle.com> wrote:
>>>>
>>>> Hi
>>>>   I don't think the URL string http://server:-1/path can be
>>>> considered a valid URL as the port value -1 is not legal port value.
>>>
>>> I agree, but java.net <http://java.net>.URL is a legacy API. The only
>>> changes we can realistically make to it now are to tighten the spec as
>>> per existing behaviour ( in a manner so as to not encourage it to be
>>> used in an incorrect way ).
>>
>> I think we could change the behavior of the constructors which receive a
>> "spec" parameter. In non of those javadocs the -1 is mentioned. ]
>
> I would be opposed to changing the behavior of any of these constructors.
>
> > But this
>> is maybe another discussion, unrelated to cleanup the javadoc. I would
>> start it in another thread, when you agree to discuss this separatly.
>
> Separately, if at all.
>
>> For now i would leave the testcase and add a longer comment to it.
>
> For now, in the test, just remove the string spec constructors, and
> leave the ones accepting the port arg.
>
>>>> However, the URL class doesn't object and throw a
>>>> MalformedURLException, which is something of an
>>>> anomaly, caused by the spec allowing a port value of -1 for the 4 arg
>>>> constructor, which maps to
>>>> the default value for the associated protocol.  Using an input port
>>>> value of -1 is unnecessary, as the 3 arg constructor
>>>> is sufficient to specify a URL with a default port value.
>>>
>>> Right, it could be argued that the original authors added to much
>>> “convenience” here.
>>>
>>>> It would seem more appropriate to encapsulate the
>>>> -1 value as an implementation detail.
>>>
>>> Right, it is a pity that the ‘-1’ has made it into the spec :-(
>>>
>>>> A further anomaly arises when a subsequent getPort() method is
>>>> invoked, and this returns a value of -1, rather than
>>>> the protocol's associated default value. The -1 doesn't seem to
>>>> activate until a connection is made using the URL's details.
>>>
>> I think we can remove almost all references in the javadoc to the
>> special -1 port value.
>
> I don't think we can do this without breaking many things. And it adds
> little value.
>
> > getPort() is one place where it cannot
>> removed, and the mention for the constructor with the 5 parameters could
>> be remove when we introduce a constructur
>> with almost the same signature just leave the port away. But i am not
>> sure if intrudicing another constructor is worth the
>> -1 removale in the javadoc.
>>
>> If it is not worth it, the -1 reference can at least be removed in the 2
>> and 3 parameter constructors javadoc. I will prepare this.
>>
>> What do you think?
>
> I do not think this is worth pursuing.
>
> -Chris.
>
>> -- Sebastian
>>> Another unfortunate mistake.
>>>
>>>> a more generalized description for MalformedURLException  could be
>>>> used, e.g.
>>>>
>>>> if the parsed URL fails to comply with the scheme specific syntax of
>>>> the associated protocol.
>>>
>>> Is this suggested wording for the “spec” accepting constructors?  I
>>> think what we have for the constructors accepting protocol, host,
>>> port, etc, is more accurate.
>>>
>>> -Chris.
>>>
>>>> regards
>>>> Mark
>>>>
>>>>
>>>>
>>>> On 10/09/2015 20:32, Sebastian Sickelmann wrote:
>>>>> Hi,
>>>>>
>>>>> first thanks to Chris and David for their helpful input . I looked
>>>>> through the existing
>>>>> Testcases and found one that is already testing for negative-port
>>>>> numbers.
>>>>> So i extended the @bug line with "4906983" which I hope is the right
>>>>> solution to do it.
>>>>>
>>>>> I am with Chris, when he says normally you only have numbers between
>>>>> 1 and
>>>>> 65535 (because many protocols are using tcp). So i changed to
>>>>> documentation as
>>>>> Chris suggested it.
>>>>>
>>>>> But ports above this "natural" barrier are valid too. It depends on
>>>>> the protocol what
>>>>> to do with the port information. So I also extended the testcase to
>>>>> check that their are
>>>>> valid port numbers also above 65535 and the special -1.
>>>>>
>>>>> But i asked myself should
>>>>> new URL("http://server:-1/path");
>>>>> be realy a valid URL? What do you think?
>>>>>
>>>>> Special thanks to David who hosted the new webrev:
>>>>> http://cr.openjdk.java.net/~dbuck/4906983.1/
>>>>>
>>>>> -- Sebastian
>>>>>
>>>>>
>>>>> Am 10.09.2015 um 12:38 schrieb Chris Hegarty:
>>>>>> Another minor comment...
>>>>>>
>>>>>> While what you have suggested is not incorrect, I’m afraid it is
>>>>>> giving the wrong impression about the typical acceptable port
>>>>>> ranges. A port of Integer.MAX_VALUE is not all that useful, since
>>>>>> it typically maps to a TCP port number ( but not always ). Maybe
>>>>>> just remove the valid values from @param port, and add something
>>>>>> like the following to MalformedURLException: “.., or the port is
>>>>>> a negative number other than -1” ?
>>>>>>
>>>>>> -Chris.
>>>>>>
>>>>>> On 10 Sep 2015, at 11:13, Chris
>>>>>> Hegarty<chris.hegarty at oracle.com>  wrote:
>>>>>>
>>>>>>> On 8 Sep 2015, at 21:01, Sebastian
>>>>>>> Sickelmann<sebastian.sickelmann at gmx.de>  wrote:
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please find my small patch[1] to javadoc in java.net.URL that
>>>>>>>> adresses
>>>>>>>> JDK-4906983(javadoc-fix)[2].
>>>>>>>>
>>>>>>>> I signed the SCA/OCA some time ago. Feel free to check at the OCA
>>>>>>>> Signatures-List[3]
>>>>>>>>
>>>>>>>> thanks to david buck for hosting this patch
>>>>>>>> oncr.openjdk.java.net <http://cr.openjdk.java.net>.
>>>>>>>>
>>>>>>>> -- Sebastian Sickelmann
>>>>>>>>
>>>>>>>> [1]http://cr.openjdk.java.net/~dbuck/4906983.0/
>>>>>>> Just to confirm this is a spec only change, that documents long
>>>>>>> standing existing behaviour, right?
>>>>>>>
>>>>>>> I think we should add a minimal testcase to cover this.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> -Chris.
>>>>>>>
>>>>>>>> [2]https://bugs.openjdk.java.net/browse/JDK-4906983
>>>>>>>>
>>>>>>>> [3]http://www.oracle.com/technetwork/community/oca-486395.html
>>>>>
>>>>
>>>
>>
>