You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/10/26 07:05:46 UTC
[GitHub] [kafka] dajac commented on a change in pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
dajac commented on a change in pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#discussion_r511746730
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -4166,10 +4167,22 @@ public DescribeUserScramCredentialsResult describeUserScramCredentials(List<Stri
Call call = new Call("describeUserScramCredentials", calcDeadlineMs(now, options.timeoutMs()),
new LeastLoadedNodeProvider()) {
@Override
- public DescribeUserScramCredentialsRequest.Builder createRequest(int timeoutMs) {
- return new DescribeUserScramCredentialsRequest.Builder(
- new DescribeUserScramCredentialsRequestData().setUsers(users.stream().map(user ->
- new DescribeUserScramCredentialsRequestData.UserName().setName(user)).collect(Collectors.toList())));
+ public DescribeUserScramCredentialsRequest.Builder createRequest(final int timeoutMs) {
+ final DescribeUserScramCredentialsRequestData requestData = new DescribeUserScramCredentialsRequestData();
+
+ if (users != null && !users.isEmpty()) {
+ final List<UserName> userNames = new ArrayList<>(users.size());
+
+ for (final String user : users) {
+ if (user != null) {
+ userNames.add(new UserName().setName(user));
+ }
+ }
+
+ requestData.setUsers(userNames);
Review comment:
nit: I suggest to continue using the stream api here. It keeps the code smaller.
##########
File path: clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java
##########
@@ -4505,50 +4505,57 @@ public void testDescribeUserScramCredentials() throws Exception {
user0CredentialInfo1.setIterations(user0Iterations1);
final String user1Name = "user1";
- ScramMechanism user1ScramMechanism = ScramMechanism.SCRAM_SHA_256;
- int user1Iterations = 4096;
+ final ScramMechanism user1ScramMechanism = ScramMechanism.SCRAM_SHA_256;
+ final int user1Iterations = 4096;
final CredentialInfo user1CredentialInfo = new CredentialInfo();
user1CredentialInfo.setMechanism(user1ScramMechanism.type());
user1CredentialInfo.setIterations(user1Iterations);
- DescribeUserScramCredentialsResponseData responseData = new DescribeUserScramCredentialsResponseData();
+ final DescribeUserScramCredentialsResponseData responseData = new DescribeUserScramCredentialsResponseData();
responseData.setResults(Arrays.asList(
new DescribeUserScramCredentialsResponseData.DescribeUserScramCredentialsResult()
.setUser(user0Name)
.setCredentialInfos(Arrays.asList(user0CredentialInfo0, user0CredentialInfo1)),
new DescribeUserScramCredentialsResponseData.DescribeUserScramCredentialsResult()
.setUser(user1Name)
- .setCredentialInfos(Arrays.asList(user1CredentialInfo))));
-
- env.kafkaClient().prepareResponse(new DescribeUserScramCredentialsResponse(responseData));
-
- List<String> usersRequestedList = asList(user0Name, user1Name);
- Set<String> usersRequestedSet = usersRequestedList.stream().collect(Collectors.toSet());
- DescribeUserScramCredentialsResult result = env.adminClient().describeUserScramCredentials(usersRequestedList);
- Map<String, UserScramCredentialsDescription> descriptionResults = result.all().get();
- KafkaFuture<UserScramCredentialsDescription> user0DescriptionFuture = result.description(user0Name);
- KafkaFuture<UserScramCredentialsDescription> user1DescriptionFuture = result.description(user1Name);
- Set<String> usersDescribedFromUsersSet = result.users().get().stream().collect(Collectors.toSet());
- assertEquals(usersRequestedSet, usersDescribedFromUsersSet);
- Set<String> usersDescribedFromMapKeySet = descriptionResults.keySet();
- assertEquals(usersRequestedSet, usersDescribedFromMapKeySet);
-
- UserScramCredentialsDescription userScramCredentialsDescription0 = descriptionResults.get(user0Name);
- assertEquals(user0Name, userScramCredentialsDescription0.name());
- assertEquals(2, userScramCredentialsDescription0.credentialInfos().size());
- assertEquals(user0ScramMechanism0, userScramCredentialsDescription0.credentialInfos().get(0).mechanism());
- assertEquals(user0Iterations0, userScramCredentialsDescription0.credentialInfos().get(0).iterations());
- assertEquals(user0ScramMechanism1, userScramCredentialsDescription0.credentialInfos().get(1).mechanism());
- assertEquals(user0Iterations1, userScramCredentialsDescription0.credentialInfos().get(1).iterations());
- assertEquals(userScramCredentialsDescription0, user0DescriptionFuture.get());
-
- UserScramCredentialsDescription userScramCredentialsDescription1 = descriptionResults.get(user1Name);
- assertEquals(user1Name, userScramCredentialsDescription1.name());
- assertEquals(1, userScramCredentialsDescription1.credentialInfos().size());
- assertEquals(user1ScramMechanism, userScramCredentialsDescription1.credentialInfos().get(0).mechanism());
- assertEquals(user1Iterations, userScramCredentialsDescription1.credentialInfos().get(0).iterations());
- assertEquals(userScramCredentialsDescription1, user1DescriptionFuture.get());
+ .setCredentialInfos(singletonList(user1CredentialInfo))));
+ final DescribeUserScramCredentialsResponse response = new DescribeUserScramCredentialsResponse(responseData);
+
+ final Set<String> usersRequestedSet = new HashSet<>();
+ usersRequestedSet.add(user0Name);
+ usersRequestedSet.add(user1Name);
+
+ for (final List<String> users : asList(null, new ArrayList<String>(), asList(user0Name, null, user1Name))) {
Review comment:
Would it make sense to extract these into separate unit tests? `testDescribeUserScramCredentials` could receive `users` as a argument such that we could reuse the code logic.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org