You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "gharris1727 (via GitHub)" <gi...@apache.org> on 2023/02/23 01:28:43 UTC
[GitHub] [kafka] gharris1727 opened a new pull request, #13294: KAFKA-5863: Avoid NPE when calls expecting no-content receive content.
gharris1727 opened a new pull request, #13294:
URL: https://github.com/apache/kafka/pull/13294
The RestClient accepts a TypeReference argument defining what kind of response to expect from the HTTP request.
If these request is expected to result in a 204 no-content, they would previously not provide a TypeReference argument (null). If a request which normally resulted in a 204 returned some other 200 code, then the RestClient would experience an NPE when it tries to deserialize the nonempty response.
Instead, we should enforce that the TypeReference is always provided to the RestClient. If we provide a TypeReference<Void> for the calls which do not expect to return content, then if they do receive content, that content will be silently dropped instead of causing an NPE.
Adds new tests for enforcing non-null arguments, and confirms the behavior of a TypeReference<Void> call-site receiving a non-empty response.
### 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
[GitHub] [kafka] github-actions[bot] commented on pull request #13294: KAFKA-5863: Avoid NPE when RestClient calls expecting no-content receive content.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13294:
URL: https://github.com/apache/kafka/pull/13294#issuecomment-1709425485
This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch) <p> If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.
--
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] KAFKA-5863: Avoid NPE when RestClient calls expecting no-content receive content. [kafka]
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #13294:
URL: https://github.com/apache/kafka/pull/13294#issuecomment-1889769357
Hey @yashmayya are you interested in reviewing this? 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] KAFKA-5863: Avoid NPE when RestClient calls expecting no-content receive content. [kafka]
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 merged PR #13294:
URL: https://github.com/apache/kafka/pull/13294
--
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
[GitHub] [kafka] hgeraldino commented on a diff in pull request #13294: KAFKA-5863: Avoid NPE when RestClient calls expecting no-content receive content.
Posted by "hgeraldino (via GitHub)" <gi...@apache.org>.
hgeraldino commented on code in PR #13294:
URL: https://github.com/apache/kafka/pull/13294#discussion_r1139545378
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/RestClientTest.java:
##########
@@ -157,23 +161,55 @@ private void setupHttpClient(int responseCode, String responseJsonString) throws
when(httpClient.newRequest(anyString())).thenReturn(req);
}
+ @Test
+ public void testNullUrl() throws Exception {
+ int statusCode = Response.Status.OK.getStatusCode();
+ setupHttpClient(statusCode, toJsonString(TEST_DTO));
+
+ assertThrows(NullPointerException.class, () -> httpRequest(
+ httpClient, null, "GET", null, null, new TypeReference<Void>() { }, null, null
Review Comment:
Because the exception being thrown is somewhat generic, I'd probably pass valid values for all arguments except the one that is under test, just to make sure that the NPE is being thrown by the proper check
--
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] KAFKA-5863: Avoid NPE when RestClient calls expecting no-content receive content. [kafka]
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #13294:
URL: https://github.com/apache/kafka/pull/13294#issuecomment-1894252929
> I think this makes sense behaviorally, but we should log a warning with the unexpected content IMO. WDYT?
I don't think we should, both because it breaks the TypeReference abstraction and because it's inherently untrusted content that shouldn't be put into the log by default.
If someone is debugging their application and wants to see the REST responses, they can turn on logging for `org.eclipse.jetty.client` and see stuff like this:
```
[2024-01-16 10:02:28,753] DEBUG Response content HttpResponse[HTTP/1.1 201 Created]@1b2c4186
DirectByteBufferR@37cab27d[p=196,l=250,c=16384,r=54]={HTTP/1.1 ...09)\r\n\r\n<<<{"name":"blah","config":{...ks":[],"type":"source"}>>>\x00\x00\x00\x00\x00\x00\x00\x00\x00...\x00\x00\x00\x00\x00\x00\x00} (org.eclipse.jetty.client.HttpReceiver:345)
```
--
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] KAFKA-5863: Avoid NPE when RestClient calls expecting no-content receive content. [kafka]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #13294:
URL: https://github.com/apache/kafka/pull/13294#discussion_r1451742635
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestClient.java:
##########
@@ -67,13 +68,13 @@ HttpClient httpClient(SslContextFactory sslContextFactory) {
/**
* Sends HTTP request to remote REST server
*
- * @param url HTTP connection will be established with this url.
- * @param method HTTP method ("GET", "POST", "PUT", etc.)
+ * @param url HTTP connection will be established with this url, non-null.
+ * @param method HTTP method ("GET", "POST", "PUT", etc.), non-null
* @param headers HTTP headers from REST endpoint
* @param requestBodyData Object to serialize as JSON and send in the request body.
- * @param responseFormat Expected format of the response to the HTTP request.
+ * @param responseFormat Expected format of the response to the HTTP request, non-null.
* @param <T> The type of the deserialized response to the HTTP request.
- * @return The deserialized response to the HTTP request, or null if no data is expected.
+ * @return The deserialized response to the HTTP request, containing null if no data is expected or returned.
Review Comment:
"no data is expected" sounds like an odd description for a return value, could we just remove that bit?
--
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] KAFKA-5863: Avoid NPE when RestClient calls expecting no-content receive content. [kafka]
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #13294:
URL: https://github.com/apache/kafka/pull/13294#issuecomment-1895192159
> Wouldn't any unexpected response here simply be indicative of a bug in the Connect runtime since the REST client is only being used internally to forward requests within a Connect cluster?
While a bug in the remote connect worker could produce unexpected content, I think it's more likely that for one of these calls to get unexpected content, that what is on the other end of the connection isn't actually a connect worker. For example, if there was a stale DNS resolution, and the request went to an unrelated service.
--
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] KAFKA-5863: Avoid NPE when RestClient calls expecting no-content receive content. [kafka]
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #13294:
URL: https://github.com/apache/kafka/pull/13294#issuecomment-1899079016
Test failures appear unrelated, and the connect and mirror tests pass locally for 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-5863: Avoid NPE when RestClient calls expecting no-content receive content. [kafka]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on PR #13294:
URL: https://github.com/apache/kafka/pull/13294#issuecomment-1894930823
> because it's inherently untrusted content that shouldn't be put into the log by default
I'm not sure I follow why this would be inherently untrusted content? Wouldn't any unexpected response here simply be indicative of a bug in the Connect runtime since the REST client is only being used internally to forward requests within a Connect cluster?
--
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