Community
Participate
Working Groups
Created attachment 173317 [details] Patch file alters JavaFacetInstallPage.java and JavaFacetInstallPage.properties When adding a new folder from the java facet page of the wizard, folder creation does not prohibit duplicate names. RESOLVED by: Modifying the input validation [ createSourceFolderInputValidator() ] for the add folder text box to check against existing folders to be added. Added checks for preexisting name, preexisting name with different case, prefix of existing path. Steps to recreate original bug: 1. Open the new EJB project wizard and go to the Java facet panel. 2. Create two folders with same name 3. Finish the wizard, you will see an error and the migration wizard is shown (see picture2)
Thanks for the patch. I took a quick scan through it, but haven't had a chance to fully evaluate it yet. The case-insensitive check seems to be at least partly wrong. Isn't this situation legal on some operating systems, such as Linux? Also, the java convention of starting variable and field names with a lower case letter is not followed in this patch. Would you be able to attach a new patch that fixes this?
I don't believe that bugs are supposed to be assigned to non-committers. Scott, this is just for proper accounting. I encourage you to keep working on this patch to get it into the final shape for contribution. As to 3.2.1 target, we will need a statement of justification as this will need to go up for PMC approval since it touches UI and externalized strings.
Had a chance to look at this patch some more... This entire validation logic should be re-located to JavaFacetInstallConfig. The validate() method on that class should check the current state. A validatePotentialSrcFolder method can be added for the benefit of the dialog. The bulk of the code and messages can be shared between validate and validatePotentialSrcFolder methods. There shouldn't be any validation logic located in UI code.
Konstantin, Thanks for looking at the patch and providing feedback, I will work on a new patch file that address the concerns you mentioned
We are running out of time for WTP 3.2.1. Keep working on those changes and we can try to get this in for 3.2.2.
Created attachment 173888 [details] Patch file alters JavaFacetInstallPage.java, JavaFacetInstallConfig.java and ADDS JavaFacetInstallConfig.properties New proposed patch: moved all validation logic out of ui into JavaFacetInstallConfig.java created JavaFacetInstallConfig.properties to handle messages
Thanks Scott - this patch looks good - moving to Kostantin for his review.
I approve of these changes. Adding the [NLS] tag due to additional strings in a new properties file. Konstantin, this does affect usage in an adopter product, and we would like to present this to the PMC for review for inclusion into WTP 3.2.1, so I am retargetting to WTP 3.2.1. Can you please review Scott's patch? And then, unless you object (or wish to do it yourself), I will put this through the PMC review process.
Hi Scott, This patch is a definite improvement, but I have identified a few more issues that need to be addressed before the patch is ready to be released. 1. New prePotentialSrcFolder() is unnecessary new API. The only thing that's done in ValidationHelper.init() method is reference assignment of paths list. Instead of having this method and the ValidationHelper.init() method, you should reference the original list directly from the validation logic. 2. The ValidationHelper inner class itself isn't necessary. It's logic should be inlined into JavaFacetInstallConfig class. For instance, validateExisting() inlines into validate() and validateCheck() inlines into checkValidPathStatus. 3. A few utility methods in ValidationHelper (getFullPath/checkValidPathStatus) are only called once and should be inlined into their caller. 4. On the other hand, there is a repeating block in validCheck where path is checked for containment (and then the same check is done in reverse). This should be pulled out into a separate function. 5. There is an unnecessary OK_STATUS constant defined. Use Status.OK_STATUS instead. 6. In the validateExisting() method, you use CopyOnWriteArrayList for a temporary list. This isn't necessary in this case. A simple ArrayList will do and have a lower memory/perf footprint. 7. Take another look at the logic in validCheck that checks for containment. It appears that in the case of non-caseSensitive FS, isPrefixOf check will be repeated twice. 8. In validCheck method's top-level loop, List size() method is called for every iteration. This is unnecessary. Either stash the size of the list in loop initialization clause or use for-each loop syntax. 9. Actually, the top-level loop in the validCheck method looks completely unnecessary. 10. One of the checks returns a ui-centric message... "Enter a valid folder name". The message should be re-worded to something more generic, like "Source folder must be specified."
Created attachment 174192 [details] Patch file alters JavaFacetInstallPage.java, JavaFacetInstallConfig.java and ADDS JavaFacetInstallConfig.properties New proposed patch for review, this patch addresses all of the issues previously mentioned, mainly: Inlined all logic from validationHelper inner class into javaFaceInstallConfig Trims unnecessary methods (adds one method, containedWithin) Reworked prefix logic completely as well as all the smaller changes mentioned
Much improved. Thanks, Scott. A few more issues... 1. It doesn't look like the termination condition in containedWithin recursion is correct. Consider what happens if bPath is shorter than aPath. 2. It would be more efficient to implement containedWithin with a loop rather than recursion. You save on not having to allocate a new stack frame and you save from not having to allocate new Path objects via paired removeFirstSegments calls. 3. Field caseSensitive does not need to be instance-level. It can be static as case sensitivity of the local operating system doesn't change with every instance of JavaFacetInstallConfig. 4. Methods that do not rely on object state like containsLowerCase, containedWithin and checkValidPathStatus should be marked static. Less runtime overhead. 5. The checkValidPathStatus method should be private. Otherwise, it unnecessarily adds to the API.
Thanks for the feedback, I'll tweak it some more, and wopes your right, it looks like I sniped off a whole section of the recursion when I was cleaning up my comments to post the patch.
Created attachment 174245 [details] Patch file alters JavaFacetInstallPage.java, JavaFacetInstallConfig.java and ADDS JavaFacetInstallConfig.properties New proposed patch for review, this patch addresses all of the newly mentioned issues, mainly: addressed static and private methods and variables where appropriate. substantualy reworked method containedWithin: -no longer implemented using recursion -changed return type and method calls of containedWithin from checkValidPathStatus to correct a false positive bug I found while testing.
Hmm... The containedWithin method seems overly complicated... I am confused by the function of the nested if block. It seems to check for condition already checked elsewhere. Don't know if you already have seen it, but take a look at Path.isPrefixOf() method for the basic logic flow that you need. You are pretty close... Here is the listing from that method. I removed the unnecessary parts. You just need to replace the plain string equality on the segment with logic that handles FS case-sensitivity issues. if (isEmpty()) { return true; } int len = segments.length; if (len > anotherPath.segmentCount()) { return false; } for (int i = 0; i < len; i++) { if (!segments[i].equals(anotherPath.segment(i))) return false; } return true;
Created attachment 174314 [details] Patch file alters JavaFacetInstallPage.java, JavaFacetInstallConfig.java and ADDS JavaFacetInstallConfig.properties Konstantin, I have removed the nested If statement completely, otherwise this patch is the same as previous, The nested if statement was an attempt to address a specific case that is unhanded, In a non case sensitive environment, If the path foo/abc is already in the list of paths to be added (sourceFolders), and the user enters a new path Foo/def, When the wizard is finished both folders foo and Foo are created and an error is thrown. The goal of the If block was to catch and stop any input Foo/* to prevent the bug from happening, While this is not strictly correct (in a non case sensitive file system "Foo/def" should ideally just create a folder "def" in "foo") it would have prevented the bug. After further review, however, I see that the if statement was incorrect, only preventing the bug when the segments immediately previous to the new folder are not matching case.
Created attachment 174361 [details] Patch v6 Scott, Thanks for your contribution. I have taken your patch and improved it further. In particular: 1. Re-worked the main validation algorithm to work in a single pass. 2. Added unit tests to check for all the different validation cases, including testing for case-sensitivity issues. 3. Added javadoc to the new API on JavaFacetInstallConfig. 4. Removed the stray this.installConfig.validate() call from createSourceFolderInputValidator(). 5. Changed the Import-Package declaration to a Require-Bundle declaration. Specified the version range. Mixing Import-Package and Require-Bundle declarations is not a good practice. Regarding the "foo/abc" and "Foo/def" case from Comment #15, this shouldn't be prevented by validation as it is always legal. The issue is that there is a bug in how JavaFacetInstallDelegate creates folders that causes this case to fail with an exception. This patch now includes a fix for that issue too. Those who want to know more about this problem can read this... https://bugs.eclipse.org/bugs/show_bug.cgi?id=3076#c2
An adopter has requested this for 3.2.1 release, so raising for PMC review. This change impacts both NLS and API. Background: Java facet does not perform validation on source folders added by users, possibly leading to exceptions when duplicate or otherwise conflicting folders are entered. NLS: Three new message strings for validation cases. API: A new method added to JavaFacetInstallConfig that let's UI better surface the validation logic. At functional level, the correctness of this patch has been validated via new unit tests and manual testing.
Thanks for the work here both Scott and Konstantin. looks good.
I don't see "(see picture2)" mention in first comment. I'm approving this, after all the work that's gone in to its review etc., (i.e. thinking "it must be right") but it is a very large change (in terms of code) for a "normal" bug at this point. Again, I'll assume this is a major usability problem if someone hits it, but can't say I understand how frequent it would occur, ... As long as it can get into a rebuild today for this week's declared build. I'd want to know more if it has to go in next week. Thanks to all for the hard (and careful) work on this! Much appreciated.
I also agree that this bug doesn't seem very severe, but given the extensive review and testing it would appear to be safe enough to include.
Released Patch v6 to WTP 3.2.1 and fproj 2.0 code streams.
