You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "eaugene (via GitHub)" <gi...@apache.org> on 2024/01/02 06:51:46 UTC
[PR] [minor] Handle NPE in controller SQL Resource [pinot]
eaugene opened a new pull request, #12211:
URL: https://github.com/apache/pinot/pull/12211
This is a minor change to handle NPE in controller SQL Resource
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [minor] Handle NPE in controller SQL Resource [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12211:
URL: https://github.com/apache/pinot/pull/12211#issuecomment-1873691857
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12211?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`5b4e19a`)](https://app.codecov.io/gh/apache/pinot/commit/5b4e19aa4e3073ce9030d2283fb80bbaf177df21?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.51% compared to head [(`ecb3f9b`)](https://app.codecov.io/gh/apache/pinot/pull/12211?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 46.46%.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12211 +/- ##
=============================================
- Coverage 61.51% 46.46% -15.06%
+ Complexity 1153 946 -207
=============================================
Files 2415 1809 -606
Lines 131157 95158 -35999
Branches 20245 15352 -4893
=============================================
- Hits 80683 44213 -36470
- Misses 44575 47799 +3224
+ Partials 5899 3146 -2753
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12211/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12211/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12211/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12211/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12211/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12211/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12211/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.46% <ø> (-14.94%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12211/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.44% <ø> (-15.06%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12211/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.44% <ø> (-14.94%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12211/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.46% <ø> (-15.06%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12211/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.46% <ø> (-15.05%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12211/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.46% <ø> (-0.14%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12211/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12211?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [minor] Handle NPE in controller SQL Resource [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #12211:
URL: https://github.com/apache/pinot/pull/12211#discussion_r1442365260
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -108,6 +108,9 @@ public class PinotQueryResource {
public String handlePostSql(String requestJsonStr, @Context HttpHeaders httpHeaders) {
try {
JsonNode requestJson = JsonUtils.stringToJsonNode(requestJsonStr);
+ if (!requestJson.has("sql")) {
+ throw new IllegalStateException("JSON Payload is missing the query string field 'sql'");
Review Comment:
should not throw IllegalStateException --> this will make the return code 500 INTERNAL_ERROR
instead we should throw a ProcessingException with a client-request error code
e.g.
```
QueryException.getException(JSON_PARSING_ERROR_CODE, "...")
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [minor] Handle NPE in controller SQL Resource [pinot]
Posted by "eaugene (via GitHub)" <gi...@apache.org>.
eaugene commented on code in PR #12211:
URL: https://github.com/apache/pinot/pull/12211#discussion_r1445900715
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -108,6 +108,9 @@ public class PinotQueryResource {
public String handlePostSql(String requestJsonStr, @Context HttpHeaders httpHeaders) {
try {
JsonNode requestJson = JsonUtils.stringToJsonNode(requestJsonStr);
+ if (!requestJson.has("sql")) {
+ throw new IllegalStateException("JSON Payload is missing the query string field 'sql'");
Review Comment:
Done
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [minor] Handle NPE in controller SQL Resource [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #12211:
URL: https://github.com/apache/pinot/pull/12211
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org