From bb699f33e20649f90bc1990a28e030160579123c Mon Sep 17 00:00:00 2001 From: Tim Weerakoon Date: Thu, 30 Apr 2020 18:06:27 -0400 Subject: [PATCH 1/9] Created and implemented closeInputStream method. Fix #848 --- .../briefcase/push/aggregate/PushToAggregate.java | 9 ++++++++- .../briefcase/reused/UncheckedFiles.java | 8 ++++++++ .../briefcase/reused/transfer/AggregateServer.java | 13 +++++++++++-- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java b/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java index ca35267c7..6faf4a1cc 100644 --- a/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java +++ b/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java @@ -21,6 +21,7 @@ import static org.opendatakit.briefcase.reused.UncheckedFiles.list; import static org.opendatakit.briefcase.reused.UncheckedFiles.size; +import java.io.InputStream; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; @@ -164,7 +165,7 @@ void pushFormAndAttachments(FormStatus form, List attachments, RunnerStatu } tracker.trackStartSendingFormAndAttachments(part, parts); - Response response = http.execute(server.getPushFormRequest(form.getFormFile(briefcaseDir), attachments)); + Response response = http.execute(server.getPushFormRequest(form.getFormFile(briefcaseDir), attachments)); if (response.isSuccess()) tracker.trackEndSendingFormAndAttachments(part, parts); else @@ -186,6 +187,12 @@ void pushSubmissionAndAttachments(Path submissionFile, List attachments, R submissionFile, attachments )); + + for (InputStream stream : server.getFileStreams()) { + UncheckedFiles.closeInputStream(stream); + } + server.getFileStreams().clear(); + if (response.isSuccess()) tracker.trackEndSendingSubmissionAndAttachments(submissionNumber, totalSubmissions, part, parts); else diff --git a/src/org/opendatakit/briefcase/reused/UncheckedFiles.java b/src/org/opendatakit/briefcase/reused/UncheckedFiles.java index fe397ed62..7e3763a6b 100644 --- a/src/org/opendatakit/briefcase/reused/UncheckedFiles.java +++ b/src/org/opendatakit/briefcase/reused/UncheckedFiles.java @@ -301,6 +301,14 @@ public static InputStream newInputStream(Path path, OpenOption... options) { } } + public static void closeInputStream(InputStream inputStream) { + try { + inputStream.close(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + public static long size(Path path) { try { return Files.size(path); diff --git a/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java b/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java index a008fadf7..2ff3ba21a 100644 --- a/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java +++ b/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java @@ -30,6 +30,7 @@ import java.net.URL; import java.nio.file.Path; import java.time.OffsetDateTime; +import java.util.ArrayList; import java.util.List; import java.util.Objects; import java.util.Optional; @@ -54,10 +55,12 @@ public class AggregateServer implements RemoteServer { private final URL baseUrl; private final Optional credentials; + private List fileStreams; public AggregateServer(URL baseUrl, Optional credentials) { this.baseUrl = baseUrl; this.credentials = credentials; + this.fileStreams = new ArrayList<>(); } public static AggregateServer authenticated(URL baseUrl, Credentials credentials) { @@ -175,6 +178,7 @@ public Request getPushFormRequest(Path formFile, List attachm } public Request getPushSubmissionRequest(Path submissionFile, List attachments) { + InputStream submissionFileStream = newInputStream(submissionFile); RequestBuilder builder = RequestBuilder.post(baseUrl) .asXmlElement() .withPath("/submission") @@ -182,14 +186,17 @@ public Request getPushSubmissionRequest(Path submissionFile, List getPushSubmissionRequest(Path submissionFile, List getFileStreams() { return fileStreams; } + private String getContentType(Path file) { return getFileExtension(file.getFileName().toString()) .map(extension -> { From 544430b1dd58b89a0af1136ca44390dc8fb09032 Mon Sep 17 00:00:00 2001 From: Tim Weerakoon Date: Tue, 12 May 2020 22:24:28 -0400 Subject: [PATCH 2/9] refactored aggregate server with closeInputStream --- .../briefcase/push/aggregate/PushToAggregate.java | 9 ++++++++- .../briefcase/reused/transfer/AggregateServer.java | 8 ++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java b/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java index 6faf4a1cc..ca364ccd1 100644 --- a/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java +++ b/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java @@ -18,6 +18,7 @@ import static java.util.Collections.emptyList; import static java.util.stream.Collectors.toList; +import static org.opendatakit.briefcase.reused.UncheckedFiles.closeInputStream; import static org.opendatakit.briefcase.reused.UncheckedFiles.list; import static org.opendatakit.briefcase.reused.UncheckedFiles.size; @@ -166,6 +167,12 @@ void pushFormAndAttachments(FormStatus form, List attachments, RunnerStatu tracker.trackStartSendingFormAndAttachments(part, parts); Response response = http.execute(server.getPushFormRequest(form.getFormFile(briefcaseDir), attachments)); + + for (InputStream stream : server.getFileStreams()) { + closeInputStream(stream); + } + server.getFileStreams().clear(); + if (response.isSuccess()) tracker.trackEndSendingFormAndAttachments(part, parts); else @@ -189,7 +196,7 @@ void pushSubmissionAndAttachments(Path submissionFile, List attachments, R )); for (InputStream stream : server.getFileStreams()) { - UncheckedFiles.closeInputStream(stream); + closeInputStream(stream); } server.getFileStreams().clear(); diff --git a/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java b/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java index 2ff3ba21a..8f5aa8f8a 100644 --- a/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java +++ b/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java @@ -154,20 +154,24 @@ public Request getInstanceIdBatchRequest(String formId, int entriesP } public Request getPushFormRequest(Path formFile, List attachments) { + InputStream formFileStream = newInputStream(formFile); RequestBuilder builder = RequestBuilder.post(baseUrl) .withPath("/formUpload") .withMultipartMessage( "form_def_file", "application/xml", formFile.getFileName().toString(), - newInputStream(formFile) + formFileStream ); + fileStreams.add(formFileStream); for (Path attachment : attachments) { + InputStream attachmentStream = newInputStream(attachment); + fileStreams.add(attachmentStream); builder = builder.withMultipartMessage( attachment.getFileName().toString(), getContentType(attachment), attachment.getFileName().toString(), - newInputStream(attachment) + attachmentStream ); } return builder From 2e1fcc308de47cb20bd6b64558cab95166dd9b5d Mon Sep 17 00:00:00 2001 From: Tim Weerakoon Date: Fri, 10 Jul 2020 20:15:18 -0400 Subject: [PATCH 3/9] Implemented review suggestions --- .../briefcase/push/aggregate/PushToAggregate.java | 9 ++------- .../briefcase/reused/transfer/AggregateServer.java | 3 +-- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java b/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java index ca364ccd1..7abcf9728 100644 --- a/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java +++ b/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java @@ -18,7 +18,6 @@ import static java.util.Collections.emptyList; import static java.util.stream.Collectors.toList; -import static org.opendatakit.briefcase.reused.UncheckedFiles.closeInputStream; import static org.opendatakit.briefcase.reused.UncheckedFiles.list; import static org.opendatakit.briefcase.reused.UncheckedFiles.size; @@ -168,9 +167,7 @@ void pushFormAndAttachments(FormStatus form, List attachments, RunnerStatu tracker.trackStartSendingFormAndAttachments(part, parts); Response response = http.execute(server.getPushFormRequest(form.getFormFile(briefcaseDir), attachments)); - for (InputStream stream : server.getFileStreams()) { - closeInputStream(stream); - } + server.getFileStreams().forEach(UncheckedFiles::closeInputStream); server.getFileStreams().clear(); if (response.isSuccess()) @@ -195,9 +192,7 @@ void pushSubmissionAndAttachments(Path submissionFile, List attachments, R attachments )); - for (InputStream stream : server.getFileStreams()) { - closeInputStream(stream); - } + server.getFileStreams().forEach(UncheckedFiles::closeInputStream); server.getFileStreams().clear(); if (response.isSuccess()) diff --git a/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java b/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java index 8f5aa8f8a..a0acad22b 100644 --- a/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java +++ b/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java @@ -55,12 +55,11 @@ public class AggregateServer implements RemoteServer { private final URL baseUrl; private final Optional credentials; - private List fileStreams; + private final List fileStreams = new ArrayList<>(); public AggregateServer(URL baseUrl, Optional credentials) { this.baseUrl = baseUrl; this.credentials = credentials; - this.fileStreams = new ArrayList<>(); } public static AggregateServer authenticated(URL baseUrl, Credentials credentials) { From 198e4db15a2c5e140861ea354c9cfa0f424f0d19 Mon Sep 17 00:00:00 2001 From: Tim Weerakoon Date: Sun, 12 Jul 2020 16:42:43 -0400 Subject: [PATCH 4/9] Reverted previous fix & implemented alternate fix --- build.gradle | 1 + .../push/aggregate/PushToAggregate.java | 6 -- .../briefcase/reused/UncheckedFiles.java | 2 + .../briefcase/reused/http/CommonsHttp.java | 14 +++++ .../reused/transfer/AggregateServer.java | 20 ++---- .../briefcase/reused/UncheckedFilesTest.java | 61 +++++++++++++++++++ 6 files changed, 82 insertions(+), 22 deletions(-) create mode 100644 test/java/org/opendatakit/briefcase/reused/UncheckedFilesTest.java diff --git a/build.gradle b/build.gradle index 728582817..239407a19 100755 --- a/build.gradle +++ b/build.gradle @@ -62,6 +62,7 @@ dependencies { testCompile 'com.github.npathai:hamcrest-optional:2.0.0' testCompile 'org.assertj:assertj-swing-junit:3.8.0' // Required for Swing tests testCompile "com.github.dreamhead:moco-core:0.12.0" + testCompile group: 'org.mockito', name: 'mockito-core', version: '3.4.0' compile files('lib/smallsql-0.21.jar') diff --git a/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java b/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java index 7abcf9728..ba4c738b2 100644 --- a/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java +++ b/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java @@ -167,9 +167,6 @@ void pushFormAndAttachments(FormStatus form, List attachments, RunnerStatu tracker.trackStartSendingFormAndAttachments(part, parts); Response response = http.execute(server.getPushFormRequest(form.getFormFile(briefcaseDir), attachments)); - server.getFileStreams().forEach(UncheckedFiles::closeInputStream); - server.getFileStreams().clear(); - if (response.isSuccess()) tracker.trackEndSendingFormAndAttachments(part, parts); else @@ -192,9 +189,6 @@ void pushSubmissionAndAttachments(Path submissionFile, List attachments, R attachments )); - server.getFileStreams().forEach(UncheckedFiles::closeInputStream); - server.getFileStreams().clear(); - if (response.isSuccess()) tracker.trackEndSendingSubmissionAndAttachments(submissionNumber, totalSubmissions, part, parts); else diff --git a/src/org/opendatakit/briefcase/reused/UncheckedFiles.java b/src/org/opendatakit/briefcase/reused/UncheckedFiles.java index 7e3763a6b..be421f05f 100644 --- a/src/org/opendatakit/briefcase/reused/UncheckedFiles.java +++ b/src/org/opendatakit/briefcase/reused/UncheckedFiles.java @@ -302,6 +302,8 @@ public static InputStream newInputStream(Path path, OpenOption... options) { } public static void closeInputStream(InputStream inputStream) { + if (inputStream == null) + return; try { inputStream.close(); } catch (IOException e) { diff --git a/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java b/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java index 02f91af38..e079457c0 100644 --- a/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java +++ b/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java @@ -39,12 +39,16 @@ import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.HttpClientBuilder; import org.opendatakit.briefcase.reused.BriefcaseException; +import org.opendatakit.briefcase.reused.UncheckedFiles; import org.opendatakit.briefcase.reused.http.response.Response; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class CommonsHttp implements Http { private Executor executor; private final int maxConnections; private final BasicCookieStore cookieStore; + private static final Logger LOG = LoggerFactory.getLogger(CommonsHttp.class); private CommonsHttp(Executor executor, int maxConnections, BasicCookieStore cookieStore) { this.executor = executor; @@ -147,6 +151,16 @@ private Response uncheckedExecute(Request request, Executor executor) throw new HttpException("The connection has timed out", e); } catch (IOException e) { throw new UncheckedIOException(e); + } finally { + try { + UncheckedFiles.closeInputStream(request.getBody()); + } catch (final BriefcaseException exception) { + LOG.error("Error: Attempted to close Requesy.body InputStream, but failed", exception); + } + if (request.multipartMessages != null) + request.multipartMessages.stream() + .map(MultipartMessage::getBody) + .forEach(UncheckedFiles::closeInputStream); } } diff --git a/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java b/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java index a0acad22b..a008fadf7 100644 --- a/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java +++ b/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java @@ -30,7 +30,6 @@ import java.net.URL; import java.nio.file.Path; import java.time.OffsetDateTime; -import java.util.ArrayList; import java.util.List; import java.util.Objects; import java.util.Optional; @@ -55,7 +54,6 @@ public class AggregateServer implements RemoteServer { private final URL baseUrl; private final Optional credentials; - private final List fileStreams = new ArrayList<>(); public AggregateServer(URL baseUrl, Optional credentials) { this.baseUrl = baseUrl; @@ -153,24 +151,20 @@ public Request getInstanceIdBatchRequest(String formId, int entriesP } public Request getPushFormRequest(Path formFile, List attachments) { - InputStream formFileStream = newInputStream(formFile); RequestBuilder builder = RequestBuilder.post(baseUrl) .withPath("/formUpload") .withMultipartMessage( "form_def_file", "application/xml", formFile.getFileName().toString(), - formFileStream + newInputStream(formFile) ); - fileStreams.add(formFileStream); for (Path attachment : attachments) { - InputStream attachmentStream = newInputStream(attachment); - fileStreams.add(attachmentStream); builder = builder.withMultipartMessage( attachment.getFileName().toString(), getContentType(attachment), attachment.getFileName().toString(), - attachmentStream + newInputStream(attachment) ); } return builder @@ -181,7 +175,6 @@ public Request getPushFormRequest(Path formFile, List attachm } public Request getPushSubmissionRequest(Path submissionFile, List attachments) { - InputStream submissionFileStream = newInputStream(submissionFile); RequestBuilder builder = RequestBuilder.post(baseUrl) .asXmlElement() .withPath("/submission") @@ -189,17 +182,14 @@ public Request getPushSubmissionRequest(Path submissionFile, List getPushSubmissionRequest(Path submissionFile, List getFileStreams() { return fileStreams; } - private String getContentType(Path file) { return getFileExtension(file.getFileName().toString()) .map(extension -> { diff --git a/test/java/org/opendatakit/briefcase/reused/UncheckedFilesTest.java b/test/java/org/opendatakit/briefcase/reused/UncheckedFilesTest.java new file mode 100644 index 000000000..ad62f8125 --- /dev/null +++ b/test/java/org/opendatakit/briefcase/reused/UncheckedFilesTest.java @@ -0,0 +1,61 @@ +package org.opendatakit.briefcase.reused; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.IOException; +import java.io.InputStream; +import java.io.UncheckedIOException; +import java.nio.file.Path; +import java.nio.file.Paths; + +import static org.opendatakit.briefcase.reused.UncheckedFiles.*; +import static org.mockito.Mockito.*; + +public class UncheckedFilesTest { + private static Path tempDir; + private Path temp; + + + @Before + public void setUp() { + tempDir = createTempDirectory("briefcase"); + temp = Paths.get(tempDir.toString() + "/test.txt"); + createFile(temp); + } + + @After + public void tearDown() { + deleteRecursive(tempDir); + } + + @Test(expected = UncheckedIOException.class) + public void newInputStream_should_throw_exception() { + UncheckedFiles.delete(temp); + UncheckedFiles.newInputStream(Paths.get(tempDir.toString() + "/test.txt")); + } + + @Test + public void closeInputStream_should_handle_null() { + UncheckedFiles.closeInputStream(null); + } + + @Test(expected = IOException.class) + public void closeInputStream_should_close() throws IOException { + InputStream inputStream = UncheckedFiles.newInputStream(Paths.get(tempDir.toString() + "/test.txt")); + // close inputStream + UncheckedFiles.closeInputStream(inputStream); + + // after close, .available() should throw exception + inputStream.available(); + } + + @Test(expected = UncheckedIOException.class) + public void closeInputStream_should_throw_exception() throws IOException { + InputStream inputStream = mock(InputStream.class); + doThrow(new IOException()).when(inputStream).close(); + + UncheckedFiles.closeInputStream(inputStream); + } +} From 81ef423b734035bf46a03432d336ce9d4d30b048 Mon Sep 17 00:00:00 2001 From: Tim Weerakoon Date: Sun, 12 Jul 2020 17:26:45 -0400 Subject: [PATCH 5/9] Fixed checkstyle issues with tests --- .../briefcase/reused/UncheckedFilesTest.java | 102 +++++++++--------- 1 file changed, 53 insertions(+), 49 deletions(-) diff --git a/test/java/org/opendatakit/briefcase/reused/UncheckedFilesTest.java b/test/java/org/opendatakit/briefcase/reused/UncheckedFilesTest.java index ad62f8125..d83d5201a 100644 --- a/test/java/org/opendatakit/briefcase/reused/UncheckedFilesTest.java +++ b/test/java/org/opendatakit/briefcase/reused/UncheckedFilesTest.java @@ -1,8 +1,10 @@ package org.opendatakit.briefcase.reused; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.opendatakit.briefcase.reused.UncheckedFiles.createFile; +import static org.opendatakit.briefcase.reused.UncheckedFiles.createTempDirectory; +import static org.opendatakit.briefcase.reused.UncheckedFiles.deleteRecursive; import java.io.IOException; import java.io.InputStream; @@ -10,52 +12,54 @@ import java.nio.file.Path; import java.nio.file.Paths; -import static org.opendatakit.briefcase.reused.UncheckedFiles.*; -import static org.mockito.Mockito.*; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + public class UncheckedFilesTest { - private static Path tempDir; - private Path temp; - - - @Before - public void setUp() { - tempDir = createTempDirectory("briefcase"); - temp = Paths.get(tempDir.toString() + "/test.txt"); - createFile(temp); - } - - @After - public void tearDown() { - deleteRecursive(tempDir); - } - - @Test(expected = UncheckedIOException.class) - public void newInputStream_should_throw_exception() { - UncheckedFiles.delete(temp); - UncheckedFiles.newInputStream(Paths.get(tempDir.toString() + "/test.txt")); - } - - @Test - public void closeInputStream_should_handle_null() { - UncheckedFiles.closeInputStream(null); - } - - @Test(expected = IOException.class) - public void closeInputStream_should_close() throws IOException { - InputStream inputStream = UncheckedFiles.newInputStream(Paths.get(tempDir.toString() + "/test.txt")); - // close inputStream - UncheckedFiles.closeInputStream(inputStream); - - // after close, .available() should throw exception - inputStream.available(); - } - - @Test(expected = UncheckedIOException.class) - public void closeInputStream_should_throw_exception() throws IOException { - InputStream inputStream = mock(InputStream.class); - doThrow(new IOException()).when(inputStream).close(); - - UncheckedFiles.closeInputStream(inputStream); - } + private static Path tempDir; + private Path temp; + + + @Before + public void setUp() { + tempDir = createTempDirectory("briefcase"); + temp = Paths.get(tempDir.toString() + "/test.txt"); + createFile(temp); + } + + @After + public void tearDown() { + deleteRecursive(tempDir); + } + + @Test(expected = UncheckedIOException.class) + public void newInputStream_should_throw_exception() { + UncheckedFiles.delete(temp); + UncheckedFiles.newInputStream(Paths.get(tempDir.toString() + "/test.txt")); + } + + @Test + public void closeInputStream_should_handle_null() { + UncheckedFiles.closeInputStream(null); + } + + @Test(expected = IOException.class) + public void closeInputStream_should_close() throws IOException { + InputStream inputStream = UncheckedFiles.newInputStream(Paths.get(tempDir.toString() + "/test.txt")); + // close inputStream + UncheckedFiles.closeInputStream(inputStream); + + // after close, .available() should throw exception + inputStream.available(); + } + + @Test(expected = UncheckedIOException.class) + public void closeInputStream_should_throw_exception() throws IOException { + InputStream inputStream = mock(InputStream.class); + doThrow(new IOException()).when(inputStream).close(); + + UncheckedFiles.closeInputStream(inputStream); + } } From 577411cdf6037d6df3bd5c687df181e1617f4422 Mon Sep 17 00:00:00 2001 From: Tim Weerakoon Date: Sun, 12 Jul 2020 18:14:16 -0400 Subject: [PATCH 6/9] Fixed typo in log message --- src/org/opendatakit/briefcase/reused/http/CommonsHttp.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java b/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java index e079457c0..2faec2103 100644 --- a/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java +++ b/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java @@ -155,7 +155,7 @@ private Response uncheckedExecute(Request request, Executor executor) try { UncheckedFiles.closeInputStream(request.getBody()); } catch (final BriefcaseException exception) { - LOG.error("Error: Attempted to close Requesy.body InputStream, but failed", exception); + LOG.error("Error: Attempted to close Request.body InputStream, but failed", exception); } if (request.multipartMessages != null) request.multipartMessages.stream() From 6c965b98dfc06f335f48784d44e85e388021a16a Mon Sep 17 00:00:00 2001 From: Tim Weerakoon Date: Sat, 25 Jul 2020 00:19:55 -0400 Subject: [PATCH 7/9] Implemented code review suggestions --- build.gradle | 1 - .../briefcase/reused/http/CommonsHttp.java | 4 +-- ...ava => UncheckedFilesInputStreamTest.java} | 34 +++++++++---------- 3 files changed, 18 insertions(+), 21 deletions(-) rename test/java/org/opendatakit/briefcase/reused/{UncheckedFilesTest.java => UncheckedFilesInputStreamTest.java} (60%) diff --git a/build.gradle b/build.gradle index 4048f4425..2c62693ba 100755 --- a/build.gradle +++ b/build.gradle @@ -62,7 +62,6 @@ dependencies { testCompile 'com.github.npathai:hamcrest-optional:2.0.0' testCompile 'org.assertj:assertj-swing-junit:3.8.0' // Required for Swing tests testCompile "com.github.dreamhead:moco-core:0.12.0" - testCompile group: 'org.mockito', name: 'mockito-core', version: '3.4.0' compile files('lib/smallsql-0.21.jar') diff --git a/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java b/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java index 2faec2103..433b249b9 100644 --- a/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java +++ b/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java @@ -48,7 +48,7 @@ public class CommonsHttp implements Http { private Executor executor; private final int maxConnections; private final BasicCookieStore cookieStore; - private static final Logger LOG = LoggerFactory.getLogger(CommonsHttp.class); + private static final Logger log = LoggerFactory.getLogger(CommonsHttp.class); private CommonsHttp(Executor executor, int maxConnections, BasicCookieStore cookieStore) { this.executor = executor; @@ -155,7 +155,7 @@ private Response uncheckedExecute(Request request, Executor executor) try { UncheckedFiles.closeInputStream(request.getBody()); } catch (final BriefcaseException exception) { - LOG.error("Error: Attempted to close Request.body InputStream, but failed", exception); + log.error("Error: Attempted to close Request.body InputStream, but failed", exception); } if (request.multipartMessages != null) request.multipartMessages.stream() diff --git a/test/java/org/opendatakit/briefcase/reused/UncheckedFilesTest.java b/test/java/org/opendatakit/briefcase/reused/UncheckedFilesInputStreamTest.java similarity index 60% rename from test/java/org/opendatakit/briefcase/reused/UncheckedFilesTest.java rename to test/java/org/opendatakit/briefcase/reused/UncheckedFilesInputStreamTest.java index d83d5201a..aa021ab69 100644 --- a/test/java/org/opendatakit/briefcase/reused/UncheckedFilesTest.java +++ b/test/java/org/opendatakit/briefcase/reused/UncheckedFilesInputStreamTest.java @@ -1,7 +1,5 @@ package org.opendatakit.briefcase.reused; -import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.mock; import static org.opendatakit.briefcase.reused.UncheckedFiles.createFile; import static org.opendatakit.briefcase.reused.UncheckedFiles.createTempDirectory; import static org.opendatakit.briefcase.reused.UncheckedFiles.deleteRecursive; @@ -10,22 +8,21 @@ import java.io.InputStream; import java.io.UncheckedIOException; import java.nio.file.Path; -import java.nio.file.Paths; import org.junit.After; import org.junit.Before; import org.junit.Test; -public class UncheckedFilesTest { - private static Path tempDir; +public class UncheckedFilesInputStreamTest { + private Path tempDir; private Path temp; @Before public void setUp() { tempDir = createTempDirectory("briefcase"); - temp = Paths.get(tempDir.toString() + "/test.txt"); + temp = tempDir.resolve("test.txt"); createFile(temp); } @@ -34,12 +31,6 @@ public void tearDown() { deleteRecursive(tempDir); } - @Test(expected = UncheckedIOException.class) - public void newInputStream_should_throw_exception() { - UncheckedFiles.delete(temp); - UncheckedFiles.newInputStream(Paths.get(tempDir.toString() + "/test.txt")); - } - @Test public void closeInputStream_should_handle_null() { UncheckedFiles.closeInputStream(null); @@ -47,8 +38,8 @@ public void closeInputStream_should_handle_null() { @Test(expected = IOException.class) public void closeInputStream_should_close() throws IOException { - InputStream inputStream = UncheckedFiles.newInputStream(Paths.get(tempDir.toString() + "/test.txt")); - // close inputStream + InputStream inputStream = UncheckedFiles.newInputStream(temp); + UncheckedFiles.closeInputStream(inputStream); // after close, .available() should throw exception @@ -57,9 +48,16 @@ public void closeInputStream_should_close() throws IOException { @Test(expected = UncheckedIOException.class) public void closeInputStream_should_throw_exception() throws IOException { - InputStream inputStream = mock(InputStream.class); - doThrow(new IOException()).when(inputStream).close(); - - UncheckedFiles.closeInputStream(inputStream); + InputStream is = new InputStream() { + @Override + public int read() throws IOException { + return 0; + } + @Override + public void close() throws IOException { + throw new IOException("chuchu"); + } + }; + UncheckedFiles.closeInputStream(is); } } From 8916aff4543c0dcd6b0a05108bb902fedacd1536 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guillermo=20Guti=C3=A9rrez?= Date: Sat, 31 Oct 2020 09:02:40 +0100 Subject: [PATCH 8/9] Apply suggestions from code review I went ahead and change some small details that I think we should really have before merging in order to be consistent with the previous code and simplify it as much as possible. --- .../briefcase/push/aggregate/PushToAggregate.java | 5 +---- src/org/opendatakit/briefcase/reused/UncheckedFiles.java | 2 -- src/org/opendatakit/briefcase/reused/http/CommonsHttp.java | 6 +----- .../briefcase/reused/UncheckedFilesInputStreamTest.java | 5 ----- 4 files changed, 2 insertions(+), 16 deletions(-) diff --git a/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java b/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java index ba4c738b2..ca35267c7 100644 --- a/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java +++ b/src/org/opendatakit/briefcase/push/aggregate/PushToAggregate.java @@ -21,7 +21,6 @@ import static org.opendatakit.briefcase.reused.UncheckedFiles.list; import static org.opendatakit.briefcase.reused.UncheckedFiles.size; -import java.io.InputStream; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; @@ -165,8 +164,7 @@ void pushFormAndAttachments(FormStatus form, List attachments, RunnerStatu } tracker.trackStartSendingFormAndAttachments(part, parts); - Response response = http.execute(server.getPushFormRequest(form.getFormFile(briefcaseDir), attachments)); - + Response response = http.execute(server.getPushFormRequest(form.getFormFile(briefcaseDir), attachments)); if (response.isSuccess()) tracker.trackEndSendingFormAndAttachments(part, parts); else @@ -188,7 +186,6 @@ void pushSubmissionAndAttachments(Path submissionFile, List attachments, R submissionFile, attachments )); - if (response.isSuccess()) tracker.trackEndSendingSubmissionAndAttachments(submissionNumber, totalSubmissions, part, parts); else diff --git a/src/org/opendatakit/briefcase/reused/UncheckedFiles.java b/src/org/opendatakit/briefcase/reused/UncheckedFiles.java index be421f05f..7e3763a6b 100644 --- a/src/org/opendatakit/briefcase/reused/UncheckedFiles.java +++ b/src/org/opendatakit/briefcase/reused/UncheckedFiles.java @@ -302,8 +302,6 @@ public static InputStream newInputStream(Path path, OpenOption... options) { } public static void closeInputStream(InputStream inputStream) { - if (inputStream == null) - return; try { inputStream.close(); } catch (IOException e) { diff --git a/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java b/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java index 433b249b9..8824ef7d7 100644 --- a/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java +++ b/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java @@ -152,11 +152,7 @@ private Response uncheckedExecute(Request request, Executor executor) } catch (IOException e) { throw new UncheckedIOException(e); } finally { - try { - UncheckedFiles.closeInputStream(request.getBody()); - } catch (final BriefcaseException exception) { - log.error("Error: Attempted to close Request.body InputStream, but failed", exception); - } + UncheckedFiles.closeInputStream(request.getBody()); if (request.multipartMessages != null) request.multipartMessages.stream() .map(MultipartMessage::getBody) diff --git a/test/java/org/opendatakit/briefcase/reused/UncheckedFilesInputStreamTest.java b/test/java/org/opendatakit/briefcase/reused/UncheckedFilesInputStreamTest.java index aa021ab69..72a4dbebd 100644 --- a/test/java/org/opendatakit/briefcase/reused/UncheckedFilesInputStreamTest.java +++ b/test/java/org/opendatakit/briefcase/reused/UncheckedFilesInputStreamTest.java @@ -31,11 +31,6 @@ public void tearDown() { deleteRecursive(tempDir); } - @Test - public void closeInputStream_should_handle_null() { - UncheckedFiles.closeInputStream(null); - } - @Test(expected = IOException.class) public void closeInputStream_should_close() throws IOException { InputStream inputStream = UncheckedFiles.newInputStream(temp); From 62f552bc16e5e8442aad342182473412d24d0a62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guillermo=20Guti=C3=A9rrez?= Date: Sat, 31 Oct 2020 09:19:54 +0100 Subject: [PATCH 9/9] Prevent BriefcaseException for trying to access a request's body when it's not present --- src/org/opendatakit/briefcase/reused/http/CommonsHttp.java | 2 +- src/org/opendatakit/briefcase/reused/http/Request.java | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java b/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java index 8824ef7d7..22550fcbd 100644 --- a/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java +++ b/src/org/opendatakit/briefcase/reused/http/CommonsHttp.java @@ -152,7 +152,7 @@ private Response uncheckedExecute(Request request, Executor executor) } catch (IOException e) { throw new UncheckedIOException(e); } finally { - UncheckedFiles.closeInputStream(request.getBody()); + request.ifBody(UncheckedFiles::closeInputStream); if (request.multipartMessages != null) request.multipartMessages.stream() .map(MultipartMessage::getBody) diff --git a/src/org/opendatakit/briefcase/reused/http/Request.java b/src/org/opendatakit/briefcase/reused/http/Request.java index 98dd2f622..7068fc655 100644 --- a/src/org/opendatakit/briefcase/reused/http/Request.java +++ b/src/org/opendatakit/briefcase/reused/http/Request.java @@ -25,6 +25,7 @@ import java.util.Objects; import java.util.Optional; import java.util.function.BiConsumer; +import java.util.function.Consumer; import java.util.function.Function; import org.opendatakit.briefcase.reused.BriefcaseException; import org.slf4j.Logger; @@ -124,6 +125,10 @@ public InputStream getBody() { return body.orElseThrow(BriefcaseException::new); } + public void ifBody(Consumer consumer) { + body.ifPresent(consumer); + } + public boolean ignoreCookies() { return ignoreCookies; }