Copilot commented on code in PR #13833:
URL: https://github.com/apache/gravitino/pull/13833#discussion_r4268327732
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java:
##########
@@ -96,6 +96,7 @@ public class MetadataAuthzHelper {
private static final String TABLE_PARENT_SCOPES = "METALAKE, CATALOG,
SCHEMA";
private static final String MODEL_PARENT_SCOPES = "METALAKE, CATALOG,
SCHEMA";
+ private static final String SCHEMA_OBJECT_PARENT_SCOPES = "METALAKE,
CATALOG, SCHEMA";
Review Comment:
`SCHEMA_OBJECT_PARENT_SCOPES` duplicates the exact value of
`TABLE_PARENT_SCOPES` and `MODEL_PARENT_SCOPES`, increasing the risk of
accidental divergence if one is updated later. Consider consolidating to a
single shared constant (e.g., one generic
`METALAKE_CATALOG_SCHEMA_PARENT_SCOPES`) and reusing it for
table/model/schema-object entries.
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java:
##########
@@ -775,6 +1123,27 @@ private GravitinoAuthorizer mockParentGrantAuthorizer(
return authorizer;
}
+ /**
+ * Runs {@code body} with the entity cache enabled and the store and schema
dispatcher that the
+ * list preload uses replaced by the given mocks.
+ */
+ private void withPreloadEnv(EntityStore store, SchemaDispatcher
schemaDispatcher, Runnable body) {
+ when(schemaDispatcher.schemaExists(any())).thenReturn(true);
+ when(gravitinoEnv.entityStore()).thenReturn(store);
+ when(gravitinoEnv.internalSchemaDispatcher()).thenReturn(schemaDispatcher);
+ when(gravitinoEnv.internalAccessControlDispatcher())
+ .thenReturn(mock(AccessControlDispatcher.class));
+ when(gravitinoEnv.cacheEnabled()).thenReturn(true);
+ try {
+ body.run();
+ } finally {
+ when(gravitinoEnv.cacheEnabled()).thenReturn(false);
+ when(gravitinoEnv.entityStore()).thenReturn(null);
+ when(gravitinoEnv.internalSchemaDispatcher()).thenReturn(null);
+ when(gravitinoEnv.internalAccessControlDispatcher()).thenReturn(null);
+ }
+ }
Review Comment:
`withPreloadEnv` mutates stubbings on the shared `gravitinoEnv` mock and
then “restores” by forcing `false`/`null`. This can introduce cross-test order
dependency if `gravitinoEnv` is expected to have different defaults or is
re-stubbed by other helpers (the teardown here will override that
unconditionally). Prefer restoring by resetting the mock and re-applying the
test class’s baseline stubbing (e.g., `Mockito.reset(gravitinoEnv)` + calling
the existing setup helper), or by capturing/restoring prior stubbings rather
than hard-coding `false`/`null`.
--
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]
