adoroszlai commented on code in PR #9493:
URL: https://github.com/apache/ozone/pull/9493#discussion_r2616157029
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/security/acl/TestRequestContext.java:
##########
@@ -51,7 +51,7 @@ public void testRecursiveAccessFlag() throws IOException {
assertFalse(context.isRecursiveAccessCheck(),
"Wrongly sets recursiveAccessCheck flag value");
- RequestContext.Builder builder = new RequestContext.Builder();
+ RequestContext.Builder builder = RequestContext.newBuilder();
assertFalse(builder.build().isRecursiveAccessCheck(),
"Wrongly sets recursive flag value");
Review Comment:
`testRecursiveAccessFlag()` has also been testing constructor, builder and
`getBuilder`. Now there are only 3 possible combinations:
- default value (without explicit `setRecursiveAccessCheck`)
- `setRecursiveAccessCheck(false)`
- `setRecursiveAccessCheck(true)`
So we can simplify this test case further:
```java
@Test
void testRecursiveAccessFlag() {
RequestContext.Builder builder = RequestContext.newBuilder();
assertFalse(builder.build().isRecursiveAccessCheck(), "default value");
builder.setRecursiveAccessCheck(true);
assertTrue(builder.build().isRecursiveAccessCheck());
builder.setRecursiveAccessCheck(false);
assertFalse(builder.build().isRecursiveAccessCheck());
}
```
`getUserRequestContext()` and `baseBuilder()` can be removed, as well as
unused imports.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/security/acl/TestRequestContext.java:
##########
@@ -60,56 +60,63 @@ public void testRecursiveAccessFlag() throws IOException {
assertTrue(builder.build().isRecursiveAccessCheck(),
"Wrongly sets recursive flag value");
- context = new RequestContext("host", null,
- null, "serviceId",
- IAccessAuthorizer.ACLIdentityType.GROUP,
- IAccessAuthorizer.ACLType.CREATE, "owner");
- assertFalse(context.isRecursiveAccessCheck(),
- "Wrongly sets recursive flag value");
+ context = baseBuilder().build();
+ assertFalse(context.isRecursiveAccessCheck());
- context = new RequestContext("host", null,
- null, "serviceId",
- IAccessAuthorizer.ACLIdentityType.GROUP,
- IAccessAuthorizer.ACLType.CREATE, "owner", false);
- assertFalse(context.isRecursiveAccessCheck(),
- "Wrongly sets recursive flag value");
+ context = baseBuilder().setRecursiveAccessCheck(false).build();
+ assertFalse(context.isRecursiveAccessCheck());
- context = new RequestContext("host", null,
- null, "serviceId",
- IAccessAuthorizer.ACLIdentityType.GROUP,
- IAccessAuthorizer.ACLType.CREATE, "owner", true);
- assertTrue(context.isRecursiveAccessCheck(),
- "Wrongly sets recursive flag value");
+ context = baseBuilder().setRecursiveAccessCheck(true).build();
+ assertTrue(context.isRecursiveAccessCheck());
}
@Test
public void testSessionPolicy() {
- final RequestContext.Builder builder = new RequestContext.Builder();
- RequestContext context = builder.build();
+ RequestContext context = RequestContext.newBuilder()
+ .build();
assertNull(context.getSessionPolicy(), "sessionPolicy should default to
null");
final String policy = "{\"Statement\":[]}";
- context = new RequestContext.Builder()
+ context = RequestContext.newBuilder()
.setSessionPolicy(policy)
.build();
assertEquals(policy, context.getSessionPolicy(), "sessionPolicy should be
set via builder");
- context = new RequestContext(
- "host", null, null, "serviceId",
IAccessAuthorizer.ACLIdentityType.GROUP,
- IAccessAuthorizer.ACLType.CREATE, "owner", true, policy);
+ context = RequestContext.newBuilder()
+ .setHost("host")
+ .setIp(null)
+ .setClientUgi(null)
+ .setServiceId("serviceId")
+ .setAclType(IAccessAuthorizer.ACLIdentityType.GROUP)
+ .setAclRights(IAccessAuthorizer.ACLType.CREATE)
+ .setOwnerName("owner")
+ .setRecursiveAccessCheck(true)
+ .setSessionPolicy(policy)
+ .build();
assertTrue(context.isRecursiveAccessCheck(), "recursiveAccessCheck should
be true");
assertEquals(policy, context.getSessionPolicy(), "sessionPolicy should be
set via constructor");
Review Comment:
Looks like `testSessionPolicy()` has been testing `sessionPolicy` for
various ways of creating `RequestContext`. Now constructor is private and
`getBuilder` no longer exists, so all variants are testing the builder. We can
remove the parts that perform duplicate verifications, and simplify this to:
```java
@Test
void testSessionPolicy() {
RequestContext.Builder builder = RequestContext.newBuilder();
assertNull(builder.build().getSessionPolicy(), "default value");
final String policy = "{\"Statement\":[]}";
builder.setSessionPolicy(policy);
assertEquals(policy, builder.build().getSessionPolicy());
}
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/security/acl/TestVolumeOwner.java:
##########
@@ -245,9 +245,15 @@ public void testKeyOps() throws Exception {
private RequestContext getUserRequestContext(String username,
IAccessAuthorizer.ACLType type, boolean isOwner, String ownerName) {
- return RequestContext.getBuilder(
- UserGroupInformation.createRemoteUser(username), null, null,
- type, ownerName).build();
+ return RequestContext.newBuilder()
+ .setClientUgi(UserGroupInformation.createRemoteUser(username))
+ .setIp(null)
+ .setHost(null)
Review Comment:
nit: `null` is the default value in the builder for these properties, so
this can be removed.
```suggestion
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
