Copilot commented on code in PR #11199:
URL: https://github.com/apache/gravitino/pull/11199#discussion_r3296602184
##########
core/src/main/java/org/apache/gravitino/listener/FilesetEventDispatcher.java:
##########
@@ -77,7 +77,9 @@ public NameIdentifier[] listFilesets(Namespace namespace)
throws NoSuchSchemaExc
eventBus.dispatchEvent(new
ListFilesetPreEvent(PrincipalUtils.getCurrentUserName(), namespace));
try {
NameIdentifier[] nameIdentifiers = dispatcher.listFilesets(namespace);
- eventBus.dispatchEvent(new
ListFilesetEvent(PrincipalUtils.getCurrentUserName(), namespace));
+ eventBus.dispatchEvent(
+ new ListFilesetEvent(
+ PrincipalUtils.getCurrentUserName(), namespace,
nameIdentifiers.length));
return nameIdentifiers;
Review Comment:
This dispatcher now emits result counts for `listFilesets`, but
`listFiles(...)` below still dispatches `ListFilesEvent` without any count, so
list-files audit rows won’t include `{count=N}` even though this PR claims
list-operation success events expose counts. Consider updating `ListFilesEvent`
to implement `ListEvent` (with a preferred constructor accepting a count,
deprecated no-count constructor) and dispatch `fileInfos.length` here.
##########
core/src/main/java/org/apache/gravitino/audit/v2/SimpleAuditLogV2.java:
##########
@@ -101,7 +103,16 @@ public Map<String, String> customInfo() {
@Override
public String toString() {
Map<String, String> info = customInfo();
- String customInfoStr = info != null && !info.isEmpty() ? info.toString() :
"";
+ String customInfoStr;
+ if (event instanceof ListEvent) {
+ int count = ((ListEvent) event).resultCount();
+ Map<String, String> merged = new LinkedHashMap<>();
+ if (count >= 0) merged.put("count", String.valueOf(count));
+ if (info != null) merged.putAll(info);
Review Comment:
In the ListEvent branch, `merged.putAll(info)` can overwrite the computed
`count` entry if `customInfo()` already contains a `count` key, resulting in an
incorrect `{count=...}` in the audit log. Consider preserving the event-derived
count (e.g., ignore/strip an incoming `count` key or apply `putAll` before
adding `count`).
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergViewOperations.java:
##########
@@ -441,4 +445,14 @@ private void verifyRenameViewSucc(Namespace ns, String
source, String dest) {
Response response = doRenameView(ns, source, dest);
Assertions.assertEquals(Response.Status.NO_CONTENT.getStatusCode(),
response.getStatus());
}
+
+ @Test
+ @SuppressWarnings("deprecation")
+ void testIcebergListViewEventDeprecatedConstructorReturnsNegativeCount() {
+ IcebergListViewEvent event =
+ new IcebergListViewEvent(
+ Mockito.mock(IcebergRequestContext.class),
+ org.apache.gravitino.NameIdentifier.of("metalake", "catalog",
"schema"));
+ Assertions.assertEquals(-1, event.resultCount());
Review Comment:
Avoid using the fully-qualified `org.apache.gravitino.NameIdentifier` in the
test body; it’s inconsistent with the rest of the module’s tests which import
`NameIdentifier` (e.g.,
`iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/dispatcher/TestIcebergTableHookDispatcher.java`).
Add an import and use `NameIdentifier.of(...)` directly.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergTableOperations.java:
##########
@@ -1046,4 +1050,14 @@ void
testLoadTableSnapshotsAllReturnsAllSnapshots(Namespace namespace) {
allTableResponse.tableMetadata().snapshots().size(),
"Default load and snapshots=all should return the same number of
snapshots");
}
+
+ @Test
+ @SuppressWarnings("deprecation")
+ void testIcebergListTableEventDeprecatedConstructorReturnsNegativeCount() {
+ IcebergListTableEvent event =
+ new IcebergListTableEvent(
+ Mockito.mock(IcebergRequestContext.class),
+ org.apache.gravitino.NameIdentifier.of("metalake", "catalog",
"schema"));
+ Assertions.assertEquals(-1, event.resultCount());
Review Comment:
Avoid using the fully-qualified `org.apache.gravitino.NameIdentifier` in the
test body; prefer importing `NameIdentifier` and using `NameIdentifier.of(...)`
directly for readability and consistency with other tests in this module.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergNamespaceOperations.java:
##########
@@ -267,18 +278,41 @@ void testListNamespace(String prefix) {
doCreateNamespace(Namespace.of("list_foo3", "a", "z"));
doCreateNamespace(Namespace.of("list_foo3", "a", "y"));
+ dummyEventListener.clearEvent();
verifyListNamespaceSucc(Optional.empty(), Arrays.asList("list_foo1",
"list_foo2", "list_foo3"));
+ Assertions.assertTrue(
+ dummyEventListener.popPreEvent() instanceof
IcebergListNamespacesPreEvent);
+ Event listEvent = dummyEventListener.popPostEvent();
+ Assertions.assertTrue(listEvent instanceof IcebergListNamespacesEvent);
+ Assertions.assertEquals(3, ((IcebergListNamespacesEvent)
listEvent).resultCount());
+
verifyListNamespaceSucc(
Optional.of(Namespace.of("list_foo3")), Arrays.asList("list_foo3.a",
"list_foo3.b"));
verifyListNamespaceSucc(
Optional.of(Namespace.of("list_foo3", "a")),
Arrays.asList("list_foo3.a.y", "list_foo3.a.z"));
+ dummyEventListener.clearEvent();
verifyListNamespaceFail(Optional.of(Namespace.of("list_fooxx")), 404);
+ Assertions.assertTrue(
+ dummyEventListener.popPreEvent() instanceof
IcebergListNamespacesPreEvent);
+ Assertions.assertTrue(
+ dummyEventListener.popPostEvent() instanceof
IcebergListNamespacesFailureEvent);
+
verifyListNamespaceFail(Optional.of(Namespace.of("list_foo3", "c")), 404);
verifyListNamespaceFail(Optional.of(Namespace.of("list_foo3", "a", "x")),
404);
}
+ @Test
+ @SuppressWarnings("deprecation")
+ void
testIcebergListNamespacesEventDeprecatedConstructorReturnsNegativeCount() {
+ IcebergListNamespacesEvent event =
+ new IcebergListNamespacesEvent(
+ Mockito.mock(IcebergRequestContext.class),
+ org.apache.gravitino.NameIdentifier.of("metalake", "catalog"));
+ Assertions.assertEquals(-1, event.resultCount());
Review Comment:
Avoid using the fully-qualified `org.apache.gravitino.NameIdentifier` in the
test body; prefer importing `NameIdentifier` and using `NameIdentifier.of(...)`
directly for readability and consistency with other tests in this module.
--
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]
