[GitHub] [tika] PeterAlfredLee opened a new pull request #382: Simplify some code in OPCPackageDetector#detect

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] PeterAlfredLee opened a new pull request #382: Simplify some code in OPCPackageDetector#detect

GitBox

PeterAlfredLee opened a new pull request #382:
URL: https://github.com/apache/tika/pull/382


   1. Simpify catch statement in method `detect`.
   2. Adjust some comment in method `detect`.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] tballison commented on a change in pull request #382: Simplify some code in OPCPackageDetector#detect

GitBox

tballison commented on a change in pull request #382:
URL: https://github.com/apache/tika/pull/382#discussion_r526860010



##########
File path: tika-parser-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/detect/microsoft/ooxml/OPCPackageDetector.java
##########
@@ -159,32 +159,32 @@
 
     @Override
     public MediaType detect(ZipFile zipFile, TikaInputStream stream) throws IOException {
-        //as of 4.x, POI throws an exception for non-POI OPC file types
-        //unless we change POI, we can't rely on POI for non-POI files
+        // as of 4.x, POI throws an exception for non-POI OPC file types
+        // unless we change POI, we can't rely on POI for non-POI files
         ZipEntrySource zipEntrySource = new ZipFileZipEntrySource(zipFile);
 
         // Use POI to open and investigate it for us
-        //Unfortunately, POI can throw a RuntimeException...so we
-        //have to catch that.
-        OPCPackage pkg = null;
-        MediaType type = null;
+        // Unfortunately, POI can throw a RuntimeException...so we have to catch that.
         try {
-            pkg = OPCPackage.open(zipEntrySource);
-            type = detectOfficeOpenXML(pkg);
+            OPCPackage pkg = OPCPackage.open(zipEntrySource);
+            MediaType type = detectOfficeOpenXML(pkg);
+
+            // only set the open container if we made it here
+            stream.setOpenContainer(pkg);
+            return type;
 
-        } catch (SecurityException e) {
-            closeQuietly(zipEntrySource);
-            closeQuietly(zipFile);
-            //TIKA-2571
-            throw e;
         } catch (InvalidFormatException | RuntimeException e) {
             closeQuietly(zipEntrySource);
             closeQuietly(zipFile);
-            return null;
+
+            if (e instanceof SecurityException) {

Review comment:
       I have a personal preference for catching/handling specific exceptions according to behavior of the response.  Is there a performance reason to check `instanceof` SecurityException?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tika] PeterAlfredLee commented on a change in pull request #382: Simplify some code in OPCPackageDetector#detect

GitBox
In reply to this post by GitBox

PeterAlfredLee commented on a change in pull request #382:
URL: https://github.com/apache/tika/pull/382#discussion_r527336143



##########
File path: tika-parser-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/detect/microsoft/ooxml/OPCPackageDetector.java
##########
@@ -159,32 +159,32 @@
 
     @Override
     public MediaType detect(ZipFile zipFile, TikaInputStream stream) throws IOException {
-        //as of 4.x, POI throws an exception for non-POI OPC file types
-        //unless we change POI, we can't rely on POI for non-POI files
+        // as of 4.x, POI throws an exception for non-POI OPC file types
+        // unless we change POI, we can't rely on POI for non-POI files
         ZipEntrySource zipEntrySource = new ZipFileZipEntrySource(zipFile);
 
         // Use POI to open and investigate it for us
-        //Unfortunately, POI can throw a RuntimeException...so we
-        //have to catch that.
-        OPCPackage pkg = null;
-        MediaType type = null;
+        // Unfortunately, POI can throw a RuntimeException...so we have to catch that.
         try {
-            pkg = OPCPackage.open(zipEntrySource);
-            type = detectOfficeOpenXML(pkg);
+            OPCPackage pkg = OPCPackage.open(zipEntrySource);
+            MediaType type = detectOfficeOpenXML(pkg);
+
+            // only set the open container if we made it here
+            stream.setOpenContainer(pkg);
+            return type;
 
-        } catch (SecurityException e) {
-            closeQuietly(zipEntrySource);
-            closeQuietly(zipFile);
-            //TIKA-2571
-            throw e;
         } catch (InvalidFormatException | RuntimeException e) {
             closeQuietly(zipEntrySource);
             closeQuietly(zipFile);
-            return null;
+
+            if (e instanceof SecurityException) {

Review comment:
       Ah, I was just thinking to simplify the two catch clause as they look similar. Haven't think about the performance here. :)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]