TrueZIP
  1. TrueZIP
  2. TRUEZIP-321

TrueZip does not detect sub archives when Java's temp directory does not exist

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: TrueZIP 7.7, TrueZIP 7.7.2
    • Fix Version/s: TrueZIP 7.7.3
    • Labels:
      None
    • Environment:

      Windows 7, 32 or 64 Bit, Java 1.6.0_26-32bit

      Description

      If the system property java.io.tmpdir refers to a non-existing directory, TrueZip is not able to detect or access sub archives.

      The assertion from the following test excerpt fails:

      System.setProperty("java.io.tmpdir", "target/temp");
      TFile subArchive = new TFile("src/test/resources/archive.zip/files.zip");
      assertTrue("File must act like a directory: " + subArchive, file.isDirectory());
      

      I know that java.io.tmpdir should always exist. But the real world sometimes is different. At least TrueZip should test it and log a warning to the console. Otherwise, the reason for a failed archive detection is hard to find.

        Activity

        Hide
        palich added a comment -

        Attaching complete sample JUnit Test:

        import static org.junit.Assert.assertEquals;
        import static org.junit.Assert.assertTrue;
        
        import org.junit.Test;
        
        import de.schlichtherle.truezip.file.TFile;
        
        public class TempDirTest {
        
        	@Test
        	public void test() {
        		System.setProperty("java.io.tmpdir", "target/temp");
        
        		// ZIP file OK
        		TFile archive = new TFile("src/test/resources/archive.zip");
        		assertArchive(archive);
        
        		// Sub ZIP file FAILS
        		TFile subArchive = new TFile("src/test/resources/archive.zip/files.zip");
        		assertArchive(subArchive);
        	}
        
        	private void assertArchive(TFile file) {
        		assertTrue("File must exist: " + file, file.exists());
        		assertTrue("File must be detected as archive: " + file,
        				file.isArchive());
        		assertTrue("File must act like a directory: " + file,
        				file.isDirectory());
        		// See TFile#falsePositives
        		assertEquals("File length must be 0: " + file, 0l, file.length());
        	}
        }
        
        
        Show
        palich added a comment - Attaching complete sample JUnit Test: import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import org.junit.Test; import de.schlichtherle.truezip.file.TFile; public class TempDirTest { @Test public void test() { System .setProperty( "java.io.tmpdir" , "target/temp" ); // ZIP file OK TFile archive = new TFile( "src/test/resources/archive.zip" ); assertArchive(archive); // Sub ZIP file FAILS TFile subArchive = new TFile( "src/test/resources/archive.zip/files.zip" ); assertArchive(subArchive); } private void assertArchive(TFile file) { assertTrue( "File must exist: " + file, file.exists()); assertTrue( "File must be detected as archive: " + file, file.isArchive()); assertTrue( "File must act like a directory: " + file, file.isDirectory()); // See TFile#falsePositives assertEquals( "File length must be 0: " + file, 0l, file.length()); } }
        Hide
        Christian Schlichtherle added a comment -

        I've looked into this but I am still undecided what to do about it.

        I don't like the concept of logging. Most of the time logging messages is a waste of resources. And then again they still don't provide exactly the information you need to fix a problem. Overall, I don't want to litter my code with logging statements for very rare corner cases like this.

        I generally prefer to throw an IOException. However, in this particular context the IOException gets resolved as if a false positive archive file had been encountered, which is why you get these responses. Note that the Kernel and the drivers use temp files for other purposes too, not just accessing nested archive files. I cannot possibly handle all special cases. Even if I handle this special case, you can only get true or false from these boolean methods, so it doesn't help a lot.

        As of now, I think I should probably throw an AssertionError or an IllegalStateException in this case. The app should then simply terminate with a nice stack trace clearly pointing out the problem. There is no easy way to recover from this error condition at runtime, but It's easy to fix at compile time.

        Show
        Christian Schlichtherle added a comment - I've looked into this but I am still undecided what to do about it. I don't like the concept of logging. Most of the time logging messages is a waste of resources. And then again they still don't provide exactly the information you need to fix a problem. Overall, I don't want to litter my code with logging statements for very rare corner cases like this. I generally prefer to throw an IOException. However, in this particular context the IOException gets resolved as if a false positive archive file had been encountered, which is why you get these responses. Note that the Kernel and the drivers use temp files for other purposes too, not just accessing nested archive files. I cannot possibly handle all special cases. Even if I handle this special case, you can only get true or false from these boolean methods, so it doesn't help a lot. As of now, I think I should probably throw an AssertionError or an IllegalStateException in this case. The app should then simply terminate with a nice stack trace clearly pointing out the problem. There is no easy way to recover from this error condition at runtime, but It's easy to fix at compile time.
        Hide
        Christian Schlichtherle added a comment -

        What I could do is to respond to the IOException with a check if the temp dir exists. If not, then create the temp dir and retry to create the temp file. Otherwise, or if this fails, simply throw a new AssertionError().

        When applied to your case, this strategy would solve the issue by silently creating the temp dir. Only if this impossible, you would get an AssertionError. Throwing the AssertionError is a good thing because if the issue can't get resolved, then continuing the program is pretty much pointless because temp files are used at several places.

        Show
        Christian Schlichtherle added a comment - What I could do is to respond to the IOException with a check if the temp dir exists. If not, then create the temp dir and retry to create the temp file. Otherwise, or if this fails, simply throw a new AssertionError(). When applied to your case, this strategy would solve the issue by silently creating the temp dir. Only if this impossible, you would get an AssertionError. Throwing the AssertionError is a good thing because if the issue can't get resolved, then continuing the program is pretty much pointless because temp files are used at several places.
        Hide
        palich added a comment -

        I like your latest proposal, trying to create the temp directory if it does not yet exist and throwing an AssertionError otherwise. This is actually the workaround I have implemented for the software I am working on.

        When I have understood you correctly, you would check the temp directory only in case of IOExceptions. Please make sure that you repeat the current operation after creating the temp directory successfully.

        Show
        palich added a comment - I like your latest proposal, trying to create the temp directory if it does not yet exist and throwing an AssertionError otherwise. This is actually the workaround I have implemented for the software I am working on. When I have understood you correctly, you would check the temp directory only in case of IOExceptions. Please make sure that you repeat the current operation after creating the temp directory successfully.
        Hide
        Christian Schlichtherle added a comment -

        As discussed, except that an IllegalArgumentException is thrown instead of an AssertionError. Both should terminate the app, though.

        Show
        Christian Schlichtherle added a comment - As discussed, except that an IllegalArgumentException is thrown instead of an AssertionError. Both should terminate the app, though.

          People

          • Assignee:
            Christian Schlichtherle
            Reporter:
            palich
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: