fix: Core returns wrong http code if if non match condition fails #560 (

#561)
epam · Nov 5, 2024 · 82e5256 · 82e5256
1 parent 83c1077
commit 82e5256
Show file tree

Hide file tree

Showing 10 changed files with 116 additions and 28 deletions.
diff --git a/server/src/main/java/com/epam/aidial/core/server/ProxyContext.java b/server/src/main/java/com/epam/aidial/core/server/ProxyContext.java
@@ -157,10 +157,15 @@ public Future<?> respond(HttpStatus status, String body) {
 
     public Future<?> respond(Throwable error, String fallbackError) {
         return error instanceof HttpException exception
-                ? respond(exception.getStatus(), exception.getMessage())
+                ? respond(exception)
                 : respond(HttpStatus.INTERNAL_SERVER_ERROR, fallbackError);
     }
 
+    public Future<?> respond(HttpException exception) {
+        response.headers().addAll(exception.getHeaders());
+        return respond(exception.getStatus(), exception.getMessage());
+    }
+
     public String getProject() {
         return key == null ? null : key.getProject();
     }

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
@@ -4,6 +4,7 @@
 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.HttpException;
 import com.epam.aidial.core.storage.http.HttpStatus;
 import com.epam.aidial.core.storage.resource.ResourceDescriptor;
 import com.epam.aidial.core.storage.util.EtagHeader;

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
@@ -257,7 +257,7 @@ private Future<?> deleteResource(ResourceDescriptor descriptor) {
 
     private void handleError(ResourceDescriptor descriptor, Throwable error) {
         if (error instanceof HttpException exception) {
-            context.respond(exception.getStatus(), exception.getMessage());
+            context.respond(exception);
         } else if (error instanceof IllegalArgumentException) {
             context.respond(HttpStatus.BAD_REQUEST, error.getMessage());
         } else if (error instanceof ResourceNotFoundException) {

diff --git a/server/src/main/java/com/epam/aidial/core/server/util/ProxyUtil.java b/server/src/main/java/com/epam/aidial/core/server/util/ProxyUtil.java
@@ -309,6 +309,6 @@ public static <T> Throwable processChain(T item, List<BaseRequestFunction<T>> ch
     }
 
     public static EtagHeader etag(HttpServerRequest request) {
-        return EtagHeader.fromHeader(request.getHeader(HttpHeaders.IF_MATCH), request.getHeader(HttpHeaders.IF_NONE_MATCH));
+        return EtagHeader.fromHeader(request.getHeader(HttpHeaders.IF_MATCH), request.getHeader(HttpHeaders.IF_NONE_MATCH), request.method().name());
     }
 }
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
@@ -18,7 +18,6 @@
 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;
 
@@ -1490,6 +1489,23 @@ public void testIfMatch(Vertx vertx, VertxTestContext context) {
                         });
                     }));
 
+            return promise.future();
+        }).compose(mapper -> {
+            Promise<Void> promise = Promise.promise();
+            // get the test file with incorrect If Non Match ETag
+            client.get(serverPort, "localhost", "/v1/files/7G9WZNcoY26Vy9D7bEgbv6zqbJGfyDp9KZyEbJR4XMZt/test_file.txt")
+                    .putHeader("Api-key", "proxyKey2")
+                    .putHeader(HttpHeaders.IF_NONE_MATCH, TEST_FILE_ETAG)
+                    .as(BodyCodec.string())
+                    .send(context.succeeding(response -> {
+                        context.verify(() -> {
+                            assertEquals(304, response.statusCode());
+                            assertEquals(TEST_FILE_ETAG, response.getHeader(HttpHeaders.ETAG));
+                            checkpoint.flag();
+                            promise.complete();
+                        });
+                    }));
+
             return promise.future();
         }).compose(mapper -> {
             Promise<Void> promise = Promise.promise();

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
@@ -169,6 +169,16 @@ void testMaxContentSize() {
     void testIfNoneMatch() {
         Response response = resourceRequest(HttpMethod.PUT, "/folder/big", CONVERSATION_BODY_1, "if-none-match", "unsupported");
         verifyNotExact(response, 200, "big");
+
+        response = resourceRequest(HttpMethod.GET, "/folder/big", CONVERSATION_BODY_1, "if-none-match", "unsupported");
+        verifyNotExact(response, 200, CONVERSATION_BODY_1);
+
+        response = resourceRequest(HttpMethod.GET, "/folder/big", CONVERSATION_BODY_1, "if-none-match", "70edd26b3686de5efcdae93fcc87c2bb");
+        assertEquals(304, response.status());
+        assertEquals("70edd26b3686de5efcdae93fcc87c2bb", response.headers().get("etag"));
+
+        response = resourceRequest(HttpMethod.PUT, "/folder/big", CONVERSATION_BODY_1, "if-none-match", "70edd26b3686de5efcdae93fcc87c2bb");
+        assertEquals(412, response.status());
     }
 
     @Test

diff --git a/storage/src/main/java/com/epam/aidial/core/storage/http/HttpException.java b/storage/src/main/java/com/epam/aidial/core/storage/http/HttpException.java
@@ -2,12 +2,24 @@
 
 import lombok.Getter;
 
+import java.util.Map;
+import java.util.Objects;
+import java.util.TreeMap;
+
 @Getter
 public class HttpException extends RuntimeException {
     private final HttpStatus status;
+    private final Map<String, String> headers;
 
     public HttpException(HttpStatus status, String message) {
+        this(status, message, Map.of());
+    }
+
+    public HttpException(HttpStatus status, String message, Map<String, String> headers) {
         super(message);
         this.status = status;
+        this.headers = new TreeMap<>(String::compareToIgnoreCase);
+        this.headers.putAll(Objects.requireNonNull(headers, "HTTP headers must not be null"));
     }
+
 }
diff --git a/storage/src/main/java/com/epam/aidial/core/storage/http/HttpStatus.java b/storage/src/main/java/com/epam/aidial/core/storage/http/HttpStatus.java
@@ -8,6 +8,7 @@
 public enum HttpStatus {
 
     OK(200),
+    NOT_MODIFIED(304),
     BAD_REQUEST(400),
     UNAUTHORIZED(401),
     FORBIDDEN(403),
@@ -34,6 +35,7 @@ public boolean is5xx() {
     public static HttpStatus fromStatusCode(int code) {
         return switch (code) {
             case 200 -> OK;
+            case 304 -> NOT_MODIFIED;
             case 400 -> BAD_REQUEST;
             case 401 -> UNAUTHORIZED;
             case 403 -> FORBIDDEN;

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
@@ -7,6 +7,7 @@
 import org.apache.commons.lang3.StringUtils;
 
 import java.util.Arrays;
+import java.util.Map;
 import java.util.Set;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
@@ -15,8 +16,8 @@
 @AllArgsConstructor(access = AccessLevel.PRIVATE)
 public class EtagHeader {
     public static final String ANY_TAG = "*";
-    public static final EtagHeader ANY = new EtagHeader(null, Set.of());
-    public static final EtagHeader NEW_ONLY = new EtagHeader(null, null);
+    public static final EtagHeader ANY = new EtagHeader(null, Set.of(), null);
+    public static final EtagHeader NEW_ONLY = new EtagHeader(null, null, null);
     /**
      * <code>null</code> means any tag '*'
      */
@@ -25,6 +26,10 @@ public class EtagHeader {
      * <code>null</code> means any tag '*'
      */
     private final Set<String> ifNoneMatchTags;
+    /**
+     * Http method, e.g. GET, PUT, POST and etc
+     */
+    private final String method;
 
     public void validate(String etag) {
         validate(() -> etag);
@@ -38,21 +43,42 @@ public void validate(Supplier<String> etagSupplier) {
             return;
         }
 
+        validateIfMatch(etag);
+
+        validateIfNoneMatch(etag);
+    }
+
+    private void validateIfNoneMatch(String etag) {
         // 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");
+            throw createIfNoneMatchException(etag, "Resource already exists");
         }
 
+        // if-non-match matches the etag
+        if (ifNoneMatchTags.contains(etag)) {
+            throw createIfNoneMatchException(etag, "if-none-match condition is failed for etag: " + etag);
+        }
+    }
+
+    private HttpException createIfNoneMatchException(String etag, String message) {
+        HttpStatus statusCode;
+        Map<String, String> headers;
+        if ("GET".equalsIgnoreCase(method) || "HEAD".equalsIgnoreCase(method)) {
+            statusCode = HttpStatus.NOT_MODIFIED;
+            headers = Map.of("etag", etag);
+        } else {
+            statusCode = HttpStatus.PRECONDITION_FAILED;
+            headers = Map.of();
+        }
+        return new HttpException(statusCode, message, headers);
+    }
+
+    private void validateIfMatch(String etag) {
         // 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);
-        }
     }
 
     /**
@@ -61,10 +87,10 @@ public void validate(Supplier<String> etagSupplier) {
      * @param ifMatch HTTP header
      * @param ifNoneMatch HTTP header
      */
-    public static EtagHeader fromHeader(String ifMatch, String ifNoneMatch) {
+    public static EtagHeader fromHeader(String ifMatch, String ifNoneMatch, String method) {
         Set<String> ifMatchTags = parseIfMatch(StringUtils.strip(ifMatch));
         Set<String> ifNoneMatchTags = parseIfNoneMatch(StringUtils.strip(ifNoneMatch));
-        return new EtagHeader(ifMatchTags, ifNoneMatchTags);
+        return new EtagHeader(ifMatchTags, ifNoneMatchTags, method);
     }
 
     @Nullable

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
@@ -2,81 +2,97 @@
 
 
 import com.epam.aidial.core.storage.http.HttpException;
+import com.epam.aidial.core.storage.http.HttpStatus;
 import org.junit.jupiter.api.Test;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 class EtagHeaderTest {
     @Test
     void testEtag() {
-        EtagHeader etag = EtagHeader.fromHeader("123", null);
+        EtagHeader etag = EtagHeader.fromHeader("123", null, "POST");
         etag.validate("123");
     }
 
     @Test
     void testEtagWithQuotes() {
-        EtagHeader etag = EtagHeader.fromHeader("\"123\"", null);
+        EtagHeader etag = EtagHeader.fromHeader("\"123\"", null, "POST");
         etag.validate("123");
     }
 
     @Test
     void testEtagList() {
-        EtagHeader etag = EtagHeader.fromHeader("\"123\",\"234\"", null);
+        EtagHeader etag = EtagHeader.fromHeader("\"123\",\"234\"", null, "POST");
         etag.validate("123");
         etag.validate("234");
     }
 
     @Test
     void testEtagAny() {
-        EtagHeader etag = EtagHeader.fromHeader(EtagHeader.ANY_TAG, null);
+        EtagHeader etag = EtagHeader.fromHeader(EtagHeader.ANY_TAG, null, "POST");
         etag.validate("any");
     }
 
     @Test
     void testMissingEtag() {
-        EtagHeader etag = EtagHeader.fromHeader(null, null);
+        EtagHeader etag = EtagHeader.fromHeader(null, null, "POST");
         etag.validate("any");
     }
 
     @Test
     void testEtagMismatch() {
-        EtagHeader etag = EtagHeader.fromHeader("123", null);
+        EtagHeader etag = EtagHeader.fromHeader("123", null, "POST");
         assertThrows(HttpException.class, () -> etag.validate("234"));
     }
 
     @Test
     void testNoOverwrite() {
-        EtagHeader etag = EtagHeader.fromHeader(null, EtagHeader.ANY_TAG);
+        EtagHeader etag = EtagHeader.fromHeader(null, EtagHeader.ANY_TAG, "POST");
         assertThrows(HttpException.class, () -> etag.validate("123"));
     }
 
     @Test
     void testIfMatchAndNoneIfMatch() {
-        EtagHeader etag = EtagHeader.fromHeader("299,123", "235,326");
+        EtagHeader etag = EtagHeader.fromHeader("299,123", "235,326", "POST");
         etag.validate("123");
     }
 
     @Test
     void testIfMatchAnyAndNoneIfMatch() {
-        EtagHeader etag = EtagHeader.fromHeader("*", "235,326");
+        EtagHeader etag = EtagHeader.fromHeader("*", "235,326", "POST");
         etag.validate("123");
     }
 
     @Test
-    void testNoneIfMatchFail() {
-        EtagHeader etag = EtagHeader.fromHeader(null, "235,326");
+    void testNoneIfMatchFail_ModifiedMethod() {
+        EtagHeader etag = EtagHeader.fromHeader(null, "235,326", "POST");
+        HttpException error = assertThrows(HttpException.class, () -> etag.validate("326"));
+        assertEquals(HttpStatus.PRECONDITION_FAILED, error.getStatus());
+    }
+
+    @Test
+    void testNoneIfMatchFail_ReadMethod() {
+        EtagHeader etag = EtagHeader.fromHeader(null, "235,326", "GET");
+        HttpException error = assertThrows(HttpException.class, () -> etag.validate("326"));
+        assertEquals(HttpStatus.NOT_MODIFIED, error.getStatus());
+    }
+
+    @Test
+    void testNoneIfMatchAnyFail_ModifiedMethod() {
+        EtagHeader etag = EtagHeader.fromHeader(null, "*", "POST");
         assertThrows(HttpException.class, () -> etag.validate("326"));
     }
 
     @Test
-    void testNoneIfMatchAnyFail() {
-        EtagHeader etag = EtagHeader.fromHeader(null, "*");
+    void testNoneIfMatchAnyFail_ReadMethod() {
+        EtagHeader etag = EtagHeader.fromHeader(null, "*", "READ");
         assertThrows(HttpException.class, () -> etag.validate("326"));
     }
 
     @Test
     void testNoneIfMatchPass() {
-        EtagHeader etag = EtagHeader.fromHeader(null, "235,326");
+        EtagHeader etag = EtagHeader.fromHeader(null, "235,326", "POST");
         etag.validate("123");
     }