[RFC][icedtea-web] Plugin doesn't halt when required elements from JNLP files are missing.

[Jiri Vanek]

TitleVendorParserOn 03/13/2012 05:47 PM, Danesh Dadachanji wrote:
> On 12/03/12 12:21 PM, Danesh Dadachanji wrote:
>>
>>
>> On 12/03/12 07:26 AM, Jiri Vanek wrote:
>>> On 03/12/2012 12:22 PM, Pavel Tisnovsky wrote:
>>>> Jiri Vanek wrote:
>>>>> On 03/12/2012 11:50 AM, Pavel Tisnovsky wrote:
>>>>>> Jiri Vanek wrote:
>>>>>>> On 03/09/2012 09:32 PM, Danesh Dadachanji wrote:
>>>>>>>> On 11/01/12 02:18 PM, Danesh Dadachanji wrote:
>>>>>>>>> On 11/01/12 11:31 AM, Jiri Vanek wrote:
>>>>>>>>>> On 01/06/2012 08:08 PM, Danesh Dadachanji wrote:
>>>>>>>>>>> On 06/01/12 12:37 PM, Jiri Vanek wrote:
>>>>>>>>>>>> On 01/06/2012 05:44 PM, Danesh Dadachanji wrote:
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Looking at the plugin docs[1],<title> and<vendor> are required
>>>>> snip
>>>>>>>>
>>>>>>>> diff --git a/netx/net/sourceforge/jnlp/Parser.java
>>>>>>>> b/netx/net/sourceforge/jnlp/Parser.java
>>>>>>>> --- a/netx/net/sourceforge/jnlp/Parser.java
>>>>>>>> +++ b/netx/net/sourceforge/jnlp/Parser.java
>>>>>>>> @@ -504,6 +504,11 @@ class Parser {
>>>>>>>> child = child.getNextSibling();
>>>>>>>> }
>>>>>>>>
>>>>>>>> + if (info.getTitle() == null || info.getTitle().equals(""))
>>>>>>>> + throw new ParseException(R("PNoTitleElement"));
>>>>>>>> + if (info.getVendor() == null || info.getVendor().equals(""))
>>>>>>>> + throw new ParseException(R("PNoVendorElement"));
>>>>>>>> +
>>>>>>>> return info;
>>>>>>>> }
>>>>>>>>
>>>>>>> I would suggest "".equals(info.get*().trim()) instead of
>>>>>>> info.get*().equals("")
>>>>>>> Usage of trim i straight forward and I think it is necessary.
>>>>>>> The "".equals instead of string.equals("") really just minor nitpick
>>>>>>> :) "".equals(null) returns false instead of NUllPointerException.
>>>>>>> I do
>>>>>>> not believe trim() will ever return null, but it is good habit (IMHO
>>>>>>> O:) )
>>>>>>> If no need for trim will be here, then just "".equals(info.get*())
>>>>>>> will
>>>>>>> be enough instead of both conditions.
>>
>> I would still need 2 conditions because I want the if to pass when
>> info.get*() is null. That check is not to look out for a NPE, it's the
>> opposite. If it's null, the tags aren't there! So I'd still need both.
>>
>> Also, I remember thinking about this case before posting the patch. It
>> already trims it before saving vendor into the info var. =) However I
>> think you're right, it should still be there in case somewhere down the
>> line, the trim before saving is removed.
>>
>>>>>>
>>>>>> String.trim() does not return null so you don't need to worry
>>>>> Yy - as I have already said ;) but STILL it is good habit O:)
>>>>>>
>>>>>> Then you could use predicate isEmpty() which is IMHO more readable
>>>>>> than equals("") (since 1.6)
>>>>> isEmpty do not trim :((( => info.get*() == null ||
>>>>> info.get*().trim().isEmpty() ... :D (if we can live with method which
>>>>> appeared in 1.6)
>>>> Yeah I mean info.get*().trim().isEmpty()
>>>> (btw I think we could end with this basic stuff ;-)
>>>>>
>>>>> But this leads me to question - which javaSE version is icedtea-web
>>>>> comaptible with and which _should_ be compatible with?
>>>>
>>>> I assume it should be compatible with 1.6 and 1.7
>>> Yap, I guess the same... Poor mainframes with ibm 1.4.2 are out of luck
>>> again ;)
>>>> (and I'm pretty sure that
>>>> the code already contains some 1.6-related code ;-) which could be easy
>>> Although i do not KNOW about it I BELIEVE to it too !-) ..and I think
>>> there is no need to keep older javas supported anyway...
>>>> to check using --source and --target)
>>> Ah Yah!
>>
>> I think it's safe to use this here. I will definitely use isEmpty() then!
>>
>> Updated patch coming shortly. Thanks again for all the comments!
>
> I ended up using get*() == null || get*().trim().isEmpty() for the condition. As mentioned before, null needs to be checked against because this implies the tag is missing altogether. If the trimmed return is empty, that means the tag was there but contained 0 or more white spaces. I figured having trim here would be a good check in case Parser was ever changed. Currently, the values are trimmed and then stored but if this changes in the future, this will catch it.
>
> I've added 2 more tests, one with an information tag missing and another with both, title and vendor tags missing.
>
> Again, this test will not work without the LaunchHandler patch in review[1], however I've used String.matches() to account for any implementation that results in [1]'s outcome.
>
>

Yap. I think this can go in now with minor fix - In changelog you have mentioned   + * tests/jnlp_tests/simple/InformationTitleVendorParser/srcs/TitleVendorParser.java, but this one is not going in (as you are using simpletest1 and the file is not present in patch), Ensure also if this  InformationTitleVendorParser/srcs dir is really empty in your testing builds (if isn't ensure all is working when emptied).
btw - spaces in changelog instead of tabs, although I suspect email clients to did so, please check before push.

Thanx for test and after minor issue being fixed, please push.

J.

> ChangeLog:
> +2012-03-13 Danesh Dadachanji <ddadacha at redhat.com>
> +
> + Applications using JNLP files without a title or vendor section
> + still run, despite them being required elements.
> + * netx/net/sourceforge/jnlp/Parser.java:
> + (getInformationDesc): If title or vendor are not found in info,
> + a new ParseException is thrown.
> + * netx/net/sourceforge/jnlp/resources/Messages.properties:
> + Added PNoTitleElement and PNoVendorElement
> + * tests/jnlp_tests/simple/InformationTitleVendorParser/resources/InformationParser.jnlp,
> + * tests/jnlp_tests/simple/InformationTitleVendorParser/resources/TitleParser.jnlp,
> + * tests/jnlp_tests/simple/InformationTitleVendorParser/resources/TitleVendorParser.jnlp,
> + * tests/jnlp_tests/simple/InformationTitleVendorParser/resources/VendorParser.jnlp,
> + * tests/jnlp_tests/simple/InformationTitleVendorParser/srcs/TitleVendorParser.java,
> + * tests/jnlp_tests/simple/InformationTitleVendorParser/testcases/TitleVendorParserTest.java:
> + New test that runs JNLPs in a combination of missing information, title
> + and vendor tags, checking for the appropriate exceptions.
> +
>
> Cheers,
> Danesh
>
> [1] http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2012-March/017666.html