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.
Summary: | LookupMerger works incorrectly when nested | ||
---|---|---|---|
Product: | projects | Reporter: | Jesse Glick <jglick> |
Component: | Generic Infrastructure | Assignee: | Milos Kleint <mkleint> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | mkleint |
Priority: | P3 | Keywords: | API, API_REVIEW_FAST |
Version: | 7.1 | ||
Hardware: | All | ||
OS: | All | ||
Issue Type: | DEFECT | Exception Reporter: | |
Bug Depends on: | |||
Bug Blocks: | 200500 | ||
Attachments: |
Removal of workaround, and introduction of failing test
Revised patch, after refactoring |
Description
Jesse Glick
2011-08-05 17:46:07 UTC
Created attachment 109821 [details]
Removal of workaround, and introduction of failing test
Created attachment 109822 [details]
Revised patch, after refactoring
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. 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 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. 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) 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 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. 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. (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.. 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. |