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

[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
>>>>
>>>
>>
>