RFR 4906983: java.net.URL constructors throw MalformedURLException in undocumented way
Sebastian Sickelmann
sebastian.sickelmann at gmx.de
Mon Sep 14 15:45:19 UTC 2015
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.
I think the constructors with the "spec" parameters are the only one
that can be
changed without much damage. But you are right the risk is to big for
the very
little value. So i will not start another thread about this.
>
>> 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.
I removed the line and my comment. Do you think i should add it back as
comment to the test-case?
// As discussed in
//
http://mail.openjdk.java.net/pipermail/net-dev/2015-September/009138.html
// and the follow up thread. This is a very unusual way to
specify the use
// of the default port. Sadly it is possible but the risk of
breaking things is to high
// for the value a change would offer.
// url = new URL("http://server:-1/path"); // Is this realy a
valid one?
>
>>>> 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.
Here is the new webrev-url:
http://cr.openjdk.java.net/~dbuck/4906983.2/
Thanks to all for the review and all other support.
-- Sebastian
>
>> -- 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
>>>>>
>>>>
>>>
>>
>
More information about the net-dev
mailing list