Created attachment 110707 [details]
Diff file

Please review

DK01 - thanks Tomas, looks good to me. The only thing I'm not sure about is whether WhiteListIndex is spi.whitelist.support or whether it would be more appropriate to be api.whitelist.index. It is API rather than SPI - the module builds index of whitelist violations and can return it to its clients like Java Editor or Web Project. My understanding of packages named "support" has always been that it is something optional which make API/SPI usage easier but everything could be as well achieve without it. In this case that's not true - spi.whitelist.support is the only way to access index.

DK01: For me it was rather spi.support as the callers were SPI implementors like TaskListProvider, HintProvider. But I have no problem move it into api as the border among API and SPI in this case is very thin. So the new package will be o.n.api.whitelist.index. I don't want to have it directly in o.n.api.whitelist as it's not the "core" API. Ideally it should be separated into 2 packages. The first index holding the WhiteListIndexListener, WhiteListIndexEvent and WhiteListIndex without the Map<? extends Tree, ? extends WhiteListQuery.Result> getWhiteListViolations(...) method as it's a support method. And support package having a support class with a single static method Map<? extends Tree, ? extends WhiteListQuery.Result> getWhiteListViolations(...). But it will introduce package with single class having just one static method.

I've changed it as I've described in the ideal solution.
Separated into 2 packages. The first index holding the WhiteListIndexListener,
WhiteListIndexEvent and WhiteListIndex without the Map<? extends Tree, ?
extends WhiteListQuery.Result> getWhiteListViolations(...) method as it's a
support method. And support package having a support class with a single static
method Map<? extends Tree, ? extends WhiteListQuery.Result>
getWhiteListViolations(...).

Created attachment 110745 [details]
Changed diff file

Thanks Tomas, looks OK to me.

In offline discussion with Tomas we have agree on two other small changes:

#1) add a variation of WhiteListIndex.getWhiteListViolations method which accepts whitelist ID:

public Collection<? extends Problem> getWhiteListViolations(
        @NonNull final FileObject root,
        @NullAllowed final FileObject file,
        @NonNull final String whitelistID) 
    throws IllegalArgumentException, UnsupportedOperationException {

Such method will be used in cases when client wants to query all problems from certain whitelist only.

#2) split current final Result class into two final classes to support merging of whitelist results. This will mean that existing public signature :

class Result {
  Result(boolean allowed, String violatedRuleName, String violatedRuleDesc)
  boolean isAllowed()
  String getViolatedRuleDescription()
  String getViolatedRuleName()
}

will change into:

class Result {
  Result(boolean allowed, List<RuleDescription> violatedRules)
  boolean isAllowed()
  List<RuleDescription> getViolatedRules()
}
class RuleDescription {
  RuleDescription(String ruleName, String ruleDesc, String whiteListID)
  String getRuleName()
  String getRuleDescription()
  String getWhiteListID()
}

Tomas please confirm that I got it right. Thanks.

Yes.
Just minor change from:

public Collection<? extends Problem> getWhiteListViolations(
        @NonNull final FileObject root,
        @NullAllowed final FileObject file,
        @NonNull final String whitelistID) 
    throws IllegalArgumentException, UnsupportedOperationException {

to:

public Collection<? extends Problem> getWhiteListViolations(
        @NonNull final FileObject root,
        @NullAllowed final FileObject file,
        @NonNull final String... whitelists) 
    throws IllegalArgumentException, UnsupportedOperationException {


And maybe the Result constructor:
Result(boolean allowed, List<RuleDescription> violatedRules)

should be splited into:

Result(List<RuleDescription> violatedRules) -> isAllowed false with given violations
Result() -> isAllowed true

So I am integrating the WhiteListIndex and WhiteListSupport (including the whitelists parameter in the getWhiteListViolations)

I've fille a separate issue for point 2 (WhiteListQuery.Result change) as it affects different class and it's incompatible change. The issue #202187

Fixed jet-main a7c56f42d43d

Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/a7c56f42d43d
User: Tomas Zezula <tzezula@netbeans.org>
Log: #202063:API review of a friend white list index

(In reply to comment #3)
> My understanding of packages named "support" has always
> been that it is something optional which make API/SPI usage easier but
> everything could be as well achieve[d] without it.

Right, that is the intention of the name. More formally, it should be possible to make a *.support package nonpublic by doing a copy refactoring of its contents (and any transitive nonpublic dependencies) into each module currently calling it. That is not the case in general for an API or SPI.

In reply to comment #14
The packaging is OK from this point of view. The WhiteListIndex is in o.n.a.whitelist.index and WhiteListSupport (holding only support method) is in o.n.a.whitelist.support.