Merge pull request #248 from whym/fixextension2

Further attempts to reduce overwrites

app/src/main/java/fr/free/nrw/commons/Utils.java

@@ -25,6 +25,7 @@ import java.text.SimpleDateFormat;
 import java.util.Date;
 import java.util.TimeZone;
 import java.util.concurrent.Executor;
+import java.util.regex.Pattern;
 
 import javax.xml.transform.TransformerConfigurationException;
 import javax.xml.transform.TransformerException;
@@ -242,13 +243,13 @@ public class Utils {
     }
 
     public static String fixExtension(String title, String extension) {
+        Pattern jpegPattern = Pattern.compile("\\.jpeg$", Pattern.CASE_INSENSITIVE);
+
         // People are used to ".jpg" more than ".jpeg" which the system gives us.
         if (extension != null && extension.toLowerCase().equals("jpeg")) {
             extension = "jpg";
         }
-        if (title.toLowerCase().endsWith(".jpeg")) {
+        title = jpegPattern.matcher(title).replaceFirst(".jpg");
-            title = title.replaceFirst("\\.jpeg$", ".jpg");
-        }
         if (extension != null && !title.toLowerCase().endsWith("." + extension.toLowerCase())) {
             title += "." + extension;
         }

app/src/main/java/fr/free/nrw/commons/upload/UploadService.java

@@ -1,8 +1,9 @@
 package fr.free.nrw.commons.upload;
 
 import java.io.*;
-import java.util.ArrayList;
 import java.util.Date;
+import java.util.HashSet;
+import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -43,6 +44,9 @@ public class UploadService extends HandlerService<Contribution> {
 
     private int toUpload;
 
+    // The file names of unfinished uploads, used to prevent overwriting
+    private Set<String> unfinishedUploads = new HashSet<>();
+
     // DO NOT HAVE NOTIFICATION ID OF 0 FOR ANYTHING
     // See http://stackoverflow.com/questions/8725909/startforeground-does-not-show-my-notification
     // Seriously, Android?
@@ -96,7 +100,7 @@ public class UploadService extends HandlerService<Contribution> {
     public void onDestroy() {
         super.onDestroy();
         contributionsProviderClient.release();
-        Log.d("Commons", "ZOMG I AM BEING KILLED HALP!");
+        Log.d("Commons", "UploadService.onDestroy; " + unfinishedUploads + " are yet to be uploaded");
     }
 
     @Override
@@ -165,7 +169,6 @@ public class UploadService extends HandlerService<Contribution> {
         return START_REDELIVER_INTENT;
     }
 
-
     private void uploadContribution(Contribution contribution) {
         MWApi api = app.getApi();
 
@@ -197,12 +200,17 @@ public class UploadService extends HandlerService<Contribution> {
 
         this.startForeground(NOTIFICATION_UPLOAD_IN_PROGRESS, curProgressNotification.build());
 
+        String filename = null;
         try {
-
+            filename = Utils.fixExtension(
-            String filename = Utils.fixExtension(
                     contribution.getFilename(),
                     MimeTypeMap.getSingleton().getExtensionFromMimeType((String)contribution.getTag("mimeType")));
-            filename = findUniqueFilename(filename);
+
+            synchronized (unfinishedUploads) {
+                Log.d("Commons", "making sure of uniqueness of name: " + filename);
+                filename = findUniqueFilename(filename);
+                unfinishedUploads.add(filename);
+            }
             if(!api.validateLogin()) {
                 // Need to revalidate!
                 if(app.revalidateAuthToken()) {
@@ -223,7 +231,6 @@ public class UploadService extends HandlerService<Contribution> {
             );
             result = api.upload(filename, file, contribution.getDataLength(), contribution.getPageContents(), contribution.getEditSummary(), notificationUpdater);
 
-
             Log.d("Commons", "Response is" + Utils.getStringFromDOM(result.getDocument()));
 
             curProgressNotification = null;
@@ -233,7 +240,7 @@ public class UploadService extends HandlerService<Contribution> {
             if(!resultStatus.equals("Success")) {
                 String errorCode = result.getString("/api/error/@code");
                 showFailedNotification(contribution);
-                fr.free.nrw.commons.EventLog.schema(CommonsApplication.EVENT_UPLOAD_ATTEMPT)
+                EventLog.schema(CommonsApplication.EVENT_UPLOAD_ATTEMPT)
                         .param("username", app.getCurrentAccount().name)
                         .param("source", contribution.getSource())
                         .param("multiple", contribution.getMultiple())
@@ -264,6 +271,9 @@ public class UploadService extends HandlerService<Contribution> {
             showFailedNotification(contribution);
             return;
         } finally {
+            if ( filename != null ) {
+                unfinishedUploads.remove(filename);
+            }
             toUpload--;
             if(toUpload == 0) {
                 // Sync modifications right after all uplaods are processed
@@ -289,36 +299,33 @@ public class UploadService extends HandlerService<Contribution> {
         contribution.save();
     }
 
-    synchronized private String findUniqueFilename(String fileName) throws IOException {
+    private String findUniqueFilename(String fileName) throws IOException {
-        return findUniqueFilename(fileName, 1);
+        MWApi api = app.getApi();
-    }
-
-    private String findUniqueFilename(String fileName, int sequenceNumber) throws IOException  {
         String sequenceFileName;
-        if (sequenceNumber == 1) {
+        for ( int sequenceNumber = 1; true; sequenceNumber++ ) {
-            sequenceFileName = fileName;
+            if (sequenceNumber == 1) {
-        } else {
+                sequenceFileName = fileName;
-            if (fileName.indexOf('.') == -1) {
-                // We really should have appended a file type suffix already.
-                // But... we might not.
-                sequenceFileName = fileName + " " + sequenceNumber;
             } else {
-                Pattern regex = Pattern.compile("^(.*)(\\..+?)$");
+                if (fileName.indexOf('.') == -1) {
-                Matcher regexMatcher = regex.matcher(fileName);
+                    // We really should have appended a file type suffix already.
-                sequenceFileName = regexMatcher.replaceAll("$1 " + sequenceNumber + "$2");
+                    // But... we might not.
+                    sequenceFileName = fileName + " " + sequenceNumber;
+                } else {
+                    Pattern regex = Pattern.compile("^(.*)(\\..+?)$");
+                    Matcher regexMatcher = regex.matcher(fileName);
+                    sequenceFileName = regexMatcher.replaceAll("$1 " + sequenceNumber + "$2");
+                }
+            }
+            if ( fileExistsWithName(api, sequenceFileName) || unfinishedUploads.contains(sequenceFileName) ) {
+                continue;
+            } else {
+                break;
             }
         }
-        Log.d("Commons", "checking for uniqueness of name " + sequenceFileName);
+        return sequenceFileName;
-
-        if (fileExistsWithName(sequenceFileName)) {
-            return findUniqueFilename(fileName, sequenceNumber + 1);
-        } else {
-            return sequenceFileName;
-        }
     }
 
-    private boolean fileExistsWithName(String fileName) throws IOException {
+    private static boolean fileExistsWithName(MWApi api, String fileName) throws IOException {
-        MWApi api = app.getApi();
         ApiResult result;
 
         result = api.action("query")
@@ -326,7 +333,6 @@ public class UploadService extends HandlerService<Contribution> {
                 .param("titles", "File:" + fileName)
                 .get();
 
-        ArrayList<ApiResult> nodes = result.getNodes("/api/query/pages/page/imageinfo");
+        return result.getNodes("/api/query/pages/page/imageinfo").size() > 0;
-        return nodes.size() > 0;
     }
 }

app/src/test/java/fr/free/nrw/commons/UtilsTest.java

@@ -1,29 +1,49 @@
 package fr.free.nrw.commons;
 import org.junit.Test;
 
-import fr.free.nrw.commons.upload.UploadController;
-
 import static org.junit.Assert.*;
+import static org.hamcrest.CoreMatchers.is;
+
 
 public class UtilsTest {
 
-    @Test public void fixExtensionJpegToJpg() {
+    @Test public void fixExtensionJpegToJpeg() {
-        assertEquals("SampleFile.jpg", Utils.fixExtension("SampleFile.jpeg", "jpeg"));
+        assertThat(Utils.fixExtension("SampleFile.jpeg", "jpeg"), is("SampleFile.jpg"));
+    }
+
+    @Test public void fixExtensionJPEGToJpg() {
+        assertThat(Utils.fixExtension("SampleFile.JPEG", null), is("SampleFile.jpg"));
+    }
+
+    @Test public void fixExtensionNull() {
+        assertThat(Utils.fixExtension("SampleFile.jpeg", "JPEG"), is("SampleFile.jpg"));
+    }
+
+    @Test public void fixExtensionJpgToJpeg() {
+        assertThat(Utils.fixExtension("SampleFile.jpg", "jpeg"), is("SampleFile.jpg"));
     }
 
     @Test public void fixExtensionJpgToJpg() {
-        assertEquals("SampleFile.jpg", Utils.fixExtension("SampleFile.jpg", "jpg"));
+        assertThat(Utils.fixExtension("SampleFile.jpg", "jpg"), is("SampleFile.jpg"));
     }
 
     @Test public void fixExtensionPngToPng() {
-        assertEquals("SampleFile.png", Utils.fixExtension("SampleFile.png", "png"));
+        assertThat(Utils.fixExtension("SampleFile.png", "png"), is("SampleFile.png"));
     }
 
     @Test public void fixExtensionEmptyToJpg() {
-        assertEquals("SampleFile.jpg", Utils.fixExtension("SampleFile", "jpg"));
+        assertThat(Utils.fixExtension("SampleFile", "jpg"), is("SampleFile.jpg"));
+    }
+
+    @Test public void fixExtensionEmptyToJpeg() {
+        assertThat(Utils.fixExtension("SampleFile", "jpeg"), is("SampleFile.jpg"));
     }
 
     @Test public void fixExtensionJpgNotExtension() {
-        assertEquals("SampleFileJpg.jpg", Utils.fixExtension("SampleFileJpg", "jpg"));
+        assertThat(Utils.fixExtension("SAMPLEjpg", "jpg"), is("SAMPLEjpg.jpg"));
+    }
+
+    @Test public void fixExtensionJpegNotExtension() {
+        assertThat(Utils.fixExtension("SAMPLE.jpeg.SAMPLE", "jpg"), is("SAMPLE.jpeg.SAMPLE.jpg"));
     }
 }