laserninja commented on code in PR #10699:
URL: https://github.com/apache/gravitino/pull/10699#discussion_r3047911788
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/CatalogWrapperForTest.java:
##########
Review Comment:
The PR's new 3-arg `registerTable` override in `CatalogWrapperForTest`
duplicates the `shouldGenerateCredential`/`injectCredentialConfig` pattern from
the parent `CatalogWrapperForREST`. This duplication is **necessary** because
the in-memory catalog can't handle `registerTable` natively (unlike
`createTable`, where the test calls `super.createTable(3-arg)` and the parent
handles everything).
However, if the credential injection logic ever changes in
`CatalogWrapperForREST`, this test would silently diverge. Consider adding a
brief comment in the test explaining why the duplication exists, e.g.:
```java
// Cannot delegate to super.registerTable(3-arg) because the in-memory
catalog
// does not support registerTable; must inline credential injection here.
```
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -191,7 +204,7 @@ protected boolean useDifferentClassLoader() {
return false;
}
Review Comment:
These methods go from `private` → `protected`, which makes them part of the
subclass API contract. This is the cleanest option given the constraints but
will need to be maintained as a stable interface going forward. A short JavaDoc
note on their intended use (subclass credential injection in cases where
`super` can't be called) would clarify intent.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergNamespaceOperations.java:
##########
@@ -257,4 +265,63 @@ void testUpdateNamespace() {
dummyEventListener.clearEvent();
verifyUpdateNamespaceSucc(Namespace.of("update_foo3", "a"));
}
+
+ @Test
+ void testRegisterTableWithCredentialVending() {
+ // register without credential vending — no credentials in response
+ verifyRegisterTableSucc("register_cred_foo1",
Namespace.of("register_cred_ns"));
+
+ // register with credential vending but local location — should NOT vend
+ Response response =
+ doRegisterTableWithCredentialVending(
+ "register_cred_foo2", Namespace.of("register_cred_ns2"), "mock");
+ Assertions.assertEquals(Status.OK.getStatusCode(), response.getStatus());
+ LoadTableResponse loadTableResponse =
response.readEntity(LoadTableResponse.class);
+
Assertions.assertTrue(!loadTableResponse.config().containsKey(Credential.CREDENTIAL_TYPE));
+
+ // register with credential vending and S3 location — SHOULD vend
+ String s3Location = "s3://dummy-bucket/register_cred_foo3";
+ response =
+ doRegisterTableWithCredentialVending(
+ "register_cred_foo3", Namespace.of("register_cred_ns3"),
s3Location);
+ Assertions.assertEquals(Status.OK.getStatusCode(), response.getStatus());
+ loadTableResponse = response.readEntity(LoadTableResponse.class);
+ Assertions.assertEquals(
Review Comment:
The S3 vending case only checks for `Credential.CREDENTIAL_TYPE` in the
config.
Asserting on a couple more expected credential properties (e.g., whatever
`DummyCredentialProvider` puts in the config) would make the test more robust
against regressions where the type is present but the actual credential values
are missing.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -239,7 +252,7 @@ private Credential getCredential(
return credential;
}
Review Comment:
same as above
--
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]
