Comment 1 Jay Arthanareeswaran 2010-07-29 07:35:29 EDT

We don't allow individual files (other than JARs) in the build path, do we? The error seems to be arising from the mistake that 'X.java' is a JAR and hence represent a package fragment root. Later when we want to check whether X.java is in the classpath, it finds a match in the build path and returns the JarPackageFragmentRoot. However, the caller was expecting only an PackageFragment for a Java file.

We can prevent the exception with a check. But might still want to fix the root cause as there could be other manifestations.

Copying Markus for inputs.

Comment 2 Dani Megert 2010-07-29 07:49:40 EDT

When adding a JAR to the build path we don't check whether the file is indeed a valid JAR file (it's another discussion whether this is good or not).

Test Case:
1. create project with 'src' as source folder
2. add Foo.java into project (not 'src')
3. select Foo.java
4. context menu > Build Path > Add to Build Path
5. open the file

Even selecting the file causes several exceptions.

Comment 3 Jay Arthanareeswaran 2010-08-23 07:28:52 EDT

I am seeing (In reply to comment #2)
> When adding a JAR to the build path we don't check whether the file is indeed a
> valid JAR file (it's another discussion whether this is good or not).
> 
> Test Case:
> 1. create project with 'src' as source folder
> 2. add Foo.java into project (not 'src')
> 3. select Foo.java
> 4. context menu > Build Path > Add to Build Path
> 5. open the file
> 
> Even selecting the file causes several exceptions.

I am seeing multiple exception and of two types:
(i) ClassCastException, which this bug has reported
(ii) ZipException, when we try to open the file assuming it's a JAR.

I was thinking it might be 'too little too late' to address the exceptions as and when they occur. For instance, the package explorer would already be showing this as an archive. 

Dani, please let me know what you think.

Comment 4 Dani Megert 2010-08-23 07:43:49 EDT

> I was thinking it might be 'too little too late' to address the exceptions as
> and when they occur. For instance, the package explorer would already be
> showing this as an archive. 
> 
> Dani, please let me know what you think.
What is the question? ;-)

Comment 5 Jay Arthanareeswaran 2010-08-23 07:50:32 EDT

(In reply to comment #4)
> > I was thinking it might be 'too little too late' to address the exceptions as
> > and when they occur. For instance, the package explorer would already be
> > showing this as an archive. 
> > 
> > Dani, please let me know what you think.
> What is the question? ;-)

Ah, I was thinking that warning the user about the invalid entry he is trying to add right when he does it (and perhaps disallowing too) might be an easier approach. Because once we allow the entry in the classpath, then we have lot more problems to deal with and also it might become costlier in terms of performance as well. I wanted to know if there is a better way.

Comment 6 Dani Megert 2010-08-23 08:59:30 EDT

>Ah, I was thinking that warning the user about the invalid entry he is trying
>to add right when he does it
JDT Core would have to know all archive formats that are on the build path and validate them e.g. in JavaConventions.validateClasspath(IJavaProject, IClasspathEntry[], IPath). Also, when doing this you need to make sure not to break existing clients, see e.g. comments in bug 182360.

Comment 7 Jay Arthanareeswaran 2010-08-23 10:35:03 EDT

(In reply to comment #6)
> >Ah, I was thinking that warning the user about the invalid entry he is trying
> >to add right when he does it
> JDT Core would have to know all archive formats that are on the build path and
> validate them e.g. in JavaConventions.validateClasspath(IJavaProject,
> IClasspathEntry[], IPath). Also, when doing this you need to make sure not to
> break existing clients, see e.g. comments in bug 182360.

Actually I meant validating the file content and see if it's indeed an archive and not just checking for their extension. Do you think this is reasonable?

Comment 8 Dani Megert 2010-08-24 01:58:44 EDT

>Actually I meant validating the file content 
How would you do that?

Comment 9 Jay Arthanareeswaran 2010-08-24 02:08:42 EDT

(In reply to comment #8)
> >Actually I meant validating the file content 
> How would you do that?

I don't know if there are other ways. But what I had in mind was opening the archive and checking whether it's a valid archive or not. Of course, we don't want to know what the archive or the MANIFEST has.

Comment 10 Dani Megert 2010-08-24 02:53:09 EDT

>But what I had in mind was opening the archive 
As I wrote:
  Also, when doing this you need to make sure not to
  break existing clients, see e.g. comments in bug 182360.
So, how do you detect a RAR (or WAR) case? How do you read a RAR or WAR? I don't think we can do that. So, I guess the only thing we can do is to either live with the exceptions or be more tolerant i.e. since we allow any kind of file as archive we have to just ignore them at the point where we can't read it.

Comment 11 Markus Keller 2010-08-24 05:44:29 EDT

> [..] just ignore them [the exceptions] at the point where we can't read it.

+1. We might also have to fix some places in the UI code, but the Java model should be made resilient first.

Comment 12 Jay Arthanareeswaran 2010-08-24 06:04:31 EDT

(In reply to comment #10)
> >But what I had in mind was opening the archive 
> As I wrote:
>   Also, when doing this you need to make sure not to
>   break existing clients, see e.g. comments in bug 182360.
> So, how do you detect a RAR (or WAR) case? How do you read a RAR or WAR? I
> don't think we can do that. So, I guess the only thing we can do is to either
> live with the exceptions or be more tolerant i.e. since we allow any kind of
> file as archive we have to just ignore them at the point where we can't read
> it.

I get it now. Just for the sake of concluding my point, I meant that we should not allow the user to add files (formats) that we can't open. But as you said since we allow any format already, we can't just disallow them. I will see how we can handle these exceptions better.

Comment 13 Jay Arthanareeswaran 2010-08-31 08:35:07 EDT

Created attachment 177828 [details]
Proposed fix

This patch should take care of the exception that is reported in comment #0. There is one more exception that is thrown with the attached stack trace. However, it gets logged only in the log file and doesn't throw any dialogs. So, I guess we need the log file reporting.

java.util.zip.ZipException: error in opening zip file
	at java.util.zip.ZipFile.open(Native Method)
	at java.util.zip.ZipFile.<init>(ZipFile.java:212)
	at java.util.zip.ZipFile.<init>(ZipFile.java:142)
	at java.util.zip.ZipFile.<init>(ZipFile.java:156)
	at org.eclipse.jdt.internal.core.JavaModelManager.getZipFile(JavaModelManager.java:2521)
	at org.eclipse.jdt.internal.core.JarPackageFragmentRoot.getJar(JarPackageFragmentRoot.java:152)
	at org.eclipse.jdt.internal.core.JarPackageFragmentRoot.computeChildren(JarPackageFragmentRoot.java:78)
	at org.eclipse.jdt.internal.core.PackageFragmentRoot.buildStructure(PackageFragmentRoot.java:154)
	at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:258)
	at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:515)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:252)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:238)
	at org.eclipse.jdt.internal.core.JavaElement.getChildren(JavaElement.java:193)

Comment 14 Jay Arthanareeswaran 2010-08-31 08:43:24 EDT

(In reply to comment #13)
> Created an attachment (id=177828) [details]
> Proposed fix
> 
> This patch should take care of the exception that is reported in comment #0.

I will be adding additional tests. The existing ones pass.

Comment 15 Jay Arthanareeswaran 2010-09-01 23:52:58 EDT

Created attachment 178015 [details]
Patch with test

The same patch with test. All tests pass. Ayush, can you take a look at this patch, please?

Comment 16 Ayushman Jain 2010-09-06 10:20:19 EDT

(In reply to comment #15)
> Created an attachment (id=178015) [details]
> Patch with test
> 
> The same patch with test. All tests pass. Ayush, can you take a look at this
> patch, please?

Jay, I think we should rectify the documentation here to align with the change. The current doc for org.eclipse.jdt.internal.core.JavaModelManager.determineIfOnClasspath(IResource, IJavaProject) says "Returns.... <code>null</code> if the given resource is not on the classpath of the given project." 
Clearly, the given resource in this case is a java file, and it *IS* in the classpath. And yet we now return null. So maybe change the doc to say
"Returns.... <code>null</code> if the given resource is not on the classpath of the given project or if it is a file with a java like extension."

Comment 17 Ayushman Jain 2010-09-06 10:26:40 EDT

(In reply to comment #16)
>[...]

Also, even with the patch i do get a CoreException when I add a java file to the build path(steps in comment 2). Is that ok?

Comment 18 Jay Arthanareeswaran 2010-09-08 09:27:48 EDT

(In reply to comment #17)
> (In reply to comment #16)
> >[...]
> 
> Also, even with the patch i do get a CoreException when I add a java file to
> the build path(steps in comment 2). Is that ok?

Just checked with Ayush - this is the same as the one mentioned in comment #13. As I mentioned, we need this exception to be logged.

(In reply to comment #16)
> Jay, I think we should rectify the documentation here to align with the change.
> The current doc for
> org.eclipse.jdt.internal.core.JavaModelManager.determineIfOnClasspath(IResource,
> IJavaProject) says "Returns.... <code>null</code> if the given resource is not
> on the classpath of the given project." 
> Clearly, the given resource in this case is a java file, and it *IS* in the
> classpath. And yet we now return null. So maybe change the doc to say
> "Returns.... <code>null</code> if the given resource is not on the classpath of
> the given project or if it is a file with a java like extension."

A valid classpath entry of type CPE_LIBRARY must either be a folder or an archive file. So, in this case, I am not really sure if we can say X.java is on the classpath. I would like to hear from Olivier/Dani on this.

Comment 19 Markus Keller 2010-09-08 10:07:50 EDT

AFAIK, the fix for bug 182360 etc. only allowed JARs with any extension on the classpath. I.e. even if a file is called archive.rar, we still consider it as a JAR (and not as a RAR-compressed file). If the file is not really a JAR the JDT/Core APIs should just throw the specified exceptions, but not a ClassCastException.

Can we keep this bug just for the CCE? Bug 229042 is the request for a build problem for invalid JARs and bug 229038 is about improving the UI (e.g. warn the user when he tries to add an invalid file to the build path).

Comment 20 Olivier Thomann 2010-09-08 10:55:42 EDT

I think  we should throw ClasspathEntry.AssertionFailedException and make sure this is specified in the documentation. We should explain that a runtime exception can be thrown when the library path is invalid.

Comment 21 Markus Keller 2010-09-08 12:38:36 EDT

> we should throw ClasspathEntry.AssertionFailedException

Where do you want to throw that? Note that throwing new exceptions is an API change, and I don't think that's necessary nor useful here.

- JavaCore#create(IResource, IJavaProject) clearly states that it returns null if no Java element could be found
- APIs like JavaCore#newLibraryEntry(IPath, IPath, IPath) don't validate the given file
- JavaConventions#validateClasspath(IJavaProject, IClasspathEntry[], IPath) is only about structural problems, not about problems with individual entries

I actually think it would be better to reduce the amount of exceptions thrown. But that's bug 229042 (create a build path error and then suppress all exceptions and consider unreadable JARs as empty).

Comment 22 Olivier Thomann 2010-09-08 12:41:57 EDT

(In reply to comment #21)
> > we should throw ClasspathEntry.AssertionFailedException
> 
> Where do you want to throw that? Note that throwing new exceptions is an API
> change, and I don't think that's necessary nor useful here.
This is already throw at different locations.

Comment 23 Markus Keller 2010-09-08 13:36:29 EDT

> This is already throw at different locations.
Yes, but only for structurally wrong entries that violate Javadocs.

Comment 24 Jay Arthanareeswaran 2010-09-08 23:29:39 EDT

We will just use the patch from comment #15 for this bug. 

Ayush, for your comment about JavaModelManager.determineIfOnClasspath() returning null: We allow the .java file to be added to the build path only because we think it could be an archive. But it's not. We will use bug #229042 to fix that. Besides, it's an internal API. Hence, I think we could just do with the fix for the CCE.

Comment 25 Jay Arthanareeswaran 2010-09-09 02:50:00 EDT

Release in HEAD for 3.7M2.

Comment 26 Ayushman Jain 2010-09-09 02:59:04 EDT

(In reply to comment #24)
> We will just use the patch from comment #15 for this bug. 
> 
> Ayush, for your comment about JavaModelManager.determineIfOnClasspath()
> returning null: We allow the .java file to be added to the build path only
> because we think it could be an archive. But it's not. We will use bug #229042
> to fix that. Besides, it's an internal API. Hence, I think we could just do
> with the fix for the CCE.

Sounds good.

Comment 27 Olivier Thomann 2010-09-14 12:55:10 EDT

We no longer get the stack trace.
Now we have an entry in the error log with:
org.eclipse.core.runtime.CoreException: I/O exception
	at org.eclipse.jdt.internal.core.JavaModelManager.getZipFile(JavaModelManager.java:2594)
	at org.eclipse.jdt.internal.core.JarPackageFragmentRoot.getJar(JarPackageFragmentRoot.java:152)
	at org.eclipse.jdt.internal.core.JarPackageFragmentRoot.computeChildren(JarPackageFragmentRoot.java:78)
	at org.eclipse.jdt.internal.core.JavaProjectElementInfo.initializePackageNames(JavaProjectElementInfo.java:252)
	at org.eclipse.jdt.internal.core.JavaProjectElementInfo.getProjectCache(JavaProjectElementInfo.java:225)
	at org.eclipse.jdt.internal.core.JavaProjectElementInfo.newNameLookup(JavaProjectElementInfo.java:290)
	at org.eclipse.jdt.internal.core.JavaProject.newNameLookup(JavaProject.java:2289)
	at org.eclipse.jdt.internal.core.SearchableEnvironment.<init>(SearchableEnvironment.java:57)
	at org.eclipse.jdt.internal.core.SearchableEnvironment.<init>(SearchableEnvironment.java:64)
	at org.eclipse.jdt.internal.core.CancelableNameEnvironment.<init>(CancelableNameEnvironment.java:26)
	at org.eclipse.jdt.internal.core.CompilationUnitProblemFinder.process(CompilationUnitProblemFinder.java:175)
	at org.eclipse.jdt.internal.core.CompilationUnitProblemFinder.process(CompilationUnitProblemFinder.java:268)
	at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.makeConsistent(ReconcileWorkingCopyOperation.java:190)
	at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.executeOperation(ReconcileWorkingCopyOperation.java:89)
	at org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:728)
	at org.eclipse.jdt.internal.core.JavaModelOperation.runOperation(JavaModelOperation.java:788)
	at org.eclipse.jdt.internal.core.CompilationUnit.reconcile(CompilationUnit.java:1244)
	at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:126)
	at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.access$0(JavaReconcilingStrategy.java:108)
	at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy$1.run(JavaReconcilingStrategy.java:89)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
	at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:87)
	at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:151)
	at org.eclipse.jdt.internal.ui.text.CompositeReconcilingStrategy.reconcile(CompositeReconcilingStrategy.java:86)
	at org.eclipse.jdt.internal.ui.text.JavaCompositeReconcilingStrategy.reconcile(JavaCompositeReconcilingStrategy.java:102)
	at org.eclipse.jface.text.reconciler.MonoReconciler.process(MonoReconciler.java:77)
	at org.eclipse.jface.text.reconciler.AbstractReconciler$BackgroundThread.run(AbstractReconciler.java:206)
Caused by: java.util.zip.ZipException: error in opening zip file
	at java.util.zip.ZipFile.open(Native Method)
	at java.util.zip.ZipFile.<init>(ZipFile.java:114)
	at java.util.zip.ZipFile.<init>(ZipFile.java:131)
	at org.eclipse.jdt.internal.core.JavaModelManager.getZipFile(JavaModelManager.java:2588)
	... 26 more

And the file is added to the build path.
Clicking on it with generate the same stack again.

Isn't it possible not to add it to the build path as this entry is invalid ?

Verified for 3.7M2 as the stacktrace no longer occurs, but I think we should still try to improve the behavior.

Comment 28 Markus Keller 2010-09-14 13:15:01 EDT

(In reply to comment #27)
I agree, see comment #19:

> Bug 229042 is the request for a build problem for invalid JARs

If there's a build problem, you could also suppress the I/O exception (or just not consider the JAR as available on the build path).

> bug 229038 is about improving the UI (e.g. warn
> the user when he tries to add an invalid file to the build path).

Comment 29 Dani Megert 2010-09-15 01:42:55 EDT

>Verified for 3.7M2 as the stacktrace no longer occurs, but I think we should
>still try to improve the behavior.
Agreed.

Comment 30 Dani Megert 2010-09-15 01:43:12 EDT

If too late to polish then move to M3.

Comment 31 Jay Arthanareeswaran 2010-10-22 15:03:15 EDT

With the latest patch posted for bug 229042, comment 4, the list invalid archives is cached and hence the multiple exception stacks in the log are avoided. Will update here once the fix for 229042 has been released.

Comment 32 Jay Arthanareeswaran 2010-10-25 06:47:58 EDT

With bug 229042 fixed, this one gets fixed too. The CoreException/IOException that was being thrown when the X.java is opened has been removed and instead only an error message is printed in the log. This happens only when the user tries to open the file or when the project's classpath is computed.

Comment 33 Jay Arthanareeswaran 2010-10-25 06:49:00 EDT

Resolved through bug 229042.

Comment 34 Satyam Kandula 2010-10-26 07:04:58 EDT

Verified for 3.7M3 using build I20101025-0901