Community
Participate
Working Groups
For a long time there've been bugs that are present only in export, but not in wst.server deployment. There have also been many present in deployment but not export. The cause is quite simply duplicate code that's inconsistent. The following patch would (begin to) move all export operations for jee projects into an operation hierarchy that actually iterates through the wst.server APIs. This makes sure that the export operation is merely a wrapper around the same APIs that deployment uses. This way, if there's a bug, it can (most likely) solely be discovered in the JEE Deployment Factories. Then, when the bug is fixed, we don't have to make sure it isn't similarly present in the export operation. A patch is attached, and while it only *really* creates one class, that class becomes the super class of five or six others that previously had a different parent. Details below: 1) Created a new class: J2EEModuleExportOperation. This class was a clone of J2EEArtifactExportOperation. Many parts of it is unchanged, but you'll see the zip handling is now present in this class. 2) The zip handling bundled in this superclass was copied from org.eclipse.jst.jee.archive.internal.ZipStreamArchiveSaveAdapterImpl, and the bulk was unchanged, however that save adapter was previously handling IArchiveResource objects, and now the clone handles IModuleResource objects instead. 3) All extending classes of J2EEArchiveExportOperation have been switched to extending J2EEModuleExportOperation. I've done rudimentary tests (I would say fairly in depth honestly) but have been unable to run the test suite. I've ensured this works flawlessly in most situations, whether it's a project reference or a binary jar / war / ejb inside an ear project. The only times it doesn't seem to work are times when the J2EEFlexProjDeployable incorrectly is handling certain boundary cases. However, as these bugs *currently* exist in the deployment model anyway, and will need to be fixed at some point, I see no harm in making sure both export and deployment use the same code. In fact, doing so might encourage these niche case bugs to be solved sooner, as now fixing one piece of code could stabalize multiple use cases (or, conversely, one breakage breaks two use cases). Left to be done: 1) cleanup. a) I left the original superclass present in case any adopters extend. Not sure if that's proper. b) There's stuff commented out that could be deleted but I wanted to leave the old code there in some of the subclass operations c) I'm quite sure the code isn't 100% up to your coding standards I am confident that this patch will help standardize and reduce complexity in this particular part of the product, trimming immediate from 4 to 2 the number of paths an export / deployment action could traverse. This will make finding bugs easier, fixing bugs easier, and give more incentive for long-standing niche border cases to be fixed properly. I look forward to seeing the progress on this bug.
Created attachment 126431 [details] A patch to make export operations traverse wst.server API
Also left: 1) I've been unable to run your test cases. I'm not terribly familiar with the setup. So unit tests would be necessary 2) Things such as include source. I don't see this as a huge issue, but possibly it is. Also, the code does use a bit of a hack. I cast / load the adapter for IEnterpriseApplication regardless of whether the currently module is an EAR project or a web project or a utility project. The reason I do this (and can do this) is that all of the flexprojdeployables *all* implement IEnterpriseApplication, and the logic in the deployable works properly to make sure to find the children.
Created attachment 126438 [details] A better patch that handles binary modules better The old patch incorrectly handled some binaries.
This is an excellent first pass. I would prefer to tackle this from the other direction, however. Already the tools extensively use IArchive, and this API has been proven to be both robust and fast (and it already handles the notion of source). I propose we fix the export to run exclusively with IArchive (there may already be a bug about this), and get off CommonArchive completely. Then, the next step is to merge both J2EEFlexDeployable and JEEFlexDeployable and have the code run off of IArchive. I believe this will be a better long term solution and reduce more code (because we will still need the extensive IArchive code anyway, but we will be able to remove most of the J2EEFlex and JeeFlex deployable code).
If you have export run off IArchive, and the deployables run off IArchive, you basically have two different batches of code (still possible to be different and inconsistant) working on IArchive. I have no problems with the deployables using IArchive. I just think to have both access IArchive could still provide inconsistancies and code duplication. Basically, you're suggesting: IArchive |- export |- deployables while what I'm suggesting is: IArchive |- deployables |- export The deployables can use the robust and fast IArchive to figure out what's going on and publish them in a way for server adopters to use *and* for export. Then export just consumes what's in the deployable, essentially eating your own dog food (eating what you're exporting to others). Let's not forget, I copied most of the zip code directly from the IArchive trace, changing only IArchiveResource to IModuleResource. I would therefore assume (danger, assume!) that it's just as fast and robust when writing out a file ;) Now that I think about it, the deployables are basically just reading from the workspace. They don't really need to introspect into any archive at all; The deployable doesn't write anything out. It just tells you the tree structure of what *could* be written out, either by a publish or an export or what have you. It's only looking at the workspace files. Right? So I don't really see how the deployables would need to use IArchive anyway. Maybe I'm confused =] I look forward to having you disabuse me of my fallacious reality.
Even if you have a unified deployable factory, and a unified export, and both are using IArchive, you're still not getting rid of the discrepency between export and deployable factory. You'll still have two differently-implemented pieces of code reading from IArchive and you'll still allow for the possibility of discrepencies in the export model vs the deployment model. I think that's bad. The primary goal as I see it is not *just* to reduce code, but more importantly to get the two pieces of code to always be in sync with each other. There's no better way to do that than make export read from the deployable. Exporting is a type of publishing,.... granted with no specific server adapter input. The export action should simply read what the deployables claim to have and export it / publish it the same way a server adapter would be forced to.
Jason: I'd still like to hear more thoughts on this issue. >> I propose we fix the export to run exclusively with IArchive >> (there may already be a bug about this), and get off CommonArchive completely. >> Then, the next step is to merge both J2EEFlexDeployable and JEEFlexDeployable >> and have the code run off of IArchive. I'd like to hear some more about this. IArchive is only needed for writing to a zipped file, correct? It would not be used, for example, to read from the flexible project model, right? So in both cases, even though they'd be using IArchive to write to a file, they would not be using IArchive to read from the project, would they? If you look at the purposes of the two tasks (export and the wst.server API), export is geared towards generating a final zip output file, whereas the deploy model is just a representation of what the final structure should be after an export is done. If this is the case, then it seems almost obvious to me that the server API should be used to represent what the final structure of the output will be, and the export operation should consume this model and generate the exported jar from it. I'd like to hear more about how you suggest to structure it. If you're still suggesting that Export consume IArchive, and deploy framework consume IArchive, you're still allowing for two potentially different pieces of code with potentially different representations, whether intentionally or via a bug.
IArchive can be used to read/write projects in addition to reading/writing components. Really, it's simply a model which can read/write the module/application. Clients utilize it by snapping adapters in for reading/writing various pieces. If you take a look at the hierarchy starting from org.eclipse.jst.jee.archive.IArchiveAdapter you will see numerous adapters which can read/write for all different types of purposes. My idea is to use IArchive model exclusively to both deal with import/export and deploy. We could handle the deploy part by writing a small layer of code bridge the two APIs, but under the covers, IArchive would be doing the work of finding everything in the workspace and determining where it needs to go. This should accomplish the same goal of having only one code path; only that code path would be IArchive rather than the deploy model. Also, there are many places in the tooling which already rely on IArchive to load various artifacts both in the form of EMF and regular files, so in this sense IArchive already is the authority here, if our goal is to have only 1 code path, we would need to update all this code too. Another benefit of using the IArchive model over deploy, is I've seen several requests both from the WTP community and from users of our adopter product for a mechanism to ignore specific files from being deployed/exported/imported (some of the obvious examples like .project, have been custom coded in the API itself, however 3rd party meta-data files are the real target here). This could be accomplished easily with IArchive; all the api hooks are already there, we just need to add an extension mechanism and preferences page for users to specify these files.
Interesting for sure... But I still feel (strongly) that export should be a consumer of deploy to ensure 100% compatability. Whether export or deploy both use extension points to limit what gets exposed is another issue, and is completely doable. I don't mind IArchive being used for reading and writing at all. That's great. I just feel pretty strongly that export should consume deploy to ensure 100% functionality. I've browsed the WTP smoke tests and they really consist of only checking export, not deploy. This is fine if one simply uses the other.... but when they're both separate (even if similar) blocks of code, I don't like it. I guess my ideal setup would be as follows: 1) Deploy uses IArchive to read the project structure and expose everything in the wst api 2) Export consumes this 3) Export filters this according to what files should be ignored 4) Export uses IArhive to write out the file. I suppose another setup I'd support, if the code were well-separated, would be an API such as: public YourResourceArrayType[] getAllResources(project){} // i forget the class name public void exportResources(YourResourceArrayType fromGet, FileFilter filter) {} If these API's existed outside of export and outside of deploy, and both *only* wrapped these in their respective API's, I'd support that. (Clearly this isn't a complete API... just meant to be an idea). If a large amount of the reading / writing is going on in the export operation class or the deploy factory classes, I wouldn't find that to be a good idea as it could lead to discrepencies again.
Jason can you elaborate a little more? Thanks.
Your last suggestions seen to be more or less in line with my idea; you nailed it with: 1) Deploy uses IArchive to read the project structure and expose everything in the wst api This should achieve what we are both after.
I'm glad we agree on step 1 ;) Can we agree (or at least dialogue) as to why the rest (including my patch) is unacceptable? If the patch just needs to account for things like source files being included / excluded, I can re-work it. But if you have a philosophical disagreement with export consuming what WTP exposes, well, let's talk about it =] Is this an issue of waiting until you've reworked the module factories before you can accept having export consume it?
just a ping on this. I know you're busy, Jason, but I'm curious if there's anything I can do here.
Seems to be waiting for Jason's review, so I'm assigning to him instead of "inbox". Since this shows up on the "old patches" query, I'd suggest marking the patch obsolete if it is really unacceptable, after giving some "feedback" of course. Or, if acceptable, then apply it! :)
Routing over to Jason Peterson who will be having a look at this for M4.
Created attachment 152761 [details] J2EEFlexDeployable utilizing IArchive APIs patch I am adding a patch that contains work I have done so far in merging both J2EEFlexDeployable and JEEFlexDeployable. I have deprecated JEEFlexDeployable. I have updated J2EEFlexDeployable to use the IArchive APIs when determining the resources to deploy. I also removed the single-root code from J2EEFlexDeployable. The code to handle single-root determination is now a separate utility class called org.eclipse.jst.j2ee.project.SingleRootUtil. This work had been primarily done as part of Bug 295519. The optimization code that is now in the IArchive code will utilize the single-root utility.
Created attachment 152982 [details] Updated patch for J2EEFlexDeployable utilizing IArchive I updated the previous patch. Biggest change is that J2EEFlexProjDeployable and JEEFlexProjDeployable will no longer be merged.
I've got significant issues with this patch as it is now. The issues really go deeper into IArchive though. I'm constantly making IVirtualComponent objects of my own class. I make use of the IReferenceResolver API. Most of my custom VC types have a dependency type of "consumes" and their purpose is to simply present a list of files that should be included. A good example of this would be a 'fileset' component, one which can draw from several different places throughout the workspace if it wants. This type of virtual component doesn't have any clear "project" that it belongs to. Some of the files may belong to ProjectA. Some may belong to ProjectB, ProjectC, etc. And via RSE (remote system explorer) some may even be remote files not imported into the workspace at all (or simply local files outside the workspace). The problem here is that I must provide a "project" for my virtual component even if my component doesn't belong inside any project if I have any hope of not causing NPE's all over the place. In the past, the outside-the-workspace lib files typically were assigned a "dummy" project of whoever their referencing component was. So if a outside-the-workspace lib file was instantiated and was a child of an EAR project, for example, this lib file virtual component would be assigned a project of it's parent, the ear project. JavaEEArchiveUtilities is not able to properly handle this virtual component, as its getProject() value is the surrounding virtual component's project, aka the ear project, and so tries to treat my custom virtual component class as an EAR and use an EARComponentArchiveLoadAdapter to change my component into something useful. It ends with an NPE. The current J2EEFlexProjDeployable code is properly able to consume random virtual components without causing errors. It seems the IArchive code needs more work to properly be able to handle any virtual component handed to it, and to properly export those resources when they are consumed rather than "used". I believe much more discussion is needed here.
Created attachment 153408 [details] A plugin with extensions that may be used to test Attached is a plugin which should be in your workspace before beginning to test. What this plugin does is register a Reference Resolver. This can turn an arbitrary URL in the wst.component xml file into some Virtual Component that is referenced. The way to test this is to create an Ear project and then inside the wst.component file, add the following lines: <dependent-module deploy-path="/" handle="module:/quick.test.segment1"> <dependent-object></dependent-object> <dependency-type>consumes</dependency-type> </dependent-module> This will try to create a dependency. When you go to publish this, you should see some NPE's as the archive load adapter does not have even the slightest clue how to load archives that are not JEE-Tools originations. I still maintain that export should more appropriately draw off the J*EEFlexProjDeployable and *then* filter undesirable files out. But admittedly this is because I'm not familiar with the IArchive code and have not gotten time to fully grok it.
For the record, the NPE trace was as follows: java.lang.NullPointerException at org.eclipse.jst.j2ee.internal.archive.ComponentArchiveLoadAdapter.getExternalFiles(ComponentArchiveLoadAdapter.java:343) at org.eclipse.jst.j2ee.internal.deployables.J2EEFlexProjDeployable.gatherExternalFiles(J2EEFlexProjDeployable.java:242) at org.eclipse.jst.j2ee.internal.deployables.J2EEFlexProjDeployable.members(J2EEFlexProjDeployable.java:158) I believe this NPE is not tragic itself, but rather masks how the IArchive code is not prepared at all to handle different types of Virtual Components, used, consumed, etc. The IArchive code also makes much much more use of custom internal JEE classes, and I find this problematic.
Created attachment 153416 [details] JUnit verified patch for J2EEFlexDeployable utilizing IArchive Made some final updates and successfully ran various WTP JUnits. The org.eclipse.jst.server.core.tests.AllTests JUnit seemed to best test the path of code that this patch has touched. I also addressed the npe that Rob mentioned. It should no longer be thrown by J2EEFlexProjDeployable.
The new patch removes the NPE but still does not properly handle my arbitrary-type virtual component. It is still treating my virtual component as if my component were an entire ear project. Tracing through my example plugin, ReferenceResolver1$TestVirtualFolder.members(etc) is never called. My virtual component in this case may expose any random collection of IResource objects in the workspace, but JavaEEArchiveUtilities is still not treating my component as it's own entity but is rather treating it as an EAR project. The resources my arbitrary-type component are trying to expose are never requested or included in the output.
Another issue I have here is that delegating to IArchive does not allow non-JEE classes to extend or make use of the functionality of J2EEFlexProjDeployable by extending it. Rather than opening up these APIs to be more generic as simply wrappers, we're delegating to classes that are in fact even more internal and more closed off. For example, I have a project type called ESB Project, which also provides its own VirtualComponent. If I wanted to make use of your wrapper deployable (J2EEFlexProjDeployable in this case), I now have the members() method calling JavaEEArchiveUtilities.INSTANCE.openArchive(component), which then checks whether the component is binary (it's not) or one of the approved JEE project types. If my module did claim binary, there would be a ClassCastException here when you cast to J2EEModuleVirtualArchiveComponent. If it's none of the above, it simply returns null. I believe that delegating to IArchive is even more restrictive, more internal-oriented, and less extendable / willing to accept arbitrary virtual component types. The current J2EEFlexProjDeployable code is much more tolerant of other virtual component types being included and I think we really need to work towards that. Also EARComponentArchiveLoadAdapter.addModulesAndUtilities(etc) does not seem to check at all whether the reference is of type "uses" or of type "consumes" which makes a very big difference when exporting / publishing.
Created attachment 153552 [details] Updated J2EEFlexDeployable patch some how the public method isSingleRootStructure() had gotten removed during all my changes. I have added it back in. It is utilizing the new single root utility.
Checked in most recent changes to HEAD. Rob and I spoke yesterday about our approach to merging deployment and export and have agreed to move forward with these changes in the short term. The long term solution will include a new layer that fits between IVirtualComponent and deployment, export and IArchive. This new layer will be very generic and based solely on IVirtualComponent and have no dependencies on Java EE. Once this new generic layer in place, the other APIs can be moved in phases to run on top of it. Rob, could you open a new bugzilla (if there isn't one already) to track this new work and cc the whole lot of us. I think we should press for M5 for tihs.
I had to update WebDeployTests and ClasspathDependencyEARTests because of the change in the return types in org.eclipse.wst.web.internal.deployables.ComponentDeployable.createModuleFile() and ensureParentExists() and the contents returned from both of the getMembers() methods- we now use org.eclipse.wst.server.core.util.ModuleFolder instead of org.eclipse.wst.server.core.util.internal.ModuleFolder and org.eclipse.wst.server.core.util.ModuleFile instead of org.eclipse.wst.server.core.util.internal.ModuleFile. Was this an intentional behavior change? I realize that the internal versions were deprecated in WTP 3.0.0, but I think we should at least announce this change, if not bump the plugin version id for org.eclipse.wst.web to 1.2.0. I am reopening this bug to either revert back to the internal classes or else to bump the plugin version id.
Created attachment 153762 [details] M4 J2EEFlexDeployable using IArchive patch I added the use of the deprecated APIs back in so this would not break clients.
I committed Jason Peterson's latest patch to HEAD.
The M4 J2EEFlexDeployable using IArchive patch caused failures in the servertools JUnits, so I am reopening this bug.
Created attachment 153820 [details] Updated M4 J2EEFlexDeployable that fixes IArchive issues IArchive was not including utility projects when computing classpath dependencies. It was also not including classpath dependency folders with a target of WEB-INF/classes. Both of these cases made it inconsistent with the deploy code. This patch fixes these cases and also fixes a classpath dependency JUnit that was incorrectly passing when looking for classpath dependencies in WEB-INF/classes. It should have been failing due to a defect that had been in the deploy code. I updated the testcase, but it will now pass correctly since deploy is using IArchive code.
Committed the Updated M4 J2EEFlexDeployable that fixes IArchive issues patch to HEAD.
*** Bug 171480 has been marked as a duplicate of this bug. ***
