Bug 200711 - LookupMerger works incorrectly when nested
LookupMerger works incorrectly when nested
Status: RESOLVED FIXED
Product: projects
Classification: Unclassified
Component: Generic Infrastructure
7.1
All All
: P3 (vote)
: 7.3
Assigned To: Milos Kleint
issues@projects
: API, API_REVIEW_FAST
Depends on:
Blocks: 200500
  Show dependency treegraph
 
Reported: 2011-08-05 17:46 UTC by Jesse Glick
Modified: 2012-11-08 10:37 UTC (History)
1 user (show)

See Also:
Issue Type: DEFECT
:


Attachments
Removal of workaround, and introduction of failing test (3.75 KB, patch)
2011-08-05 17:48 UTC, Jesse Glick
Details | Diff
Revised patch, after refactoring (3.32 KB, patch)
2011-08-05 18:11 UTC, Jesse Glick
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Glick 2011-08-05 17:46:07 UTC
A merged object can be run on another instance of the same merged class, but the duplicates are not correctly removed.
Comment 1 Jesse Glick 2011-08-05 17:48:33 UTC
Created attachment 109821 [details]
Removal of workaround, and introduction of failing test
Comment 2 Jesse Glick 2011-08-05 18:11:10 UTC
Created attachment 109822 [details]
Revised patch, after refactoring
Comment 3 Jesse Glick 2011-08-05 19:09:14 UTC
Not clear yet exactly what is wrong, but the use of ProxyLookup is making DelegatingLookupImpl rather complicated. Possible fix is to directly implement Lookup and intercept all its calls. Would be easier to do the proxying in this way, though then need to do extra work to handle listening on results.
Comment 4 Milos Kleint 2012-09-24 09:41:03 UTC
one possibly important feature of the current impl in maven support is that the lookups are not naturally ordered.

basic non declarative -> per-packaging declarative lookup -> generic declarative for maven support

IMHO it would be wrapped the other way round
 basic -> generic -> packaging
Comment 5 Jesse Glick 2012-09-24 14:24:12 UTC
A reordering would make intuitive sense. IIRC I tried it and ran into some problems and had to back out. But I do not recall details now.
Comment 6 Milos Kleint 2012-09-24 20:01:05 UTC
it appears that the problem with reverting to base->packaging is the
testPackagingTypeSpecificLookup test in NbMavenProjectImplTest.  
I'm not sure what the problem is but it's most likely related to
@LookupMerger.Registration. When I changes that to regular
@ProjectServiceProvider (and hack the annotation processor that prohibits it)
the test finishes fine (with some tweaking of the expected array string format,
the nested mergers add another level of brackets there. I suppose the test
would also fail in original state if the lookupMerger.registration would happen
on packaging, rather than base lookup.

intuitively my understanding how mergers should work on this level is that 
1. the inner lookup will collect the merger results and and for each create the
merged object
2. the merged object is the only one appearing to the outside, all merged
instance are hidden.
3. when wrapped into the outer layer, the merger from inner lookup is still
visible and found and again a merged object is created this time only wrapping
the inner merged object (and any other instances in outer lookup only)
Comment 7 Milos Kleint 2012-09-26 08:23:33 UTC
for maven projects I've worked around the problem of nesting. requires a new api method: LookupProviderSupport.createCompositeLookup(Lookup baseLookup, Lookup providers) where as providers parameter we pass a ProxyLookup with both the /maven and /maven/<jar> locations. That should get rid of the nesting and the failing tests work now.

http://hg.netbeans.org/core-main/rev/3c77a3e17dd9
Comment 8 Milos Kleint 2012-09-26 08:27:48 UTC
sorry, only now I've realized projectapi change is public and should go through the apireview process.

please review the change in http://hg.netbeans.org/core-main/diff/3c77a3e17dd9/projectapi/src/org/netbeans/spi/project/support/LookupProviderSupport.java

it's meant as replacement for nesting composite lookups which proved hard to achieve with all the lookupmergers around.
Comment 9 Jesse Glick 2012-09-26 15:53:28 UTC
apichanges.xml needed BTW.

Does testNestedComposites now pass?

This is a more powerful alternative to my originally proposed http://netbeans.org/bugzilla/attachment.cgi?id=109823&action=diff but please read bug #200500 comment #3 about possible constraints which may need documentation.
Comment 10 Milos Kleint 2012-09-26 18:11:07 UTC
(In reply to comment #9)
> apichanges.xml needed BTW.

thanks. http://hg.netbeans.org/core-main/rev/099000748234

> 
> Does testNestedComposites now pass?

Yes it does (and did before this change) I've rewritten the test though, I believe there was an error in the setup. In any case the current api change allows me to bypass the problem.

> 
> This is a more powerful alternative to my originally proposed
> http://netbeans.org/bugzilla/attachment.cgi?id=109823&action=diff but please
> read bug #200500 comment #3 about possible constraints which may need
> documentation.

Unrelated to the review, but..
Lookups are way too abstract for me, but my intuitive idea was that the inner delegate lookup would convert and process all the mergers correctly, providing the merged instance to the outside. Then the outer delegate would do the same, getting just the inner merger from the inner lookup and merging it with possible instances coming from the outer lookup. And in tests it worked, when I commented a restriction on @ProjectServiceProvider not to provide Mergers.
What I had problems with when debugging the code was the lazy meta mergers, that's where I believe the problem with nesting hides, but I could not find the problem for quite some time so I moved on..
Comment 11 Quality Engineering 2012-10-01 12:05:38 UTC
Integrated into 'main-golden', will be available in build *201210010929* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/3c77a3e17dd9
User: Milos Kleint <mkleint@netbeans.org>
Log: #200711 merge 2 project composite lookups into one, prevents problems with nesting and is overall simpler.


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