From 7829cf07f1b85ca986fa493fab630d3553efd37a Mon Sep 17 00:00:00 2001 From: Bruno Thomas Date: Mon, 4 Dec 2023 14:27:53 +0000 Subject: [PATCH] fix: returns a FileResult with size/path #1081 --- .../datashare/tasks/BatchDownloadLoop.java | 4 +-- .../datashare/tasks/BatchDownloadRunner.java | 12 ++++----- .../org/icij/datashare/tasks/FileResult.java | 25 +++++++++++++++++++ .../org/icij/datashare/tasks/TaskFactory.java | 3 --- .../org/icij/datashare/web/TaskResource.java | 7 +++--- .../tasks/BatchDownloadRunnerIntTest.java | 5 ++-- .../tasks/BatchDownloadRunnerTest.java | 9 +++---- .../datashare/web/UserTaskResourceTest.java | 12 ++++++--- pom.xml | 2 +- 9 files changed, 50 insertions(+), 29 deletions(-) create mode 100644 datashare-app/src/main/java/org/icij/datashare/tasks/FileResult.java diff --git a/datashare-app/src/main/java/org/icij/datashare/tasks/BatchDownloadLoop.java b/datashare-app/src/main/java/org/icij/datashare/tasks/BatchDownloadLoop.java index c89884101..3b3ad0224 100644 --- a/datashare-app/src/main/java/org/icij/datashare/tasks/BatchDownloadLoop.java +++ b/datashare-app/src/main/java/org/icij/datashare/tasks/BatchDownloadLoop.java @@ -41,8 +41,8 @@ public Integer call() { if (currentTask != null && !POISON.equals(currentTask)) { BatchDownloadRunner downloadRunner = factory.createDownloadRunner(currentTask, taskSupplier::progress); - File zipResult = downloadRunner.call(); - taskSupplier.result(currentTask.id, zipResult); + FileResult result = downloadRunner.call(); + taskSupplier.result(currentTask.id, result); nbTasks++; } } catch (Throwable ex) { diff --git a/datashare-app/src/main/java/org/icij/datashare/tasks/BatchDownloadRunner.java b/datashare-app/src/main/java/org/icij/datashare/tasks/BatchDownloadRunner.java index 4746e3fed..d37129d63 100644 --- a/datashare-app/src/main/java/org/icij/datashare/tasks/BatchDownloadRunner.java +++ b/datashare-app/src/main/java/org/icij/datashare/tasks/BatchDownloadRunner.java @@ -47,14 +47,13 @@ import static java.lang.Integer.min; import static java.lang.Integer.parseInt; import static java.lang.String.valueOf; -import static java.util.Optional.ofNullable; import static java.util.stream.Collectors.toList; import static org.icij.datashare.cli.DatashareCliOptions.BATCH_DOWNLOAD_MAX_NB_FILES; import static org.icij.datashare.cli.DatashareCliOptions.BATCH_DOWNLOAD_MAX_SIZE; import static org.icij.datashare.cli.DatashareCliOptions.BATCH_THROTTLE; import static org.icij.datashare.cli.DatashareCliOptions.SCROLL_SIZE; -public class BatchDownloadRunner implements Callable, Monitorable, UserTask { +public class BatchDownloadRunner implements Callable, Monitorable, UserTask { private final static Logger logger = LoggerFactory.getLogger(BatchDownloadRunner.class); static final int MAX_SCROLL_SIZE = 3500; static final int MAX_BATCH_RESULT_SIZE = 10000; @@ -83,7 +82,7 @@ public BatchDownloadRunner(Indexer indexer, PropertiesProvider propertiesProvide } @Override - public File call() throws Exception { + public FileResult call() throws Exception { int throttleMs = parseInt(propertiesProvider.get(BATCH_THROTTLE).orElse("0")); int maxResultSize = parseInt(propertiesProvider.get(BATCH_DOWNLOAD_MAX_NB_FILES).orElse(valueOf(MAX_BATCH_RESULT_SIZE))); int scrollSize = min(parseInt(propertiesProvider.get(SCROLL_SIZE).orElse("1000")), MAX_SCROLL_SIZE); @@ -120,16 +119,15 @@ public File call() throws Exception { if (addedBytes > 0) { zippedFilesSize += addedBytes; numberOfResults.incrementAndGet(); - batchDownload.setZipSize(zippedFilesSize); progressCallback.apply(task.id, getProgressRate()); } } docsToProcess = searcher.scroll().collect(toList()); } } - logger.info("created batch download file {} ({} bytes/{} entries) for user {}", - batchDownload.filename, Files.size(batchDownload.filename), numberOfResults, batchDownload.user.getId()); - return batchDownload.filename.toFile(); + FileResult result = new FileResult(batchDownload.filename.toFile(), Files.size(batchDownload.filename)); + logger.info("created batch download file {} of {} entries for user {}", result, numberOfResults.get(), batchDownload.user.getId()); + return result; } private Zipper createZipper(BatchDownload batchDownload, PropertiesProvider propertiesProvider, Function mailSenderSupplier) throws URISyntaxException, IOException { diff --git a/datashare-app/src/main/java/org/icij/datashare/tasks/FileResult.java b/datashare-app/src/main/java/org/icij/datashare/tasks/FileResult.java new file mode 100644 index 000000000..3101d2362 --- /dev/null +++ b/datashare-app/src/main/java/org/icij/datashare/tasks/FileResult.java @@ -0,0 +1,25 @@ +package org.icij.datashare.tasks; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; + +import java.io.File; +import java.io.Serializable; + +import static java.lang.String.format; + +public class FileResult implements Serializable { + public final File file; + public final long size; + + @JsonCreator + public FileResult(@JsonProperty("file") File file, @JsonProperty("size") long size) { + this.file = file; + this.size = size; + } + + @Override + public String toString() { + return format("%s (%d bytes)", file, size); + } +} diff --git a/datashare-app/src/main/java/org/icij/datashare/tasks/TaskFactory.java b/datashare-app/src/main/java/org/icij/datashare/tasks/TaskFactory.java index 8c4caf841..561384453 100644 --- a/datashare-app/src/main/java/org/icij/datashare/tasks/TaskFactory.java +++ b/datashare-app/src/main/java/org/icij/datashare/tasks/TaskFactory.java @@ -1,6 +1,5 @@ package org.icij.datashare.tasks; -import org.icij.datashare.batch.BatchDownload; import org.icij.datashare.batch.BatchSearch; import org.icij.datashare.function.TerFunction; import org.icij.datashare.nlp.NlpApp; @@ -8,13 +7,11 @@ import org.icij.datashare.text.nlp.Pipeline; import org.icij.datashare.user.User; -import java.io.File; import java.nio.file.Path; import java.util.List; import java.util.Properties; import java.util.Set; import java.util.function.BiFunction; -import java.util.function.Function; public interface TaskFactory { ResumeNlpTask createResumeNlpTask(final User user, Set pipelines, Properties taskProperties); diff --git a/datashare-app/src/main/java/org/icij/datashare/web/TaskResource.java b/datashare-app/src/main/java/org/icij/datashare/web/TaskResource.java index fefe290ce..ce17cfd39 100644 --- a/datashare-app/src/main/java/org/icij/datashare/web/TaskResource.java +++ b/datashare-app/src/main/java/org/icij/datashare/web/TaskResource.java @@ -10,7 +10,6 @@ import io.swagger.v3.oas.annotations.media.Schema; import io.swagger.v3.oas.annotations.parameters.RequestBody; import io.swagger.v3.oas.annotations.responses.ApiResponse; -import io.swagger.v3.oas.models.security.SecurityScheme; import net.codestory.http.Context; import net.codestory.http.annotations.*; import net.codestory.http.errors.ForbiddenException; @@ -94,11 +93,11 @@ public TaskView getTask(@Parameter(name = "id", description = "task id", in = public Payload getTaskResult(@Parameter(name = "id", description = "task id", in = ParameterIn.PATH) String id, Context context) { TaskView task = forbiddenIfNotSameUser(context, notFoundIfNull(taskManager.getTask(id))); Object result = task.getResult(); - if (result instanceof File) { - final Path appPath = ((File) result).isAbsolute() ? + if (result instanceof FileResult) { + final Path appPath = ((FileResult) result).file.isAbsolute() ? get(System.getProperty("user.dir")).resolve(context.env().appFolder()) : get(context.env().appFolder()); - Path resultPath = appPath.relativize(((File) result).toPath()); + Path resultPath = appPath.relativize(((FileResult) result).file.toPath()); return new Payload(resultPath).withHeader("Content-Disposition", "attachment;filename=\"" + resultPath.getFileName() + "\""); } return result == null ? new Payload(204) : new Payload(result); diff --git a/datashare-app/src/test/java/org/icij/datashare/tasks/BatchDownloadRunnerIntTest.java b/datashare-app/src/test/java/org/icij/datashare/tasks/BatchDownloadRunnerIntTest.java index fefbc6c99..1cf3aef43 100644 --- a/datashare-app/src/test/java/org/icij/datashare/tasks/BatchDownloadRunnerIntTest.java +++ b/datashare-app/src/test/java/org/icij/datashare/tasks/BatchDownloadRunnerIntTest.java @@ -90,10 +90,9 @@ public void test_update_batch_download_zip_size() throws Exception { new IndexerHelper(es.client).indexFile("doc2.txt", "Portez ce vieux whisky au juge blond qui fume", fs); BatchDownload bd = createBatchDownload("*"); - long sizeAtCreation = bd.zipSize; - new BatchDownloadRunner(indexer, createProvider(), createTaskView(bd), taskModifier::progress).call(); + FileResult result = new BatchDownloadRunner(indexer, createProvider(), createTaskView(bd), taskModifier::progress).call(); - assertThat(bd.zipSize).isGreaterThan(sizeAtCreation); + assertThat(result.size).isGreaterThan(0); } @Test diff --git a/datashare-app/src/test/java/org/icij/datashare/tasks/BatchDownloadRunnerTest.java b/datashare-app/src/test/java/org/icij/datashare/tasks/BatchDownloadRunnerTest.java index 7cb0705f3..a5d102a10 100644 --- a/datashare-app/src/test/java/org/icij/datashare/tasks/BatchDownloadRunnerTest.java +++ b/datashare-app/src/test/java/org/icij/datashare/tasks/BatchDownloadRunnerTest.java @@ -21,7 +21,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.HashMap; -import java.util.function.Function; import java.util.stream.IntStream; import java.util.zip.ZipFile; @@ -49,24 +48,24 @@ public void test_max_default_results() throws Exception { Document[] documents = IntStream.range(0, 3).mapToObj(i -> createDoc("doc" + i).with(createFile(i)).build()).toArray(Document[]::new); mockSearch.willReturn(2, documents); BatchDownload batchDownload = new BatchDownload(singletonList(project("test-datashare")), User.local(), "query"); - File zip = new BatchDownloadRunner(indexer, new PropertiesProvider(new HashMap<>() {{ + FileResult result = new BatchDownloadRunner(indexer, new PropertiesProvider(new HashMap<>() {{ put(BATCH_DOWNLOAD_MAX_NB_FILES, "3"); put(SCROLL_SIZE, "3"); }}), getTaskView(batchDownload), updater::progress).call(); - assertThat(new ZipFile(zip).size()).isEqualTo(3); + assertThat(new ZipFile(result.file).size()).isEqualTo(3); } @Test public void test_max_zip_size() throws Exception { Document[] documents = IntStream.range(0, 3).mapToObj(i -> createDoc("doc" + i).with(createFile(i)).with("hello world " + i).build()).toArray(Document[]::new); mockSearch.willReturn(2, documents); - File zip = new BatchDownloadRunner(indexer, new PropertiesProvider(new HashMap<>() {{ + FileResult result = new BatchDownloadRunner(indexer, new PropertiesProvider(new HashMap<>() {{ put(BATCH_DOWNLOAD_MAX_SIZE, valueOf("hello world 1".getBytes(StandardCharsets.UTF_8).length * 3)); put(SCROLL_SIZE, "3"); }}), getTaskView(new BatchDownload(singletonList(project("test-datashare")), User.local(), "query")), updater::progress).call(); - assertThat(new ZipFile(zip).size()).isEqualTo(4); + assertThat(new ZipFile(result.file).size()).isEqualTo(4); } @Test(expected = ElasticsearchStatusException.class) diff --git a/datashare-app/src/test/java/org/icij/datashare/web/UserTaskResourceTest.java b/datashare-app/src/test/java/org/icij/datashare/web/UserTaskResourceTest.java index 4f60bb0db..827d641e1 100644 --- a/datashare-app/src/test/java/org/icij/datashare/web/UserTaskResourceTest.java +++ b/datashare-app/src/test/java/org/icij/datashare/web/UserTaskResourceTest.java @@ -17,6 +17,7 @@ import org.junit.Test; import java.io.File; +import java.nio.file.Files; import java.nio.file.Paths; import java.util.HashMap; import java.util.concurrent.ArrayBlockingQueue; @@ -82,9 +83,11 @@ public void test_get_task_result_with_int_result() { } @Test - public void test_get_task_result_with_file_result__should_relativize_result_with_app_folder() { + public void test_get_task_result_with_file_result__should_relativize_result_with_app_folder() throws Exception { setupAppWith("foo"); - TaskView t = taskManager.startTask(new DummyUserTask<>("foo", () -> Paths.get("app", "index.html").toFile())); + File file = new File(ClassLoader.getSystemResource("app/index.html").toURI()); + FileResult indexHtml = new FileResult(file, Files.size(file.toPath())); + TaskView t = taskManager.startTask(new DummyUserTask<>("foo", () -> indexHtml)); get("/api/task/" + t.id + "/result").withPreemptiveAuthentication("foo", "qux"). should().respond(200). should().haveType("text/html;charset=UTF-8"). @@ -95,8 +98,9 @@ public void test_get_task_result_with_file_result__should_relativize_result_with @Test public void test_get_task_result_with_file_result_and_absolute_path__should_relativize_result_with_app_folder() throws Exception { setupAppWith("foo"); - File indexHtml = new File(ClassLoader.getSystemResource("app/index.html").toURI()); - TaskView t = taskManager.startTask(new DummyUserTask<>("foo", () -> indexHtml)); + File file = new File(ClassLoader.getSystemResource("app/index.html").toURI()); + FileResult indexHtml = new FileResult(file, Files.size(file.toPath())); + TaskView t = taskManager.startTask(new DummyUserTask<>("foo", () -> indexHtml)); get("/api/task/" + t.id + "/result").withPreemptiveAuthentication("foo", "qux").should().respond(200); } diff --git a/pom.xml b/pom.xml index 8748b4228..74da30377 100644 --- a/pom.xml +++ b/pom.xml @@ -76,7 +76,7 @@ 11 11 - 12.4.2 + 12.4.3 13.5.0 ${project.version}