Bugzilla – Full Text Bug Listing
| Summary: | [NLS] ZipException if non-archive file added to Deployment Assembly page | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP Java EE Tools | Reporter: | Aidyl Kareh <amkareh> | ||||||||||||
| Component: | jst.j2ee | Assignee: | Aidyl Kareh <amkareh> | ||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Chuck Bridgham <cbridgha> | ||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | CC: | amkareh, ccc, david_williams, jsholl | ||||||||||||
| Version: | 3.2 | Flags: | david_williams:
pmc_approved+
amkareh: pmc_approved? (raghunathan.srinivasan) amkareh: pmc_approved? (naci.dai) amkareh: pmc_approved? (deboer) amkareh: pmc_approved? (neil.hauge) amkareh: pmc_approved? (kaloyan) cbridgha: review+ |
||||||||||||
| Target Milestone: | 3.2.1 | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Windows Vista | ||||||||||||||
| Whiteboard: | PMC_approved | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Aidyl Kareh
Created attachment 172244 [details]
Proposed patch
Patch adds code to verify that each selected reference is a valid archive file by opening each as an archive. If it is not a valid archive, a dialog is shown informing the user of the invalid archives which were not added.
Created attachment 172475 [details] Updated Proposed Patch Updating patch since it conflicted with patch for bug 317101. Created attachment 172477 [details]
Proposed Patch Update 2
This patch contains all the initial changes plus code to avoid same problem when adding a Variable or External Archive reference.
approved * 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.
This patch fixes a java.util.zip.ZipException that is logged if a non-archive file is added to Deployment Assembly page using the 'Archive', 'External Archive' and 'Variable' wizards for adding new references.
* Is there a work-around? If so, why do you believe the work-around is
insufficient?
No.
* How has the fix been tested? Is there a test case attached to the
bugzilla record? Has a JUnit Test been added?
Tested through UI.
* Give a brief technical overview. Who has reviewed this fix?
Patch adds code to verify that each selected reference is a valid archive file by opening each as an archive. If it is not a valid archive, a dialog is shown informing the user of the invalid archives which were not added. Since a new warning dialog is now shown which lists the invalid files, a new string was added for the warning message. This patch has been reviewed by Chuck.
* What is the risk associated with this fix?
None.
I have no great objection to this ... but, seems like a pretty big fix for something I'd think was rare. Does this happen a lot? Is it really worth a pop-up dialog? Or, could it be handled in some other way? Such as not a simple warning message in log? Or, better yet, not let it be selected in the first place if its not a valid archive? Created attachment 172564 [details]
Proposed Patch Update 3
After further testing the previous patch had some issues if the path had only one segment. Updated code so that this issue no longer occurs.
Created attachment 172821 [details]
Proposed Patch Updated
This patch changes the filtering of the 'Archive' wizard so that some known non-archive extensions are not shown in this window. Thus limiting how often the warning pop-up window would appear. This patch also includes all previous changes.
Regarding David's comments, before my latest patch we allowed the user to select any file. This would cause issue if the user chose non-archive files since an exception was logged (how often would depend on user selection). Here are the cases covered with this patch: - For the 'Archive' dialog, we are adding some filtering based on excluding known non-archive extensions, so most of the elements listed should be valid. - For the 'Variable' window, we would only allow the finish button to be enabled for valid content (no pop-up dialog needed). - For the 'External Archive' window, we don't really want to limit the files that the user can select based on extension since the user can have a valid archive that is called 'Web.mywar' or use an EAR as a jar. So for this scenario the pop-up dialog could show more often since the user can pick any file in the file system. The warning pop-up dialog would list the files which are invalid and this files would not be included in the list of archives that will be added. If we were to just show a warning on the window listing the invalid files and not allow the wizard to finish then the user would have to go back and select only the valid files since this window does not have a remove button. Thanks for considering the usability improvements. This change as is, is fine by me. I guess you are saying its just too expensive to test files if they are valid archives by "peeking" inside? I'm not sure what test you later do to decide that it is not a valid archive -- you know, when you pop up the window -- if it is something simple like presence of web.xml? or a long complex series of conditions? are is user typically choosing from a list of scores of files, or a list of hundreds ... but I'm fine leaving that as a future enhancement or consideration if you ever feel the urge. I think the solution you've arrived at is a good compromise, definitely an improvement over an exception in the log :) Thanks. code checked into head for 3.2.1 |