Please use the Apache issue tracking system for new NetBeans issues (https://issues.apache.org/jira/projects/NETBEANS0/issues) !!
Bug 53058 - add support for lookup filtering
add support for lookup filtering
Status: CLOSED FIXED
Product: platform
Classification: Unclassified
Component: Lookup
4.x
All All
: P2 (vote)
: 4.x
Assigned To: Jaroslav Tulach
issues@platform
: API
Depends on: 205151
Blocks: 53733
  Show dependency treegraph
 
Reported: 2005-01-06 13:50 UTC by David Konecny
Modified: 2011-12-07 21:07 UTC (History)
6 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
Lookups.exclude method and implementation (20.15 KB, patch)
2005-01-06 18:37 UTC, Jaroslav Tulach
Details | Diff
Jesse's test and implementation (8.99 KB, patch)
2005-01-19 21:43 UTC, Jaroslav Tulach
Details | Diff
Implementation based on the concept of barrier (11.86 KB, patch)
2005-01-21 09:51 UTC, Jaroslav Tulach
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Konecny 2005-01-06 13:50:07 UTC
Usecase: freeform project merges lookups provided
by project natures. It has to filter out some
instances and replaced them with different ones,
e.g. project's lookup must have only one
ClasspathProvider and if two natures both provide
a ClasspathProvider then they must be taken out
and replaced with one.

After talking with Yarda about this he suggested
API addition:

  Lookup Lookups.filter(Lookup, Class[])

which would from the given lookup filter out all
instances of the given classes.

Yarda also said that in the past similar usecase
was presented couple of times.
Comment 1 Jaroslav Tulach 2005-01-06 16:42:23 UTC
I know it has been used in core/windows and maybe once by Jesse. I
plan to provide such impl for 4.1
Comment 2 Jaroslav Tulach 2005-01-06 18:36:59 UTC
Reviewers, please review.
Comment 3 Jaroslav Tulach 2005-01-06 18:37:45 UTC
Created attachment 19534 [details]
Lookups.exclude method and implementation
Comment 4 Milos Kleint 2005-01-07 08:09:14 UTC
looks good, I had a usecase for filtering out ActionMap from the
lookup when merging multiview elements..
Comment 5 Jesse Glick 2005-01-10 18:35:19 UTC
Sounds good to me. I know I've needed this at least once (though I
can't now remember where).
Comment 6 Jaroslav Tulach 2005-01-12 10:28:30 UTC
I'll put it in by the end of week. 
Comment 7 Jaroslav Tulach 2005-01-14 13:14:45 UTC
cvs -q ci -m "#53058: Lookups.exclude for easier filtering out of
instances of certain type"

Milos, David please try to use this api.


Checking in openide-spec-vers.properties;
/cvs/openide/openide-spec-vers.properties,v  <-- 
openide-spec-vers.properties
new revision: 1.165; previous revision: 1.164
done
Processing log script arguments...
More commits to come...
Checking in api/doc/changes/apichanges.xml;
/cvs/openide/api/doc/changes/apichanges.xml,v  <--  apichanges.xml
new revision: 1.232; previous revision: 1.231
done
Processing log script arguments...
More commits to come...
Checking in src/org/openide/util/lookup/AbstractLookup.java;
/cvs/openide/src/org/openide/util/lookup/AbstractLookup.java,v  <-- 
AbstractLookup.java
new revision: 1.47; previous revision: 1.46
done
RCS file: /cvs/openide/src/org/openide/util/lookup/ExcludingLookup.java,v
done
Checking in src/org/openide/util/lookup/ExcludingLookup.java;
/cvs/openide/src/org/openide/util/lookup/ExcludingLookup.java,v  <-- 
ExcludingLookup.java
initial revision: 1.1
done
Checking in src/org/openide/util/lookup/Lookups.java;
/cvs/openide/src/org/openide/util/lookup/Lookups.java,v  <--  Lookups.java
new revision: 1.11; previous revision: 1.10
done
Processing log script arguments...
More commits to come...
RCS file:
/cvs/openide/test/unit/src/org/openide/util/lookup/ExcludingLookupTest.java,v
done
Checking in
test/unit/src/org/openide/util/lookup/ExcludingLookupTest.java;
/cvs/openide/test/unit/src/org/openide/util/lookup/ExcludingLookupTest.java,v
 <--  ExcludingLookupTest.java
initial revision: 1.1
Comment 8 Jesse Glick 2005-01-14 17:47:56 UTC
Can DefaultTopComponentLookup.NoNodeLookup be replaced by the new
method? In the case of DTCL.NNL, you have a Node[] each of which has a
Lookup, and you want to make a ProxyLookup based on the supplied
Lookup[] yet exclude all the nodes. Specifically, NNL rejects any
attempt to look up Node.class, and also any lookup attempt which would
return one of the Node[] members. Will Lookups.exclude omit any
instances which are assignable to one of the exclude classes, even if
the caller was not asking for one of those classes? It seems that it
will, but its Javadoc is too vague to tell for sure.
Comment 9 David Konecny 2005-01-19 14:11:11 UTC
Issue 53653 discovered following problem: if a class in lookup
implements two interfaces and you Lookup.excludes() first one the
second one is excluded too.
Comment 10 Jaroslav Tulach 2005-01-19 16:01:43 UTC
The exclusion is not done on classes, but on instances. If an instance
implements interface that is supposed to be excluded, it is excluded.

Every instance implements java.lang.Object, does that mean that I
should never exclude anything if Object.class is not one of the
excluded classes?

If you can better specify what you what (the best by a written test),
I am willing to do something with it, but at first sight your request
seems to be pretty strange (as described in the above paragraph).
Comment 11 David Konecny 2005-01-19 16:47:22 UTC
"The exclusion is not done on classes, but on instances. If an instance
implements interface that is supposed to be excluded, it is excluded."
- yes and I do not think it is correct. If the instance implements
also some other interface then that interface is excluded too. And
that's unexpected. And as a client of this API I have no control over
how providers implemented their instances.

So from this point of view I cannot use your method safely. Is it
clear now? Writting test should be easy and can try to do that. But
later, at the moment fixing some last before-feature-freeze issues.
Comment 12 Jesse Glick 2005-01-19 17:47:46 UTC
I guess what David means is that

interface A {}
interface B {}
class C implements A, B {}
Object c = new C();
Lookup l1 = Lookups.singleton(c);
Lookup l2 = Lookups.exclude(l1, new Class[] {A.class});
assertNull(l2.lookup(A.class));
assertEquals(c, l2.lookup(B.class));

The above is what I would expect the behavior to be. The current
semantics seems very odd, akin to getting all instances from the
original lookup (template: Object.class), removing any that are
instanceof one of the specified classes, then making a Lookups.fixed
from the result.

This would be a good time to comment that the semantics of Lookup have
always been needlessly complicated and stupid. If it were simply
redefined to behave something like Map<Class,LookupList> (where
LookupList extends List and supports LookupListener), with providers
responsible for registering instances under whichever Class keys they
wanted to register, things like filtering would be trivial and there
would be no confusion over what it meant - you would simply remove
some map entries.
Comment 13 Jaroslav Tulach 2005-01-19 18:18:09 UTC
To David: If the case will not be provide soon, the probability of the
fix is decreasing.

To all: if you think that the implementation should be reverted until
fixed as you wish to see it fixed. Say so.
Comment 14 Jaroslav Tulach 2005-01-19 21:43:00 UTC
Created attachment 19796 [details]
Jesse's test and implementation
Comment 15 Jaroslav Tulach 2005-01-19 21:45:54 UTC
The attached diff shows what happend when I added Jesse's test -
implementation was simpliefied a lot, but I had to remove two tests as
now it is possible (and as far as I understand desirable) to get
Number/Serializable when numbers are excluded and one asks for an Object.

I just have to add that this seems pretty strange behaviour to me.
Comment 16 Jesse Glick 2005-01-19 22:35:04 UTC
I can imagine either semantics being useful in different
circumstances; the question is which semantics (or both?) are desired
for the immediately known use cases. I guess that's for David to clarify.
Comment 17 David Konecny 2005-01-20 11:33:28 UTC
Yes, Jesse's test case is exactly what is needed and how I was
expecting this to work. Thanks.

Yarda, could you please explain why do you think it is strange
behaviour? I'm asking because I still do not understand in which cases
your semantics (i.e. filtering our instances) is useful. You can
safely remove concrete instance from lookup only when you are provider
of the lookup and you put such an instance there yourself. Otherwise
you may remove too much.
Comment 18 Milos Kleint 2005-01-20 12:04:24 UTC
agree with jesse and david. as a user of lookup I don't care about the
actual object instance, but about the interfaces and classes provided
by the lookup.
Comment 19 Jaroslav Tulach 2005-01-20 12:09:29 UTC
The best examples of what is broken in current impl are the tests I
needed to comment out. Here is simpler desirable version:

class A {}
Object a = new A();
Lookup l1 = Lookups.singleton(a);
Lookup l2 = Lookups.exclude(l1, new Class[] {A.class});
assertNull(l2.lookup(A.class));
assertNull(l2.lookup(Object.class)); 

The filtering is broken in the last fix. The second lookup returns a.
Comment 20 Milos Kleint 2005-01-20 12:19:23 UTC
I firmly belive that it correctly returns an instance.

I have 2 unrelated interfaces and filter one of them.
if these are implemented by 2 distinct classes then I will get a
result for the second interface and when it's "by accident"
implemented by the same class, then I don't get it.
That way an implementation detail on the lookup provider side will
cause  "random" results from the lookup. If just the class/interface
in question is filtered out, you get more consistent and predictable
results. IMHO at least.
Comment 21 David Konecny 2005-01-20 12:50:08 UTC
"The second lookup returns a" - but is not that OK? You did not tell
lookup to filter out Object.class so it should be returned.
Comment 22 Jaroslav Tulach 2005-01-20 14:38:33 UTC
To milos: I do not know what your comment is related to. Definitively
not my post from "2005-01-20 04:09 PST" as you are refering to
unrelated interfaces, but in my post there is nothing unrelated. A is
subclass of Object.

To david: It is not ok. As then the returned result for Object would
not be superset of the returner result for A. All other implementation
of Lookup I am aware of fulfill this and also the lookup's idol - jini
does the same. There even used to be a time when Jesse requested this
superset relation. See issue 29851.
Comment 23 Jesse Glick 2005-01-20 16:26:00 UTC
I think the question revolves around whether anyone actually uses the
superclass/superset property of most Lookup implementations. I know we
used to use it occasionally for service types, but we don't do that
any more. Yes it is true in issue #29851 that it would be useful to
find all instances in a Lookup - for diagnostic code like apisupport,
not for regular functionality - but I only asked for
Lookup.Template(Object) to do this because that was the only way to
get it. If lookup behaved more like a map, that would certainly be
preferable - could just ask for entrySet(). Really I can't think of
any remaining use cases for this obsolete property.

I agree with Milos' comment that the current implementation is
potentially dangerous since it can hide useful instances for no
apparent reason. Let's say I have two widely separated pieces of code
(different modules, etc.) joined by a common API:

public interface A {void a();}
public interface B {void b();}
// ------------------------
public static Lookup services() {
    return Lookups.fixed(new Object[] {
        new A() {public void a() {}},
        new B() {public void b() {}},
    });
}
// ------------------------
public static Lookup servicesWithProxyB() {
    final Lookup orig = services();
    final B bProxy = new B() {
        public void b() {
            for (B delegate : orig.lookup(new Lookup.Template(B.class))) {
                delegate.b();
            }
        }
    };
    Lookup plusProxy = new ProxyLookup() {
        {
            setLookups(new Lookup[] {
                Lookups.exclude(orig, new Class[] {B.class}),
                Lookups.singleton(bProxy),
            });
        }
    };
    return plusProxy;
}
// ---------------------

Pretty routine stuff. And in the above code, it will work. However, if
the original lookup provider makes a harmless-looking implementation
change to save some inner classes:

public static Lookup services() {
    class AB implements A, B {
        public void a() {}
        public void b() {}
    }
    return Lookups.singleton(new AB());
}

then suddenly servicesWithProxyB().lookup(A.class) starts returning
null instead of the desired implementation! This is quite unexpected
to the author of services(). This example is not hypothetical, it is
based on my understanding of the bugs we encountered soon after using
the new Lookups.exclude method, but David please confirm.

So at this point, unless new information comes up, I would recommend
replacing the existing method implementation with the revised version
passing the new tests, and make sure the Javadoc is explicit that
filtering applies to queries, not to instances.
Comment 24 David Konecny 2005-01-20 20:36:25 UTC
Issue 53653 confirms what Jesse says. web/freeform module put into
lookup one instance which implements both ClasspathProvider and
WebModuleProvider. ant/freeform module needs to exclude
ClasspathProvider and replace it with a merged one, BUT side effect of
current implementation of Lookups.exclude is that WebModuleProvider is
excluded from lookup too which results in lot of unexpected problems.
Comment 25 Jaroslav Tulach 2005-01-21 09:42:33 UTC
Ok, the current implementation shall be treated as a bug. Either fixed
or removed for 4.1.
Comment 26 Jaroslav Tulach 2005-01-21 09:50:08 UTC
I'd like to thank Jesse, David and Milos for your valuable user
feedback. Your voice of customer shall never be forgotten. 

I've spent two restless nights thinking about proper way of
implementing your voc, and I believe I found implementation that
satisfies all previously existing testcases (except one which
contradicts your requirement, but that is ok) and also the test case
provided by Jesse. Moreover it has the property of returning superset
results for superclass queries. Both of this satisfies me and I am
sure we are getting closer to the solution of our problems.

I'll attach diff, please verify that it works for you, then I'll
updated javadoc to give at least a bit of insight to the algorithm and
integrate the change. 

Btw. The implementation can be made faster if necessary. It uses depth
first search, which could be replaced with the "wide first" one and
also cache of isAccessible results might be added, if necessary. But
as pnejedly says, this can wait until we find we really need it.
Comment 27 Jaroslav Tulach 2005-01-21 09:51:52 UTC
Created attachment 19851 [details]
Implementation based on the concept of barrier
Comment 28 David Konecny 2005-01-21 10:38:28 UTC
If Jesse's test passes then go ahead and put it to trunk. I will fix
then issue 53733 and that should also fix one failing test under
ant/freeform.

<sidenote>
I did not look at your impl much. From what you wrote it sounds to me
like the impl is trying to be too smart, trying to solve all usecases?
If that's true then I do not think it is right. It increases
complexity of code, makes maintanance harder and all that for what? to
support a usecase which may never be used? Is it worth it? I would
prefer straightforward and simple solution for the usecase we have.
</sidenote>
Comment 29 Jaroslav Tulach 2005-01-22 13:34:51 UTC
I smell defensive enginering approach from your side note. Ok, but
instead of trying to educate me you could invest the time to verify
that the solution works. That would be more valuable and probably more
engineering way of doing the integration. 

I am all for usecases based work, but the fact that you have usecase
for "1+1 to be 3" does not force me and hopefully anyone with a bit of
mind to try to change the way how natural numbers work.

As I wrote above, thank you for your voc, that was valuable, but keep
the rest, that just misses the forest for the trees.
Comment 30 Jaroslav Tulach 2005-01-22 15:11:27 UTC
Integrated.

Checking in src/org/openide/util/lookup/ExcludingLookup.java;
/cvs/openide/src/org/openide/util/lookup/ExcludingLookup.java,v  <-- 
ExcludingLookup.java
new revision: 1.2; previous revision: 1.1
done
Checking in src/org/openide/util/lookup/Lookups.java;
/cvs/openide/src/org/openide/util/lookup/Lookups.java,v  <--  Lookups.java
new revision: 1.12; previous revision: 1.11
done
Processing log script arguments...
More commits to come...
Checking in
test/unit/src/org/openide/util/lookup/ExcludingLookupTest.java;
/cvs/openide/test/unit/src/org/openide/util/lookup/ExcludingLookupTest.java,v
 <--  ExcludingLookupTest.java
new revision: 1.2; previous revision: 1.1
Comment 31 David Konecny 2005-01-23 21:03:51 UTC
I'm not trying to educate you - that was just my honest opinion.

Re. verifying - as I said Jesse's testcase exactly covers my usecase.
If it passes then it works.

Re. "1+1 to be 3" - what are you talking about? Nobody force you to
anything. You mentioned JINI and it made clear to me what was your
motivation and why you implemented the method the way you did. But I
guess our usage of Lookup is a bit different or is shifting during the
time? Whether it is OK or not I do not know. But according to comments
in this issue for other people is this non-JINI behaviour also
natural. So, what we will do with that? Trying to satisfy both
contracts is IMO way to hell.

I can verify that your fix resolved issue 53733.
Comment 32 Jaroslav Tulach 2005-01-24 03:05:46 UTC
"Is the lookup impl shifting during the time?" - No, the constraint
that if A.class.isAssignableFrom(B.class) then lookup.lookup(new
Template (A.class)) is superset of lookup.lookup(new Template
(B.class)) has been present in every implementation I know of -
AbstractLookup, ProxyLookup, meta-inf & NodeLookup (dynamically, but
once is there, it holds), TopComponentLookup, etc. That is why I
object about breaking it just due to issue 53733, as nobody is going
to change 1+1=2 even it may from time to time seem handy. 

It is imho better to choose a solution that keeps natural expecations
and satisfies new expectations, if such exists. Which is what happened.
Comment 33 Jesse Glick 2005-01-24 21:27:32 UTC
Re. "if A.class.isAssignableFrom(B.class) then lookup.lookup(new
Template (A.class)) is superset of lookup.lookup(new Template
(B.class))" - pleasant enough if it happens to be true, but not really
needed by anyone. The only case in all our APIs where I can think of a
public subinterface you could look up is EditorCookie.Observable.
There is not even much reason to look up that subinterface; you would
normally look up EditorCookie and cast to EC.O in case you got one and
you wanted to listen to it. But we have this huge body of lookup code,
which is rather difficult to understand and optimize, trying to make
sure that

Object instance = new EC.O() {...};
Lookup l = Lookups.fixed(instance);
assertNotNull(l.lookup(EC.class));
assertNotNull(l.lookup(EC.O.class)); // who does this??

when we could instead simply require the provider to declare what they
are providing:

Lookup l = Lookups.fixed2(instance,
                          {EC.class, /*optionally*/EC.O.class});
assertNotNull(l.lookup(EC.class));
assertNotNull(l.lookup(EC.O.class)); // probably bad form
Comment 34 Jaroslav Tulach 2005-01-25 06:20:26 UTC
"pleasant enough if it happens to be true", but dangerous if it gets
broken. I know about one case when one asks for superinterface and
expects correct values for subinterfaces - NodeLookup. It is true,
that it only listens on Node.Cookie and does not query it, but still
it signals that if superinterface deleation got broken certain parts
of our code would stop to work.
Comment 35 Jesse Glick 2005-01-25 22:14:08 UTC
So what about NodeLookup? I guess you just mean that Node.getCookie
has a similar contract. But it is even less important for getCookie
because if B extends A and n.getCookie(B.class) == bInstance you
cannot assume n.getCookie(A.class) == bInstance as well. It might be a
different instance of some other subinterface of A. The getCookie
Javadoc doesn't even discuss this issue except to say that the return
value must be assignable to the type; an impl would be free to have

n.getCookie(A.class) == null
n.getCookie(B.class) == bInstance

or

n.getCookie(A.class) == bInstance
n.getCookie(B.class) == null

or whatever it likes. I know CookieSet behaves in the way you like,
but no one is forced to use CookieSet, it is just a convenient default
impl you can override.

BTW for the sake of concreteness I posted my thoughts on nbdev; would
probably be best to resume the discussion there since this issue is
closed.
Comment 36 Jaroslav Tulach 2005-01-26 13:30:11 UTC
Comments on nbdev are fine, I just do not have much to say to them,
but as I love to have the last word, here is explanation of NodeLookup
problem: It does not matter much whether the behaviour of getCookie
can get broken - the problem is that if you use new Node (yourLookup)
the node starts to listen on yourLookup.lookup
(Template(Node.Cookie.class)) and expects notification of changes in
all subclasses. This contradicts your proposal that there is no
relation between superclass results and subclass results.
Comment 37 Jesse Glick 2005-01-26 18:25:03 UTC
Re. Lookup.Template(Node.Cookie.class) - sure, this would need the
superclass semantics. But this is just a b/c hack, not a real use case.
Comment 38 Jaroslav Tulach 2005-01-27 07:17:23 UTC
It is not hack. It is one of the original usecases.


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2014, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo