Incorporating suggestions from @josemduarte

sbliven · sbliven · commit f5b325f76ee9 · 2018-06-12T15:54:06.000+02:00
See comments on biojava#774
diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java
@@ -636,7 +636,7 @@ private void parseCathDomall(BufferedReader bufferedReader) throws IOException{
 
 	protected void downloadFileFromRemote(URL remoteURL, File localFile) throws IOException{
 //        System.out.println("downloading " + remoteURL + " to: " + localFile);
-		LOGGER.info("Downloaded file {} to local file {}", remoteURL, localFile);
+		LOGGER.info("Downloading file {} to local file {}", remoteURL, localFile);
 
 		long timeS = System.currentTimeMillis();
 		File tempFile  = File.createTempFile(FileDownloadUtils.getFilePrefix(localFile), "."+ FileDownloadUtils.getFileExtension(localFile));
diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java
@@ -35,6 +35,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
+import java.nio.file.Files;
 import java.text.ParseException;
 import java.text.SimpleDateFormat;
 import java.util.*;
@@ -405,8 +406,9 @@ public void prefetchStructure(String pdbId) throws IOException {
 	 * Attempts to delete all versions of a structure from the local directory.
 	 * @param pdbId
 	 * @return True if one or more files were deleted
+	 * @throws IOException if the file cannot be deleted
 	 */
-	public boolean deleteStructure(String pdbId){
+	public boolean deleteStructure(String pdbId) throws IOException{
 		boolean deleted = false;
 		// Force getLocalFile to check in obsolete locations
 		ObsoleteBehavior obsolete = getObsoleteBehavior();
@@ -663,8 +665,9 @@ protected File getDir(String pdbId, boolean obsolete) {
 	 * Searches for previously downloaded files
 	 * @param pdbId
 	 * @return A file pointing to the existing file, or null if not found
+	 * @throws IOException If the file exists but is empty and can't be deleted
 	 */
-	public File getLocalFile(String pdbId) {
+	public File getLocalFile(String pdbId) throws IOException {
 
 		// Search for existing files
 
@@ -692,11 +695,8 @@ public File getLocalFile(String pdbId) {
 					if ( f.exists()) {
 						// delete files that are too short to have contents
 						if( f.length() < MIN_PDB_FILE_SIZE ) {
-							boolean success = f.delete();
-							if( ! success) {
-								return null;
-							}
-							assert(!f.exists());
+							Files.delete(f.toPath());
+							return null;
 						}
 						return f;
 					}
@@ -708,9 +708,11 @@ public File getLocalFile(String pdbId) {
 	}
 
 	protected boolean checkFileExists(String pdbId){
-		File path =  getLocalFile(pdbId);
-		if ( path != null)
-			return true;
+		try {
+			File path =  getLocalFile(pdbId);
+			if ( path != null)
+				return true;
+		} catch(IOException e) {}
 		return false;
 	}
 
diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/mmcif/ChemCompGroupFactory.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/mmcif/ChemCompGroupFactory.java
@@ -69,7 +69,7 @@ public static ChemComp getChemComp(String recordName){
 	 * code executed afterwards.
 	 * <p>
 	 * Changing the provider also resets the cache, so any groups
-	 * previously accessed will be re-downloaded and reread.
+	 * previously accessed will be reread or re-downloaded.
 	 *
 	 * @param provider
 	 */
@@ -85,7 +85,9 @@ public static ChemCompProvider getChemCompProvider(){
 	}
 	
 	/**
-	 * Force the cache to be reset.
+	 * Force the in-memory cache to be reset.
+	 * 
+	 * Note that the ChemCompProvider may have additional memory or disk caches that need to be cleared too.
 	 */
 	public static void clearCache() {
 		cache.clear();
diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/mmcif/DownloadChemCompProvider.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/mmcif/DownloadChemCompProvider.java
@@ -306,6 +306,7 @@ public  ChemComp getChemComp(String recordName) {
 			fallback = new ReducedChemCompProvider();
 		}
 
+		logger.warn("Falling back to ReducedChemCompProvider for {}. This could indicate a network error.", recordName);
 		return fallback.getChemComp(recordName);
 
 	}
@@ -347,6 +348,8 @@ private static  boolean fileExists(String recordName){
 
 		// delete files that are too short to have contents
 		if( f.length() < LocalPDBDirectory.MIN_PDB_FILE_SIZE ) {
+			// Delete defensively.
+			// Note that if delete is unsuccessful, we re-download the file anyways
 			f.delete();
 			return false;
 		}
diff --git a/biojava-structure/src/test/java/org/biojava/nbio/structure/TestAtomCache.java b/biojava-structure/src/test/java/org/biojava/nbio/structure/TestAtomCache.java
@@ -46,7 +46,7 @@ public class TestAtomCache {
 	private AtomCache cache;
 
 	@Before
-	public void setUp() {
+	public void setUp() throws IOException {
 		cache = new AtomCache();
 
 		// Delete files which were cached in previous tests
diff --git a/biojava-structure/src/test/java/org/biojava/nbio/structure/align/util/AtomCacheTest.java b/biojava-structure/src/test/java/org/biojava/nbio/structure/align/util/AtomCacheTest.java
@@ -404,7 +404,7 @@ public void testEmptyChemComp() throws IOException, StructureException {
 			assertTrue(chem.getAtoms().size() > 0);
 			assertEquals("NON-POLYMER", chem.getType());
 		} finally {
-//			FileDownloadUtils.deleteDirectory(tmpCache);
+			FileDownloadUtils.deleteDirectory(tmpCache);
 		}
 	}
 	
@@ -467,7 +467,7 @@ public void testEmptyGZChemComp() throws IOException, StructureException {
 			assertTrue(chem.getAtoms().size() > 0);
 			assertEquals("NON-POLYMER", chem.getType());
 		} finally {
-//			FileDownloadUtils.deleteDirectory(tmpCache);
+			FileDownloadUtils.deleteDirectory(tmpCache);
 		}
 	}
 	

Original file line number	Diff line number	Diff line change
`@@ -306,6 +306,7 @@ public ChemComp getChemComp(String recordName) {`
`306`	`306`	`fallback = new ReducedChemCompProvider();`
`307`	`307`	`}`
`308`	`308`
	`309`	`+ logger.warn("Falling back to ReducedChemCompProvider for {}. This could indicate a network error.", recordName);`
`309`	`310`	`return fallback.getChemComp(recordName);`
`310`	`311`
`311`	`312`	`}`
`@@ -347,6 +348,8 @@ private static boolean fileExists(String recordName){`
`347`	`348`
`348`	`349`	`// delete files that are too short to have contents`
`349`	`350`	`if( f.length() < LocalPDBDirectory.MIN_PDB_FILE_SIZE ) {`
	`351`	`+ // Delete defensively.`
	`352`	`+ // Note that if delete is unsuccessful, we re-download the file anyways`
`350`	`353`	`f.delete();`
`351`	`354`	`return false;`
`352`	`355`	`}`
Original file line number	Diff line number	Diff line change
`@@ -404,7 +404,7 @@ public void testEmptyChemComp() throws IOException, StructureException {`
`404`	`404`	`assertTrue(chem.getAtoms().size() > 0);`
`405`	`405`	`assertEquals("NON-POLYMER", chem.getType());`
`406`	`406`	`} finally {`
`407`		`-// FileDownloadUtils.deleteDirectory(tmpCache);`
	`407`	`+ FileDownloadUtils.deleteDirectory(tmpCache);`
`408`	`408`	`}`
`409`	`409`	`}`
`410`	`410`
`@@ -467,7 +467,7 @@ public void testEmptyGZChemComp() throws IOException, StructureException {`
`467`	`467`	`assertTrue(chem.getAtoms().size() > 0);`
`468`	`468`	`assertEquals("NON-POLYMER", chem.getType());`
`469`	`469`	`} finally {`
`470`		`-// FileDownloadUtils.deleteDirectory(tmpCache);`
	`470`	`+ FileDownloadUtils.deleteDirectory(tmpCache);`
`471`	`471`	`}`
`472`	`472`	`}`
`473`	`473`