Copilot commented on code in PR #11168:
URL: https://github.com/apache/gravitino/pull/11168#discussion_r3272376928
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergNamespaceOperations.java:
##########
@@ -183,6 +183,12 @@ void testDropNamespace() {
verifyCreateNamespaceSucc(Namespace.of("drop_foo3", "a"));
verifyDropNamespaceFail(404, Namespace.of("drop_foo3", "b"));
verifyDropNamespaceSucc(Namespace.of("drop_foo3", "a"));
+
+ // drop non-empty namespace should return 409 Conflict per Iceberg REST
spec
+ Namespace nonEmptyNs = Namespace.of("drop_nonempty_foo");
+ verifyCreateNamespaceSucc(nonEmptyNs);
+ verifyRegisterTableSucc("drop_nonempty_table", nonEmptyNs);
Review Comment:
This test creates a namespace and a table but doesn’t clean them up. Since
the final assertion intentionally leaves the namespace non-empty, the
namespace/table can persist and make the suite order-dependent if the test
fixture shares state across tests. Consider adding teardown in this test (e.g.,
drop the table, then drop the namespace successfully) or using unique
randomized names plus cleanup in an @AfterEach/@AfterAll hook.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergExceptionMapper.java:
##########
@@ -48,7 +48,7 @@ private void checkExceptionStatus(Exception exception, int
statusCode) {
public void testIcebergExceptionMapper() {
checkExceptionStatus(new IllegalArgumentException(""), 400);
checkExceptionStatus(new ValidationException(""), 400);
- checkExceptionStatus(new NamespaceNotEmptyException(""), 400);
+ checkExceptionStatus(new NamespaceNotEmptyException(""), 409);
Review Comment:
The test hard-codes HTTP status codes (e.g., 409). To improve readability
and reduce magic numbers, prefer using a standard constant such as
`Response.Status.CONFLICT.getStatusCode()` (and similarly for 400, 401, etc.)
if the rest of the test suite follows that pattern.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergExceptionMapper.java:
##########
@@ -59,7 +59,7 @@ public class IcebergExceptionMapper implements
ExceptionMapper<Exception> {
.put(IllegalArgumentException.class, 400)
.put(ValidationException.class, 400)
.put(IllegalNameIdentifierException.class, 400)
- .put(NamespaceNotEmptyException.class, 400)
+ .put(NamespaceNotEmptyException.class, 409)
.put(NotAuthorizedException.class, 401)
Review Comment:
The exception-to-status mapping uses raw numeric literals (e.g., 409). Using
JAX-RS constants like `Response.Status.CONFLICT.getStatusCode()` improves
clarity and reduces the chance of mismatched codes when updating mappings.
--
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]
