Bugzilla – Full Text Bug Listing
| Summary: | Need extension point for classpath dependencies | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP Java EE Tools | Reporter: | Peter Moogk <pmoogk> | ||||||
| Component: | jst.j2ee | Assignee: | Salvador Zalapa <zalapa> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | Chuck Bridgham <cbridgha> | ||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | ccc, david_williams, jsholl, shr31223, zina | ||||||
| Version: | 3.2 | Flags: | david_williams:
pmc_approved+
cbridgha: pmc_approved? (raghunathan.srinivasan) cbridgha: pmc_approved? (naci.dai) cbridgha: pmc_approved? (deboer) cbridgha: pmc_approved? (neil.hauge) cbridgha: pmc_approved? (kaloyan) cbridgha: review+ |
||||||
| Target Milestone: | 3.2.3 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | PMC_approved | ||||||||
| Attachments: |
|
||||||||
Peter - can you explain how the "enablement" works for this extension? It looks like the implementation classes will have a "projectHandlesExport" method, and if this returns true - then they can override behavior? I would expect an enablement expression like facet or similar to be on the extpoint, so the registry reader/manager can return either the WTP default or an overridable extension implementation. (In reply to comment #1) > Peter - can you explain how the "enablement" works for this extension? > > It looks like the implementation classes will have a "projectHandlesExport" > method, and if this returns true - then they can override behavior? > > > I would expect an enablement expression like facet or similar to be on the > extpoint, so the registry reader/manager can return either the WTP default or > an overridable extension implementation. Hi Chuck, The extension registry that I wrote always checks the enablement expression first before it even tries to create an instance of the extenders class. It was implemented this way so that extender classes are only fluffed up when they really need to be. We don't want extender plugins to be loaded unnecessarily. If the enablement expression evaluates to true the extender's class will be fluffed up if it hasn't already been done. At this point the extender has a method called projectHandlesExport. This method allows the extender to check in more detail whether it really does handle the export or not. So it would be possible for the enablement expression to evaluate to true, but after checking the project and classpath in more detail the extender could return false. The same is true for getClasspathdependencyAttribute method. The enablement expression is checked first and then this method is called. If the extender wants to override the dependency value they will return some non-null value. If they want the default WTP value they will return null. Thanks for the explaination..... I approve * 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.
Currently when a classpath entry is added to a project, the J2EE validation warning about this will not be available when deployed. So, a quickfix is available, however when this quickfix is applyed, the classpath attribute is calculated from the runtime and there are some scenarios where it should be calculated differently.
* Is there a work-around? If so, why do you believe the work-around is insufficient?
The work around could be edit the .classpath file manually what is no acceptable since this file should be edited by users.
* How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added?
It have been test by my own manually.
* Give a brief technical overview. Who has reviewed this fix?
Patch code provides an extension point so the path calculation can be overriden a provider by an extender. The extension always checks the enablement expression
first before it even tries to create an instance of the extenders class
* What is the risk associated with this fix?
The risk is low.
I've some questions about this new extension point. First, is this "extra classpath" needed entirely to avoid the validation warning? Or is it used in other ways too? If the former, then seems like this data/value should be part of the validator that creates this warning? Also, does this extension provide the one and only one classpath? Or can all "providers" contribute to it ... with a sort of additive effect. Its just not clear from the description (might be in code?) so thought I'd ask. More minor, I noticed an empty class abstract class ClasspathDependencyExtension That "providers must subclass". Am I looking at that right? Seems odd to have an abstract class as a "marker interface". Usually the Interface suffices. Lastly, is the "lazy fluffing" a critical part of this? If so, sometimes "creating a new instance" is not the only thing that will cause a class to be loaded (thus resulting in the bundle being activated) ... so, if a critical part of this I wonder if that's been tested? I'd also encourage better documentation in the extension point schema. Appears some [defaults in brackets] were still there. Not to mention, its not very clear who (else) would use this when. I'm not suggesting you document product/server specific things ... but seems like a little more abstracted information could be said about who, why, and when this could be used. Hi David,
This extension is used for two purposes. One is to avoid the validation error and two to provide a value for the path. This path value is used when the project is exported. Specifically, it is the path where a particular jar file should be located within an exported project.
The extension currently only provides one path. It is not additive. You are correct that more than one extender could provide different path values, but I think this is a very remote case.
The abstract ClasspathDependencyExtension class is used at Jason Sholl's request. It was done this way to accommodate potentially new methods in the abstract without breaking existing clients.
The lazy fluffing isn't critical. It was just done as a good practice for performance.
You are correct. The schema could be better documented. I can send an updated version for Salvador to commit.
Thanks.
(In reply to comment #6) > Hi David, > This extension is used for two purposes. One is to avoid the validation > error and two to provide a value for the path. This path value is used when > the project is exported. Specifically, it is the path where a particular jar > file should be located within an exported project. > > The extension currently only provides one path. Ok, thanks Peter. I'm fine with this then with improved documentation. If it wasn't too much trouble for you, I'd suggest some sort of improved name, but not sure what would be better. Its just that "ClasspathDependencyExtension" sounds like something that would effect "runtime" behavior (in workbench) or "compilation" behavior but sounds like it doesn't? Maybe "ExportingClasspathDependency" or something? Or ... just well document in extension point schema what is does effect, and what it does not effect. Thanks again for your care and attention. Created attachment 187758 [details]
Updates to the extension schema
Attached the entire extension schema file with updated documentation.
Hi David, Thanks for your comments. I decided to leave the extension name as is if this is Ok with you, but I'd be fine if it was decided that a better name was required. I attached to this defect an updated version of the schema with additional documentation. Thanks. Peter Committed to R3_2_maintenance |
Created attachment 187332 [details] Patch for this defect Currently when a jar is added to the classpath a J2EE validation warning is issued that this jar will not be available when deployed. Currently, the path of the classpath attribute is calculated from the runtime. However, there are some scenario where the path should be calculated differently. An extension point should be provided so that path calculation can be overridden an provided by an extender. A patch has been provided that implements extension.