gaborkaszab commented on code in PR #14808:
URL: https://github.com/apache/iceberg/pull/14808#discussion_r2613719336
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3311,6 +3312,92 @@ public void
testClientDoesNotSendIdempotencyWhenServerNotAdvertising() {
local.dropTable(ident);
}
+ @Test
+ public void testClientWithDefaultNamespaceSeparator() {
+ 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);
+
+ // Do verification with paths using the default namespace separator.
+ ResourcePaths paths =
ResourcePaths.forCatalogProperties(ImmutableMap.of());
+
+ runConfigurableNamespaceSeparatorTest(
+ catalog, adapter, paths,
RESTUtil.NAMESPACE_SEPARATOR_URLENCODED_UTF_8);
+ }
+
+ @Test
+ public void testClientNamespaceSeparatorOverridden() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+
+ 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);
Review Comment:
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.
--
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]
