Bugzilla – Full Text Bug Listing
| Summary: | FUP of bug 317264: Refactoring is not possible if the commons-lang.jar is in the path | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Satyam Kandula <satyam.kandula> | ||||||||||
| Component: | Core | Assignee: | Satyam Kandula <satyam.kandula> | ||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | amj87.iitr, daniel_megert, frederic_fusier, markus.kell.r, Olivier_Thomann, snjezana.peco | ||||||||||
| Version: | 3.6 | Flags: | daniel_megert:
pmc_approved+
Olivier_Thomann: review+ |
||||||||||
| Target Milestone: | 3.6.2 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows XP | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Satyam Kandula
Is it good enough to also check for commons-lang.jar? Are there any other names that are generally used? If not we should not check for the name of the jar but should do the enum check for all the jars. Any comments? We already saw with first patch of bug 317264 that it was too costly to test all jars. Hence, I would fix this new bug by testing the second package name in PatternSearchJob to set the SHOULD_FILTER_ENUM static variable... We could also have only test (and has 0 impact on performance) if we would change the test to see if the package name contains "commons-lang", but of course, the test would be less accurate then... (In reply to comment #2) > We already saw with first patch of bug 317264 that it was too costly to test > all jars. Please correct me if I'm wrong, but I thought the first patch from bug 317264 checked the package name for each match (after the match was found). Can't you do something similar as bug 293861 comment 34 and detect this problem already while you index a JAR? In this case, you can't skip the package, but you could store an additional bit in the index that tells if the JAR contains "enum" or "assert" in a package name. Then you can do the complete check only for JARs that are known to be problematic (performance is not an issue for those), and in the normal case, search speed is at 100%. The test for a special JAR name should be removed completely. Targeting 3.6.2 as 317264 is also in the maintenance stream. (In reply to comment #3) > In this case, you can't skip the package, but > you could store an additional bit in the index that tells if the JAR contains As far as I could see, adding additional bit could mean changing the format of the index file which is not good. (In reply to comment #2) > We already saw with first patch of bug 317264 that it was too costly to test > all jars. The first patch did full checks without any assumption. In the second patch, we only check for the 5th package element to be enum. Taking this into consideration, I don't think we will notice any performance degradation. Let me try this out -- I didn't notice the degradation on my m/c with the first patch also :(. But let me try running more tests. Markus, Do you know or see people using different names for this jar? > Markus, Do you know or see people using different names for this jar? I don't know, but we also didn't know about commons-lang.jar until it showed up. The fact that we have to touch this for 3.6.2 again is a good indication that this problem should finally be solved at its root and that name-specific fixes are not general enough. > only check for the 5th package element to be enum That's also a limitation that should be removed. AddJarFileToIndex#isValidPackageNameForClass(String) looks like a good place to add the check (and to remember whether the JAR potentially has problems.). This should be fixed once for all. (In reply to comment #7) > > only check for the 5th package element to be enum > > That's also a limitation that should be removed. We haven't seen any other places where enum is used and generalizing is having performance problem. > AddJarFileToIndex#isValidPackageNameForClass(String) looks like a good place to > add the check (and to remember whether the JAR potentially has problems.). We cannot add here because we don't have the project compliance at this time. Created attachment 186246 [details]
Proposed patch
In this patch, the name of the jar is no longer checked. However, the package name check is retained. I don't see any performance impact with this patch.
(In reply to comment #10) Released this patch on HEAD. Reopened as this needs to be fixed for 3.6.2 Created attachment 186492 [details]
Patch for 3.6 Maintenance
This is for 3.6.2.
Olivier, Dani: Need +1 to release on 3.6.2
Created attachment 186498 [details]
Jar file for the added test
This jar file need to be copied in the folder \org.eclipse.jdt.core.tests.model\workspace\JavaSearchBugs\lib\b327654 for the test to run successfully.
+1. In filterEnum: // enum was found in org.apache.commons.lang.enum at index 5 should be replaced by: // enum was found in org.apache.commons.lang.enum at index 4 Created attachment 186520 [details]
Patch for 3.6 Maintenance
Patch with Olivier's comment..
> This is for 3.6.2. > Olivier, Dani: Need +1 to release on 3.6.2 While this bug might be justified and also the fix might be OK, I failed to reproduce the problem that triggered this: bug 293861 comment 62 says: >- import the project >https://anonsvn.jboss.org/repos/jbosstools/workspace/snjeza/jbide5036 >/jbide5036b.zip >- select the test.Test.getName method and call Refactor>Rename > >You will get an exception (see jbide5036-2.log) > >The project has the commons-lang.jar library that contains the >"org.apache.commons.lang.enum" package that is invalid in JDK>=1.5. I tried those steps on 3.6.1 (even with additional step that adds commons-lang.jar) but I always run into an NPE (bug 250958). Before I start to review the patch I need reproducible steps that I can use to verify the fix. > >- import the project > >https://anonsvn.jboss.org/repos/jbosstools/workspace/snjeza/jbide5036 > >/jbide5036b.zip > >- select the test.Test.getName method and call Refactor>Rename > > > >You will get an exception (see jbide5036-2.log) > > > >The project has the commons-lang.jar library that contains the > >"org.apache.commons.lang.enum" package that is invalid in JDK>=1.5. > > I tried those steps on 3.6.1 (even with additional step that adds > commons-lang.jar) but I always run into an NPE (bug 250958). > I downloaded the ZIPs for both test cases and then mixed a and b. Bad news is that the NPE still occurs but with https://anonsvn.jboss.org/repos/jbosstools/workspace/snjeza/jbide5036/jbide5036a.zip good news is that I can reproduce this bug now using https://anonsvn.jboss.org/repos/jbosstools/workspace/snjeza/jbide5036/jbide5036b.zip Verified that the patch works. The patch itself looks good and the performance results (compared I20110112-0800 with 3.7 M4) don't indicate a problem. +1 for 3.6.2. Released on 3.6 Maintenance branch Verified for 3.6.2 using build M20110119-0834. Verified for 3.8M2 |