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

[Danesh Dadachanji]

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.


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: title-vendor-required-04.patch
Type: text/x-patch
Size: 15150 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120313/2ba14993/title-vendor-required-04.patch