From 8a500af887cbd1b398ba205f161399d84408fbf9 Mon Sep 17 00:00:00 2001 From: Aliaksandr Stsiapanay Date: Mon, 4 Nov 2024 12:13:49 +0300 Subject: [PATCH] feat: Support If-Match and If-None-Match for get file request #549 (#556) --- .../controller/DownloadFileController.java | 6 +- .../server/controller/ResourceController.java | 11 +-- .../server/service/ApplicationService.java | 6 +- .../core/server/service/RuleService.java | 3 +- .../epam/aidial/core/server/FileApiTest.java | 42 ++++++++++- .../aidial/core/server/ResourceApiTest.java | 10 +-- .../core/storage/service/ResourceService.java | 32 ++++---- .../aidial/core/storage/util/EtagHeader.java | 73 +++++++++++-------- .../core/storage/util/EtagHeaderTest.java | 31 ++++++++ 9 files changed, 153 insertions(+), 61 deletions(-) diff --git a/server/src/main/java/com/epam/aidial/core/server/controller/DownloadFileController.java b/server/src/main/java/com/epam/aidial/core/server/controller/DownloadFileController.java index d48b38c5..76cde2f3 100644 --- a/server/src/main/java/com/epam/aidial/core/server/controller/DownloadFileController.java +++ b/server/src/main/java/com/epam/aidial/core/server/controller/DownloadFileController.java @@ -2,9 +2,11 @@ import com.epam.aidial.core.server.Proxy; import com.epam.aidial.core.server.ProxyContext; +import com.epam.aidial.core.server.util.ProxyUtil; import com.epam.aidial.core.server.vertx.stream.InputStreamReader; import com.epam.aidial.core.storage.http.HttpStatus; import com.epam.aidial.core.storage.resource.ResourceDescriptor; +import com.epam.aidial.core.storage.util.EtagHeader; import io.vertx.core.Future; import io.vertx.core.http.HttpHeaders; import io.vertx.core.http.HttpServerResponse; @@ -22,8 +24,8 @@ protected Future handle(ResourceDescriptor resource, boolean hasWriteAccess) if (resource.isFolder()) { return context.respond(HttpStatus.BAD_REQUEST, "Can't download a folder"); } - - proxy.getVertx().executeBlocking(() -> proxy.getResourceService().getResourceStream(resource), false) + EtagHeader etagHeader = ProxyUtil.etag(context.getRequest()); + proxy.getVertx().executeBlocking(() -> proxy.getResourceService().getResourceStream(resource, etagHeader), false) .compose(resourceStream -> { if (resourceStream == null) { return context.respond(HttpStatus.NOT_FOUND); diff --git a/server/src/main/java/com/epam/aidial/core/server/controller/ResourceController.java b/server/src/main/java/com/epam/aidial/core/server/controller/ResourceController.java index d9d88603..8acf05cd 100644 --- a/server/src/main/java/com/epam/aidial/core/server/controller/ResourceController.java +++ b/server/src/main/java/com/epam/aidial/core/server/controller/ResourceController.java @@ -123,8 +123,9 @@ private Future getResource(ResourceDescriptor descriptor, boolean hasWriteAcc return context.respond(HttpStatus.BAD_REQUEST, "Folder not allowed: " + descriptor.getUrl()); } + EtagHeader etagHeader = ProxyUtil.etag(context.getRequest()); Future> responseFuture = (descriptor.getType() == ResourceTypes.APPLICATION) - ? getApplicationData(descriptor, hasWriteAccess) : getResourceData(descriptor); + ? getApplicationData(descriptor, hasWriteAccess, etagHeader) : getResourceData(descriptor, etagHeader); responseFuture.onSuccess(pair -> { context.putHeader(HttpHeaders.ETAG, pair.getKey().getEtag()) @@ -136,9 +137,9 @@ private Future getResource(ResourceDescriptor descriptor, boolean hasWriteAcc return Future.succeededFuture(); } - private Future> getApplicationData(ResourceDescriptor descriptor, boolean hasWriteAccess) { + private Future> getApplicationData(ResourceDescriptor descriptor, boolean hasWriteAccess, EtagHeader etagHeader) { return vertx.executeBlocking(() -> { - Pair result = applicationService.getApplication(descriptor); + Pair result = applicationService.getApplication(descriptor, etagHeader); ResourceItemMetadata meta = result.getKey(); Application application = result.getValue(); @@ -151,9 +152,9 @@ private Future> getApplicationData(ResourceDe }, false); } - private Future> getResourceData(ResourceDescriptor descriptor) { + private Future> getResourceData(ResourceDescriptor descriptor, EtagHeader etag) { return vertx.executeBlocking(() -> { - Pair result = service.getResourceWithMetadata(descriptor); + Pair result = service.getResourceWithMetadata(descriptor, etag); if (result == null) { throw new ResourceNotFoundException(); diff --git a/server/src/main/java/com/epam/aidial/core/server/service/ApplicationService.java b/server/src/main/java/com/epam/aidial/core/server/service/ApplicationService.java index 13ed1429..4eb45018 100644 --- a/server/src/main/java/com/epam/aidial/core/server/service/ApplicationService.java +++ b/server/src/main/java/com/epam/aidial/core/server/service/ApplicationService.java @@ -147,8 +147,12 @@ public List getPublicApplications(ProxyContext context) { } public Pair getApplication(ResourceDescriptor resource) { + return getApplication(resource, EtagHeader.ANY); + } + + public Pair getApplication(ResourceDescriptor resource, EtagHeader etagHeader) { verifyApplication(resource); - Pair result = resourceService.getResourceWithMetadata(resource); + Pair result = resourceService.getResourceWithMetadata(resource, etagHeader); if (result == null) { throw new ResourceNotFoundException("Application is not found: " + resource.getUrl()); diff --git a/server/src/main/java/com/epam/aidial/core/server/service/RuleService.java b/server/src/main/java/com/epam/aidial/core/server/service/RuleService.java index 9481ff16..c97ccd4a 100644 --- a/server/src/main/java/com/epam/aidial/core/server/service/RuleService.java +++ b/server/src/main/java/com/epam/aidial/core/server/service/RuleService.java @@ -12,6 +12,7 @@ import com.epam.aidial.core.storage.data.ResourceItemMetadata; import com.epam.aidial.core.storage.resource.ResourceDescriptor; import com.epam.aidial.core.storage.service.ResourceService; +import com.epam.aidial.core.storage.util.EtagHeader; import com.fasterxml.jackson.core.type.TypeReference; import org.apache.commons.lang3.tuple.Pair; @@ -129,7 +130,7 @@ private Map> getCachedRules() { Pair>> current = cachedRules.get(); if (current == null || current.getKey() != key) { - Pair resource = resources.getResourceWithMetadata(PUBLIC_RULES); + Pair resource = resources.getResourceWithMetadata(PUBLIC_RULES, EtagHeader.ANY); Pair>> next = (resource == null) ? Pair.of(Long.MIN_VALUE, decodeRules(null)) : Pair.of(resource.getKey().getUpdatedAt(), decodeRules(resource.getValue())); diff --git a/server/src/test/java/com/epam/aidial/core/server/FileApiTest.java b/server/src/test/java/com/epam/aidial/core/server/FileApiTest.java index 2677abcb..5129969d 100644 --- a/server/src/test/java/com/epam/aidial/core/server/FileApiTest.java +++ b/server/src/test/java/com/epam/aidial/core/server/FileApiTest.java @@ -18,12 +18,14 @@ import io.vertx.junit5.VertxTestContext; import lombok.extern.slf4j.Slf4j; import org.apache.http.HttpHeaders; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import java.util.Map; import java.util.stream.IntStream; +import static org.junit.Assert.assertTrue; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; @@ -1410,7 +1412,43 @@ public void testIfMatch(Vertx vertx, VertxTestContext context) { context.succeeding(response -> { context.verify(() -> { assertEquals(412, response.statusCode()); - assertEquals("ETag 123 is rejected", response.bodyAsString()); + assertTrue(response.bodyAsString().startsWith("If-match condition is failed for etag")); + checkpoint.flag(); + promise.complete(); + }); + }) + ); + + return promise.future(); + }).compose(mapper -> { + Promise promise = Promise.promise(); + // get the test file with incorrect ETag + client.get(serverPort, "localhost", "/v1/files/7G9WZNcoY26Vy9D7bEgbv6zqbJGfyDp9KZyEbJR4XMZt/test_file.txt") + .putHeader("Api-key", "proxyKey2") + .putHeader(HttpHeaders.IF_MATCH, "123") + .as(BodyCodec.string()) + .send(context.succeeding(response -> { + context.verify(() -> { + assertEquals(412, response.statusCode()); + assertTrue(response.body().startsWith("If-match condition is failed for etag")); + checkpoint.flag(); + promise.complete(); + }); + })); + + return promise.future(); + }).compose(mapper -> { + Promise promise = Promise.promise(); + // get the test file with correct ETag + client.get(serverPort, "localhost", "/v1/files/7G9WZNcoY26Vy9D7bEgbv6zqbJGfyDp9KZyEbJR4XMZt/test_file.txt") + .putHeader("Api-key", "proxyKey2") + .putHeader(HttpHeaders.IF_MATCH, TEST_FILE_ETAG) + .as(BodyCodec.string()) + .send( + context.succeeding(response -> { + context.verify(() -> { + assertEquals(200, response.statusCode()); + assertEquals(TEST_FILE_CONTENT, response.body()); checkpoint.flag(); promise.complete(); }); @@ -1476,7 +1514,7 @@ public void testIfMatch(Vertx vertx, VertxTestContext context) { .send(context.succeeding(response -> { context.verify(() -> { assertEquals(412, response.statusCode()); - assertEquals("ETag %s is rejected".formatted(TEST_FILE_ETAG), response.bodyAsString()); + assertTrue(response.bodyAsString().startsWith("If-match condition is failed for etag")); checkpoint.flag(); promise.complete(); }); diff --git a/server/src/test/java/com/epam/aidial/core/server/ResourceApiTest.java b/server/src/test/java/com/epam/aidial/core/server/ResourceApiTest.java index 88d986c6..c4b0cf85 100644 --- a/server/src/test/java/com/epam/aidial/core/server/ResourceApiTest.java +++ b/server/src/test/java/com/epam/aidial/core/server/ResourceApiTest.java @@ -121,7 +121,7 @@ public void testIfMatch() { verifyNotExact(response, 200, "\"etag\":\"70edd26b3686de5efcdae93fcc87c2bb\""); response = resourceRequest(HttpMethod.PUT, "/folder/conversation", CONVERSATION_BODY_2, "if-match", "123"); - verifyNotExact(response, 412, "ETag 123 is rejected"); + verifyNotExact(response, 412, "If-match condition is failed for etag"); response = resourceRequest(HttpMethod.PUT, "/folder/conversation", CONVERSATION_BODY_2, "if-match", "70edd26b3686de5efcdae93fcc87c2bb"); verifyNotExact(response, 200, "\"etag\":\"82833ed7a10a4f99253fccdef4091ad9\""); @@ -132,7 +132,7 @@ public void testIfMatch() { verifyNotExact(response, 200, "\"etag\":\"82833ed7a10a4f99253fccdef4091ad9\""); response = resourceRequest(HttpMethod.DELETE, "/folder/conversation", "", "if-match", "123"); - verify(response, 412, "ETag 123 is rejected"); + verifyNotExact(response, 412, "If-match condition is failed for etag"); response = resourceRequest(HttpMethod.DELETE, "/folder/conversation", "", "if-match", "82833ed7a10a4f99253fccdef4091ad9"); verify(response, 200, ""); @@ -166,9 +166,9 @@ void testMaxContentSize() { } @Test - void testUnsupportedIfNoneMatchHeader() { - Response response = resourceRequest(HttpMethod.PUT, "/folder/big", "1", "if-none-match", "unsupported"); - verify(response, 400, "only header if-none-match=* is supported"); + void testIfNoneMatch() { + Response response = resourceRequest(HttpMethod.PUT, "/folder/big", CONVERSATION_BODY_1, "if-none-match", "unsupported"); + verifyNotExact(response, 200, "big"); } @Test diff --git a/storage/src/main/java/com/epam/aidial/core/storage/service/ResourceService.java b/storage/src/main/java/com/epam/aidial/core/storage/service/ResourceService.java index 01fec49e..c57446b2 100644 --- a/storage/src/main/java/com/epam/aidial/core/storage/service/ResourceService.java +++ b/storage/src/main/java/com/epam/aidial/core/storage/service/ResourceService.java @@ -278,12 +278,12 @@ public boolean hasResource(ResourceDescriptor descriptor) { } @Nullable - public Pair getResourceWithMetadata(ResourceDescriptor descriptor) { - return getResourceWithMetadata(descriptor, true); + public Pair getResourceWithMetadata(ResourceDescriptor descriptor, EtagHeader etag) { + return getResourceWithMetadata(descriptor, etag, true); } @Nullable - private Pair getResourceWithMetadata(ResourceDescriptor descriptor, boolean lock) { + private Pair getResourceWithMetadata(ResourceDescriptor descriptor, EtagHeader etagHeader, boolean lock) { String redisKey = redisKey(descriptor); Result result = redisGet(redisKey, true); @@ -300,6 +300,7 @@ private Pair getResourceWithMetadata(ResourceDescr } if (result.exists()) { + etagHeader.validate(result.etag); return Pair.of( toResourceItemMetadata(descriptor, result), new String(result.body, StandardCharsets.UTF_8)); @@ -310,16 +311,16 @@ private Pair getResourceWithMetadata(ResourceDescr @Nullable public String getResource(ResourceDescriptor descriptor) { - return getResource(descriptor, true); + return getResource(descriptor, EtagHeader.ANY, true); } @Nullable - private String getResource(ResourceDescriptor descriptor, boolean lock) { - Pair result = getResourceWithMetadata(descriptor, lock); + private String getResource(ResourceDescriptor descriptor, EtagHeader etag, boolean lock) { + Pair result = getResourceWithMetadata(descriptor, etag, lock); return (result == null) ? null : result.getRight(); } - public ResourceStream getResourceStream(ResourceDescriptor resource) throws IOException { + public ResourceStream getResourceStream(ResourceDescriptor resource, EtagHeader etagHeader) throws IOException { if (resource.getType().requireCompression()) { throw new IllegalArgumentException("Streaming is supported for uncompressed resources only"); } @@ -327,13 +328,13 @@ public ResourceStream getResourceStream(ResourceDescriptor resource) throws IOEx String key = redisKey(resource); Result result = redisGet(key, true); if (result != null) { - return ResourceStream.fromResult(result); + return ResourceStream.fromResult(result, etagHeader); } try (LockService.Lock ignored = lockService.lock(key)) { result = redisGet(key, true); if (result != null) { - return ResourceStream.fromResult(result); + return ResourceStream.fromResult(result, etagHeader); } Blob blob = blobStore.load(resource.getAbsoluteFilePath()); @@ -351,9 +352,10 @@ public ResourceStream getResourceStream(ResourceDescriptor resource) throws IOEx if (length <= maxSize) { result = blobToResult(blob, metadata); redisPut(key, result); - return ResourceStream.fromResult(result); + return ResourceStream.fromResult(result, etagHeader); } + etagHeader.validate(etag); return new ResourceStream(payload.openStream(), etag, contentType, length); } } @@ -457,11 +459,7 @@ public ResourceItemMetadata computeResource(ResourceDescriptor descriptor, EtagH String redisKey = redisKey(descriptor); try (var ignore = lockService.lock(redisKey)) { - Pair oldResult = getResourceWithMetadata(descriptor, false); - - if (oldResult != null) { - etag.validate(oldResult.getKey().getEtag()); - } + Pair oldResult = getResourceWithMetadata(descriptor, etag, false); String oldBody = oldResult == null ? null : oldResult.getValue(); String newBody = fn.apply(oldBody); @@ -843,11 +841,13 @@ public void close() throws IOException { } @Nullable - private static ResourceStream fromResult(Result item) { + private static ResourceStream fromResult(Result item, EtagHeader etagHeader) { if (!item.exists()) { return null; } + etagHeader.validate(item.etag); + return new ResourceStream( new ByteArrayInputStream(item.body), item.etag(), diff --git a/storage/src/main/java/com/epam/aidial/core/storage/util/EtagHeader.java b/storage/src/main/java/com/epam/aidial/core/storage/util/EtagHeader.java index 72078c99..782e0fd2 100644 --- a/storage/src/main/java/com/epam/aidial/core/storage/util/EtagHeader.java +++ b/storage/src/main/java/com/epam/aidial/core/storage/util/EtagHeader.java @@ -4,32 +4,33 @@ import com.epam.aidial.core.storage.http.HttpStatus; import lombok.AccessLevel; import lombok.AllArgsConstructor; -import lombok.Getter; import org.apache.commons.lang3.StringUtils; import java.util.Arrays; import java.util.Set; import java.util.function.Supplier; import java.util.stream.Collectors; +import javax.annotation.Nullable; @AllArgsConstructor(access = AccessLevel.PRIVATE) public class EtagHeader { public static final String ANY_TAG = "*"; - public static final EtagHeader ANY = new EtagHeader(Set.of(), "", true); - public static final EtagHeader NEW_ONLY = new EtagHeader(Set.of(), "", false); - private final Set tags; - private final String raw; - @Getter - private final boolean overwrite; + public static final EtagHeader ANY = new EtagHeader(null, Set.of()); + public static final EtagHeader NEW_ONLY = new EtagHeader(null, null); + /** + * null means any tag '*' + */ + private final Set ifMatchTags; + /** + * null means any tag '*' + */ + private final Set ifNoneMatchTags; public void validate(String etag) { validate(() -> etag); } public void validate(Supplier etagSupplier) { - if (tags.isEmpty() && overwrite) { - return; - } String etag = etagSupplier.get(); if (etag == null) { @@ -37,12 +38,20 @@ public void validate(Supplier etagSupplier) { return; } - if (!overwrite) { + // If-None-Match used with the * value can be used to save a file not known to exist, + // guaranteeing that another upload didn't happen before + if (ifNoneMatchTags == null) { throw new HttpException(HttpStatus.PRECONDITION_FAILED, "Resource already exists"); } - if (!tags.contains(etag)) { - throw new HttpException(HttpStatus.PRECONDITION_FAILED, "ETag %s is rejected".formatted(raw)); + // if-match is not any and doesn't match the etag + if (ifMatchTags != null && !ifMatchTags.contains(etag)) { + throw new HttpException(HttpStatus.PRECONDITION_FAILED, "If-match condition is failed for etag: " + etag); + } + + // if-non-match matches the etag + if (ifNoneMatchTags.contains(etag)) { + throw new HttpException(HttpStatus.PRECONDITION_FAILED, "if-none-match condition is failed for etag: " + etag); } } @@ -53,30 +62,36 @@ public void validate(Supplier etagSupplier) { * @param ifNoneMatch HTTP header */ public static EtagHeader fromHeader(String ifMatch, String ifNoneMatch) { - Set tags = parseIfMatch(StringUtils.strip(ifMatch)); - boolean overwrite = parseOverwrite(StringUtils.strip(ifNoneMatch)); - return new EtagHeader(tags, ifMatch, overwrite); + Set ifMatchTags = parseIfMatch(StringUtils.strip(ifMatch)); + Set ifNoneMatchTags = parseIfNoneMatch(StringUtils.strip(ifNoneMatch)); + return new EtagHeader(ifMatchTags, ifNoneMatchTags); } - private static Set parseIfMatch(String value) { - if (StringUtils.isEmpty(value) || ANY_TAG.equals(value)) { + @Nullable + private static Set parseIfNoneMatch(@Nullable String value) { + if (ANY_TAG.equals(value)) { + return null; + } + + if (value == null) { return Set.of(); } - return Arrays.stream(value.split(",")) - .map(tag -> StringUtils.strip(tag, "\"")) - .collect(Collectors.toUnmodifiableSet()); + return parseTagValue(value); } - private static boolean parseOverwrite(String value) { - if (ANY_TAG.equals(value)) { - return false; - } - - if (value != null) { - throw new HttpException(HttpStatus.BAD_REQUEST, "only header if-none-match=* is supported"); + @Nullable + private static Set parseIfMatch(@Nullable String value) { + if (ANY_TAG.equals(value) || StringUtils.isEmpty(value)) { + return null; } + return parseTagValue(value); + } - return true; + private static Set parseTagValue(String value) { + return Arrays.stream(value.split(",")) + .map(tag -> StringUtils.strip(tag, "\"")) + .collect(Collectors.toUnmodifiableSet()); } + } diff --git a/storage/src/test/java/com/epam/aidial/core/storage/util/EtagHeaderTest.java b/storage/src/test/java/com/epam/aidial/core/storage/util/EtagHeaderTest.java index 62bdbc4b..8e8faf57 100644 --- a/storage/src/test/java/com/epam/aidial/core/storage/util/EtagHeaderTest.java +++ b/storage/src/test/java/com/epam/aidial/core/storage/util/EtagHeaderTest.java @@ -49,4 +49,35 @@ void testNoOverwrite() { EtagHeader etag = EtagHeader.fromHeader(null, EtagHeader.ANY_TAG); assertThrows(HttpException.class, () -> etag.validate("123")); } + + @Test + void testIfMatchAndNoneIfMatch() { + EtagHeader etag = EtagHeader.fromHeader("299,123", "235,326"); + etag.validate("123"); + } + + @Test + void testIfMatchAnyAndNoneIfMatch() { + EtagHeader etag = EtagHeader.fromHeader("*", "235,326"); + etag.validate("123"); + } + + @Test + void testNoneIfMatchFail() { + EtagHeader etag = EtagHeader.fromHeader(null, "235,326"); + assertThrows(HttpException.class, () -> etag.validate("326")); + } + + @Test + void testNoneIfMatchAnyFail() { + EtagHeader etag = EtagHeader.fromHeader(null, "*"); + assertThrows(HttpException.class, () -> etag.validate("326")); + } + + @Test + void testNoneIfMatchPass() { + EtagHeader etag = EtagHeader.fromHeader(null, "235,326"); + etag.validate("123"); + } + } \ No newline at end of file