From 976c96bf57bbd800e8a6bee8c58a1910993ab542 Mon Sep 17 00:00:00 2001 From: Bruno Thomas Date: Wed, 18 Sep 2024 10:32:40 +0000 Subject: [PATCH] fix: bug for UriResult deserialization #1575 --- .../org/icij/datashare/web/TaskResource.java | 5 ++- .../tasks/BatchDownloadRunnerTest.java | 4 +-- .../datashare/tasks/TaskWorkerIntTest.java | 2 +- .../org/icij/datashare/asynctasks/Task.java | 8 +++++ .../asynctasks/bus/amqp/UriResult.java | 25 +------------ .../asynctasks/TaskManagerAmqpTest.java | 12 +++---- .../asynctasks/TaskManagerRedisCodecTest.java | 35 +++++++++++++++++-- 7 files changed, 52 insertions(+), 39 deletions(-) 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 4b6fe9819..ae98437e4 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 @@ -122,9 +122,8 @@ public Payload createTask(@Parameter(name = "id", description = "task id", r public Payload getTaskResult(@Parameter(name = "id", description = "task id", in = ParameterIn.PATH) String id, Context context) throws IOException { Task task = forbiddenIfNotSameUser(context, notFoundIfNull(taskManager.getTask(id))); Object result = task.getResult(); - if (result instanceof UriResult) { - UriResult uriResult = (UriResult) result; - Path filePath = Path.of(uriResult.uri.getPath()); + if (result instanceof UriResult uriResult) { + Path filePath = Path.of(uriResult.uri().getPath()); String fileName = filePath.getFileName().toString(); String contentDisposition = "attachment;filename=\"" + fileName + "\""; InputStream fileInputStream = Files.newInputStream(filePath); 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 87a1408d5..2fbceb8e3 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 @@ -57,7 +57,7 @@ public void test_max_default_results() throws Exception { put(SCROLL_SIZE_OPT, "3"); }}), taskView, taskView.progress(updater::progress)).call(); - assertThat(new ZipFile(new File(result.uri)).size()).isEqualTo(3); + assertThat(new ZipFile(new File(result.uri())).size()).isEqualTo(3); } @Test @@ -70,7 +70,7 @@ public void test_max_zip_size() throws Exception { put(SCROLL_SIZE_OPT, "3"); }}), taskView, taskView.progress(updater::progress)).call(); - assertThat(new ZipFile(new File(result.uri)).size()).isEqualTo(3); // the 4th doc must have been skipped + assertThat(new ZipFile(new File(result.uri())).size()).isEqualTo(3); // the 4th doc must have been skipped } @Test(expected = ElasticsearchException.class) diff --git a/datashare-app/src/test/java/org/icij/datashare/tasks/TaskWorkerIntTest.java b/datashare-app/src/test/java/org/icij/datashare/tasks/TaskWorkerIntTest.java index 615aa1b56..c53755494 100644 --- a/datashare-app/src/test/java/org/icij/datashare/tasks/TaskWorkerIntTest.java +++ b/datashare-app/src/test/java/org/icij/datashare/tasks/TaskWorkerIntTest.java @@ -111,7 +111,7 @@ public void test_update_batch_download_zip_size() throws Exception { Task taskView = createTaskView(bd); UriResult result = new BatchDownloadRunner(indexer, createProvider(), taskView, taskView.progress(taskModifier::progress)).call(); - assertThat(result.size).isGreaterThan(0); + assertThat(result.size()).isGreaterThan(0); } @Test diff --git a/datashare-tasks/src/main/java/org/icij/datashare/asynctasks/Task.java b/datashare-tasks/src/main/java/org/icij/datashare/asynctasks/Task.java index 7c5140a2b..d4af9aed1 100644 --- a/datashare-tasks/src/main/java/org/icij/datashare/asynctasks/Task.java +++ b/datashare-tasks/src/main/java/org/icij/datashare/asynctasks/Task.java @@ -4,10 +4,12 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonSubTypes; import com.fasterxml.jackson.annotation.JsonTypeInfo; import org.icij.datashare.Entity; import org.icij.datashare.asynctasks.bus.amqp.Event; import org.icij.datashare.asynctasks.bus.amqp.TaskError; +import org.icij.datashare.asynctasks.bus.amqp.UriResult; import org.icij.datashare.user.User; import java.io.Serializable; @@ -39,6 +41,12 @@ public enum State {CREATED, QUEUED, RUNNING, CANCELLED, ERROR, DONE} volatile TaskError error; private volatile State state; private volatile double progress; + + @JsonSubTypes({ + @JsonSubTypes.Type(value = UriResult.class), + @JsonSubTypes.Type(value = Long.class) + }) + @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "@type") private volatile V result; public Task(String name, User user, Map args) { diff --git a/datashare-tasks/src/main/java/org/icij/datashare/asynctasks/bus/amqp/UriResult.java b/datashare-tasks/src/main/java/org/icij/datashare/asynctasks/bus/amqp/UriResult.java index d61079232..3b8cae99f 100644 --- a/datashare-tasks/src/main/java/org/icij/datashare/asynctasks/bus/amqp/UriResult.java +++ b/datashare-tasks/src/main/java/org/icij/datashare/asynctasks/bus/amqp/UriResult.java @@ -6,37 +6,14 @@ import java.io.Serializable; import java.net.URI; -import java.util.Objects; import static java.lang.String.format; @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "@type") -public class UriResult implements Serializable { - public final URI uri; - public final long size; - +public record UriResult(URI uri, long size) implements Serializable { @JsonCreator public UriResult(@JsonProperty("uri") URI uri, @JsonProperty("size") long size) { this.uri = uri; this.size = size; } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - UriResult uriResult = (UriResult) o; - return Objects.equals(uri, uriResult.uri) - && Objects.equals(size, uriResult.size); - } - - @Override - public int hashCode() { - return Objects.hash(uri, size); - } - - @Override - public String toString() { - return format("%s (%d bytes)", uri, size); - } } diff --git a/datashare-tasks/src/test/java/org/icij/datashare/asynctasks/TaskManagerAmqpTest.java b/datashare-tasks/src/test/java/org/icij/datashare/asynctasks/TaskManagerAmqpTest.java index 576921bfc..033d842a6 100644 --- a/datashare-tasks/src/test/java/org/icij/datashare/asynctasks/TaskManagerAmqpTest.java +++ b/datashare-tasks/src/test/java/org/icij/datashare/asynctasks/TaskManagerAmqpTest.java @@ -2,9 +2,9 @@ import org.fest.assertions.Assertions; import org.icij.datashare.PropertiesProvider; -import org.icij.datashare.asynctasks.bus.amqp.AmqpServerRule; import org.icij.datashare.asynctasks.bus.amqp.AmqpInterlocutor; import org.icij.datashare.asynctasks.bus.amqp.AmqpQueue; +import org.icij.datashare.asynctasks.bus.amqp.AmqpServerRule; import org.icij.datashare.asynctasks.bus.amqp.TaskError; import org.icij.datashare.user.User; import org.icij.extract.redis.RedissonClientFactory; @@ -15,6 +15,11 @@ import org.junit.BeforeClass; import org.junit.ClassRule; import org.junit.Test; +import org.redisson.Redisson; +import org.redisson.RedissonMap; +import org.redisson.api.RedissonClient; +import org.redisson.command.CommandSyncService; +import org.redisson.liveobject.core.RedissonObjectBuilder; import java.io.IOException; import java.io.Serializable; @@ -24,11 +29,6 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; -import org.redisson.Redisson; -import org.redisson.RedissonMap; -import org.redisson.api.RedissonClient; -import org.redisson.command.CommandSyncService; -import org.redisson.liveobject.core.RedissonObjectBuilder; import static org.fest.assertions.Assertions.assertThat; diff --git a/datashare-tasks/src/test/java/org/icij/datashare/asynctasks/TaskManagerRedisCodecTest.java b/datashare-tasks/src/test/java/org/icij/datashare/asynctasks/TaskManagerRedisCodecTest.java index 125976054..6722e9abb 100644 --- a/datashare-tasks/src/test/java/org/icij/datashare/asynctasks/TaskManagerRedisCodecTest.java +++ b/datashare-tasks/src/test/java/org/icij/datashare/asynctasks/TaskManagerRedisCodecTest.java @@ -2,24 +2,27 @@ import io.netty.buffer.Unpooled; import org.fest.assertions.Assertions; +import org.icij.datashare.asynctasks.bus.amqp.UriResult; import org.icij.datashare.user.User; import org.junit.Test; import org.redisson.client.handler.State; +import java.io.IOException; +import java.net.URI; import java.nio.charset.Charset; import java.util.HashMap; +import java.util.Map; import static org.fest.assertions.Assertions.assertThat; import static org.fest.assertions.MapAssert.entry; +import static org.icij.datashare.json.JsonObjectMapper.MAPPER; public class TaskManagerRedisCodecTest { TaskManagerRedis.TaskViewCodec codec = new TaskManagerRedis.TaskViewCodec(); @Test public void test_json_serialize_deserialize_with_inline_properties_map() throws Exception { - Task taskView = new Task<>("name", User.local(), new HashMap<>() {{ - put("key", "value"); - }}); + Task taskView = new Task<>("name", User.local(), Map.of("key", "value")); String json = codec.getValueEncoder().encode(taskView).toString(Charset.defaultCharset()); assertThat(json).contains("\"key\":\"value\""); @@ -31,4 +34,30 @@ public void test_json_serialize_deserialize_with_inline_properties_map() throws Assertions.assertThat(actualTask.args).includes(entry("key", "value")); Assertions.assertThat(actualTask.getUser()).isEqualTo(User.local()); } + + @Test + public void test_uri_result() throws Exception { + Task task = new Task<>("name", User.local(), new HashMap<>()); + task.setResult(new UriResult(new URI("file://uri"), 123L)); + + assertThat(encodeDecode(task).getResult()).isInstanceOf(UriResult.class); + } + + @Test + public void test_simple_results() throws Exception { + Task task = new Task<>("name", User.local(), new HashMap<>()); + task.setResult(123L); + assertThat(encodeDecode(task).getResult()).isInstanceOf(Long.class); + + task.setResult("string"); + assertThat(encodeDecode(task).getResult()).isInstanceOf(String.class); + + task.setResult(123); + assertThat(encodeDecode(task).getResult()).isInstanceOf(Integer.class); + } + + private Task encodeDecode(Task task) throws IOException { + String json = codec.getValueEncoder().encode(task).toString(Charset.defaultCharset()); + return (Task) codec.getValueDecoder().decode(Unpooled.wrappedBuffer(json.getBytes()), new State()); + } }