Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 311598

Summary: Classpath container support for Module Assembly page classpath containers
Product: [WebTools] WTP Java EE Tools Reporter: Jason Sholl <jsholl>
Component: jst.j2eeAssignee: Jason Sholl <jsholl>
Status: RESOLVED FIXED QA Contact: Chuck Bridgham <cbridgha>
Severity: normal    
Priority: P3 CC: cbridgha, david_williams, jasonpet, larryisaacs, stryker
Version: 3.2Flags: david_williams: pmc_approved+
stryker: pmc_approved? (raghunathan.srinivasan)
stryker: pmc_approved? (naci.dai)
stryker: pmc_approved? (deboer)
stryker: pmc_approved? (neil.hauge)
stryker: pmc_approved? (kaloyan)
cbridgha: review+
Target Milestone: 3.2 RC1   
Hardware: PC   
OS: Windows XP   
Whiteboard: PMC_approved
Attachments:
Description Flags
patch
none
Additional patch to address NPE still present in VirtualReference stryker: iplog+

Description Jason Sholl CLA 2010-05-04 14:13:58 EDT 
Currently if a user assembles an EAR 5/6 project to include a classpath container mapped to the lib folder it is also necessary for the user to add that classpath container to each module's classpath.  This bug will remove that required user step by taking advantage of the EAR Libraries classpath container to add the appropriate classpath artifacts to each module's classpath.

Similarly assembling a classpath container to a Web project's WEB-INF/lib folder will now automatically resolve the classpath via the Web App Libraries classpath container instead of requiring an extra step by the user.

The changes in J2EEComponentClasspathContainer handle the EAR case.  The changes in ClasspathContainerVirtualComponent, FlexibleProjectContainer, and WebAppLibrariesContainer handle the Web case.  The changes in VirtualReference handle a NPE I encountered while testing the above with the ModuleAssemblies page and is unrelated to the other changes.
Comment 1 Jason Sholl CLA 2010-05-04 14:16:15 EDT 
Created attachment 167002 [details]
patch
Comment 2 Chuck Bridgham CLA 2010-05-04 14:59:28 EDT 
I approve this patch - Please add PMC review
Comment 3 Chuck Bridgham CLA 2010-05-04 15:02:09 EDT 
Rob can you also take a look?
Comment 4 Rob Stryker CLA 2010-05-05 08:29:54 EDT 
I haven't tested it, but by looking at the patch, it looks like it's doing 100% what I would expect it to do...  Specifically the changes look good and seem to appropriately handle both used and consumed scenerios in both classpath containers. 

It'd be nice to see some unit tests but I don't believe they're necessary for me to vote +1 on this, so, +1. ;)
Comment 5 Rob Stryker CLA 2010-05-05 08:46:25 EDT 
    * Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such. 

Jars (from consumed references or otherwise) which may be bundled and exported in one project are not currently properly added to the classpath of other related projects. 

    * Is there a work-around? If so, why do you believe the work-around is insufficient? 

The workaround is to manually add such jars to the classpath. This is very inefficient for the user and would require them adding a jar to assembly and then again adding it to the classpath. 

    * How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? 

I don't believe there are unit tests for this bug.

    * Give a brief technical overview. Who has reviewed this fix? 

Briefly, the classpath containers will now also scan the virtual components for other suitable jars and automatically add those to the classpath. This has been reviewed and approved by Chuck, and also reviewed by myself. 

    * What is the risk associated with this fix? 

I see little risk with this patch currently but I have not tested it thorougly so its possible some tiny defect shows through. As currently coded, it seems a well-written and proper patch for the behaviour of these classpath containers.
Comment 6 Larry Isaacs CLA 2010-05-05 09:47:27 EDT 
*** Bug 311620 has been marked as a duplicate of this bug. ***
Comment 7 Jason Sholl CLA 2010-05-05 16:19:59 EDT 
code checked into HEAD for RC1
Comment 8 Jason Peterson CLA 2010-05-06 12:04:12 EDT 
I see a cycle dependency error when testing this out for a project that is not a web or EAR project.  I added a user library to a connector project and instead of adding the correct jar/s the project itself was added to the EAR libraries container causing a cycle dependency.
Comment 9 Jason Sholl CLA 2010-05-06 12:44:49 EDT 
The problem mentioned in comment 8 is unrelated; you can reproduce it without these changes; That problem will be handled with a separate bug.
Comment 10 Larry Isaacs CLA 2010-05-06 20:18:44 EDT 
Re-opening because it turns out that the change to VirtualResource address the NPE doesn't work for the use case in Bug 311620.  In that case, the first test on line 165 (i.e. "getArchiveName() == null && otherRef.getArchiveName() == null") will evaluate to false with getArchiveName() returning null and otherRef.getArchiveName() returning non-null.  In this case, the companion "or" test takes place and generates the NPE.  I think the patch in Bug 311620 would work better.  I'll attach an updated version here.
Comment 11 Larry Isaacs CLA 2010-05-06 20:22:07 EDT 
Created attachment 167422 [details]
Additional patch to address NPE still present in VirtualReference

For convenience, the patch also updates the copyright year.
Comment 12 Rob Stryker CLA 2010-05-07 02:46:33 EDT 
What is the process for re-submitting this bug for approvals? Should I reset the flags? Which ones? Would it simply be easier to put this in a separate bug?
Comment 13 Jason Sholl CLA 2010-05-07 10:39:01 EDT 
Ouch; can't believe I made that mistake with the and/or logic.  Thanks for catching.  Considering this is such an obvious error and simple that needs to be fixed, I'm checking this in based on the original PMC review.