Bugzilla – Full Text Bug Listing
| Summary: | It is possible to create two packages with a different case in two source folders | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Krzysztof Daniel <krzysztof.daniel> | ||||||||||||||||||||||||||||||||
| Component: | UI | Assignee: | Noopur Gupta <noopur_gupta> | ||||||||||||||||||||||||||||||||
| Status: | RESOLVED WONTFIX | QA Contact: | |||||||||||||||||||||||||||||||||
| Severity: | minor | ||||||||||||||||||||||||||||||||||
| Priority: | P3 | CC: | daniel_megert, deepakazad, ericdp, markus.kell.r, noopur_gupta, pwebster | ||||||||||||||||||||||||||||||||
| Version: | 3.7 | Flags: | markus.kell.r:
review-
markus.kell.r: review- deepakazad: review- deepakazad: review- deepakazad: review+ daniel_megert: review- |
||||||||||||||||||||||||||||||||
| Target Milestone: | 4.3 | ||||||||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||||||||
| OS: | Windows XP | ||||||||||||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||||||||||||
| Bug Depends on: | 323835 | ||||||||||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||
|
Description
Krzysztof Daniel
Since we disallow this inside the same source folder on Windows, we should do the same here. Created attachment 176520 [details]
Patch
Checks all source folders in the project for package names with different case and sets to error and avoids seeing a build problem later.
Didn't review the patch but a quick glance tells me that it doesn't cover the DnD case. Created attachment 176757 [details] Updated patch (In reply to comment #3) > Didn't review the patch but a quick glance tells me that it doesn't cover the > DnD case. Ah yes missed that one, thnx Dani for pointing it out!.. added a check in ReorgPolicyFactory.java. Markus, could you pls check if the check is in the right place? We first need to get a statement from JDT/Core why packages with different casing don't work (bug 323835). Only then, we can decide how we can try to prevent creation of those in the UI (or maybe we will just warn the user in some cases). Furthermore, there are more ways how packages can be created: New Type wizards, via rename of a package or a package and subpackages. (In reply to comment #4) In the runtime workspace, I tried to add a new package in org.eclipse.core.expressions => NPE: java.lang.NullPointerException at org.eclipse.jdt.ui.wizards.NewPackageWizardPage.packageChanged(NewPackageWizardPage.java:280) => The problem is a 'bin' folder on the classpath. Solution: Use IPackageFragmentRoot#getKind() instead of #isArchive(). The checking code should be moved into class Checks, maybe into checkPackageName(..)? Created attachment 178689 [details] Patch update (In reply to comment #5) > We first need to get a statement from JDT/Core why packages with different > casing don't work (bug 323835). Only then, we can decide how we can try to > prevent creation of those in the UI (or maybe we will just warn the user in > some cases). > JDT/Core has no plans to fix the issue(bug 323835) so we have to prevent the creation in UI. > java.lang.NullPointerException > at > org.eclipse.jdt.ui.wizards.NewPackageWizardPage.packageChanged(NewPackageWizardPage.java:280) > > => The problem is a 'bin' folder on the classpath. > Solution: Use IPackageFragmentRoot#getKind() instead of #isArchive(). Done. > > The checking code should be moved into class Checks, maybe into > checkPackageName(..)? Yes indeed! Extracted the common checking code into Checks.java and also added additional checks for package/subpackage creation/renaming by various means. AFAICS, the patch forbids operations that would end up with clashing package names. We will not prevent the user from doing this. We will only show a warning message. If you have to clean up existing code, you may end up with clashing package names, and we should not block useful refactoring operations just because the compiler may have problems with an intermediate state. The checks don't seem to consider the output folders. If the output folders don't clash, we shouldn't show any warning. I guess we also need the check in copy packages, not only in move. Javadoc of Checks#checkPackageName(..) should tell that exactly one of the last two arguments should be null. Anyway, we should have two different methods checkPackageName(.., String) and checkPackageName*s*(.., IPackageFragment[]) as API (can still share a private implementation if that's convenient). Created attachment 183653 [details] Patch_2 (In reply to comment #7) > AFAICS, the patch forbids operations that would end up with clashing package > names. We will not prevent the user from doing this. We will only show a > warning message. If you have to clean up existing code, you may end up with > clashing package names, and we should not block useful refactoring operations > just because the compiler may have problems with an intermediate state. Changed the error into warning for all operations. For copy/move drag and drop operations a warning dialog is shown and then the package is created. Since we dont want to prevent creating the package in any way I thought showing a warning dialog will tell the user about the error seen in the error view. For the new package/type dialogs, warning is shown as a status msg. > The checks don't seem to consider the output folders. If the output folders > don't clash, we shouldn't show any warning. I have added a check to check whether the new package has a path that is a prefix of project's output location or the other way round. I'm not sure if this is all you meant or should we also consider whether the output locations of the source folders are different? I did not find a way to test that by fetching the output locations of a src folder. > I guess we also need the check in copy packages, not only in move. > > Javadoc of Checks#checkPackageName(..) should tell that exactly one of the last > two arguments should be null. Anyway, we should have two different methods > checkPackageName(.., String) and checkPackageName*s*(.., IPackageFragment[]) as > API (can still share a private implementation if that's convenient). Done. * NPE in tests:
java.lang.NullPointerException
at org.eclipse.jdt.ui.tests.refactoring.RenamePackageTests$PackageRename.createAndPerform(RenamePackageTests.java:346)
at org.eclipse.jdt.ui.tests.refactoring.RenamePackageTests.testHierarchicalToSuperpackageFail(RenamePackageTests.java:928)
* Checks#checkPackageName(IPackageFragmentRoot, IPackageFragment[], String):
- if (!(pRoot.getKind() == IPackageFragmentRoot.K_SOURCE)):
=> use !=
- move stuff that doesn't depend on the loop index out of inner loops, e.g.
IPath rootPath= pRoot.getPath();
IPath outputPath= pRoot.getJavaProject().getOutputLocation();
packages[j].getElementName()
- the check for clashes with the output folder is fine, but what I originally meant is that we should only put up a warning if we really have a problem at compile time: In a project with 2 source folders with separate output locations, we cannot run into clashes. See
packageFragmentRoot.getResolvedClasspathEntry().getOutputLocation().
- the warning message should always contain the conflicting package (such that the user can see the actual problem and even copy/paste the name in some status dialogs)
- "A subpackage has the same name with a different case as the existing package ''{0}''":
- Package names are case sensitive, so they cannot be "the same" if they have different case.
- AFAIK, "with a different case as..." is grammatically wrong
(should be than...)
=> How about: "A subpackage already exists with a different case: ''{0}''."?
(note that the other message should also end with a full stop)
Created attachment 184270 [details] Patch (In reply to comment #9) Fixed the NPE and made the other changes. > - the warning message should always contain the conflicting package (such that > the user can see the actual problem and even copy/paste the name in some status > dialogs) > > => How about: "A subpackage already exists with a different case: ''{0}''."? > (note that the other message should also end with a full stop) Yep it sounds better. But there is no easy way to find the conflicting package other than looping over all packages in the the package root and finding one whose name matches the entered package name (this is what essentially done at the resource level too : org.eclipse.core.internal.resources.Resource.findVariant(String, String[])). We only check whether a resource already exists in the given path and if there is we print the error message. Is it really necessary/helpful to print the name of the conflicting package as well? > Is it really necessary/helpful to print the name of
> the conflicting package as well?
Yes. Whatever the user will do about the conflicting name, she needs to know the concrete conflict. Furthermore, the warning message
"Package already exists with a different case: 'upper'." doesn't make sense when I already have a package UPPER and then try to create a new package upper.
Also make sure the warning messages are actually correct. E.g. with and existing package UPPER and new package upper.a, the message is:
"A subpackage already exists with a different case: 'upper'."
=> 'upper' is not a subpackage -- it's a containing package (that's how they call it in JLS3 §6.7)
Another bug with two source folders src & src2 and packages src/one and src2/ONE:
When I try to create a class XXX in src2/ORG, I get an invalid error message "Package already exists with a different case: 'ORG'."
Created attachment 188500 [details]
Patch with appropriate warnings
Patch with appropriate warning messages.
Two basic tests fail 1. (In reply to comment #8) > Changed the error into warning for all operations. This is not good. - One source folder ‘src’ - One package ‘p’ - File > New > Package > ‘P’ => warning is shown, but it should be an error as on Finish you get an error Same problem if you use some other means e.g DnD to create package ‘P’ in a source folder containing package ‘p’ Showing a warning is ok, if ‘P’ is created in a different source folder 2. Warning is there even if the 2 source folders have different output folders src (bin) -p src1 (bin1) - <try to create P in this folder> Created attachment 188761 [details] Modified Patch (In reply to comment #13) > Two basic tests fail > > 1. > (In reply to comment #8) > > Changed the error into warning for all operations. > This is not good. > > - One source folder ‘src’ > - One package ‘p’ > - File > New > Package > ‘P’ => warning is shown, but it should be an error as > on Finish you get an error Yeah creating into the same source folder is not possible. Added a check. > Showing a warning is ok, if ‘P’ is created in a different source folder > > 2. Warning is there even if the 2 source folders have different output folders > src (bin) > -p > src1 (bin1) > - <try to create P in this folder> This already works for 2 different output folders neither containing the default folder, added a check for default output folder as well. Deepak can you pls check now? Created attachment 188770 [details]
Patch
Modified previous patch to handle some more cases:
1) rename of a package to a different case
2) to show an error in this case instead of a warning :
3 src folders src1, src2, src3 with packages test, Test and TEST respectively.
Drag test into src3.
- Is there a reason for not adding some tests to RenamePackageTests ?
- Create src, src1 with packages p, q (output folder is default for both). Rename q to ‘P’ => only a warning is shown about package names starting with lower case but the refactoring fails on clicking next.
- In NewTypeWizardPage and NewPackageWizardPage
status= Checks.checkPackageName(root, packName);
if (status.getSeverity() == IStatus.WARNING)
status.setWarning(status.getMessage());
else if (status.getSeverity() == IStatus.ERROR) {
status.setError(status.getMessage());
return status;
}
Firstly, this overwrites any warnings set previously, even if this check does not produce any warning. Can we add both warning messages to the status, something like RefactoringStatus.merge(...) ?
Secondly status.setWarning and status.setError do not do anything here, since the message is already set in Checks.
Quick code review (not complete)
- Checks.checkPackageName(IPackageFragmentRoot, IPackageFragment[], String) => change this to Checks.checkPackageName(IPackageFragmentRoot, IPackageFragment[]). And create the array from the string in Checks.checkPackageName(IPackageFragmentRoot, String)
- Checks.checkPackageName(IPackageFragmentRoot, IPackageFragment[]) => rename to checkPackageNames
- In Checks.checkPackageName(IPackageFragmentRoot, IPackageFragment[], String) List rootList= new ArrayList();
rootList.addAll(Arrays.asList(roots));
=>
List rootList= Arrays.asList(roots);
- In Checks.checkPackageName(IPackageFragmentRoot, IPackageFragment[], String) return only when you encounter an error, because if you return on a warning you may miss an error later in the loop.
- RenamePackageProcessor.getStatus(String, IPackageFragmentRoot, String) – rename stat to status
(In reply to comment #16) > - In Checks.checkPackageName(IPackageFragmentRoot, IPackageFragment[], String) > return only when you encounter an error, because if you return on a warning you > may miss an error later in the loop. Note that if you do this there is no need to check the destination first. Created attachment 188957 [details] Patch (In reply to comment #15) > Created attachment 188770 [details] [diff] > Patch > > Modified previous patch to handle some more cases: > > 1) rename of a package to a different case While testing this I found some more cases while renaming a parent package to its sub package name and vice versa with a different case which would result in an error. Fixed them. Will add them to the tests in the next patch. (In reply to comment #16) > - Is there a reason for not adding some tests to RenamePackageTests ? Nope. Adding the cases mentioned above along with the rest to the next patch. > > - Create src, src1 with packages p, q (output folder is default for both). > Rename q to ‘P’ => only a warning is shown about package names starting with > lower case but the refactoring fails on clicking next. Fixed. > > - In NewTypeWizardPage and NewPackageWizardPage > status= Checks.checkPackageName(root, packName); > if (status.getSeverity() == IStatus.WARNING) > status.setWarning(status.getMessage()); > else if (status.getSeverity() == IStatus.ERROR) { > status.setError(status.getMessage()); > return status; > } > > Firstly, this overwrites any warnings set previously, even if this check does > not produce any warning. Can we add both warning messages to the status, > something like RefactoringStatus.merge(...) ? We can use MultiStatus and add only the warnings to that and then return all at once, but we need to still use another IStatus object for errors as we show only one at a time. I do not think this is necessary in a status line as it might look cluttered and also if you consider the New Type creation wizard if there are warnings in more than one field only one is shown at a time. MultiStatus is usually used for collecting all errors and throwing an exception or showing them in an error dialog. > Secondly status.setWarning and status.setError do not do anything here, since > the message is already set in Checks. Oops, a variable is missing here. > > Quick code review (not complete) > - Checks.checkPackageName(IPackageFragmentRoot, IPackageFragment[], String) => > change this to Checks.checkPackageName(IPackageFragmentRoot, > IPackageFragment[]). And create the array from the string in > Checks.checkPackageName(IPackageFragmentRoot, String) > Yeah since only the package name is used to compare and not its path, can use the same method. > - In Checks.checkPackageName(IPackageFragmentRoot, IPackageFragment[], String) > List rootList= new ArrayList(); > rootList.addAll(Arrays.asList(roots)); > => > List rootList= Arrays.asList(roots); > This will not work. However, it can be reduced like this : List rootList= new ArrayList(Arrays.asList(roots)); > - In Checks.checkPackageName(IPackageFragmentRoot, IPackageFragment[], String) > return only when you encounter an error, because if you return on a warning you > may miss an error later in the loop. > An error will occur only if the package exists with a different case in the destination root folder. In other cases, a warning is shown whether one of the other folders have a package with a different case or all. So we need not waste time looping all the folders checking multiple levels but instead exit after the first error/warning. Hence checking destination folder for any errors and then looking for the first warning is better. > - RenamePackageProcessor.getStatus(String, IPackageFragmentRoot, String) – > rename stat to status done ;) (In reply to comment #18) > While testing this I found some more cases while renaming a parent package to > its sub package name and vice versa with a different case which would result in > an error. Fixed them. Will add them to the tests in the next patch. > > (In reply to comment #16) > > - Is there a reason for not adding some tests to RenamePackageTests ? > Nope. Adding the cases mentioned above along with the rest to the next patch. Ok, I will wait for the next patch. Created attachment 189163 [details]
Patch + tests
Patch with tests.
Created attachment 189274 [details] suggested tweaks in tests (In reply to comment #18) > > - Create src, src1 with packages p, q (output folder is default for both). > > Rename q to ‘P’ => only a warning is shown about package names starting with > > lower case but the refactoring fails on clicking next. > > Fixed. Once you click 'Preview' the warning is shown twice. Besides this the patch works ok. (Have not looked at the code yet) - In testHierarchical04, testHierarchical05, testHierarchical07, testHierarchical08, testHierarchical09, testHierarchical10 the fourth package name ‘p’ is not used. See the for loops in RenamePackageTests.PackageRename.PackageRename(RenamePackageTests, String[], String[][], String, boolean). (Ok, the package p is created in testHierarchical10 but it is still not required). - I did not understand what is 'hierarchical' about testHierarchical06 ? testDifferentCaseInMultiRoots1 and testDifferentCaseInMultiRoots2 – the for loops can be extracted to a method to avoid code duplication - there is no need to use different names in the two tests - it just makes it harder (for me) to understand them. (the attached patch contains these tweaks) (In reply to comment #14) > > Showing a warning is ok, if ‘P’ is created in a different source folder > > > > 2. Warning is there even if the 2 source folders have different output folders > > src (bin) > > -p > > src1 (bin1) > > - <try to create P in this folder> I tried to add a test for this (testDifferentCaseInMultiRoots3), but it gives an exception. I suspect a problem with test teardown as a result of adding the output folder in testDifferentCaseInMultiRoots2. Note that testDifferentCaseInMultiRoots3 passes when it is run alone. You can also use org.eclipse.jdt.testplugin.JavaProjectHelper.addSourceContainer(IJavaProject, String, IPath[], IPath[], String) to specify an output folder while creating a source folder. Is there a test for the comment "Ensure the destination root is checked first..." in Checks.checkPackageNames(IPackageFragmentRoot, IPackageFragment[]) ? Maybe it will go in org.eclipse.jdt.ui.tests.refactoring.ccp.MoveTest. Ping! Created attachment 193492 [details] Patch + tests with tweaks (In reply to comment #21) > > > - Create src, src1 with packages p, q (output folder is default for both). > > > Rename q to ‘P’ => only a warning is shown about package names starting with > > > lower case but the refactoring fails on clicking next. > > > > Fixed. > Once you click 'Preview' the warning is shown twice. This is fixed. > testDifferentCaseInMultiRoots1 and testDifferentCaseInMultiRoots2 > – the for loops can be extracted to a method to avoid code duplication > - there is no need to use different names in the two tests - it just makes it > harder (for me) to understand them. Sure, helps. > > > 2. Warning is there even if the 2 source folders have different output folders > > > src (bin) > > > -p > > > src1 (bin1) > > > - <try to create P in this folder> > > I tried to add a test for this (testDifferentCaseInMultiRoots3), but it gives > an exception. I suspect a problem with test teardown as a result of adding the > output folder in testDifferentCaseInMultiRoots2. Note that > testDifferentCaseInMultiRoots3 passes when it is run alone. Fixed. testDifferentCaseInMultiRoots3 does more or less the same as testDifferentCaseInMultiRoots2 but added it. I think the tests are sufficient to cover the new code, even though the calls are from different places it tests the same checks. So not adding the tests redundantly to other places. Created attachment 193556 [details]
Patch + tests with tweaks
Prev patch was out of sync for a couple of files.. take this one.
(In reply to comment #21) contd.. > > - In testHierarchical04, testHierarchical05, testHierarchical07, > testHierarchical08, testHierarchical09, testHierarchical10 the fourth package > name ‘p’ is not used. See the for loops in > RenamePackageTests.PackageRename.PackageRename(RenamePackageTests, String[], > String[][], String, boolean). (Ok, the package p is created in > testHierarchical10 but it is still not required). Forgot to add : those tests need the package p as it is created by default in the setup and comparing with original state would result in an error without the default package. So you see whichever test uses this check uses default package too. > - I did not understand what is 'hierarchical' about testHierarchical06 ? > I just grouped it together with the other tests for this fix, even though this tests only a single package. May be it could be renamed without the 'hierarchical'. (In reply to comment #25) > (In reply to comment #21) contd.. > > > > > - In testHierarchical04, testHierarchical05, testHierarchical07, > > testHierarchical08, testHierarchical09, testHierarchical10 the fourth package > > name ‘p’ is not used. See the for loops in > > RenamePackageTests.PackageRename.PackageRename(RenamePackageTests, String[], > > String[][], String, boolean). (Ok, the package p is created in > > testHierarchical10 but it is still not required). > Forgot to add : those tests need the package p as it is created by default in > the setup and comparing with original state would result in an error without > the default package. So you see whichever test uses this check uses default > package too. As discussed on sametime, please add this as a comment to the test. > > - I did not understand what is 'hierarchical' about testHierarchical06 ? > > > I just grouped it together with the other tests for this fix, even though this > tests only a single package. May be it could be renamed without the > 'hierarchical'. Yes, please rename the test. Rest is ok. > (In reply to comment #25)
Thanks, Committed to HEAD.
Some of the new tests fail on Linux, I have reverted the fix for now. Given - each potential patch had an issue - the current one causes test failures on Linux - the patch touches many files - we have RC0 testing in 5 days - the problem exists since 1.0 I decided to move this to 3.8. We have to see how/whether we really fix this and also coordinate with JDT Core (see bug 323835). (In reply to comment #29) > Given > - each potential patch had an issue > - the current one causes test failures on Linux > - the patch touches many files > - we have RC0 testing in 5 days > - the problem exists since 1.0 > I decided to move this to 3.8. We have to see how/whether we really fix this > and also coordinate with JDT Core (see bug 323835). Thanks for the update... adopter product is looking for a fix/patch for eclipse 342 for a customer reported problem. Created attachment 193850 [details]
Patch + tests_v2
Patch that fixes the Linux failures. Attaching it so that its easier to pick up for 3.8
(In reply to comment #31) > Created attachment 193850 [details] [diff] > Patch + tests_v2 > > Patch that fixes the Linux failures. What was the cause of failure ? And what was the fix ? Created attachment 226092 [details] Updated - Patch + tests Updated the copyright and @since information and fixed some compilation errors in the previous patch. The behavior looks OK and the tests pass on Windows 7. As mentioned in Comment #28, the tests were failing on Linux. Running the tests on Linux and verifying the same is pending. (In reply to comment #33) > Created attachment 226092 [details] [diff] > Updated - Patch + tests > > Updated the copyright and @since information and fixed some compilation > errors in the previous patch. > The behavior looks OK and the tests pass on Windows 7. > As mentioned in Comment #28, the tests were failing on Linux. Running the > tests on Linux and verifying the same is pending. Manju is trying to get a Linux machine. Once that's in place, please run the tests on it, investigate why they are failing and then fix either the code or the tests. (In reply to comment #34) > (In reply to comment #33) > > Created attachment 226092 [details] [diff] [diff] > > Updated - Patch + tests > > > > Updated the copyright and @since information and fixed some compilation > > errors in the previous patch. > > The behavior looks OK and the tests pass on Windows 7. > > As mentioned in Comment #28, the tests were failing on Linux. Running the > > tests on Linux and verifying the same is pending. > > Manju is trying to get a Linux machine. Once that's in place, please run the > tests on it, investigate why they are failing and then fix either the code > or the tests. The tests pass on Linux also. (As mentioned in comment #31, the tests were updated with fix for Linux failures.) The patch still has issues, e.g. when I have P/aFolder P/src/afolder and try to move 'aFolder' into 'src' via DnD, it just does nothing, except for logging an error. I don't like that we behave differently (and different UI messages) when the duplication happens in the same source folder vs. duplication via second source folder. Given that the problem is minor, several attempts to solve this failed, and that the changes are quite big, I decide to not spend further time on this. |