Created attachment 151855 [details]
Proposed patch

Please review new method hasMIMEType(FileObject, String...) in class FileUtil.

Thank you.

The patch seems good to me. Thanks for it, Jardo!

TM01: You said that you could return - instead of boolean - some Options (?) class which could tell us easily whether the FileObject has one of the given MIME types or even what MIME type exactly it has if needed. This sounds good to me, moreover, getMIMEType(FileObject, String...) could be deprecated then (since its usage is really not intuitive, at least I think so :).

Thanks.

(In reply to Tomas Mysik from comment #3)
> TM01: You said that you could return - instead of boolean - some Options (?)
> class which could tell us easily whether the FileObject has one of the given
> MIME types or even what MIME type exactly it has if needed. This sounds good
> to me, moreover, getMIMEType(FileObject, String...) could be deprecated then
> (since its usage is really not intuitive, at least I think so :).
I thought about using java.util.Optional, but then I realized that it was introduced in JDK8, so we will need to wait some time before using it in NetBeans.
(And introducing a custom alternative to Optional would be overkill, I think.)

Another alternative is havening method like "String checkMIMEType(FileObject, String...)", which can return the actual MIME type (including "content/unknown") if it is among the queried MIME types, or null if MIME type of the file is something else.

This is very similar to the solution with optional, but can be used in NetBeans immediately. (And also allows us to deprecate getMIMEType(FileObject, String...).) 
I'm just slightly afraid that it is not too intuitive.

What would you prefer? Waiting for Optional, using just hasMIMEType, or having checkMIMEType possibly returning null?

Thank you.

I have no strong opinion on it. I would like to see the current getMIMEType() deprecated since its usage is really not nice and I see this API review as a chance to do it, of course :) But I am definitely OK with just adding the boolean alternative. Maybe Tomas will have some preference?

Thanks.

Created attachment 151986 [details]
Proposed Patch v2

> Another alternative is havening method like "String checkMIMEType(FileObject, 
> String...)", which can return the actual MIME type (including 
> "content/unknown") if it is among the queried MIME types, or null if MIME type
> of the file is something else.
I've modified the patch to use this approach. 
Method FileUtil.getMIMEType(FileObject, String...) was deprecated there.

Please check the updated patch.
Thank you for your comments.

TM02: The unit test should check the exact returned value, not only <not null>.

Otherwise the patch seems good to me. Thanks.

Why I don't understand FileUtil.getMIMEType is not sufficient and is even considered contra-intuitive? I read all the comments and the final diff twice, but I don't see any explanation. Did I miss some discussion at cafeteria?

(In reply to Jaroslav Tulach from comment #8)
> is even considered contra-intuitive?

Try to use it :) One needs to check the _exact_ return value since it can be different from the given/input MIME types. So, the typical usage is e.g.:

String mimeType = FileUtil.getMIMEType(file, PHP_MIME_TYPE, HTML_MIME_TYPE);
return PHP_MIME_TYPE.equals(mimeType)
        || HTML_MIME_TYPE.equals(mimeType);


Imagine that you would like to check even more MIME types... And yes, it is in the Javadoc but the usage is really contra-intuitive and easy to misuse [1].

The new method will return MIME type only if it is the same as one of the given/input MIME types so one can do just check against null.

Thanks.
[1] BTW Javadoc of the return value was improved in a separate commit, on my request :)

I see. The original purpose of the method was to avoid opening content of files in case one only needs MIME type that can be deduced from extension, etc.

What is the other value the method can return? "content/unknown" or even something else? If something else, do you know how that can happen?

(In reply to Jaroslav Tulach from comment #10)
> What is the other value the method can return? "content/unknown" or even
> something else? If something else, do you know how that can happen?

JardaH will know that better but IMHO the return value can be even the proper MIME type if it is already known for the given FileObject.

PS: I suggested to "fix" behavior of the existing getMIMEType() method but that would be a BC break, of course :)

(In reply to Jaroslav Tulach from comment #10)
> The original purpose of the method was to avoid opening content of
> files in case one only needs MIME type that can be deduced from extension,
> etc.

BTW I thought that the purpose is to speed-up MIME type recognition since we use only corresponding MIME resolvers and not all of them. But I can be wrong, of course.

(In reply to Tomas Mysik from comment #12)
> (In reply to Jaroslav Tulach from comment #10)
> > The original purpose of the method was to avoid opening content of
> > files in case one only needs MIME type that can be deduced from extension,
> > etc.
> 
> BTW I thought that the purpose is to speed-up MIME type recognition since we
> use only corresponding MIME resolvers and not all of them. But I can be
> wrong, of course.

I meant: If you wanted just text/x-java mimetype, you could ask for it and no resolvers that needed to open the file and read its content (ruby, php, etc.) would be triggered at all. So at the end it "speed-up MIME type recognition since we use only corresponding MIME resolvers and not all of them".

(In reply to Jaroslav Tulach from comment #13)
> I meant: If you wanted just text/x-java mimetype, you could ask for it and
> no resolvers that needed to open the file and read its content (ruby, php,
> etc.) would be triggered at all. So at the end it "speed-up MIME type
> recognition since we use only corresponding MIME resolvers and not all of
> them".

OK :)

> PS: I suggested to "fix" behavior of the existing getMIMEType() method but
> that would be a BC break, of course :)

Balancing bugfixes and compatiblity is tricky. I agree adding new method is overkill, but changing existing behavior would certainly hurt somebody. The only safe-enough change that comes to my mind would be:


diff -r e5e003b5048d openide.filesystems/src/org/openide/filesystems/FileUtil.java
--- a/openide.filesystems/src/org/openide/filesystems/FileUtil.java     Mon Feb 16 14:05:40 2015 +0000
+++ b/openide.filesystems/src/org/openide/filesystems/FileUtil.java     Fri Feb 20 09:00:28 2015 +0100
@@ -1308,20 +1308,39 @@
     }
 
     /** Resolves MIME type. Registered resolvers are invoked and used to achieve this goal.
-     * Resolvers must subclass MIMEResolver.
+     * Resolvers must subclass MIMEResolver. By default it is possible for the 
+     * method to return {@code content/unknown} instead of {@code null} - if
+     * you want to avoid such behavior, include <code>null</code> in the
+     * list of requested <code>withinMIMETypes</code> - in such case the
+     * return value is guaranteed to be one of the values in <code>withinMIMETypes</code>
+     * or <code>null</code>.
+     * 
      * @param fo whose MIME type should be recognized
      * @param withinMIMETypes an array of MIME types. Only resolvers whose
      * {@link MIMEResolver#getMIMETypes} contain one or more of the requested
      * MIME types will be asked if they recognize the file. It is possible for
      * the resulting MIME type to not be a member of this list.
      * @return the MIME type for the FileObject, or <code>null</code> if 
-     * the FileObject is unrecognized. It may return {@code content/unknown} instead of {@code null}.
-     * It is possible for the resulting MIME type to not be a member of given list.
+     * the FileObject is unrecognized. 
      * @since 7.13
      */
     public static String getMIMEType(FileObject fo, String... withinMIMETypes) {
         Parameters.notNull("withinMIMETypes", withinMIMETypes);  //NOI18N
-        return MIMESupport.findMIMEType(fo, withinMIMETypes);
+        String res = MIMESupport.findMIMEType(fo, withinMIMETypes);
+        if (res == null) {
+            return null;
+        }
+        boolean foundNull = false;
+        for(String t : withinMIMETypes) {
+            if (t == null) {
+                foundNull = true;
+                continue;
+            }
+            if (res.equals(t)) {
+                return t;
+            }
+        }
+        return foundNull ? null : res;
     }
     
     /** Registers specified extension to be recognized as specified MIME type.
diff -r e5e003b5048d openide.filesystems/test/unit/src/org/openide/filesystems/FileUtilTest.java
--- a/openide.filesystems/test/unit/src/org/openide/filesystems/FileUtilTest.java       Mon Feb 16 14:05:40 2015 +0000
+++ b/openide.filesystems/test/unit/src/org/openide/filesystems/FileUtilTest.java       Fri Feb 20 09:00:28 2015 +0100
@@ -459,6 +459,7 @@
         } catch (NullPointerException npe) {
             // exception correctly thrown
         }
+        assertNull(fo.getMIMEType("text/x-fakemime", null));
         
         fo = FileUtil.createData(testFolder, "fo2.mime1");
         withinMIMETypes = new String[0];

> Balancing bugfixes and compatiblity is tricky. I agree adding new method is
> overkill, but changing existing behavior would certainly hurt somebody. The
> only safe-enough change that comes to my mind would be:
> diff ...
That would work. The intuitiveness is still not too high, but client code would be significantly shorter. Adding example code and recommendation to use null in withinMIMETypes into JavaDoc could be helpful.

So if Tomas is also fine with extra null value in MIME list, I'll integrate your solution (and add example code into JavaDoc).
Thank you very much.

(In reply to Jaroslav Havlin from comment #16)
> So if Tomas is also fine with extra null value in MIME list, I'll integrate
> your solution (and add example code into JavaDoc).
> Thank you very much.

Yes, I like it! Just please verify that one would not get the real MIME type of the FO if it is different from the given list of MIME types; in other words, the return value must be one of the given MIME types or null.

Thanks!

Fixed.
http://hg.netbeans.org/core-main/rev/aaa72c7b4958

I've realized that there is now slight inconsistency between FileUtil.getMIMEType(FileObject, String...) and FileObject.getMIMEType(String...).

Custom implementations of the FileObject's method don't support new semantics of null in the list, so calling fo.getMIMEType(null, "my/mime-type") is unsafe.
FileUtil.getMIMEType(fo, null, "my/mime-type") should always work correctly.
Hopefully this is not a big issue.

Thank you for your comments.

Integrated into 'main-silver', will be available in build *201502210001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)

Changeset: http://hg.netbeans.org/main-silver/rev/aaa72c7b4958
User: Jaroslav Havlin <jhavlin@netbeans.org>
Log: #250264: Add convenient method for checking MIME-types

Patch by Jaroslav Tulach