You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "singhpk234 (via GitHub)" <gi...@apache.org> on 2026/09/08 16:01:31 UTC
[PR] Core: Send the referenced-by view chain to REST catalogs [iceberg]
singhpk234 opened a new pull request, #18015:
URL: https://github.com/apache/iceberg/pull/18015
When a table or view is loaded through a view, the catalog can't currently tell. This sends the chain of referencing views to the REST server as the spec'd referenced-by query parameter, and forwards it to credential providers, so a server can scope authorization and credentials to how the data was reached.
Builds on the LoadContext API from #15895. No spec changes needed — referenced-by is already defined.
Flow: LoadContext.referencedBy([outer, …, inner]) → RESTCatalog → RESTSessionCatalog → referenced-by on the load request → the table's FileIO config → /v1/credentials.
Notes for reviewers
- The whole chain is a single referenced-by key, with entries comma-separated and ordered outermost first, per the spec. A comma inside a view name is encoded as %2C so the delimiter stays unambiguous.
- The value is appended verbatim in HTTPRequest.requestUri instead of going through URIBuilder. Entries are percent-encoded and the delimiter is literal, and addParameter would double-encode % (%1F → %251F). Nothing on the classpath produces this format.
- Entries are encoded like the parent parameter, using PercentCodec. The namespace separator is used as configured.
- The ETag cache is untouched. Servers already owe distinct ETags for responses that differ by query parameter, so this needs no client-side handling.
- Credential providers read the chain from rest-referenced-by, an internal property mirroring rest-scan-plan-id. It's added inside tableFileIO, so the server's config still decides whether the catalog FileIO can be reused.
-----------------
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: [2/N] Send the referenced-by view chain to REST catalogs [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #18015:
URL: https://github.com/apache/iceberg/pull/18015#issuecomment-5703215468
Thanks for the review @RussellSpitzer @huaxingao !
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: [2/N] Send the referenced-by view chain to REST catalogs [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #18015:
URL: https://github.com/apache/iceberg/pull/18015#discussion_r4021013250
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java:
##########
@@ -200,6 +205,93 @@ public void encodePathAsOldAndNewClientDecodeAsNewServer() {
assertThat(RESTUtil.decodePathSegment(encodedOther)).isEqualTo(input);
}
+ @ParameterizedTest
+ @ValueSource(strings = {"%1F", "%2D", "%2E", "#", "_"})
+ public void encodeReferencedBy(String namespaceSeparator) {
+ // the separator is used as configured, joining the namespace levels and the view name
+ assertThat(
+ RESTUtil.encodeReferencedBy(
+ ImmutableList.of(TableIdentifier.of(Namespace.of("ns"), "viewName")),
+ namespaceSeparator))
+ .isEqualTo(String.format("ns%sviewName", namespaceSeparator));
Review Comment:
I think this test passes no matter what the separator is, because it builds the expected value from the same separator it just passed in. So it checks that the function used the separator, but not that the result is safe to put in a URL.
Could we build the real URL with `HTTPRequest.requestUri()` and check the query string? 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: [2/N] Send the referenced-by view chain to REST catalogs [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 merged PR #18015:
URL: https://github.com/apache/iceberg/pull/18015
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: [2/N] Send the referenced-by view chain to REST catalogs [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #18015:
URL: https://github.com/apache/iceberg/pull/18015#discussion_r4022981084
##########
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:
server defines of ETAG means corresponding to request, so this might not be an issue !
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: [2/N] Send the referenced-by view chain to REST catalogs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #18015:
URL: https://github.com/apache/iceberg/pull/18015#discussion_r3992976076
##########
core/src/main/java/org/apache/iceberg/rest/RESTUtil.java:
##########
@@ -436,4 +441,46 @@ 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()}.
+ */
+ public static String encodeReferencedBy(
Review Comment:
Maybe package private for this
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: [2/N] Send the referenced-by view chain to REST catalogs [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: [2/N] Send the referenced-by view chain to REST catalogs [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
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:
In the below `cachedTable`, 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?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: [2/N] Send the referenced-by view chain to REST catalogs [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #18015:
URL: https://github.com/apache/iceberg/pull/18015#discussion_r4020491072
##########
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org