This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
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.
I know it has been used in core/windows and maybe once by Jesse. I plan to provide such impl for 4.1
Reviewers, please review.
Created attachment 19534 [details] Lookups.exclude method and implementation
looks good, I had a usecase for filtering out ActionMap from the lookup when merging multiview elements..
Sounds good to me. I know I've needed this at least once (though I can't now remember where).
I'll put it in by the end of week.
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
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.
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.
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).
"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.
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.
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.
Created attachment 19796 [details] Jesse's test and implementation
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.
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.
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.
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.
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.
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.
"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.
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.
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.
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.
Ok, the current implementation shall be treated as a bug. Either fixed or removed for 4.1.
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.
Created attachment 19851 [details] Implementation based on the concept of barrier
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>
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.
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
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.
"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.
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
"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.
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.
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.
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.
It is not hack. It is one of the original usecases.