This is an automated email from the ASF dual-hosted git repository.
amoghj pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg.git
The following commit(s) were added to refs/heads/main by this push:
new f631298fd4 Core: Return 304 from reference IRC (#14035)
f631298fd4 is described below
commit f631298fd43c7425dc08f23c5b85a91d8c373aa2
Author: gaborkaszab <[email protected]>
AuthorDate: Mon Oct 27 16:30:48 2025 +0100
Core: Return 304 from reference IRC (#14035)
---
.../java/org/apache/iceberg/rest/HTTPClient.java | 13 ++-
.../java/org/apache/iceberg/rest/HTTPHeaders.java | 6 ++
.../apache/iceberg/rest/RESTCatalogAdapter.java | 13 ++-
.../apache/iceberg/rest/RESTCatalogServlet.java | 9 ++
.../org/apache/iceberg/rest/TestRESTCatalog.java | 118 ++++++++++++++++-----
5 files changed, 130 insertions(+), 29 deletions(-)
diff --git a/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java
b/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java
index e3067d1834..b30caf1c7d 100644
--- a/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java
+++ b/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java
@@ -180,7 +180,8 @@ public class HTTPClient extends BaseHTTPClient {
int code = response.getCode();
return code == HttpStatus.SC_OK
|| code == HttpStatus.SC_ACCEPTED
- || code == HttpStatus.SC_NO_CONTENT;
+ || code == HttpStatus.SC_NO_CONTENT
+ || code == HttpStatus.SC_NOT_MODIFIED;
}
private static ErrorResponse buildDefaultErrorResponse(CloseableHttpResponse
response) {
@@ -324,8 +325,7 @@ public class HTTPClient extends BaseHTTPClient {
responseHeaders.accept(respHeaders);
// Skip parsing the response stream for any successful request not
expecting a response body
- if (response.getCode() == HttpStatus.SC_NO_CONTENT
- || (responseType == null && isSuccessful(response))) {
+ if (emptyBody(response, responseType)) {
return null;
}
@@ -360,6 +360,13 @@ public class HTTPClient extends BaseHTTPClient {
}
}
+ private <T extends RESTResponse> boolean emptyBody(
+ CloseableHttpResponse response, Class<T> responseType) {
+ return response.getCode() == HttpStatus.SC_NO_CONTENT
+ || response.getCode() == HttpStatus.SC_NOT_MODIFIED
+ || (responseType == null && isSuccessful(response));
+ }
+
@Override
public void close() throws IOException {
// Do not close the AuthSession as it's managed by the owner of this
HTTPClient.
diff --git a/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java
b/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java
index 23263899b9..4ee63934af 100644
--- a/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java
+++ b/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java
@@ -20,6 +20,7 @@ package org.apache.iceberg.rest;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
@@ -51,6 +52,11 @@ public interface HTTPHeaders {
.collect(Collectors.toSet());
}
+ /** Returns the first entry in this group for the given name
(case-insensitive). */
+ default Optional<HTTPHeader> firstEntry(String name) {
+ return entries().stream().filter(header ->
header.name().equalsIgnoreCase(name)).findFirst();
+ }
+
/** Returns whether this group contains an entry with the given name
(case-insensitive). */
default boolean contains(String name) {
return entries().stream().anyMatch(header ->
header.name().equalsIgnoreCase(name));
diff --git a/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
b/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
index d6d6200d4e..5c9fac6302 100644
--- a/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
+++ b/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
@@ -25,6 +25,7 @@ import java.util.Arrays;
import java.util.List;
import java.util.Locale;
import java.util.Map;
+import java.util.Optional;
import java.util.function.Consumer;
import java.util.stream.Collectors;
import org.apache.http.HttpHeaders;
@@ -279,8 +280,16 @@ public class RESTCatalogAdapter extends BaseHTTPClient {
tableIdentFromPathVars(vars),
snapshotModeFromQueryParams(httpRequest.queryParameters()));
- responseHeaders.accept(
- ImmutableMap.of(HttpHeaders.ETAG,
ETagProvider.of(response.metadataLocation())));
+ Optional<HTTPHeaders.HTTPHeader> ifNoneMatchHeader =
+ httpRequest.headers().firstEntry(HttpHeaders.IF_NONE_MATCH);
+
+ String eTag = ETagProvider.of(response.metadataLocation());
+
+ if (ifNoneMatchHeader.isPresent() &&
eTag.equals(ifNoneMatchHeader.get().value())) {
+ return null;
+ }
+
+ responseHeaders.accept(ImmutableMap.of(HttpHeaders.ETAG, eTag));
return castResponse(responseType, response);
}
diff --git a/core/src/test/java/org/apache/iceberg/rest/RESTCatalogServlet.java
b/core/src/test/java/org/apache/iceberg/rest/RESTCatalogServlet.java
index 80f66af4b0..6996db4805 100644
--- a/core/src/test/java/org/apache/iceberg/rest/RESTCatalogServlet.java
+++ b/core/src/test/java/org/apache/iceberg/rest/RESTCatalogServlet.java
@@ -114,6 +114,15 @@ public class RESTCatalogServlet extends HttpServlet {
if (responseBody != null) {
RESTObjectMapper.mapper().writeValue(response.getWriter(),
responseBody);
+ } else {
+ Pair<Route, Map<String, String>> routeAndVars =
+ Route.from(request.method(), request.path());
+ if (routeAndVars != null) {
+ Route route = routeAndVars.first();
+ if (route == Route.LOAD_TABLE) {
+ response.setStatus(HttpServletResponse.SC_NOT_MODIFIED);
+ }
+ }
}
} catch (RESTException e) {
LOG.error("Error processing REST request", e);
diff --git a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
index abfe52bad6..6f7af7ae75 100644
--- a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
+++ b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
@@ -115,6 +115,7 @@ public class TestRESTCatalog extends
CatalogTests<RESTCatalog> {
private RESTCatalog restCatalog;
private InMemoryCatalog backendCatalog;
private Server httpServer;
+ private RESTCatalogAdapter adapterForRESTServer;
@BeforeEach
public void createCatalog() throws Exception {
@@ -140,34 +141,36 @@ public class TestRESTCatalog extends
CatalogTests<RESTCatalog> {
"test-header",
"test-value"));
- RESTCatalogAdapter adaptor =
- new RESTCatalogAdapter(backendCatalog) {
- @Override
- public <T extends RESTResponse> T execute(
- HTTPRequest request,
- Class<T> responseType,
- Consumer<ErrorResponse> errorHandler,
- Consumer<Map<String, String>> responseHeaders) {
- // this doesn't use a Mockito spy because this is used for catalog
tests, which have
- // different method calls
- if (!ResourcePaths.tokens().equals(request.path())) {
- if (ResourcePaths.config().equals(request.path())) {
-
assertThat(request.headers().entries()).containsAll(catalogHeaders.entries());
- } else {
-
assertThat(request.headers().entries()).containsAll(contextHeaders.entries());
+ adapterForRESTServer =
+ Mockito.spy(
+ new RESTCatalogAdapter(backendCatalog) {
+ @Override
+ public <T extends RESTResponse> T execute(
+ HTTPRequest request,
+ Class<T> responseType,
+ Consumer<ErrorResponse> errorHandler,
+ Consumer<Map<String, String>> responseHeaders) {
+ // this doesn't use a Mockito spy because this is used for
catalog tests, which have
+ // different method calls
+ if (!ResourcePaths.tokens().equals(request.path())) {
+ if (ResourcePaths.config().equals(request.path())) {
+
assertThat(request.headers().entries()).containsAll(catalogHeaders.entries());
+ } else {
+
assertThat(request.headers().entries()).containsAll(contextHeaders.entries());
+ }
+ }
+ Object body = roundTripSerialize(request.body(), "request");
+ HTTPRequest req =
ImmutableHTTPRequest.builder().from(request).body(body).build();
+ T response = super.execute(req, responseType, errorHandler,
responseHeaders);
+ T responseAfterSerialization = roundTripSerialize(response,
"response");
+ return responseAfterSerialization;
}
- }
- Object body = roundTripSerialize(request.body(), "request");
- HTTPRequest req =
ImmutableHTTPRequest.builder().from(request).body(body).build();
- T response = super.execute(req, responseType, errorHandler,
responseHeaders);
- T responseAfterSerialization = roundTripSerialize(response,
"response");
- return responseAfterSerialization;
- }
- };
+ });
ServletContextHandler servletContext =
new ServletContextHandler(ServletContextHandler.NO_SESSIONS);
- servletContext.addServlet(new ServletHolder(new
RESTCatalogServlet(adaptor)), "/*");
+ servletContext.addServlet(
+ new ServletHolder(new RESTCatalogServlet(adapterForRESTServer)), "/*");
servletContext.setHandler(new GzipHandler());
this.httpServer = new Server(new
InetSocketAddress(InetAddress.getLoopbackAddress(), 0));
@@ -2885,6 +2888,73 @@ public class TestRESTCatalog extends
CatalogTests<RESTCatalog> {
assertThat(respHeaders).containsEntry(HttpHeaders.ETAG, eTag);
}
+ @SuppressWarnings("checkstyle:AssertThatThrownByWithMessageCheck")
+ @Test
+ public void testNotModified() {
+ catalog().createNamespace(TABLE.namespace());
+
+ Table table = catalog().createTable(TABLE, SCHEMA);
+
+ String eTag =
+ ETagProvider.of(((BaseTable)
table).operations().current().metadataFileLocation());
+
+ Mockito.doAnswer(
+ invocation -> {
+ HTTPRequest originalRequest = invocation.getArgument(0);
+
+ HTTPHeaders extendedHeaders =
+ ImmutableHTTPHeaders.copyOf(originalRequest.headers())
+ .putIfAbsent(
+ ImmutableHTTPHeader.builder()
+ .name(HttpHeaders.IF_NONE_MATCH)
+ .value(eTag)
+ .build());
+
+ ImmutableHTTPRequest extendedRequest =
+ ImmutableHTTPRequest.builder()
+ .from(originalRequest)
+ .headers(extendedHeaders)
+ .build();
+
+ return adapterForRESTServer.execute(
+ extendedRequest,
+ LoadTableResponse.class,
+ invocation.getArgument(2),
+ invocation.getArgument(3),
+ ParserContext.builder().build());
+ })
+ .when(adapterForRESTServer)
+ .execute(
+ reqMatcher(HTTPMethod.GET, RESOURCE_PATHS.table(TABLE)),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ // TODO: This won't throw when client side of freshness-aware loading is
implemented
+ assertThatThrownBy(() ->
catalog().loadTable(TABLE)).isInstanceOf(NullPointerException.class);
+
+ TableIdentifier metadataTableIdentifier =
+ TableIdentifier.of(TABLE.namespace().toString(), TABLE.name(),
"partitions");
+
+ // TODO: This won't throw when client side of freshness-aware loading is
implemented
+ assertThatThrownBy(() -> catalog().loadTable(metadataTableIdentifier))
+ .isInstanceOf(NullPointerException.class);
+
+ Mockito.verify(adapterForRESTServer, times(2))
+ .execute(
+ reqMatcher(HTTPMethod.GET, RESOURCE_PATHS.table(TABLE)),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ verify(adapterForRESTServer)
+ .execute(
+ reqMatcher(HTTPMethod.GET,
RESOURCE_PATHS.table(metadataTableIdentifier)),
+ any(),
+ any(),
+ any());
+ }
+
private RESTCatalog catalogWithResponseHeaders(Map<String, String>
respHeaders) {
RESTCatalogAdapter adapter =
new RESTCatalogAdapter(backendCatalog) {
