nastra commented on code in PR #9782:
URL: https://github.com/apache/iceberg/pull/9782#discussion_r1568415371
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -2329,6 +2332,120 @@ public void
multipleDiffsAgainstMultipleTablesLastFails() {
assertThat(schema2.columns()).hasSize(1);
}
+ @Test
+ public void testInvalidRestPageSize() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config)
-> adapter);
+ Assertions.assertThatThrownBy(
+ () ->
+ catalog.initialize(
+ "test", ImmutableMap.of(RESTSessionCatalog.REST_PAGE_SIZE,
"-1")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid value for pageSize, must be a positive integer");
+ }
+
+ @Test
+ public void testPaginationForListNamespaces() {
+
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+ RESTCatalog catalog =
+ new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config)
-> adapter);
+ catalog.initialize("test",
ImmutableMap.of(RESTSessionCatalog.REST_PAGE_SIZE, "10"));
+ int numberOfItems = 100;
+ String namespaceName = "newdb";
+ Map<String, String> queryParams = ImmutableMap.of("pageToken", "",
"pageSize", "10");
+
+ // create several namespaces for listing and verify
+ for (int i = 0; i < numberOfItems; i++) {
+ String nameSpaceName = namespaceName + i;
+ catalog.createNamespace(Namespace.of(nameSpaceName));
+ }
+
+ assertThat(catalog.listNamespaces()).hasSize(numberOfItems);
+
+ Mockito.verify(adapter)
+ .execute(
+ eq(HTTPMethod.GET),
+ eq("v1/config"),
+ any(),
+ any(),
+ eq(ConfigResponse.class),
+ any(),
+ any());
+
+ Mockito.verify(adapter, times(numberOfItems))
+ .execute(
+ eq(HTTPMethod.POST),
+ eq("v1/namespaces"),
+ any(),
+ any(),
+ eq(CreateNamespaceResponse.class),
+ any(),
+ any());
+
+ Mockito.verify(adapter)
Review Comment:
this only tests the initial request with `pageSize = 10` and `pageToken =
""`. Since there's no server-side handling in `RESTCatalogAdapter` /
`CatalogHandlers`, the server always responds with all namespaces, thus
ignoring the pagination and not returning `next-page-token` in the response.
`CatalogHandlers` can be seen as a reference implementation for how the
behavior would be done on the server-side, so I believe we also need to update
`RESTCatalogAdapter` / `CatalogHandlers`. So you would probably need something
similar to
```
diff --git a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
index e4e3c065f..8ce73784a 100644
--- a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
+++ b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
@@ -117,6 +117,21 @@ public class CatalogHandlers {
return ListNamespacesResponse.builder().addAll(results).build();
}
+ public static ListNamespacesResponse listNamespaces(
+ SupportsNamespaces catalog, Namespace parent, String pageToken,
Integer pageSize) {
+ List<Namespace> results;
+ if (parent.isEmpty()) {
+ results = catalog.listNamespaces();
+ } else {
+ results = catalog.listNamespaces(parent);
+ }
+
+ // only return subResults to match pageToken/pageSize
+ subResults = results.subList(from, to)
+
+ return
ListNamespacesResponse.builder().addAll(subResults).nextPageToken(...).build();
+ }
+
```
```
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 7fccc4e97..6c6fa061b 100644
--- a/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
+++ b/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
@@ -298,7 +298,17 @@ public class RESTCatalogAdapter implements RESTClient {
ns = Namespace.empty();
}
- return castResponse(responseType,
CatalogHandlers.listNamespaces(asNamespaceCatalog, ns));
+ String pageToken = PropertyUtil.propertyAsString(vars,
"pageToken", null);
+ Integer pageSize = PropertyUtil.propertyAsNullableInt(vars,
"pageSize");
+
+ if (null != pageSize) {
+ return castResponse(
+ responseType,
+ CatalogHandlers.listNamespaces(asNamespaceCatalog, ns,
pageToken, pageSize));
+ } else {
+ return castResponse(
+ responseType,
CatalogHandlers.listNamespaces(asNamespaceCatalog, ns));
+ }
}
break;
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
