-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support custom request headers from java client and migrations tool #8787
Changes from 4 commits
58b32b2
9176f46
978c28b
3628298
d2f9d42
81e8549
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ | |
package io.confluent.ksql.api.client.impl; | ||
|
||
import io.confluent.ksql.api.client.ClientOptions; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
import java.util.Objects; | ||
|
||
public class ClientOptionsImpl implements ClientOptions { | ||
|
@@ -36,6 +38,7 @@ public class ClientOptionsImpl implements ClientOptions { | |
private String basicAuthPassword; | ||
private int executeQueryMaxResultRows = ClientOptions.DEFAULT_EXECUTE_QUERY_MAX_RESULT_ROWS; | ||
private int http2MultiplexingLimit = ClientOptions.DEFAULT_HTTP2_MULTIPLEXING_LIMIT; | ||
private Map<String, String> requestHeaders; | ||
|
||
/** | ||
* {@code ClientOptions} should be instantiated via {@link ClientOptions#create}, NOT via this | ||
|
@@ -53,7 +56,8 @@ private ClientOptionsImpl( | |
final String trustStorePath, final String trustStorePassword, | ||
final String keyStorePath, final String keyStorePassword, final String keyPassword, | ||
final String keyAlias, final String basicAuthUsername, final String basicAuthPassword, | ||
final int executeQueryMaxResultRows, final int http2MultiplexingLimit) { | ||
final int executeQueryMaxResultRows, final int http2MultiplexingLimit, | ||
final Map<String, String> requestHeaders) { | ||
this.host = Objects.requireNonNull(host); | ||
this.port = port; | ||
this.useTls = useTls; | ||
|
@@ -70,6 +74,7 @@ private ClientOptionsImpl( | |
this.basicAuthPassword = basicAuthPassword; | ||
this.executeQueryMaxResultRows = executeQueryMaxResultRows; | ||
this.http2MultiplexingLimit = http2MultiplexingLimit; | ||
this.requestHeaders = requestHeaders; | ||
} | ||
|
||
@Override | ||
|
@@ -158,6 +163,12 @@ public ClientOptions setHttp2MultiplexingLimit(final int http2MultiplexingLimit) | |
return this; | ||
} | ||
|
||
@Override | ||
public ClientOptions setRequestHeaders(final Map<String, String> requestHeaders) { | ||
this.requestHeaders = requestHeaders; | ||
return this; | ||
} | ||
|
||
@Override | ||
public String getHost() { | ||
return host == null ? "" : host; | ||
|
@@ -238,6 +249,11 @@ public int getHttp2MultiplexingLimit() { | |
return http2MultiplexingLimit; | ||
} | ||
|
||
@Override | ||
public Map<String, String> getRequestHeaders() { | ||
return requestHeaders == null ? null : new HashMap<>(requestHeaders); | ||
} | ||
|
||
@Override | ||
public ClientOptions copy() { | ||
return new ClientOptionsImpl( | ||
|
@@ -247,7 +263,8 @@ public ClientOptions copy() { | |
trustStorePath, trustStorePassword, | ||
keyStorePath, keyStorePassword, keyPassword, keyAlias, | ||
basicAuthUsername, basicAuthPassword, | ||
executeQueryMaxResultRows, http2MultiplexingLimit); | ||
executeQueryMaxResultRows, http2MultiplexingLimit, | ||
requestHeaders); | ||
} | ||
|
||
// CHECKSTYLE_RULES.OFF: CyclomaticComplexity | ||
|
@@ -275,14 +292,16 @@ public boolean equals(final Object o) { | |
&& Objects.equals(keyAlias, that.keyAlias) | ||
&& Objects.equals(basicAuthUsername, that.basicAuthUsername) | ||
&& Objects.equals(basicAuthPassword, that.basicAuthPassword) | ||
&& http2MultiplexingLimit == that.http2MultiplexingLimit; | ||
&& http2MultiplexingLimit == that.http2MultiplexingLimit | ||
&& Objects.equals(requestHeaders, that.requestHeaders); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(host, port, useTls, verifyHost, useAlpn, trustStorePath, | ||
trustStorePassword, keyStorePath, keyStorePassword, keyPassword, keyAlias, | ||
basicAuthUsername, basicAuthPassword, executeQueryMaxResultRows, http2MultiplexingLimit); | ||
basicAuthUsername, basicAuthPassword, executeQueryMaxResultRows, http2MultiplexingLimit, | ||
requestHeaders); | ||
} | ||
|
||
@Override | ||
|
@@ -301,8 +320,9 @@ public String toString() { | |
+ ", keyAlias='" + keyAlias + '\'' | ||
+ ", basicAuthUsername='" + basicAuthUsername + '\'' | ||
+ ", basicAuthPassword='" + basicAuthPassword + '\'' | ||
+ ", executeQueryMaxResultRows=" + executeQueryMaxResultRows + '\'' | ||
+ ", executeQueryMaxResultRows=" + executeQueryMaxResultRows | ||
+ ", http2MultiplexingLimit=" + http2MultiplexingLimit | ||
+ ", requestHeaders='" + requestHeaders + '\'' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Random question, have you seen this output? (Asking in case it'd look goofy or not print things.) Separately, are any of the headers potentially sensitive / something that should not be printed / logged? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The headers are whatever users set (they're completely custom), so in theory they could be sensitive but there's no way for them to specify that as such. Do you think it's better to not log the headers then (or at least the values)? I'm not sure when/if this would actually ever get printed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there's not already some strong guidance or precedent here, we are probably fine. |
||
+ '}'; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to return an empty map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly, though it is a public API so we should decide now. (The javadoc I added says it returns null if it was never set but that can easily be changed.) Would you expect an empty map? I can make that change in order to be consistent with the String fields returning the empty string if not set, rather than null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I tried to look for other similar API instances briefly.
I did this: https://github.com/confluentinc/ksql/blob/master/ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/impl/HttpRequestImpl.java#L29-L30 where empty maps are used rather than null values. I think that's a slight node to changing to empty maps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, updated.