Skip to content

Commit d23b0d3

Browse files
gaborkaszabgaborkaszab
authored
Core: Include query params into ETag calculation in reference IRC (#15057)
1 parent 00df493 commit d23b0d3

6 files changed

Lines changed: 127 additions & 21 deletions

File tree

‎core/src/main/java/org/apache/iceberg/rest/ETagProvider.java‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,19 +19,32 @@
1919
package org.apache.iceberg.rest;
2020

2121
import java.nio.charset.StandardCharsets;
22+
import java.util.Map;
23+
import java.util.TreeMap;
24+
import org.apache.iceberg.relocated.com.google.common.base.Joiner;
2225
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
2326
import org.apache.iceberg.relocated.com.google.common.hash.HashFunction;
2427
import org.apache.iceberg.relocated.com.google.common.hash.Hashing;
2528

2629
class ETagProvider {
2730
private static final HashFunction MURMUR3 = Hashing.murmur3_32_fixed();
2831

32+
private static final Joiner.MapJoiner PARAMS_JOINER = Joiner.on(",").withKeyValueSeparator("=");
33+
private static final Joiner COMMA = Joiner.on(',');
34+
2935
private ETagProvider() {}
3036

31-
public static String of(String metadataLocation) {
37+
public static String of(String metadataLocation, Map<String, String> params) {
3238
Preconditions.checkArgument(null != metadataLocation, "Invalid metadata location: null");
3339
Preconditions.checkArgument(!metadataLocation.isEmpty(), "Invalid metadata location: empty");
3440

35-
return MURMUR3.hashString(metadataLocation, StandardCharsets.UTF_8).toString();
41+
String stringToHash = metadataLocation;
42+
if (params != null && !params.isEmpty()) {
43+
Map<String, String> orderedParams = new TreeMap<>(params);
44+
45+
stringToHash = COMMA.join(metadataLocation, PARAMS_JOINER.join(orderedParams));
46+
}
47+
48+
return MURMUR3.hashString(stringToHash, StandardCharsets.UTF_8).toString();
3649
}
3750
}

‎core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ private RESTCatalogProperties() {}
2626

2727
public static final String SNAPSHOT_LOADING_MODE = "snapshot-loading-mode";
2828
public static final String SNAPSHOT_LOADING_MODE_DEFAULT = SnapshotMode.ALL.name();
29+
public static final String SNAPSHOTS_QUERY_PARAMETER = "snapshots";
2930

3031
public static final String METRICS_REPORTING_ENABLED = "rest-metrics-reporting-enabled";
3132
public static final boolean METRICS_REPORTING_ENABLED_DEFAULT = true;

‎core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -426,7 +426,8 @@ public boolean tableExists(SessionContext context, TableIdentifier identifier) {
426426
}
427427

428428
private static Map<String, String> snapshotModeToParam(SnapshotMode mode) {
429-
return ImmutableMap.of("snapshots", mode.name().toLowerCase(Locale.US));
429+
return ImmutableMap.of(
430+
RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, mode.name().toLowerCase(Locale.US));
430431
}
431432

432433
private LoadTableResponse loadInternal(

‎core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,8 @@ public <T extends RESTResponse> T handleRequest(
285285
CatalogHandlers.createTable(catalog, namespace, request);
286286
responseHeaders.accept(
287287
ImmutableMap.of(
288-
HttpHeaders.ETAG, ETagProvider.of(response.metadataLocation())));
288+
HttpHeaders.ETAG,
289+
ETagProvider.of(response.metadataLocation(), defaultQueryParams())));
289290
return castResponse(responseType, response);
290291
});
291292
}
@@ -323,7 +324,7 @@ public <T extends RESTResponse> T handleRequest(
323324
Optional<HTTPHeaders.HTTPHeader> ifNoneMatchHeader =
324325
httpRequest.headers().firstEntry(HttpHeaders.IF_NONE_MATCH);
325326

326-
String eTag = ETagProvider.of(response.metadataLocation());
327+
String eTag = ETagProvider.of(response.metadataLocation(), httpRequest.queryParameters());
327328

328329
if (ifNoneMatchHeader.isPresent() && eTag.equals(ifNoneMatchHeader.get().value())) {
329330
return null;
@@ -383,7 +384,8 @@ public <T extends RESTResponse> T handleRequest(
383384

384385
responseHeaders.accept(
385386
ImmutableMap.of(
386-
HttpHeaders.ETAG, ETagProvider.of(response.metadataLocation())));
387+
HttpHeaders.ETAG,
388+
ETagProvider.of(response.metadataLocation(), defaultQueryParams())));
387389

388390
return castResponse(responseType, response);
389391
});
@@ -402,7 +404,8 @@ public <T extends RESTResponse> T handleRequest(
402404

403405
responseHeaders.accept(
404406
ImmutableMap.of(
405-
HttpHeaders.ETAG, ETagProvider.of(response.metadataLocation())));
407+
HttpHeaders.ETAG,
408+
ETagProvider.of(response.metadataLocation(), defaultQueryParams())));
406409

407410
return castResponse(responseType, response);
408411
});
@@ -537,6 +540,12 @@ public <T extends RESTResponse> T handleRequest(
537540
return null;
538541
}
539542

543+
private static Map<String, String> defaultQueryParams() {
544+
return Map.of(
545+
RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER,
546+
SnapshotMode.ALL.toString().toLowerCase(Locale.US));
547+
}
548+
540549
/**
541550
* This is a very simplistic approach that only validates the requirements for each table and does
542551
* not do any other conflict detection. Therefore, it does not guarantee true transactional
@@ -735,7 +744,9 @@ private static String planIDFromPathVars(Map<String, String> pathVars) {
735744
private static SnapshotMode snapshotModeFromQueryParams(Map<String, String> queryParams) {
736745
return SnapshotMode.valueOf(
737746
queryParams
738-
.getOrDefault("snapshots", RESTCatalogProperties.SNAPSHOT_LOADING_MODE_DEFAULT)
747+
.getOrDefault(
748+
RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER,
749+
RESTCatalogProperties.SNAPSHOT_LOADING_MODE_DEFAULT)
739750
.toUpperCase(Locale.US));
740751
}
741752
}

‎core/src/test/java/org/apache/iceberg/rest/TestETagProvider.java‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,30 +21,49 @@
2121
import static org.assertj.core.api.Assertions.assertThat;
2222
import static org.assertj.core.api.Assertions.assertThatThrownBy;
2323

24+
import java.util.Map;
2425
import org.junit.jupiter.api.Test;
2526

2627
public class TestETagProvider {
28+
static final String METADATA_LOCATION =
29+
"/var/folders/20/290st0_52y5fyjcj2mlg49500000gn/T/junit-3064022805908958416/db_name/tbl_name/metadata/00000-f7a7956e-61d0-499b-be60-b141283f8229.metadata.json";
30+
2731
@Test
2832
public void testNullInput() {
29-
assertThatThrownBy(() -> ETagProvider.of(null))
33+
assertThatThrownBy(() -> ETagProvider.of(null, null))
3034
.isInstanceOf(IllegalArgumentException.class)
3135
.hasMessageContaining("Invalid metadata location: null");
3236
}
3337

3438
@Test
3539
public void testEmptyInput() {
36-
assertThatThrownBy(() -> ETagProvider.of(""))
40+
assertThatThrownBy(() -> ETagProvider.of("", null))
3741
.isInstanceOf(IllegalArgumentException.class)
3842
.hasMessageContaining("Invalid metadata location: empty");
3943
}
4044

4145
@Test
4246
public void testETagContent() {
43-
assertThat("1f865717")
47+
assertThat("90b8ad4e")
48+
.isEqualTo(
49+
ETagProvider.of(METADATA_LOCATION, Map.of("param1", "value1", "param2", "value2")));
50+
51+
assertThat("cb787e6a")
4452
.isEqualTo(
4553
ETagProvider.of(
46-
"/var/folders/20/290st0_52y5fyjcj2mlg49500000gn/T/junit-3064022805908958416/db_name/tbl_name/metadata/00000-f7a7956e-61d0-499b-be60-b141283f8229.metadata.json"));
54+
METADATA_LOCATION, Map.of("param1", "other_value1", "param2", "other_value2")));
55+
56+
assertThat("55faa5d9").isEqualTo(ETagProvider.of("/short/path", null));
57+
58+
assertThat("55faa5d9").isEqualTo(ETagProvider.of("/short/path", Map.of()));
4759

48-
assertThat("55faa5d9").isEqualTo(ETagProvider.of("/short/path"));
60+
assertThat("8adf3766").isEqualTo(ETagProvider.of("/short/path", Map.of("param", "some_value")));
61+
}
62+
63+
@Test
64+
public void testDifferentParameterOrderGiveSameETag() {
65+
assertThat(ETagProvider.of(METADATA_LOCATION, Map.of("param1", "value1", "param2", "value2")))
66+
.isEqualTo(
67+
ETagProvider.of(METADATA_LOCATION, Map.of("param2", "value2", "param1", "value1")));
4968
}
5069
}

‎core/src/test/java/org/apache/iceberg/rest/TestFreshnessAwareLoading.java‎

Lines changed: 69 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -169,14 +169,68 @@ public void eTagWithRegisterTable() {
169169
assertThat(respHeaders).containsEntry(HttpHeaders.ETAG, eTag);
170170
}
171171

172+
@Test
173+
public void differentETagForDifferentSnapshotMode() {
174+
Map<String, String> responseHeaders = Maps.newHashMap();
175+
RESTCatalogAdapter adapter = adapterCapturingResponseHeaders(responseHeaders);
176+
RESTCatalog catalog =
177+
new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), config -> adapter);
178+
catalog.initialize(
179+
"test",
180+
ImmutableMap.of(
181+
RESTCatalogProperties.SNAPSHOT_LOADING_MODE,
182+
RESTCatalogProperties.SnapshotMode.REFS.name()));
183+
184+
catalog.createNamespace(TABLE.namespace());
185+
catalog.createTable(TABLE, SCHEMA);
186+
187+
assertThat(responseHeaders).containsKey(HttpHeaders.ETAG);
188+
String eTagForCreateTable = responseHeaders.get(HttpHeaders.ETAG);
189+
responseHeaders.clear();
190+
191+
catalog.loadTable(TABLE);
192+
193+
assertThat(responseHeaders).containsKey(HttpHeaders.ETAG);
194+
assertThat(eTagForCreateTable).isNotEqualTo(responseHeaders.get(HttpHeaders.ETAG));
195+
196+
// Verify that table load used the refs query parameter
197+
verify(adapter, times(1))
198+
.execute(
199+
matches(
200+
HTTPRequest.HTTPMethod.GET,
201+
RESOURCE_PATHS.table(TABLE),
202+
Map.of(),
203+
Map.of("snapshots", "refs")),
204+
eq(LoadTableResponse.class),
205+
any(),
206+
any());
207+
}
208+
172209
@Test
173210
public void notModifiedResponse() {
211+
// Capture the response headers from createTable to get an ETag.
212+
Map<String, String> responseHeaders = Maps.newHashMap();
213+
Mockito.doAnswer(
214+
invocation ->
215+
adapterForRESTServer.execute(
216+
invocation.getArgument(0),
217+
invocation.getArgument(1),
218+
invocation.getArgument(2),
219+
responseHeaders::putAll,
220+
ParserContext.builder().build()))
221+
.when(adapterForRESTServer)
222+
.execute(
223+
matches(HTTPRequest.HTTPMethod.POST, RESOURCE_PATHS.tables(TABLE.namespace())),
224+
eq(LoadTableResponse.class),
225+
any(),
226+
any());
227+
174228
restCatalog.createNamespace(TABLE.namespace());
175229
restCatalog.createTable(TABLE, SCHEMA);
176-
Table table = restCatalog.loadTable(TABLE);
230+
restCatalog.loadTable(TABLE);
177231

178-
String eTag =
179-
ETagProvider.of(((BaseTable) table).operations().current().metadataFileLocation());
232+
assertThat(responseHeaders).containsKeys(HttpHeaders.ETAG);
233+
String eTag = responseHeaders.get(HttpHeaders.ETAG);
180234

181235
Mockito.doAnswer(
182236
invocation -> {
@@ -730,20 +784,27 @@ protected RESTTableOperations newTableOps(
730784
assertThat(table.operations()).isInstanceOf(CustomTableOps.class);
731785
}
732786

733-
private RESTCatalog catalogWithResponseHeaders(Map<String, String> respHeaders) {
734-
RESTCatalogAdapter adapter =
787+
private RESTCatalogAdapter adapterCapturingResponseHeaders(Map<String, String> respHeaders) {
788+
return Mockito.spy(
735789
new RESTCatalogAdapter(backendCatalog) {
736790
@Override
737791
public <T extends RESTResponse> T execute(
738792
HTTPRequest request,
739793
Class<T> responseType,
740794
Consumer<ErrorResponse> errorHandler,
741795
Consumer<Map<String, String>> responseHeaders) {
742-
return super.execute(request, responseType, errorHandler, respHeaders::putAll);
796+
Consumer<Map<String, String>> compositeConsumer =
797+
headers -> {
798+
responseHeaders.accept(headers);
799+
respHeaders.putAll(headers);
800+
};
801+
return super.execute(request, responseType, errorHandler, compositeConsumer);
743802
}
744-
};
803+
});
804+
}
745805

746-
return catalog(adapter);
806+
private RESTCatalog catalogWithResponseHeaders(Map<String, String> respHeaders) {
807+
return catalog(adapterCapturingResponseHeaders(respHeaders));
747808
}
748809

749810
private RESTCatalog catalog(RESTCatalogAdapter adapter) {

0 commit comments

Comments
 (0)