huaxingao commented on code in PR #18015:
URL: https://github.com/apache/iceberg/pull/18015#discussion_r4020682538
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -478,6 +495,7 @@ public Table loadTable(SessionContext context,
TableIdentifier identifier) {
context,
identifier,
snapshotMode,
+ loadContext,
Review Comment:
The cache key here is `(sessionId, identifier)` and doesn't include the view
chain, so a load through `view_B` finds the entry saved by an earlier load
through `view_A`. On a `200` that's invisible, since the client rebuilds with
the current context, but on a `304` it returns the cached entry and its
`FileIO` still carries `view_A`. Would the credentials then be scoped to the
wrong view?
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -590,7 +617,8 @@ private Supplier<BaseTable> createTableSupplier(
paths.table(identifier),
Map::of,
mutationHeaders,
- tableFileIO(identifier, context, tableConf, credentials,
remoteSigningConfig),
+ tableFileIO(
+ identifier, context, tableConf, loadContext, credentials,
remoteSigningConfig),
Review Comment:
`loadContext` is used in `tableFileIO` here, but it isn't passed to
`newTableOps`. So the table operations object never gets it, and its
`refresh()` sends no query parameters at all, including no `referenced-by`.
`refresh()` runs on its own, not only when someone calls `loadTable`: on
stale reads and on commit retries. So the server sees the view chain on the
first load, but not on any refresh after that.
##########
core/src/main/java/org/apache/iceberg/rest/RESTUtil.java:
##########
@@ -436,4 +441,45 @@ public static Map<String, String>
configHeaders(Map<String, String> properties)
public static Map<String, String> idempotencyHeaders() {
return ImmutableMap.of(IDEMPOTENCY_KEY_HEADER,
UUIDUtil.generateUuidV7().toString());
}
+
+ /** Query parameters for the loadCredentials endpoint, from client-side
request context. */
+ public static Map<String, String> credentialsQueryParams(Map<String, String>
properties) {
+ ImmutableMap.Builder<String, String> queryParams = ImmutableMap.builder();
+ String planId = properties.get(RESTCatalogProperties.REST_SCAN_PLAN_ID);
+ if (planId != null) {
+ queryParams.put(RESTCatalogProperties.PLAN_ID_QUERY_PARAMETER, planId);
+ }
+
+ String referencedBy =
properties.get(RESTCatalogProperties.REST_REFERENCED_BY);
+ if (referencedBy != null) {
+ queryParams.put(RESTCatalogProperties.REFERENCED_BY_QUERY_PARAMETER,
referencedBy);
+ }
+
+ return queryParams.build();
+ }
+
+ /**
+ * Encodes a view chain (outermost first) as the {@code referenced-by} value.
+ *
+ * <p>Within an entry, the namespace levels and the view name are encoded
like the {@code parent}
+ * query parameter, as the spec requires, and joined by the namespace
separator as-is; entries are
+ * joined by a literal comma. The result is already percent-encoded and must
reach the wire
+ * verbatim, see {@link HTTPRequest#requestUri()}.
+ */
+ static String encodeReferencedBy(List<TableIdentifier> referencedBy, String
namespaceSeparator) {
+ if (referencedBy == null || referencedBy.isEmpty()) {
+ return null;
+ }
+
+ Preconditions.checkArgument(
+ !Strings.isNullOrEmpty(namespaceSeparator), "Invalid separator: null
or empty");
+
+ return referencedBy.stream()
+ .map(
+ ident ->
+ Stream.concat(Arrays.stream(ident.namespace().levels()),
Stream.of(ident.name()))
+ .map(level -> PercentCodec.encode(level,
StandardCharsets.UTF_8))
+ .collect(Collectors.joining(namespaceSeparator)))
Review Comment:
The configured separator is pasted into the URL unescaped, the levels and
view name go through `PercentCodec`, but the separator is just the string
handed to `joining()`, and this value skips `URIBuilder`, so nothing escapes
it. Fine for the default `%1F` since that's already encoded, but `#` makes the
view name become the URI fragment (never sent, so the server gets a truncated
chain) and a raw `\u001F` makes `requestUri()` throw. Could this normalize the
separator like `namespaceToQueryParam` does and then encode it like the levels
above?
##########
core/src/test/java/org/apache/iceberg/rest/TestReferencedByQueryParam.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.rest;
+
+import static org.apache.iceberg.rest.RequestMatcher.matches;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+
+import java.util.Map;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.catalog.LoadContext;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.SessionCatalog;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.catalog.ViewCatalog;
+import org.apache.iceberg.inmemory.InMemoryCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.rest.HTTPRequest.HTTPMethod;
+import org.apache.iceberg.rest.responses.LoadTableResponse;
+import org.apache.iceberg.rest.responses.LoadViewResponse;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+public class TestReferencedByQueryParam {
+
+ private static final Schema SCHEMA =
+ new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get()));
+
+ private static final Namespace NS = Namespace.of("ns");
+ private static final TableIdentifier TABLE_IDENT = TableIdentifier.of(NS,
"test_table");
+
+ private RESTCatalogAdapter adapter;
+ private RESTCatalog restCatalog;
+
+ @BeforeEach
+ public void before() {
+ InMemoryCatalog backendCatalog = new InMemoryCatalog();
+ backendCatalog.initialize("test", ImmutableMap.of());
+
+ adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+ restCatalog = new RESTCatalog(SessionCatalog.SessionContext.createEmpty(),
(config) -> adapter);
+ restCatalog.initialize(
+ "test",
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
"org.apache.iceberg.inmemory.InMemoryFileIO"));
+
+ restCatalog.createNamespace(NS);
+ restCatalog.buildTable(TABLE_IDENT, SCHEMA).create();
+ Mockito.clearInvocations(adapter);
+ }
+
+ @AfterEach
+ public void after() throws Exception {
+ if (restCatalog != null) {
+ restCatalog.close();
+ }
+ }
+
+ @Test
+ public void loadTableSendsReferencedBy() {
+ restCatalog.loadTable(TABLE_IDENT, referencedBy("outer_view"));
+
+ // the test adapter uses %2E as the namespace separator
+ Mockito.verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ "v1/namespaces/ns/tables/test_table",
+ Map.of(),
+ ImmutableMap.of(
+ "snapshots",
+ "all",
+ RESTCatalogProperties.REFERENCED_BY_QUERY_PARAMETER,
+ "ns%2Eouter_view")),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+ }
+
+ @Test
+ public void loadTableWithoutContextHasNoReferencedByParam() {
+ restCatalog.loadTable(TABLE_IDENT);
+
+ Mockito.verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ "v1/namespaces/ns/tables/test_table",
+ Map.of(),
+ ImmutableMap.of("snapshots", "all")),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+ }
+
+ @Test
+ public void loadViewSendsReferencedBy() {
+ TableIdentifier viewIdent = createView();
+
+ restCatalog.loadView(viewIdent, referencedBy("outer_view"));
+
+ Mockito.verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ "v1/namespaces/ns/views/test_view",
+ Map.of(),
+ ImmutableMap.of(
+ RESTCatalogProperties.REFERENCED_BY_QUERY_PARAMETER,
"ns%2Eouter_view")),
+ eq(LoadViewResponse.class),
+ any(),
+ any());
+ }
+
+ @Test
+ public void loadViewWithoutContextHasNoReferencedByParam() {
+ TableIdentifier viewIdent = createView();
+
+ restCatalog.loadView(viewIdent);
+
+ Mockito.verify(adapter)
+ .execute(
+ matches(HTTPMethod.GET, "v1/namespaces/ns/views/test_view",
Map.of(), Map.of()),
+ eq(LoadViewResponse.class),
+ any(),
+ any());
+ }
+
+ @Test
+ public void loadViewThroughViewCatalogBridgeSendsReferencedBy() {
+ TableIdentifier viewIdent = createView();
+
+ // the ViewCatalog returned by asViewCatalog must forward the load
context, otherwise
+ // ViewCatalog's default implementation silently drops the view chain
+ SessionCatalog.SessionContext session =
SessionCatalog.SessionContext.createEmpty();
+ ViewCatalog viewCatalog =
restCatalog.sessionCatalog().asViewCatalog(session);
+
+ viewCatalog.loadView(viewIdent, referencedBy("outer_view"));
+
+ Mockito.verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ "v1/namespaces/ns/views/test_view",
+ Map.of(),
+ ImmutableMap.of(
+ RESTCatalogProperties.REFERENCED_BY_QUERY_PARAMETER,
"ns%2Eouter_view")),
+ eq(LoadViewResponse.class),
+ any(),
+ any());
+ }
+
+ @Test
+ public void referencedByReachesTableFileIOProperties() {
+ // a non-empty table config forces a table-level FileIO; that FileIO's
properties are what
+ // credential providers read the chain back out of
+ Mockito.doAnswer(
+ invocation -> {
+ LoadTableResponse response = (LoadTableResponse)
invocation.callRealMethod();
+ return LoadTableResponse.builder()
+ .withTableMetadata(response.tableMetadata())
+ .addAllConfig(response.config())
+ .addAllConfig(ImmutableMap.of("table-scoped", "config"))
+ .build();
+ })
+ .when(adapter)
+ .execute(
+ matches(HTTPMethod.GET, "v1/namespaces/ns/tables/test_table"),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ Table table = restCatalog.loadTable(TABLE_IDENT,
referencedBy("outer_view"));
+
+ assertThat(table.io().properties())
+ .containsEntry(RESTCatalogProperties.REST_REFERENCED_BY,
"ns%2Eouter_view");
+ }
+
+ @Test
+ public void referencedByLoadStillReusesCatalogFileIO() {
+ // with no table-scoped config from the server there are no per-table
credentials to scope, so
+ // the chain must not force a new FileIO per load
+ Table plain = restCatalog.loadTable(TABLE_IDENT);
+ Table viaView = restCatalog.loadTable(TABLE_IDENT,
referencedBy("outer_view"));
+
+ assertThat(viaView.io()).isSameAs(plain.io());
Review Comment:
This builds the expected value with the same separator it passes in
(String.format("ns%sviewName", namespaceSeparator)), so it holds for any
separator and can only
fail if the function ignores the argument. I added "&", "=", and "?" to this
list and all eight
cases still passed.
That's why `#` is asserted as correct here even though it breaks once the
value is appended to a
query string (see the comment on encodeReferencedBy).
Could one case run the result through HTTPRequest.requestUri() and assert on
getRawQuery()?
That's where the encoding actually has to hold, and it's the only thing that
would catch a
separator that isn't URL-safe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
