nastra commented on code in PR #14808:
URL: https://github.com/apache/iceberg/pull/14808#discussion_r2606734608
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3311,6 +3312,59 @@ public void
testClientDoesNotSendIdempotencyWhenServerNotAdvertising() {
local.dropTable(ident);
}
+ @Test
+ public void testClientWithDefaultNamespaceSeparator() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+
+ 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);
+
+ Namespace ns = Namespace.of("ns1", "ns2");
+ TableIdentifier table = TableIdentifier.of(ns, "tbl");
+
+ catalog.createNamespace(ns);
+
+ catalog.createTable(table, SCHEMA);
+
+ ResourcePaths paths =
ResourcePaths.forCatalogProperties(ImmutableMap.of());
+ assertThat(paths.tables(ns))
Review Comment:
it seems like what you're really trying to test is
```
@Test
public void nestedNamespaceWithLegacyAndNewSeparator() {
Namespace namespace = Namespace.of("first", "second", "third");
String legacySeparator = RESTUtil.NAMESPACE_SEPARATOR_URLENCODED_UTF_8;
String newSeparator = "%2E";
// legacy separator is always used by default, so no need to configure it
ResourcePaths withLegacySeparator =
ResourcePaths.forCatalogProperties(ImmutableMap.of());
ResourcePaths withNewSeparator =
ResourcePaths.forCatalogProperties(
ImmutableMap.of(RESTCatalogProperties.NAMESPACE_SEPARATOR,
newSeparator));
// old client would call encodeNamespace without specifying a separator
when constructing a
// resource path
String legacyEncodedNamespace = RESTUtil.encodeNamespace(namespace);
assertThat(withLegacySeparator.namespace(namespace))
.contains(legacyEncodedNamespace)
.contains(legacySeparator);
// old client would decode without specifying the separator
assertThat(RESTUtil.decodeNamespace(legacyEncodedNamespace)).isEqualTo(namespace);
// new server would decode the namespace using the new separator
assertThat(RESTUtil.decodeNamespace(legacyEncodedNamespace,
newSeparator)).isEqualTo(namespace);
// new client would call encodeNamespace with specifying a separator
when constructing a
// resource path
String newEncodedSeparator = RESTUtil.encodeNamespace(namespace,
newSeparator);
assertThat(withNewSeparator.namespace(namespace))
.contains(newEncodedSeparator)
.contains(newSeparator);
// new server would decode the namespace using the new separator
assertThat(RESTUtil.decodeNamespace(newEncodedSeparator,
newSeparator)).isEqualTo(namespace);
}
```
In that case the test should probably live in `TestResourcePaths`
--
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]
