[rfc][icedtea-web][policyeditor] Parsing enhancements and unit tests

[Andrew Azores]

Well, I guess I could make one matcher, check if it matches, and then if not, make the other and check with it, but how much does it really matter... ? I think the intention is clearer, having the conjunction in the first if check. I doubt it will make much of a perfomance difference for most users, unless they end up with much more massive custom policy files than I'm expecting.

And of course the CustomPermission and PolicyEditorPermissions fromString() methods are "suspiciously" similar - they are both reading from the same source and attempting to parse into more or less the same end result. Why would they be really different? I guess the logic is mostly the same so they could be given a parent class or some utility method.

Thanks,

Andrew A

----- Original Message -----
From: "Jiri Vanek" <jvanek at redhat.com>
To: "Andrew Azores" <aazores at redhat.com>
Cc: "IcedTea" <distro-pkg-dev at openjdk.java.net>
Sent: Friday, March 7, 2014 8:24:09 AM
Subject: Re: [rfc][icedtea-web][policyeditor] Parsing enhancements and unit tests

The messages.properties changes are still not listed in changelog.


This is probably nit, but:
+        final Matcher targetMatcher = PolicyEditorPermissions.TARGET_PERMISSION.matcher(string);
+        final Matcher actionMatcher = PolicyEditorPermissions.ACTIONS_PERMISSION.matcher(string);
+        if (!targetMatcher.matches() && !actionMatcher.matches()) {
+            return null;
+        }
+
+        final String typeStr, targetStr, actionsStr;
+
+        if (actionMatcher.matches()) {
+            typeStr = actionMatcher.group(1);
+            targetStr = actionMatcher.group(2);
+            actionsStr = actionMatcher.group(3);
+        } else {
+        	typeStr = targetMatcher.group(1);
+            targetStr = targetMatcher.group(2);
+            actionsStr = "";
          }

the first matcher is needed only when second one fails. Maybe would be nice to remove the 
unnecessary matching.

similar for PolicyEditorPermissions which is still suspiciously similar:(


The chnage [\\w.] -> [\\w\\.]  looks enough :)



Thanx!
J.
On 03/06/2014 09:18 PM, Andrew Azores wrote:
> Thanks for review! Hopefully the new attached patch is nicer.
>
> Thanks,
>
> Andrew A
>
> ----- Original Message -----
> From: "Jiri Vanek" <jvanek at redhat.com>
> To: "Andrew Azores" <aazores at redhat.com>
> Cc: "IcedTea" <distro-pkg-dev at openjdk.java.net>
> Sent: Thursday, March 6, 2014 9:07:46 AM
> Subject: Re: [rfc][icedtea-web][policyeditor] Parsing enhancements and unit tests
>
> On 03/05/2014 06:01 PM, Andrew Azores wrote:
>> Hi,
>>
>> The previous thread was diverging into two quite different patches, so I'm splitting it.
>>
>> This patch improves parsing, especially handling of comments, and adds a lot of new unit testing for this. A few small bugs were found and fixed along the way.
>>
>
> The Messages.properties changes are missing in in-pathc chagelog
>
> public static CustomPermission fromString - more more and more tests. Try to hack yourself.  But generally ok.
> Well the comment is poem-writer's masterpiece :) But better then nothing :)
> The [\\w.] do not osund correct. The dot should be escaped to match dot, not any char. Or not? So this part will be:
> [[\\w\\.]+\\w+] (nottested - to match [java.][io.][permissions] and nto eg dsgfgogfdsgjsfdghjsfd[hj as now :)
> The content of quotes may be anything, ok? What about content to be qutes itself? (not jsut ${} eg?
> The first quotes are mandatory, and the scond not? As far as I read from regex.
> Why is compiled patter not global constant? It should be. It will not need recompile each launch time... Then there can be also some tests only to the regex itself.
>
>
> Why is the  CustomPermission.fromStirng copypasted to PolicyEditorPermissions ???
>
> Quick glance over tests is ook.
>
> Thanx!
>     J.
>