This is an automated email from the ASF dual-hosted git repository.
etudenhoefner 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 bc23a774a3 Core: Adjust namespace separator in TestRESTCatalog (#14808)
bc23a774a3 is described below
commit bc23a774a3cb3b06a63289e445dbf91beb8b5254
Author: gaborkaszab <[email protected]>
AuthorDate: Sat Dec 13 09:09:08 2025 +0100
Core: Adjust namespace separator in TestRESTCatalog (#14808)
Co-authored-by: Eduard Tudenhoefner <[email protected]>
---
.../apache/iceberg/rest/RESTCatalogAdapter.java | 3 +-
.../org/apache/iceberg/rest/TestRESTCatalog.java | 136 ++++++++++++++++++---
.../org/apache/iceberg/rest/TestResourcePaths.java | 41 +++++++
3 files changed, 164 insertions(+), 16 deletions(-)
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 8d59ee0393..524b3e760c 100644
--- a/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
+++ b/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
@@ -56,6 +56,7 @@ import org.apache.iceberg.exceptions.NotAuthorizedException;
import org.apache.iceberg.exceptions.RESTException;
import org.apache.iceberg.exceptions.UnprocessableEntityException;
import org.apache.iceberg.exceptions.ValidationException;
+import
org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.rest.HTTPRequest.HTTPMethod;
@@ -85,7 +86,7 @@ public class RESTCatalogAdapter extends BaseHTTPClient {
@SuppressWarnings("AvoidEscapedUnicodeCharacters")
private static final String NAMESPACE_SEPARATOR_UNICODE = "\u002e";
- private static final String NAMESPACE_SEPARATOR_URLENCODED_UTF_8 = "%2E";
+ @VisibleForTesting static final String NAMESPACE_SEPARATOR_URLENCODED_UTF_8
= "%2E";
private static final Map<Class<? extends Exception>, Integer>
EXCEPTION_ERROR_CODES =
ImmutableMap.<Class<? extends Exception>, Integer>builder()
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 753d8cb247..df4ba3214a 100644
--- a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
+++ b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
@@ -118,7 +118,10 @@ import org.mockito.stubbing.Answer;
public class TestRESTCatalog extends CatalogTests<RESTCatalog> {
private static final ObjectMapper MAPPER = RESTObjectMapper.mapper();
private static final ResourcePaths RESOURCE_PATHS =
- ResourcePaths.forCatalogProperties(Maps.newHashMap());
+ ResourcePaths.forCatalogProperties(
+ ImmutableMap.of(
+ RESTCatalogProperties.NAMESPACE_SEPARATOR,
+ RESTCatalogAdapter.NAMESPACE_SEPARATOR_URLENCODED_UTF_8));
@TempDir public Path temp;
@@ -944,8 +947,6 @@ public class TestRESTCatalog extends
CatalogTests<RESTCatalog> {
.build())
.commit();
- ResourcePaths paths =
ResourcePaths.forCatalogProperties(Maps.newHashMap());
-
Table refsTable = catalog.loadTable(TABLE);
// don't call snapshots() directly as that would cause to load all
snapshots. Instead,
@@ -960,7 +961,8 @@ public class TestRESTCatalog extends
CatalogTests<RESTCatalog> {
// verify that the table was loaded with the refs argument
verify(adapter, times(1))
.execute(
- reqMatcher(HTTPMethod.GET, paths.table(TABLE), Map.of(),
Map.of("snapshots", "refs")),
+ reqMatcher(
+ HTTPMethod.GET, RESOURCE_PATHS.table(TABLE), Map.of(),
Map.of("snapshots", "refs")),
eq(LoadTableResponse.class),
any(),
any());
@@ -969,7 +971,8 @@ public class TestRESTCatalog extends
CatalogTests<RESTCatalog> {
assertThat(refsTable.snapshots()).containsExactlyInAnyOrderElementsOf(table.snapshots());
verify(adapter, times(1))
.execute(
- reqMatcher(HTTPMethod.GET, paths.table(TABLE), Map.of(),
Map.of("snapshots", "all")),
+ reqMatcher(
+ HTTPMethod.GET, RESOURCE_PATHS.table(TABLE), Map.of(),
Map.of("snapshots", "all")),
eq(LoadTableResponse.class),
any(),
any());
@@ -1038,8 +1041,6 @@ public class TestRESTCatalog extends
CatalogTests<RESTCatalog> {
.toBranch(branch)
.commit();
- ResourcePaths paths =
ResourcePaths.forCatalogProperties(Maps.newHashMap());
-
Table refsTable = catalog.loadTable(TABLE);
// don't call snapshots() directly as that would cause to load all
snapshots. Instead,
@@ -1054,7 +1055,8 @@ public class TestRESTCatalog extends
CatalogTests<RESTCatalog> {
// verify that the table was loaded with the refs argument
verify(adapter, times(1))
.execute(
- reqMatcher(HTTPMethod.GET, paths.table(TABLE), Map.of(),
Map.of("snapshots", "refs")),
+ reqMatcher(
+ HTTPMethod.GET, RESOURCE_PATHS.table(TABLE), Map.of(),
Map.of("snapshots", "refs")),
eq(LoadTableResponse.class),
any(),
any());
@@ -1064,7 +1066,8 @@ public class TestRESTCatalog extends
CatalogTests<RESTCatalog> {
.containsExactlyInAnyOrderElementsOf(table.snapshots());
verify(adapter, times(1))
.execute(
- reqMatcher(HTTPMethod.GET, paths.table(TABLE), Map.of(),
Map.of("snapshots", "all")),
+ reqMatcher(
+ HTTPMethod.GET, RESOURCE_PATHS.table(TABLE), Map.of(),
Map.of("snapshots", "all")),
eq(LoadTableResponse.class),
any(),
any());
@@ -2956,14 +2959,12 @@ public class TestRESTCatalog extends
CatalogTests<RESTCatalog> {
any(),
any());
- // RESTCatalogAdapter uses %2E as a namespace separator, and we're
verifying here which
- // server-side path was called
- ResourcePaths paths =
- ResourcePaths.forCatalogProperties(
- ImmutableMap.of(RESTCatalogProperties.NAMESPACE_SEPARATOR, "%2E"));
verify(adapterForRESTServer)
.execute(
- reqMatcher(HTTPMethod.GET, paths.table(metadataTableIdentifier)),
any(), any(), any());
+ reqMatcher(HTTPMethod.GET,
RESOURCE_PATHS.table(metadataTableIdentifier)),
+ any(),
+ any(),
+ any());
}
@Test
@@ -3311,6 +3312,111 @@ public class TestRESTCatalog extends
CatalogTests<RESTCatalog> {
local.dropTable(ident);
}
+ @Test
+ public void nestedNamespaceWithLegacySeparator() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+
+ // Simulate that the server doesn't send the namespace separator in the
overrides
+ Mockito.doAnswer(
+ invocation -> {
+ ConfigResponse configResponse = (ConfigResponse)
invocation.callRealMethod();
+
+ Map<String, String> overridesWithoutNamespaceSeparator =
configResponse.overrides();
+
overridesWithoutNamespaceSeparator.remove(RESTCatalogProperties.NAMESPACE_SEPARATOR);
+
+ return ConfigResponse.builder()
+ .withDefaults(configResponse.defaults())
+ .withOverrides(overridesWithoutNamespaceSeparator)
+ .withEndpoints(configResponse.endpoints())
+
.withIdempotencyKeyLifetime(configResponse.idempotencyKeyLifetime())
+ .build();
+ })
+ .when(adapter)
+ .execute(
+ reqMatcher(HTTPMethod.GET, ResourcePaths.config()),
+ eq(ConfigResponse.class),
+ any(),
+ any());
+
+ RESTCatalog catalog = catalog(adapter);
+
+ ResourcePaths pathsWithLegacySeparator =
ResourcePaths.forCatalogProperties(ImmutableMap.of());
+
+ runConfigurableNamespaceSeparatorTest(
+ catalog, adapter, pathsWithLegacySeparator,
RESTUtil.NAMESPACE_SEPARATOR_URLENCODED_UTF_8);
+ }
+
+ @Test
+ public void nestedNamespaceWithOverriddenSeparator() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+
+ // When initializing the catalog, the adapter always sends an override for
the namespace
+ // separator.
+ Mockito.doAnswer(
+ invocation -> {
+ ConfigResponse configResponse = (ConfigResponse)
invocation.callRealMethod();
+
+ assertThat(configResponse.overrides())
+ .containsEntry(
+ RESTCatalogProperties.NAMESPACE_SEPARATOR,
+ RESTCatalogAdapter.NAMESPACE_SEPARATOR_URLENCODED_UTF_8);
+
+ return configResponse;
+ })
+ .when(adapter)
+ .execute(
+ reqMatcher(HTTPMethod.GET, ResourcePaths.config()),
+ eq(ConfigResponse.class),
+ any(),
+ any());
+
+ RESTCatalog catalog = catalog(adapter);
+
+ runConfigurableNamespaceSeparatorTest(
+ catalog, adapter, RESOURCE_PATHS,
RESTCatalogAdapter.NAMESPACE_SEPARATOR_URLENCODED_UTF_8);
+ }
+
+ private void runConfigurableNamespaceSeparatorTest(
+ RESTCatalog catalog,
+ RESTCatalogAdapter adapter,
+ ResourcePaths expectedPaths,
+ String expectedSeparator) {
+ Namespace nestedNamespace = Namespace.of("ns1", "ns2", "ns3");
+ Namespace parentNamespace = Namespace.of("ns1", "ns2");
+ TableIdentifier table = TableIdentifier.of(nestedNamespace, "tbl");
+
+ catalog.createNamespace(nestedNamespace);
+
+ catalog.createTable(table, SCHEMA);
+
+
assertThat(catalog.listNamespaces(parentNamespace)).containsExactly(nestedNamespace);
+
+ // Verify the namespace separator in the path
+ Mockito.verify(adapter)
+ .execute(
+ reqMatcher(HTTPMethod.POST, expectedPaths.tables(nestedNamespace)),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ // Verify the namespace separator in query parameters
+ Mockito.verify(adapter)
+ .execute(
+ reqMatcher(
+ HTTPMethod.GET,
+ expectedPaths.namespaces(),
+ Map.of(),
+ Map.of(
+ "parent",
+ RESTUtil.namespaceToQueryParam(parentNamespace,
expectedSeparator),
+ "pageToken",
+ ""),
+ null),
+ eq(ListNamespacesResponse.class),
+ any(),
+ any());
+ }
+
private RESTCatalog createCatalogWithIdempAdapter(ConfigResponse cfg,
boolean expectOnMutations) {
RESTCatalogAdapter adapter =
Mockito.spy(
diff --git a/core/src/test/java/org/apache/iceberg/rest/TestResourcePaths.java
b/core/src/test/java/org/apache/iceberg/rest/TestResourcePaths.java
index fcf97e3abc..1f6306eab0 100644
--- a/core/src/test/java/org/apache/iceberg/rest/TestResourcePaths.java
+++ b/core/src/test/java/org/apache/iceberg/rest/TestResourcePaths.java
@@ -110,6 +110,47 @@ public class TestResourcePaths {
.isEqualTo("v1/namespaces/" + namespace);
}
+ @Test
+ public void nestedNamespaceWithLegacySeparator() {
+ Namespace namespace = Namespace.of("first", "second", "third");
+ String legacySeparator = RESTUtil.NAMESPACE_SEPARATOR_URLENCODED_UTF_8;
+ String newSeparator =
RESTCatalogAdapter.NAMESPACE_SEPARATOR_URLENCODED_UTF_8;
+
+ // legacy separator is always used by default, so no need to configure it
+ ResourcePaths pathsWithLegacySeparator =
ResourcePaths.forCatalogProperties(ImmutableMap.of());
+
+ // Encode namespace using legacy separator. No need to provide the
separator to encodeNamespace
+ String legacyEncodedNamespace = RESTUtil.encodeNamespace(namespace);
+ assertThat(pathsWithLegacySeparator.namespace(namespace))
+ .contains(legacyEncodedNamespace)
+ .contains(legacySeparator);
+
+ // Decode the namespace containing legacy separator without providing the
separator
+
assertThat(RESTUtil.decodeNamespace(legacyEncodedNamespace)).isEqualTo(namespace);
+
+ // Decode the namespace containing legacy separator with providing the new
separator
+ assertThat(RESTUtil.decodeNamespace(legacyEncodedNamespace,
newSeparator)).isEqualTo(namespace);
+ }
+
+ @Test
+ public void nestedNamespaceWithNewSeparator() {
+ Namespace namespace = Namespace.of("first", "second", "third");
+ String newSeparator =
RESTCatalogAdapter.NAMESPACE_SEPARATOR_URLENCODED_UTF_8;
+
+ ResourcePaths pathsWithNewSeparator =
+ ResourcePaths.forCatalogProperties(
+ ImmutableMap.of(RESTCatalogProperties.NAMESPACE_SEPARATOR,
newSeparator));
+
+ // Encode namespace using new separator
+ String newEncodedSeparator = RESTUtil.encodeNamespace(namespace,
newSeparator);
+ assertThat(pathsWithNewSeparator.namespace(namespace))
+ .contains(newEncodedSeparator)
+ .contains(newSeparator);
+
+ // Decode the namespace containing new separator with explicitly providing
the separator
+ assertThat(RESTUtil.decodeNamespace(newEncodedSeparator,
newSeparator)).isEqualTo(namespace);
+ }
+
@Test
public void testNamespaceProperties() {
Namespace ns = Namespace.of("ns");
