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/05 15:30:05 UTC
[GitHub] [kafka] Fleshgrinder opened a new pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Fleshgrinder opened a new pull request #9374:
URL: https://github.com/apache/kafka/pull/9374
This bug was detected as part of #9370 and @rondagostino and I decided that this should be fixed right away and not until the other PR eventually gets merged (or not). I am not only properly handling `null` everywhere but also rewrote the function to be more readable (and more efficient, but I think this is not important here). I extended the existing test to go through all possible permutations of the users argument to make sure that we never get an NPE.
----------------------------------------------------------------
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
[GitHub] [kafka] Fleshgrinder edited a comment on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
Fleshgrinder edited a comment on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-704057098
@chia7712 the server gets null right now, and the code you propose is more or less what I had originally. However, at least @rondagostino was directly confused by the ternary and precedence that's why we rewrote it to be easier to understand and easier to debug (now it's possible to set individual breakpoints). Modern languages like Kotlin and Rust have no ternary for a good reason. 😉 Imho it's better as is, despite being longer, especially because length doesn't translate directly to complex.
We'd also still be collecting `null` usernames. The field is nullable in the `UserName` class but collecting them is still a bad idea.
I need to check the server side.
----------------------------------------------------------------
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
[GitHub] [kafka] Fleshgrinder commented on a change in pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on a change in pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#discussion_r511774755
##########
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:
Not really, if we keep all checks in place it will only be a little shorter but definitely less efficient:
```java
final DescribeUserScramCredentialsRequestData requestData = new DescribeUserScramCredentialsRequestData();
if (users != null && !users.isEmpty()) {
requestData.setUsers(users.stream()
.filter(Objects::nonNull)
.map(it -> new UserName().setName(it))
.collect(Collectors.toList())
);
}
return new DescribeUserScramCredentialsRequest.Builder(requestData);
```
----------------------------------------------------------------
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
[GitHub] [kafka] rondagostino commented on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-705531979
----------------------------------------------------------------
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
[GitHub] [kafka] Fleshgrinder commented on a change in pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on a change in pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#discussion_r511810517
##########
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:
I have none either, for me Java is always just clunky no matter which API you use. A normal loop is just faster, that's all I can say, but I doubt that it matters at all.
----------------------------------------------------------------
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
[GitHub] [kafka] dajac commented on a change in pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#discussion_r511800360
##########
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:
Sorry, I was not clear. It would be great if we could define separate unit tests for the different cases that we want to test. In case of failure, we would know directly which one of the cases has failed. This is why I suggested to pass an argument `users` as an argument to `testDescribeUserScramCredentials`.
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-716090509
We can merge this patch so as to avoid making a release with (known) unavailable Public APIs. Honoring the protocol (null handle) on server-side can be a follow-up. Just my two cents.
----------------------------------------------------------------
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
[GitHub] [kafka] Fleshgrinder commented on a change in pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on a change in pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#discussion_r511774755
##########
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:
Not really, if we keep all checks in place it will only be a little shorter but definitely less efficient:
```java
final DescribeUserScramCredentialsRequestData requestData = new DescribeUserScramCredentialsRequestData();
if (users != null && !users.isEmpty()) {
requestData.setUsers(users.stream()
.filter(Objects::nonNull)
.map(it -> new DescribeUserScramCredentialsRequestData.UserName().setName(it))
.collect(Collectors.toList())
);
}
return new DescribeUserScramCredentialsRequest.Builder(requestData);
```
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-704046977
Nice finding!
Is there also a potential NPE in server-side? According to the protocol schema (https://github.com/apache/kafka/blob/trunk/clients/src/main/resources/common/message/DescribeUserScramCredentialsRequest.json#L23), the field ```users``` is nullable. Does server have to handle null as well?(https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/KafkaApis.scala#L3068).
For another, if server is able to handle null, it should be fine to set null to ```users```. Hence, we can simplify client code as following style.
```java
@Override
public DescribeUserScramCredentialsRequest.Builder createRequest(int timeoutMs) {
return new DescribeUserScramCredentialsRequest.Builder(
new DescribeUserScramCredentialsRequestData().setUsers(users == null ? null : users.stream().map(user ->
new DescribeUserScramCredentialsRequestData.UserName().setName(user)).collect(Collectors.toList())));
}
```
please let me know what you think :)
----------------------------------------------------------------
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
[GitHub] [kafka] dajac merged pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
dajac merged pull request #9374:
URL: https://github.com/apache/kafka/pull/9374
----------------------------------------------------------------
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
[GitHub] [kafka] rondagostino commented on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-716065197
This isn’t a regression, so I don’t think it is a blocker. NPE would only occur if someone were to code against the new API. The kafka-configs.sh CLI works fine because it doesn’t exercise the API in a way that generates the NPE.
Would I like to t this fix in? Yes. But is it a blocker? I don’t think so.
----------------------------------------------------------------
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
[GitHub] [kafka] dajac commented on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-716061853
@rondagostino Is this issue a blocker for 2.7? cc @bbejeck
----------------------------------------------------------------
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
[GitHub] [kafka] dajac commented on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-716659252
Test failures not related. Merging to trunk and 2.7.
----------------------------------------------------------------
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
[GitHub] [kafka] rondagostino edited a comment on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
rondagostino edited a comment on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-716065197
This isn’t a regression, so I don’t think it is a blocker. NPE would only occur if someone were to code against the new API. The kafka-configs.sh CLI works fine because it doesn’t exercise the API in a way that generates the NPE.
Would I like to get this fix in? Yes. But is it a blocker? I don’t think so.
----------------------------------------------------------------
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
[GitHub] [kafka] dajac commented on a change in pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#discussion_r511804502
##########
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:
We tend to use the steam api for such small transformations but I don't feel strong about this.
----------------------------------------------------------------
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
[GitHub] [kafka] rondagostino commented on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-705531979
This feature is just being released in 2.7 and will itself be mentioned in the release notes, so a separate ticket for this issue seems unnecessary to me.
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-705340299
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-705340299
this PR is to fix a critical bug. Maybe we should file a jira for it (and so it can be in 2.7.0 release plan)
----------------------------------------------------------------
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
[GitHub] [kafka] Fleshgrinder commented on a change in pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on a change in pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#discussion_r511810517
##########
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:
I have no feelings regarding this either, for me Java is always just clunky no matter which API you use. A normal loop is just faster, that's all I can say, but I doubt that it matters at all.
----------------------------------------------------------------
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
[GitHub] [kafka] Fleshgrinder commented on a change in pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on a change in pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#discussion_r511806251
##########
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:
We can rewrite it to a test factory, this way we can keep it in one function and reuse the initialization logic but will have _n_ different tests being executed so that it is clear which one failed.
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-705551760
> This feature is just being released in 2.7 and will itself be mentioned in the release notes, so a separate ticket for this issue seems unnecessary to me.
I just worry this "minor" patch is not included by 2.7.0.
----------------------------------------------------------------
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
[GitHub] [kafka] dajac commented on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-716559206
FYI - I just opened a PR to fix the issue on the broker side: https://github.com/apache/kafka/pull/9504.
----------------------------------------------------------------
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
[GitHub] [kafka] Fleshgrinder commented on a change in pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on a change in pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#discussion_r511771687
##########
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:
We could but I do not see where we could reuse it, YAGNI?
----------------------------------------------------------------
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
[GitHub] [kafka] Fleshgrinder commented on a change in pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on a change in pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#discussion_r499687848
##########
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 ->
Review comment:
`users.stream()` is the NPE source
----------------------------------------------------------------
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
[GitHub] [kafka] dajac commented on a change in pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [kafka] Fleshgrinder commented on pull request #9374: MINOR: Fix NPE in KafkaAdminClient.describeUserScramCredentials
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on pull request #9374:
URL: https://github.com/apache/kafka/pull/9374#issuecomment-704057098
@chia7712 the server gets null right now, and the code you propose is more or less what I had originally. However, at least @rondagostino was directly confused by the ternary and precedence that's why we rewrote it to be easier to understand and easier to debug (now it's possible to set individual breakpoints). Modern languages like Kotlin and Rust have no ternary for a good reason. 😉 Imho it's better as is, despite being longer, especially because length doesn't translate directly to complex.
----------------------------------------------------------------
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