Skip to content

Core: Adjust namespace separator in TestRESTCatalog #14808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nastra merged 5 commits into from
Dec 13, 2025
+164 −16
Merged

Core: Adjust namespace separator in TestRESTCatalog #14808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d379d99
Core: Adjust namespace separator in TestRESTCatalog
gaborkaszab Dec 9, 2025
0e26d23
Apply suggestions from code review
nastra Dec 12, 2025
4ca1294
Apply suggestions from code review
nastra Dec 12, 2025
b8c26de
Update core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
nastra Dec 12, 2025
e82ee75
Apply suggestions from code review
nastra Dec 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
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;
Expand Down Expand Up @@ -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()
Expand Down
136 changes: 121 additions & 15 deletions core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,10 @@
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;

Expand Down Expand Up @@ -944,8 +947,6 @@ public void testTableSnapshotLoading() {
.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,
Expand All @@ -960,7 +961,8 @@ public void testTableSnapshotLoading() {
// 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());
Expand All @@ -969,7 +971,8 @@ public void testTableSnapshotLoading() {
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());
Expand Down Expand Up @@ -1038,8 +1041,6 @@ public void testTableSnapshotLoadingWithDivergedBranches(String formatVersion) {
.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,
Expand All @@ -1054,7 +1055,8 @@ public void testTableSnapshotLoadingWithDivergedBranches(String formatVersion) {
// 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());
Expand All @@ -1064,7 +1066,8 @@ public void testTableSnapshotLoadingWithDivergedBranches(String formatVersion) {
.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());
Expand Down Expand Up @@ -2956,14 +2959,12 @@ public void testNotModified() {
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
Expand Down Expand Up @@ -3311,6 +3312,111 @@ public void testClientDoesNotSendIdempotencyWhenServerNotAdvertising() {
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));
Copy link
Contributor

@nastra nastra Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might want to add a comment that says that the server now always sends back the separator in the config response so that this is clear here

Copy link
Collaborator Author

@gaborkaszab gaborkaszab Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I added an extra check also to verify the override is sent by the adapter. Makes the test more future proof, however, this may be an overkill. Let me know and I'll remove the check


// 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);
Copy link
Contributor

@nastra nastra Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't really need the table creation here

Copy link
Collaborator Author

@gaborkaszab gaborkaszab Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use this so that I can also verify that the paths use the desired namespace separators. I know you've commented that it rather belongs to TestResourcePaths, but in general I think whenever an end-to-end scenario is easy to do, it makes sense to add test coverage there (besides narrower unit testing). In this case it's pretty simple to cover, and add more test coverage not just to ResourcePaths but also to the RESTCatalog code around creating and using the paths. This gives use more complete tests here: the paths and the params together.


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(
Expand Down
41 changes: 41 additions & 0 deletions core/src/test/java/org/apache/iceberg/rest/TestResourcePaths.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,47 @@ public void testNamespaceWithDot(String namespaceSeparator) {
.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");
Expand Down