Skip to content
This repository has been archived by the owner on Apr 24, 2021. It is now read-only.

fix #48 by detecting file type using jmimemagic Java library #49

Merged
merged 3 commits into from Dec 10, 2013
Merged

fix #48 by detecting file type using jmimemagic Java library #49

merged 3 commits into from Dec 10, 2013

Conversation

kumarshantanu
Copy link
Contributor

This PR uses jMimeMagic to detect file type if the file extension is not one among the white-listed types. Also adds .cljx and .edn to known Clojure file extensions.

@oakes
Copy link
Owner

oakes commented Dec 9, 2013

Thanks very much. Are you sure there isn't a built-in solution for reading the mime type of a file? I found some suggestions here. I only ask because this library adds 1.8MB to the final jar file size. It's fine if there is no other way; I'm just looking at the other options.

@oakes
Copy link
Owner

oakes commented Dec 9, 2013

I found that if you add :exclusions [commons-logging junit log4j] to the dependency line, it adds 1.4MB, so that's an improvement.

@kumarshantanu
Copy link
Contributor Author

If we can make the minimum requirement to be Java 7 for running Nightcode, we do not need any 3rd party library to detect the file type. Java 7 has the API to probe the content. This might be a breaking change for some; Nightcode should switch to version 0.2.0 to include this change. Please let me know what you think - I will update the pull request.

@oakes
Copy link
Owner

oakes commented Dec 10, 2013

A lot of Mac users are still using Java 6, because that's what it comes with. I wonder if there is a simpler solution. Maybe we should just remove the contains? check entirely, and just put a sane upper limit on the file size (such as 10MB). That way, if you accidentally click on a binary file, it will display gibberish in the editor but won't accidentally load a 1GB file into memory. What do you think?

@kumarshantanu
Copy link
Contributor Author

I tried running Nightcode on Java 6 and got the below exception:

Exception in thread "AWT-EventQueue-0" java.lang.NoClassDefFoundError: java/awt/Window$Type
    at java.lang.Class.getDeclaredMethods0(Native Method)
    at java.lang.Class.privateGetDeclaredMethods(Class.java:2436)
    at java.lang.Class.getDeclaredMethod(Class.java:1937)
    at java.awt.Component.isCoalesceEventsOverriden(Component.java:5982)
    at java.awt.Component.access$500(Component.java:169)
    at java.awt.Component$3.run(Component.java:5936)
    at java.awt.Component$3.run(Component.java:5934)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.awt.Component.checkCoalescing(Component.java:5933)
    at java.awt.Component.<init>(Component.java:5902)
    at java.awt.Container.<init>(Container.java:249)
    at java.awt.Window.<init>(Window.java:432)
    at java.awt.Frame.<init>(Frame.java:403)
    at java.awt.Frame.<init>(Frame.java:368)
    at javax.swing.JFrame.<init>(JFrame.java:158)
    at seesaw.core.proxy$javax.swing.JFrame$Tag$a79ba523.<init>(Unknown Source)
    at seesaw.core$frame.doInvoke(core.clj:2870)
    at clojure.lang.RestFn.invoke(RestFn.java:805)
    at nightcode.core$_main$fn__11277.invoke(core.clj:124)
    at clojure.lang.AFn.applyToHelper(AFn.java:159)
    at clojure.lang.AFn.applyTo(AFn.java:151)
    at clojure.core$apply.invoke(core.clj:617)
    at seesaw.invoke$invoke_later_STAR_$fn__485.invoke(invoke.clj:14)
    at clojure.lang.AFn.run(AFn.java:24)
    at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209)
    at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:672)
    at java.awt.EventQueue.access$400(EventQueue.java:81)
    at java.awt.EventQueue$2.run(EventQueue.java:633)
    at java.awt.EventQueue$2.run(EventQueue.java:631)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.AccessControlContext$1.doIntersectionPrivilege(AccessControlContext.java:87)
    at java.awt.EventQueue.dispatchEvent(EventQueue.java:642)
    at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)
    at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)
    at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161)
    at java.awt.EventDispatchThread.run(EventDispatchThread.java:122)
Caused by: java.lang.ClassNotFoundException: java.awt.Window$Type
    at java.net.URLClassLoader$1.run(URLClassLoader.java:202)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.net.URLClassLoader.findClass(URLClassLoader.java:190)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:306)
    at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:301)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:247)
    ... 38 more

It seems that Nightcode already requires Java 7 to run. My plan was to use the Java-7 feature dynamically if available else fallback to 10MB-limit based detection, which would be unnecessary if Java 7 is a requirement.

@birdspider
Copy link

JRE6 had its end in Feb2013 (http://www.oracle.com/technetwork/java/eol-135779.html),
JRE7 is installable on mac (>=Lion AFAIK)

-- just my 2c

@oakes
Copy link
Owner

oakes commented Dec 10, 2013

I wasn't aware there were still problems running it with Java 6. I compiled the latest version with OpenJDK 6, so that is puzzling. Perhaps requiring Java 7 is the best option then. If you modify the PR with the change I'd be happy to accept. I'll make sure to mention the change when I release it.

@kumarshantanu
Copy link
Contributor Author

It was my fault that I built the JAR with Java 7 and ran it on Java 6 - the problem turned out to be AOT compilation that required Java 7 classes. I have updated the PR in a way that on Java 7 the detection will work using Java-7 API via reflection and on Java 6 it falls back on length check.

Will appreciate if somebody can test in other ways - I might have forgotten about corner cases.

@oakes
Copy link
Owner

oakes commented Dec 10, 2013

I tried the change and it seems to have issues when I select any file with a non-whitelisted extension. I don't see any error output in the console, however.

@oakes
Copy link
Owner

oakes commented Dec 10, 2013

Maybe reflecting Java 7 functions won't work since we are targeting Java 6 in the project.clj? It may be less error prone to just check the file size.

@kumarshantanu
Copy link
Contributor Author

Did you run into the issue on Java 7? It's working on Java 7 + 64-bit Ubuntu for me. Only in Java 6 I can see somehow the non-whitelisted files are not working.

@oakes oakes merged commit 6fc46f1 into oakes:master Dec 10, 2013
@oakes
Copy link
Owner

oakes commented Dec 10, 2013

I switched to compile with OpenJDK 7 and it seems to work now. Technically this may still cause issues for Mac users running Java 6 -- see this issue for reference. For some reason, Java 6 on OS X has problems running Swing apps compiled with JDK 7, even when they target Java 6. That said, I think it's an OK trade-off to make since Java 6 is so outdated.

@kumarshantanu
Copy link
Contributor Author

I found the issue - I was not catching ClassNotFoundException. Now it works on both Java 7 (type detection) and Java 6 (size detection) but the behavior would appear inconsistent to the user. Should I fix it by disallowing to open non-whitelisted file types on Java 6? Should I send a new PR?

@oakes
Copy link
Owner

oakes commented Dec 10, 2013

Sure please do. Would this allow me to compile with OpenJDK 6?
On Dec 10, 2013 12:04 PM, "Shantanu Kumar" notifications@github.com wrote:

I found the issue - I was not catching ClassNotFoundException. Now it
works on both Java 7 (type detection) and Java 6 (size detection) but the
behavior would appear inconsistent to the user. Should I fix it by
disallowing to open non-whitelisted file types on Java 6? Should I send a
new PR?


Reply to this email directly or view it on GitHubhttps://github.com//pull/49#issuecomment-30246381
.

@kumarshantanu
Copy link
Contributor Author

I believe it should. I compiled using Sun JDK 6 and ran using both Sun JDK 6 and Sun JDK 7.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants