You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "mimaison (via GitHub)" <gi...@apache.org> on 2024/11/21 12:53:24 UTC
[PR] MINOR: Various cleanups in clients [kafka]
mimaison opened a new pull request, #17895:
URL: https://github.com/apache/kafka/pull/17895
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Various cleanups in clients [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #17895:
URL: https://github.com/apache/kafka/pull/17895#issuecomment-2493431578
Done! Thanks
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Various cleanups in clients [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #17895:
URL: https://github.com/apache/kafka/pull/17895#discussion_r1853335982
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpAccessTokenRetriever.java:
##########
@@ -371,22 +369,17 @@ static String formatAuthorizationHeader(String clientId, String clientSecret, bo
return String.format("Basic %s", encoded);
}
- static String formatRequestBody(String scope) throws IOException {
- try {
- StringBuilder requestParameters = new StringBuilder();
- requestParameters.append("grant_type=client_credentials");
-
- if (scope != null && !scope.trim().isEmpty()) {
- scope = scope.trim();
- String encodedScope = URLEncoder.encode(scope, StandardCharsets.UTF_8.name());
- requestParameters.append("&scope=").append(encodedScope);
- }
+ static String formatRequestBody(String scope) {
+ StringBuilder requestParameters = new StringBuilder();
+ requestParameters.append("grant_type=client_credentials");
- return requestParameters.toString();
- } catch (UnsupportedEncodingException e) {
- // The world has gone crazy!
- throw new IOException(String.format("Encoding %s not supported", StandardCharsets.UTF_8.name()));
Review Comment:
Just out of curiosity, all linux distributions aleady support `utf-8`?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Various cleanups in clients [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #17895:
URL: https://github.com/apache/kafka/pull/17895#discussion_r1854154416
##########
clients/src/test/java/org/apache/kafka/common/security/oauthbearer/internals/OAuthBearerSaslServerTest.java:
##########
@@ -52,14 +52,10 @@
public class OAuthBearerSaslServerTest {
private static final String USER = "user";
- private static final Map<String, ?> CONFIGS;
- static {
- String jaasConfigText = "org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginModule Required"
+ private static final String JAAS_CONFIG_TEXT = "org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginModule Required"
+ " unsecuredLoginStringClaim_sub=\"" + USER + "\";";
- Map<String, Object> tmp = new HashMap<>();
- tmp.put(SaslConfigs.SASL_JAAS_CONFIG, new Password(jaasConfigText));
- CONFIGS = Collections.unmodifiableMap(tmp);
- }
+ private static final Map<String, ?> CONFIGS = Map.<String, Object>of(SaslConfigs.SASL_JAAS_CONFIG, new Password(JAAS_CONFIG_TEXT));
Review Comment:
You're right, that's not needed. Removed
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Various cleanups in clients [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #17895:
URL: https://github.com/apache/kafka/pull/17895#discussion_r1854124554
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpAccessTokenRetriever.java:
##########
@@ -371,22 +369,17 @@ static String formatAuthorizationHeader(String clientId, String clientSecret, bo
return String.format("Basic %s", encoded);
}
- static String formatRequestBody(String scope) throws IOException {
- try {
- StringBuilder requestParameters = new StringBuilder();
- requestParameters.append("grant_type=client_credentials");
-
- if (scope != null && !scope.trim().isEmpty()) {
- scope = scope.trim();
- String encodedScope = URLEncoder.encode(scope, StandardCharsets.UTF_8.name());
- requestParameters.append("&scope=").append(encodedScope);
- }
+ static String formatRequestBody(String scope) {
+ StringBuilder requestParameters = new StringBuilder();
+ requestParameters.append("grant_type=client_credentials");
- return requestParameters.toString();
- } catch (UnsupportedEncodingException e) {
- // The world has gone crazy!
- throw new IOException(String.format("Encoding %s not supported", StandardCharsets.UTF_8.name()));
Review Comment:
Thanks for your explanation!
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Various cleanups in clients [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison merged PR #17895:
URL: https://github.com/apache/kafka/pull/17895
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Various cleanups in clients [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17895:
URL: https://github.com/apache/kafka/pull/17895#discussion_r1854141973
##########
clients/src/test/java/org/apache/kafka/common/security/oauthbearer/internals/OAuthBearerSaslServerTest.java:
##########
@@ -52,14 +52,10 @@
public class OAuthBearerSaslServerTest {
private static final String USER = "user";
- private static final Map<String, ?> CONFIGS;
- static {
- String jaasConfigText = "org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginModule Required"
+ private static final String JAAS_CONFIG_TEXT = "org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginModule Required"
+ " unsecuredLoginStringClaim_sub=\"" + USER + "\";";
- Map<String, Object> tmp = new HashMap<>();
- tmp.put(SaslConfigs.SASL_JAAS_CONFIG, new Password(jaasConfigText));
- CONFIGS = Collections.unmodifiableMap(tmp);
- }
+ private static final Map<String, ?> CONFIGS = Map.<String, Object>of(SaslConfigs.SASL_JAAS_CONFIG, new Password(JAAS_CONFIG_TEXT));
Review Comment:
we don't need `<String, Object>`, right?
##########
clients/src/main/java/org/apache/kafka/common/requests/DescribeConfigsResponse.java:
##########
@@ -17,7 +17,6 @@
package org.apache.kafka.common.requests;
-import org.apache.kafka.clients.admin.ConfigEntry;
Review Comment:
this is interesting. we have two almost same `ConfigEntry` ... `org.apache.kafka.clients.admin.ConfigEntry` and `org.apache.kafka.common.requests.DescribeConfigsResponse.ConfigEntry`
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Various cleanups in clients [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #17895:
URL: https://github.com/apache/kafka/pull/17895#issuecomment-2493395437
@mimaison please fix the conflicts, thanks !
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Various cleanups in clients [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #17895:
URL: https://github.com/apache/kafka/pull/17895#discussion_r1853538869
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpAccessTokenRetriever.java:
##########
@@ -371,22 +369,17 @@ static String formatAuthorizationHeader(String clientId, String clientSecret, bo
return String.format("Basic %s", encoded);
}
- static String formatRequestBody(String scope) throws IOException {
- try {
- StringBuilder requestParameters = new StringBuilder();
- requestParameters.append("grant_type=client_credentials");
-
- if (scope != null && !scope.trim().isEmpty()) {
- scope = scope.trim();
- String encodedScope = URLEncoder.encode(scope, StandardCharsets.UTF_8.name());
- requestParameters.append("&scope=").append(encodedScope);
- }
+ static String formatRequestBody(String scope) {
+ StringBuilder requestParameters = new StringBuilder();
+ requestParameters.append("grant_type=client_credentials");
- return requestParameters.toString();
- } catch (UnsupportedEncodingException e) {
- // The world has gone crazy!
- throw new IOException(String.format("Encoding %s not supported", StandardCharsets.UTF_8.name()));
Review Comment:
This exception can be thrown by `URLEncoder.encode(String, String)` when the 2nd argument is not a valid charset. Since Java 10, there's a new method `encode(String, Charset)`. We can now use this new method and pass `StandardCharsets.UTF_8`, which is a `Charset` object directly instead of `StandardCharsets.UTF_8.name` which is a `String`. That way we are sure `encode(String, Charset)` won't throw `UnsupportedEncodingException`.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org