Copilot commented on code in PR #10258:
URL: https://github.com/apache/gravitino/pull/10258#discussion_r2891016045
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java:
##########
@@ -812,4 +812,52 @@ public void testOverridePrivileges() throws IOException {
Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE,
errorResponse2.getCode());
Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResponse2.getType());
}
+
+ @Test
+ public void testGrantRolesToUserWithNullRequest() throws Exception {
+ PermissionOperations operations = new PermissionOperations();
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ when(request.getAttribute(any())).thenReturn(null);
+ FieldUtils.writeField(operations, "httpRequest", request, true);
+
+ Response response = operations.grantRolesToUser("metalake1", "user1",
null);
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
response.getStatus());
+ }
+
+ @Test
+ public void testGrantRolesToGroupWithNullRequest() throws Exception {
+ PermissionOperations operations = new PermissionOperations();
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ when(request.getAttribute(any())).thenReturn(null);
+ FieldUtils.writeField(operations, "httpRequest", request, true);
+
+ Response response = operations.grantRolesToGroup("metalake1", "group1",
null);
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
response.getStatus());
+ }
Review Comment:
Same as above: `grantRolesToGroup(...)` checks
`MetalakeManager.checkMetalakeInUse` before dereferencing `request`, so this
test can fail with `404 NOT_FOUND` if the shared `entityStore` mock is left in
a throwing state by other tests. Please stub `entityStore.get(...)` (in-use
metalake) or reset/restub the mock within this test to avoid order dependence.
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java:
##########
@@ -812,4 +812,52 @@ public void testOverridePrivileges() throws IOException {
Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE,
errorResponse2.getCode());
Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResponse2.getType());
}
+
+ @Test
+ public void testGrantRolesToUserWithNullRequest() throws Exception {
+ PermissionOperations operations = new PermissionOperations();
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ when(request.getAttribute(any())).thenReturn(null);
+ FieldUtils.writeField(operations, "httpRequest", request, true);
+
+ Response response = operations.grantRolesToUser("metalake1", "user1",
null);
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
response.getStatus());
+ }
Review Comment:
The null-request tests for `grantRolesToUser`/`grantRolesToGroup` depend on
`MetalakeManager.checkMetalakeInUse(metalake)` succeeding before the
`request.validate()` NPE happens. Since `entityStore` is a shared static mock
and is reset/stubbed differently across tests in this class, these tests can
become order-dependent (e.g., `entityStore.get(...)` throwing
`NoSuchEntityException` would make the response `404 NOT_FOUND` instead of
`500`). To make the tests deterministic, explicitly stub `entityStore.get(...)`
in each test to return an in-use `BaseMetalake` (or reset + restub the mock in
the test) before calling the operation.
--
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]
