gaborkaszab commented on code in PR #13979: URL: https://github.com/apache/iceberg/pull/13979#discussion_r2904041098
########## api/src/main/java/org/apache/iceberg/catalog/ContextAwareCatalog.java: ########## @@ -0,0 +1,78 @@ +/* + * 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.catalog; + +import java.util.List; +import java.util.Map; +import org.apache.iceberg.Table; +import org.apache.iceberg.exceptions.NoSuchTableException; +import org.apache.iceberg.exceptions.NoSuchViewException; +import org.apache.iceberg.view.View; + +/** + * Extension interface for Catalog that supports context-aware loading. This enables passing + * additional context (such as view information) when loading tables or views. + */ +public interface ContextAwareCatalog { + + /** Context key for the view identifier chain that references a table. */ + String VIEW_IDENTIFIER_KEY = "view.referenced-by"; + + /** + * Load a table with additional context information. + * + * <p>Common context keys: + * + * <ul> + * <li>{@link #VIEW_IDENTIFIER_KEY}: A {@link List} of {@link TableIdentifier} representing the + * chain of views that reference this table. The list is ordered from outermost view first + * to innermost view last. For a single view reference, the list contains one element. + * </ul> + * + * <p>The default implementation throws {@link UnsupportedOperationException}. Implementations + * that support context-aware table loading should override this method. + * + * @param identifier the table identifier to load + * @param loadingContext additional context information as key-value pairs + * @return the loaded table + * @throws NoSuchTableException if the table does not exist + */ + default Table loadTable(TableIdentifier identifier, Map<String, Object> loadingContext) + throws NoSuchTableException { + throw new UnsupportedOperationException( + "Context-aware table loading is not supported by this catalog"); + } + + /** + * Load a view with additional context information. + * + * <p>The default implementation throws {@link UnsupportedOperationException}. Implementations Review Comment: Same comments as above ########## api/src/main/java/org/apache/iceberg/catalog/ContextAwareCatalog.java: ########## @@ -0,0 +1,78 @@ +/* + * 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.catalog; + +import java.util.List; +import java.util.Map; +import org.apache.iceberg.Table; +import org.apache.iceberg.exceptions.NoSuchTableException; +import org.apache.iceberg.exceptions.NoSuchViewException; +import org.apache.iceberg.view.View; + +/** + * Extension interface for Catalog that supports context-aware loading. This enables passing + * additional context (such as view information) when loading tables or views. + */ +public interface ContextAwareCatalog { + + /** Context key for the view identifier chain that references a table. */ + String VIEW_IDENTIFIER_KEY = "view.referenced-by"; + + /** + * Load a table with additional context information. + * + * <p>Common context keys: + * + * <ul> + * <li>{@link #VIEW_IDENTIFIER_KEY}: A {@link List} of {@link TableIdentifier} representing the + * chain of views that reference this table. The list is ordered from outermost view first + * to innermost view last. For a single view reference, the list contains one element. + * </ul> + * + * <p>The default implementation throws {@link UnsupportedOperationException}. Implementations Review Comment: I don't think that this piece of the comment is needed. New APIs usually throw UOE that implementations has to override. ########## api/src/main/java/org/apache/iceberg/catalog/ContextAwareCatalog.java: ########## @@ -0,0 +1,78 @@ +/* + * 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.catalog; + +import java.util.List; +import java.util.Map; +import org.apache.iceberg.Table; +import org.apache.iceberg.exceptions.NoSuchTableException; +import org.apache.iceberg.exceptions.NoSuchViewException; +import org.apache.iceberg.view.View; + +/** + * Extension interface for Catalog that supports context-aware loading. This enables passing + * additional context (such as view information) when loading tables or views. + */ +public interface ContextAwareCatalog { + + /** Context key for the view identifier chain that references a table. */ + String VIEW_IDENTIFIER_KEY = "view.referenced-by"; + + /** + * Load a table with additional context information. + * + * <p>Common context keys: + * + * <ul> + * <li>{@link #VIEW_IDENTIFIER_KEY}: A {@link List} of {@link TableIdentifier} representing the + * chain of views that reference this table. The list is ordered from outermost view first + * to innermost view last. For a single view reference, the list contains one element. + * </ul> + * + * <p>The default implementation throws {@link UnsupportedOperationException}. Implementations + * that support context-aware table loading should override this method. + * + * @param identifier the table identifier to load + * @param loadingContext additional context information as key-value pairs + * @return the loaded table + * @throws NoSuchTableException if the table does not exist + */ + default Table loadTable(TableIdentifier identifier, Map<String, Object> loadingContext) + throws NoSuchTableException { Review Comment: Probably no need to add this here. I don't see other interfaces having 'throws NoSuchTableException' ########## azure/src/main/java/org/apache/iceberg/azure/adlsv2/VendedAdlsCredentialProvider.java: ########## @@ -173,12 +173,24 @@ private LoadCredentialsResponse fetchCredentials() { return httpClient() .get( credentialsEndpoint, - null != planId ? Map.of("planId", planId) : null, + credentialsQueryParams(), LoadCredentialsResponse.class, Map.of(), ErrorHandlers.defaultErrorHandler()); } + private Map<String, String> credentialsQueryParams() { Review Comment: This seems identical to the one in VendedCredentialsProvider. Maybe have a shared implementation in some common utility class? ########## core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java: ########## @@ -436,21 +437,108 @@ private LoadTableResponse loadInternal( SnapshotMode mode, Map<String, String> headers, Consumer<Map<String, String>> responseHeaders) { + return loadInternal(context, identifier, mode, Map.of(), headers, responseHeaders); + } + + private LoadTableResponse loadInternal( + SessionContext context, + TableIdentifier identifier, + SnapshotMode mode, + Map<String, Object> loadingContext, + Map<String, String> headers, + Consumer<Map<String, String>> responseHeaders) { Endpoint.check(endpoints, Endpoint.V1_LOAD_TABLE); AuthSession contextualSession = authManager.contextualSession(context, catalogAuth); return client .withAuthSession(contextualSession) .get( paths.table(identifier), - snapshotModeToParam(mode), + paramsForLoadTable(mode, loadingContext), LoadTableResponse.class, headers, ErrorHandlers.tableErrorHandler(), responseHeaders); } + // Visible for testing + Map<String, String> paramsForLoadTable(SnapshotMode mode, Map<String, Object> loadingContext) { + return referencedByToQueryParam(snapshotModeToParam(mode), loadingContext); + } + + // Visible for testing + Map<String, String> referencedByToQueryParam( + Map<String, String> params, Map<String, Object> context) { + List<TableIdentifier> viewChain = extractViewChain(context); + if (viewChain.isEmpty()) { + return params; + } + + Map<String, String> queryParams = Maps.newHashMap(params); + String referencedBy = + viewChain.stream() + .map( + ident -> + RESTUtil.encodeNamespace(ident.namespace(), namespaceSeparator) + + namespaceSeparator + + RESTUtil.encodeString(ident.name())) + .collect(Collectors.joining(",")); + + queryParams.put(RESTCatalogProperties.REFERENCED_BY_QUERY_PARAMETER, referencedBy); + + return queryParams; + } + + private List<TableIdentifier> extractViewChain(Map<String, Object> context) { + if (context.isEmpty() || !context.containsKey(ContextAwareCatalog.VIEW_IDENTIFIER_KEY)) { + return List.of(); + } + + Object viewIdentifierObj = context.get(ContextAwareCatalog.VIEW_IDENTIFIER_KEY); + + Preconditions.checkArgument( + viewIdentifierObj instanceof List, + "Invalid view identifier in context, expected List<TableIdentifier>: %s", + viewIdentifierObj); + + List<?> rawList = (List<?>) viewIdentifierObj; + for (Object element : rawList) { + Preconditions.checkArgument( + element instanceof TableIdentifier, + "Invalid element in view identifier list, expected TableIdentifier: %s", + element); + } + + @SuppressWarnings("unchecked") + List<TableIdentifier> viewChain = (List<TableIdentifier>) rawList; + return viewChain; + } + + /** + * Injects the pre-encoded referenced-by value into config properties so that credential providers + * can extract it and pass it as a query parameter to the loadCredentials endpoint. + */ + private Map<String, String> injectReferencedBy( Review Comment: I'm not sure of the purpose of this function. Is it calling `referencedByToQueryParams` and then merge it with some existing configs? Can't we simply do something like ``` Map<String, String> configs = response.Config(); configs.putAll(referencedByToQueryParam(loadingContext)); ``` Note, I deliberately omitted the first param of that function as the previous params shouldn't matter for such a function. ########## core/src/test/java/org/apache/iceberg/rest/TestReferencedByQueryParam.java: ########## @@ -0,0 +1,128 @@ +/* + * 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.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.List; +import java.util.Map; +import org.apache.iceberg.catalog.ContextAwareCatalog; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.junit.jupiter.api.Test; + +public class TestReferencedByQueryParam { + + private final RESTSessionCatalog catalog = new RESTSessionCatalog(config -> null, null); Review Comment: I see you use a dummy RESTSessionCatalog here. Do you think it makes sense to implement very basic functionality in the reference IRC. If we only do input validation there I think it still holds some value. ########## core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java: ########## @@ -436,21 +437,108 @@ private LoadTableResponse loadInternal( SnapshotMode mode, Map<String, String> headers, Consumer<Map<String, String>> responseHeaders) { + return loadInternal(context, identifier, mode, Map.of(), headers, responseHeaders); + } + + private LoadTableResponse loadInternal( + SessionContext context, + TableIdentifier identifier, + SnapshotMode mode, + Map<String, Object> loadingContext, + Map<String, String> headers, + Consumer<Map<String, String>> responseHeaders) { Endpoint.check(endpoints, Endpoint.V1_LOAD_TABLE); AuthSession contextualSession = authManager.contextualSession(context, catalogAuth); return client .withAuthSession(contextualSession) .get( paths.table(identifier), - snapshotModeToParam(mode), + paramsForLoadTable(mode, loadingContext), LoadTableResponse.class, headers, ErrorHandlers.tableErrorHandler(), responseHeaders); } + // Visible for testing + Map<String, String> paramsForLoadTable(SnapshotMode mode, Map<String, Object> loadingContext) { + return referencedByToQueryParam(snapshotModeToParam(mode), loadingContext); + } + + // Visible for testing + Map<String, String> referencedByToQueryParam( + Map<String, String> params, Map<String, Object> context) { + List<TableIdentifier> viewChain = extractViewChain(context); + if (viewChain.isEmpty()) { + return params; + } + + Map<String, String> queryParams = Maps.newHashMap(params); + String referencedBy = + viewChain.stream() + .map( + ident -> + RESTUtil.encodeNamespace(ident.namespace(), namespaceSeparator) + + namespaceSeparator Review Comment: nit: maybe use a Joiner for this? ########## core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java: ########## @@ -436,21 +437,108 @@ private LoadTableResponse loadInternal( SnapshotMode mode, Map<String, String> headers, Consumer<Map<String, String>> responseHeaders) { + return loadInternal(context, identifier, mode, Map.of(), headers, responseHeaders); + } + + private LoadTableResponse loadInternal( + SessionContext context, + TableIdentifier identifier, + SnapshotMode mode, + Map<String, Object> loadingContext, + Map<String, String> headers, + Consumer<Map<String, String>> responseHeaders) { Endpoint.check(endpoints, Endpoint.V1_LOAD_TABLE); AuthSession contextualSession = authManager.contextualSession(context, catalogAuth); return client .withAuthSession(contextualSession) .get( paths.table(identifier), - snapshotModeToParam(mode), + paramsForLoadTable(mode, loadingContext), LoadTableResponse.class, headers, ErrorHandlers.tableErrorHandler(), responseHeaders); } + // Visible for testing + Map<String, String> paramsForLoadTable(SnapshotMode mode, Map<String, Object> loadingContext) { + return referencedByToQueryParam(snapshotModeToParam(mode), loadingContext); Review Comment: Here there is a fix order how we can produce params: First the snapshot mode, then the referenced by stuff. Since they are independent can't we change `referencedByToQueryParam()` not to get an input `params` that it extends but simply return the params needed for referenced by. On the callsite we can merge the params maps returned by each function to produce the result. ########## core/src/test/java/org/apache/iceberg/rest/TestReferencedByQueryParam.java: ########## @@ -0,0 +1,128 @@ +/* + * 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.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.List; +import java.util.Map; +import org.apache.iceberg.catalog.ContextAwareCatalog; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.junit.jupiter.api.Test; + +public class TestReferencedByQueryParam { + + private final RESTSessionCatalog catalog = new RESTSessionCatalog(config -> null, null); + + @Test + public void testSingleViewSimpleNamespace() { Review Comment: nit: I was asked on my reviews not to use the `test` prefix for my tests ########## core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java: ########## @@ -170,7 +171,7 @@ public class RESTSessionCatalog extends BaseViewSessionCatalog private CloseableGroup closeables = null; private Set<Endpoint> endpoints; private Supplier<Map<String, String>> mutationHeaders = Map::of; - private String namespaceSeparator = null; + private String namespaceSeparator = RESTUtil.NAMESPACE_SEPARATOR_URLENCODED_UTF_8; Review Comment: nit: is this change needed? In initialize() we default this to the given value anyway. ########## core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java: ########## @@ -27,6 +27,7 @@ private RESTCatalogProperties() {} public static final String SNAPSHOT_LOADING_MODE = "snapshot-loading-mode"; public static final String SNAPSHOT_LOADING_MODE_DEFAULT = SnapshotMode.ALL.name(); public static final String SNAPSHOTS_QUERY_PARAMETER = "snapshots"; + public static final String REFERENCED_BY_QUERY_PARAMETER = "referenced-by"; Review Comment: nit: I probably wouldn't put this in the same block as the snapshot mode stuff ########## core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java: ########## @@ -436,21 +437,108 @@ private LoadTableResponse loadInternal( SnapshotMode mode, Map<String, String> headers, Consumer<Map<String, String>> responseHeaders) { + return loadInternal(context, identifier, mode, Map.of(), headers, responseHeaders); + } + + private LoadTableResponse loadInternal( + SessionContext context, + TableIdentifier identifier, + SnapshotMode mode, + Map<String, Object> loadingContext, + Map<String, String> headers, + Consumer<Map<String, String>> responseHeaders) { Endpoint.check(endpoints, Endpoint.V1_LOAD_TABLE); AuthSession contextualSession = authManager.contextualSession(context, catalogAuth); return client .withAuthSession(contextualSession) .get( paths.table(identifier), - snapshotModeToParam(mode), + paramsForLoadTable(mode, loadingContext), LoadTableResponse.class, headers, ErrorHandlers.tableErrorHandler(), responseHeaders); } + // Visible for testing Review Comment: nit: `@VisibleForTesting` annotation? Also below ########## aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java: ########## @@ -111,12 +112,24 @@ private LoadCredentialsResponse fetchCredentials() { return httpClient() .get( credentialsEndpoint, - null != planId ? Map.of("planId", planId) : null, + credentialsQueryParams(), LoadCredentialsResponse.class, Map.of(), ErrorHandlers.defaultErrorHandler()); } + private Map<String, String> credentialsQueryParams() { + Map<String, String> queryParams = Maps.newHashMap(); + if (null != planId) { + queryParams.put("planId", planId); Review Comment: Is this "planId" used elsewhere? If yes we can consider introducing a constant in RESTCatalogProperties similarly to REFERENCED_BY_QUERY_PARAMETER ########## core/src/test/java/org/apache/iceberg/rest/TestReferencedByQueryParam.java: ########## @@ -0,0 +1,128 @@ +/* + * 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.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.List; +import java.util.Map; +import org.apache.iceberg.catalog.ContextAwareCatalog; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.junit.jupiter.api.Test; + +public class TestReferencedByQueryParam { + + private final RESTSessionCatalog catalog = new RESTSessionCatalog(config -> null, null); + + @Test + public void testSingleViewSimpleNamespace() { + List<TableIdentifier> chain = + ImmutableList.of(TableIdentifier.of(Namespace.of("ns"), "viewName")); + Map<String, Object> context = ImmutableMap.of(ContextAwareCatalog.VIEW_IDENTIFIER_KEY, chain); + + Map<String, String> result = catalog.referencedByToQueryParam(Map.of(), context); + + assertThat(result) + .containsEntry(RESTCatalogProperties.REFERENCED_BY_QUERY_PARAMETER, "ns%1FviewName"); + } + + @Test + public void testSingleViewNestedNamespace() { + List<TableIdentifier> chain = + ImmutableList.of(TableIdentifier.of(Namespace.of("prod", "analytics"), "quarterly_view")); + Map<String, Object> context = ImmutableMap.of(ContextAwareCatalog.VIEW_IDENTIFIER_KEY, chain); + + Map<String, String> result = catalog.referencedByToQueryParam(Map.of(), context); + + assertThat(result) + .containsEntry( + RESTCatalogProperties.REFERENCED_BY_QUERY_PARAMETER, + "prod%1Fanalytics%1Fquarterly_view"); + } + + @Test + public void testNestedViewChain() { + List<TableIdentifier> chain = + ImmutableList.of( + TableIdentifier.of(Namespace.of("outer_ns"), "outer_view"), + TableIdentifier.of(Namespace.of("inner_ns"), "inner_view")); + Map<String, Object> context = ImmutableMap.of(ContextAwareCatalog.VIEW_IDENTIFIER_KEY, chain); + + Map<String, String> result = catalog.referencedByToQueryParam(Map.of(), context); + + assertThat(result) + .containsEntry( + RESTCatalogProperties.REFERENCED_BY_QUERY_PARAMETER, + "outer_ns%1Fouter_view,inner_ns%1Finner_view"); + } + + @Test + public void testViewNameWithCommaIsEncoded() { + List<TableIdentifier> chain = + ImmutableList.of(TableIdentifier.of(Namespace.of("ns"), "view,name")); + Map<String, Object> context = ImmutableMap.of(ContextAwareCatalog.VIEW_IDENTIFIER_KEY, chain); + + Map<String, String> result = catalog.referencedByToQueryParam(Map.of(), context); + + // Comma in view name should be encoded as %2C + assertThat(result) + .containsEntry(RESTCatalogProperties.REFERENCED_BY_QUERY_PARAMETER, "ns%1Fview%2Cname"); + } + + @Test + public void testInvalidContextValueType() { + Map<String, Object> context = + ImmutableMap.of(ContextAwareCatalog.VIEW_IDENTIFIER_KEY, "not-a-list"); + + assertThatThrownBy(() -> catalog.referencedByToQueryParam(Map.of(), context)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("expected List<TableIdentifier>"); + } + + @Test + public void testExistingParamsArePreserved() { + List<TableIdentifier> chain = ImmutableList.of(TableIdentifier.of(Namespace.of("ns"), "view")); + Map<String, Object> context = ImmutableMap.of(ContextAwareCatalog.VIEW_IDENTIFIER_KEY, chain); + Map<String, String> original = ImmutableMap.of("snapshots", "all"); + + Map<String, String> result = catalog.referencedByToQueryParam(original, context); + + assertThat(result) + .containsEntry("snapshots", "all") + .containsEntry(RESTCatalogProperties.REFERENCED_BY_QUERY_PARAMETER, "ns%1Fview"); + } + + @Test + public void testNamespaceWithSpecialCharsEncoded() { + List<TableIdentifier> chain = + ImmutableList.of(TableIdentifier.of(Namespace.of("ns with spaces"), "view/name")); + Map<String, Object> context = ImmutableMap.of(ContextAwareCatalog.VIEW_IDENTIFIER_KEY, chain); + + Map<String, String> result = catalog.referencedByToQueryParam(Map.of(), context); Review Comment: I quickly scrolled through the test suite and I see that you mainly test the `referencedByToQueryParam()` function instead of going through the full chain of the catalog's loadTable() function calls. I personally would prefer to have a more end to end like test suite for this where the outgoing messages and its params could be verified similarly as it's in other REST catalog related test suites. -- 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]
