Copilot commented on code in PR #11199:
URL: https://github.com/apache/gravitino/pull/11199#discussion_r3301772062
##########
core/src/main/java/org/apache/gravitino/listener/TableEventDispatcher.java:
##########
@@ -83,7 +83,11 @@ public NameIdentifier[] listTables(Namespace namespace)
throws NoSuchSchemaExcep
eventBus.dispatchEvent(new
ListTablePreEvent(PrincipalUtils.getCurrentUserName(), namespace));
try {
NameIdentifier[] nameIdentifiers = dispatcher.listTables(namespace);
- eventBus.dispatchEvent(new
ListTableEvent(PrincipalUtils.getCurrentUserName(), namespace));
+ eventBus.dispatchEvent(
+ new ListTableEvent(
+ PrincipalUtils.getCurrentUserName(),
+ namespace,
+ nameIdentifiers != null ? nameIdentifiers.length : 0));
Review Comment:
The result count uses `... != null ? ... : 0`. If the underlying dispatcher
returns `null` (which is allowed/handled elsewhere), this records
`resultCount=0` and will surface `{count=0}` in audit logs, which is
indistinguishable from a real empty list. Consider using `-1` when the returned
array is `null` so the formatter omits the count (per `ListEvent` contract),
while still using `0` for an actual empty array.
##########
core/src/main/java/org/apache/gravitino/listener/TopicEventDispatcher.java:
##########
@@ -106,7 +106,11 @@ public NameIdentifier[] listTopics(Namespace namespace)
throws NoSuchTopicExcept
eventBus.dispatchEvent(new
ListTopicPreEvent(PrincipalUtils.getCurrentUserName(), namespace));
try {
NameIdentifier[] nameIdentifiers = dispatcher.listTopics(namespace);
- eventBus.dispatchEvent(new
ListTopicEvent(PrincipalUtils.getCurrentUserName(), namespace));
+ eventBus.dispatchEvent(
+ new ListTopicEvent(
+ PrincipalUtils.getCurrentUserName(),
+ namespace,
+ nameIdentifiers != null ? nameIdentifiers.length : 0));
Review Comment:
`nameIdentifiers != null ? nameIdentifiers.length : 0` treats a `null`
return from `listTopics` as an empty result and will surface `{count=0}` in
audit logs. If `null` is used to indicate "not captured" in some
implementations, consider using `-1` for the count when the array is `null` so
the audit formatter can omit it.
##########
core/src/main/java/org/apache/gravitino/listener/TagEventDispatcher.java:
##########
@@ -81,7 +81,11 @@ public String[] listTags(String metalake) {
eventBus.dispatchEvent(new
ListTagsPreEvent(PrincipalUtils.getCurrentUserName(), metalake));
try {
String[] tagNames = dispatcher.listTags(metalake);
- eventBus.dispatchEvent(new
ListTagsEvent(PrincipalUtils.getCurrentUserName(), metalake));
+ eventBus.dispatchEvent(
+ new ListTagsEvent(
+ PrincipalUtils.getCurrentUserName(),
+ metalake,
+ tagNames != null ? tagNames.length : 0));
return tagNames;
Review Comment:
Several list operations in this dispatcher use `... != null ? ... : 0` for
the emitted count (e.g., `listTags`, `listTagsInfo`,
`listMetadataObjectsForTag`, etc.). If the underlying dispatcher returns
`null`, this will log `{count=0}` and incorrectly imply an empty list. Consider
using `-1` when the returned array is `null` so `SimpleAuditLogV2` omits the
count, and keep `0` for an actual empty array.
##########
core/src/main/java/org/apache/gravitino/listener/JobEventDispatcher.java:
##########
@@ -78,7 +78,10 @@ public List<JobTemplateEntity> listJobTemplates(String
metalake) {
try {
List<JobTemplateEntity> jobTemplates =
jobOperationDispatcher.listJobTemplates(metalake);
eventBus.dispatchEvent(
- new ListJobTemplatesEvent(PrincipalUtils.getCurrentUserName(),
metalake));
+ new ListJobTemplatesEvent(
+ PrincipalUtils.getCurrentUserName(),
+ metalake,
+ jobTemplates != null ? jobTemplates.size() : 0));
Review Comment:
`listJobTemplates` and `listJobs` use `... != null ? ... : 0` for count. If
the underlying dispatcher returns `null`, audit output will include `{count=0}`
even though the list wasn't captured. Consider using `-1` when the returned
`List` is `null` so `SimpleAuditLogV2` omits the count field.
##########
core/src/main/java/org/apache/gravitino/listener/StatisticEventDispatcher.java:
##########
@@ -73,7 +73,8 @@ public List<Statistic> listStatistics(String metalake,
MetadataObject metadataOb
try {
List<Statistic> statistics = dispatcher.listStatistics(metalake,
metadataObject);
- eventBus.dispatchEvent(new ListStatisticsEvent(user, identifier));
+ eventBus.dispatchEvent(
+ new ListStatisticsEvent(user, identifier, statistics != null ?
statistics.size() : 0));
Review Comment:
Both `listStatistics` and `listPartitionStatistics` use `... != null ? ... :
0` for result counts. If an implementation returns `null` instead of an empty
list, this will emit `{count=0}` and misrepresent "unknown/not captured" as an
empty list. Consider using `-1` when the returned `List` is `null` so the
formatter omits the count.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergViewEventDispatcher.java:
##########
@@ -174,15 +175,18 @@ public ListTablesResponse listView(IcebergRequestContext
context, Namespace name
NameIdentifier gravitinoNameIdentifier =
IcebergRESTUtils.getGravitinoNameIdentifier(metalakeName,
context.catalogName(), namespace);
eventBus.dispatchEvent(new IcebergListViewPreEvent(context,
gravitinoNameIdentifier));
- ListTablesResponse listViewsResponse;
try {
- listViewsResponse = icebergViewOperationDispatcher.listView(context,
namespace);
+ ListTablesResponse listViewsResponse =
+ icebergViewOperationDispatcher.listView(context, namespace);
+ List<TableIdentifier> identifiers = listViewsResponse.identifiers();
+ eventBus.dispatchEvent(
+ new IcebergListViewEvent(
+ context, gravitinoNameIdentifier, identifiers != null ?
identifiers.size() : 0));
+ return listViewsResponse;
Review Comment:
If `listViewsResponse.identifiers()` is `null`, emitting `resultCount=0`
will log `{count=0}` and can be misread as an empty list rather than "not
captured". Consider using `-1` when identifiers is `null` so the audit
formatter omits the count field.
##########
core/src/main/java/org/apache/gravitino/listener/ViewEventDispatcher.java:
##########
@@ -74,7 +74,11 @@ public NameIdentifier[] listViews(Namespace namespace)
throws NoSuchSchemaExcept
eventBus.dispatchEvent(new
ListViewPreEvent(PrincipalUtils.getCurrentUserName(), namespace));
try {
NameIdentifier[] identifiers = dispatcher.listViews(namespace);
- eventBus.dispatchEvent(new
ListViewEvent(PrincipalUtils.getCurrentUserName(), namespace));
+ eventBus.dispatchEvent(
+ new ListViewEvent(
+ PrincipalUtils.getCurrentUserName(),
+ namespace,
+ identifiers != null ? identifiers.length : 0));
Review Comment:
`identifiers != null ? identifiers.length : 0` will log a `{count=0}` when
the underlying `listViews` returns `null`, which can misrepresent "unknown/not
captured" as an empty list. To align with `ListEvent` semantics (`-1` means not
captured), consider passing `-1` when `identifiers` is `null`.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergNamespaceEventDispatcher.java:
##########
@@ -191,16 +192,18 @@ public ListNamespacesResponse listNamespaces(
metalakeName, context.catalogName(), parentNamespace);
eventBus.dispatchEvent(new IcebergListNamespacesPreEvent(context,
nameIdentifier));
- ListNamespacesResponse listResponse;
try {
- listResponse = operationDispatcher.listNamespaces(context,
parentNamespace);
+ ListNamespacesResponse listResponse =
+ operationDispatcher.listNamespaces(context, parentNamespace);
+ List<Namespace> namespaces = listResponse.namespaces();
+ eventBus.dispatchEvent(
+ new IcebergListNamespacesEvent(
+ context, nameIdentifier, namespaces != null ? namespaces.size()
: 0));
+ return listResponse;
Review Comment:
If `listResponse.namespaces()` is `null`, this records `resultCount=0` and
will surface `{count=0}` in audit logs, conflating "unknown" with a genuine
empty list. Consider using `-1` when the returned list is `null` so the count
is omitted.
##########
core/src/main/java/org/apache/gravitino/listener/MetalakeEventDispatcher.java:
##########
@@ -79,7 +79,9 @@ public Metalake[] listMetalakes() {
eventBus.dispatchEvent(new
ListMetalakePreEvent(PrincipalUtils.getCurrentUserName()));
try {
Metalake[] metalakes = dispatcher.listMetalakes();
- eventBus.dispatchEvent(new
ListMetalakeEvent(PrincipalUtils.getCurrentUserName()));
+ eventBus.dispatchEvent(
+ new ListMetalakeEvent(
+ PrincipalUtils.getCurrentUserName(), metalakes != null ?
metalakes.length : 0));
Review Comment:
`metalakes != null ? metalakes.length : 0` treats a `null` return from
`listMetalakes()` as an empty list and will emit `{count=0}` in audit logs.
Consider passing `-1` when `metalakes` is `null` so the count is omitted and
doesn't get confused with a genuine empty response.
##########
core/src/main/java/org/apache/gravitino/listener/SchemaEventDispatcher.java:
##########
@@ -75,7 +75,11 @@ public NameIdentifier[] listSchemas(Namespace namespace)
throws NoSuchCatalogExc
eventBus.dispatchEvent(new
ListSchemaPreEvent(PrincipalUtils.getCurrentUserName(), namespace));
try {
NameIdentifier[] nameIdentifiers = dispatcher.listSchemas(namespace);
- eventBus.dispatchEvent(new
ListSchemaEvent(PrincipalUtils.getCurrentUserName(), namespace));
+ eventBus.dispatchEvent(
+ new ListSchemaEvent(
+ PrincipalUtils.getCurrentUserName(),
+ namespace,
+ nameIdentifiers != null ? nameIdentifiers.length : 0));
Review Comment:
`nameIdentifiers != null ? nameIdentifiers.length : 0` will emit `{count=0}`
when `listSchemas` returns `null`, which can conflate "unknown/not captured"
with an actual empty list. Consider using `-1` when the result is `null` (and
`0` only when the result is a non-null empty array) to match the `ListEvent`
contract.
##########
core/src/main/java/org/apache/gravitino/listener/CatalogEventDispatcher.java:
##########
@@ -82,7 +82,11 @@ public NameIdentifier[] listCatalogs(Namespace namespace)
throws NoSuchMetalakeE
eventBus.dispatchEvent(new
ListCatalogPreEvent(PrincipalUtils.getCurrentUserName(), namespace));
try {
NameIdentifier[] nameIdentifiers = dispatcher.listCatalogs(namespace);
- eventBus.dispatchEvent(new
ListCatalogEvent(PrincipalUtils.getCurrentUserName(), namespace));
+ eventBus.dispatchEvent(
+ new ListCatalogEvent(
+ PrincipalUtils.getCurrentUserName(),
+ namespace,
+ nameIdentifiers != null ? nameIdentifiers.length : 0));
return nameIdentifiers;
Review Comment:
Both `listCatalogs` and `listCatalogsInfo` use `... != null ? ... : 0` for
the count. If the underlying dispatcher returns `null`, this will surface
`{count=0}` in audit logs, which can be inaccurate. Consider using `-1` when
the result array is `null` so the formatter omits the count, and reserve `0`
for a real empty result.
##########
core/src/main/java/org/apache/gravitino/listener/PolicyEventDispatcher.java:
##########
@@ -86,7 +86,11 @@ public String[] listPolicies(String metalake) {
eventBus.dispatchEvent(new
ListPoliciesPreEvent(PrincipalUtils.getCurrentUserName(), metalake));
try {
String[] policyNames = dispatcher.listPolicies(metalake);
- eventBus.dispatchEvent(new
ListPoliciesEvent(PrincipalUtils.getCurrentUserName(), metalake));
+ eventBus.dispatchEvent(
+ new ListPoliciesEvent(
+ PrincipalUtils.getCurrentUserName(),
+ metalake,
+ policyNames != null ? policyNames.length : 0));
return policyNames;
Review Comment:
`listPolicies`/`listPolicyInfos` use `... != null ? ... : 0` for the emitted
count. If the underlying dispatcher returns `null`, this will surface
`{count=0}` (empty list) in audit logs even though the result wasn't captured.
Consider using `-1` when the returned array is `null` to preserve the "unknown"
meaning.
##########
core/src/main/java/org/apache/gravitino/listener/ModelEventDispatcher.java:
##########
@@ -193,7 +193,8 @@ public NameIdentifier[] listModels(Namespace namespace)
throws NoSuchSchemaExcep
eventBus.dispatchEvent(new ListModelPreEvent(user, namespace));
try {
NameIdentifier[] models = dispatcher.listModels(namespace);
- eventBus.dispatchEvent(new ListModelEvent(user, namespace));
+ eventBus.dispatchEvent(
+ new ListModelEvent(user, namespace, models != null ? models.length :
0));
Review Comment:
`models != null ? models.length : 0` will emit `{count=0}` when `listModels`
returns `null`, conflating "unknown" with an empty list. Consider using `-1`
when `models` is `null` to align with the `ListEvent` contract used by the
audit formatter.
##########
core/src/main/java/org/apache/gravitino/listener/AccessControlEventDispatcher.java:
##########
@@ -188,7 +188,8 @@ public User[] listUsers(String metalake) throws
NoSuchMetalakeException {
eventBus.dispatchEvent(new ListUsersPreEvent(initiator, metalake));
try {
User[] users = dispatcher.listUsers(metalake);
- eventBus.dispatchEvent(new ListUsersEvent(initiator, metalake));
+ eventBus.dispatchEvent(
+ new ListUsersEvent(initiator, metalake, users != null ? users.length
: 0));
Review Comment:
Multiple list methods (`listUsers`, `listUserNames`, `listGroups`,
`listGroupNames`, `listRoleNames`, etc.) use `... != null ? ... : 0` for the
emitted count. If the underlying dispatcher returns `null`, this logs
`{count=0}` (empty list) which may be incorrect. Consider using `-1` when the
returned array is `null` so `SimpleAuditLogV2` omits the count field,
preserving the distinction between empty vs not captured.
##########
core/src/main/java/org/apache/gravitino/listener/PartitionEventDispatcher.java:
##########
@@ -132,7 +132,11 @@ public Partition[] listPartitions(NameIdentifier ident) {
eventBus.dispatchEvent(new
ListPartitionPreEvent(PrincipalUtils.getCurrentUserName(), ident));
try {
Partition[] listPartitions = dispatcher.listPartitions(ident);
- eventBus.dispatchEvent(new
ListPartitionEvent(PrincipalUtils.getCurrentUserName(), ident));
+ eventBus.dispatchEvent(
+ new ListPartitionEvent(
+ PrincipalUtils.getCurrentUserName(),
+ ident,
+ listPartitions != null ? listPartitions.length : 0));
Review Comment:
Both `listPartitions` and `listPartitionNames` use `... != null ? ... : 0`
for the emitted count. If the underlying dispatcher returns `null`, this logs
`{count=0}` and can be misread as an empty result. Consider using `-1` when the
returned array is `null` so the audit formatter can omit the count.
##########
core/src/main/java/org/apache/gravitino/listener/FunctionEventDispatcher.java:
##########
@@ -77,7 +77,9 @@ public NameIdentifier[] listFunctions(Namespace namespace)
throws NoSuchSchemaEx
eventBus.dispatchEvent(new ListFunctionPreEvent(user, namespace));
try {
NameIdentifier[] nameIdentifiers = dispatcher.listFunctions(namespace);
- eventBus.dispatchEvent(new ListFunctionEvent(user, namespace));
+ eventBus.dispatchEvent(
+ new ListFunctionEvent(
+ user, namespace, nameIdentifiers != null ?
nameIdentifiers.length : 0));
Review Comment:
`nameIdentifiers != null ? nameIdentifiers.length : 0` treats a `null`
return from `listFunctions` as an empty list and will log `{count=0}`. Consider
passing `-1` when the result is `null` so the audit formatter omits the count,
and keep `0` for a non-null empty array.
##########
core/src/main/java/org/apache/gravitino/listener/FilesetEventDispatcher.java:
##########
@@ -77,7 +77,11 @@ 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 != null ? nameIdentifiers.length : 0));
Review Comment:
`nameIdentifiers != null ? nameIdentifiers.length : 0` will surface
`{count=0}` when `listFilesets` returns `null`, which can be misinterpreted as
an empty list rather than "not captured". Consider using `-1` when the result
array is `null` so the audit formatter omits the count.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableEventDispatcher.java:
##########
@@ -191,15 +192,18 @@ public ListTablesResponse listTable(IcebergRequestContext
context, Namespace nam
NameIdentifier gravitinoNameIdentifier =
IcebergRESTUtils.getGravitinoNameIdentifier(metalakeName,
context.catalogName(), namespace);
eventBus.dispatchEvent(new IcebergListTablePreEvent(context,
gravitinoNameIdentifier));
- ListTablesResponse listTablesResponse;
try {
- listTablesResponse = icebergTableOperationDispatcher.listTable(context,
namespace);
+ ListTablesResponse listTablesResponse =
+ icebergTableOperationDispatcher.listTable(context, namespace);
+ List<TableIdentifier> identifiers = listTablesResponse.identifiers();
+ eventBus.dispatchEvent(
+ new IcebergListTableEvent(
+ context, gravitinoNameIdentifier, identifiers != null ?
identifiers.size() : 0));
+ return listTablesResponse;
Review Comment:
If `listTablesResponse.identifiers()` is `null`, this code records
`resultCount=0` and emits `{count=0}` in audit logs, which is indistinguishable
from a real empty list. Consider passing `-1` when identifiers is `null` (so
`SimpleAuditLogV2` omits the count) and `0` only when the list is non-null but
empty.
--
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]
