From c434d209fc0349d52b1657ea404959466832f6b7 Mon Sep 17 00:00:00 2001 From: Yusuke Matsubara Date: Sat, 3 Sep 2016 18:39:45 +0900 Subject: [PATCH] Further attempts to reduce overwrites - Keep file names of unfinished uploads to avoid overwriting - Normalize capital-letter file extensions --- .../main/java/fr/free/nrw/commons/Utils.java | 7 +- .../nrw/commons/upload/UploadService.java | 74 ++++++++++--------- .../java/fr/free/nrw/commons/UtilsTest.java | 36 +++++++-- 3 files changed, 72 insertions(+), 45 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/Utils.java b/app/src/main/java/fr/free/nrw/commons/Utils.java index 6ba4a4d73..044037cc4 100644 --- a/app/src/main/java/fr/free/nrw/commons/Utils.java +++ b/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 = title.replaceFirst("\\.jpeg$", ".jpg"); - } + title = jpegPattern.matcher(title).replaceFirst(".jpg"); if (extension != null && !title.toLowerCase().endsWith("." + extension.toLowerCase())) { title += "." + extension; } diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java b/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java index a5ab0dd3c..65a1e2cd6 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java +++ b/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 { private int toUpload; + // The file names of unfinished uploads, used to prevent overwriting + private Set 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 { 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 { return START_REDELIVER_INTENT; } - private void uploadContribution(Contribution contribution) { MWApi api = app.getApi(); @@ -197,12 +200,17 @@ public class UploadService extends HandlerService { this.startForeground(NOTIFICATION_UPLOAD_IN_PROGRESS, curProgressNotification.build()); + String filename = null; try { - - String filename = Utils.fixExtension( + 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 { ); 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 { 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 { 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.save(); } - synchronized private String findUniqueFilename(String fileName) throws IOException { - return findUniqueFilename(fileName, 1); - } - - private String findUniqueFilename(String fileName, int sequenceNumber) throws IOException { + private String findUniqueFilename(String fileName) throws IOException { + MWApi api = app.getApi(); String sequenceFileName; - if (sequenceNumber == 1) { - sequenceFileName = fileName; - } else { - if (fileName.indexOf('.') == -1) { - // We really should have appended a file type suffix already. - // But... we might not. - sequenceFileName = fileName + " " + sequenceNumber; + for ( int sequenceNumber = 1; true; sequenceNumber++ ) { + if (sequenceNumber == 1) { + sequenceFileName = fileName; } else { - Pattern regex = Pattern.compile("^(.*)(\\..+?)$"); - Matcher regexMatcher = regex.matcher(fileName); - sequenceFileName = regexMatcher.replaceAll("$1 " + sequenceNumber + "$2"); + 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("^(.*)(\\..+?)$"); + 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); - - if (fileExistsWithName(sequenceFileName)) { - return findUniqueFilename(fileName, sequenceNumber + 1); - } else { - return sequenceFileName; - } + return sequenceFileName; } - private boolean fileExistsWithName(String fileName) throws IOException { - MWApi api = app.getApi(); + private static boolean fileExistsWithName(MWApi api, String fileName) throws IOException { ApiResult result; result = api.action("query") @@ -326,7 +333,6 @@ public class UploadService extends HandlerService { .param("titles", "File:" + fileName) .get(); - ArrayList nodes = result.getNodes("/api/query/pages/page/imageinfo"); - return nodes.size() > 0; + return result.getNodes("/api/query/pages/page/imageinfo").size() > 0; } } diff --git a/app/src/test/java/fr/free/nrw/commons/UtilsTest.java b/app/src/test/java/fr/free/nrw/commons/UtilsTest.java index 544b18920..6e78ab928 100644 --- a/app/src/test/java/fr/free/nrw/commons/UtilsTest.java +++ b/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() { - assertEquals("SampleFile.jpg", Utils.fixExtension("SampleFile.jpeg", "jpeg")); + @Test public void fixExtensionJpegToJpeg() { + 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")); } }