-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Adding dropResults parameter in queryOptions to drop the resultTable field from the response #10419
Conversation
…field from the response.
if (request.has(Broker.Request.QUERY_OPTIONS)) { | ||
String queryOptions = request.get(Broker.Request.QUERY_OPTIONS).asText(); | ||
Map<String, String> optionsFromString = RequestUtils.getOptionsFromString(queryOptions); | ||
if (optionsFromString.getOrDefault(Broker.Request.QueryOptionKey.DROP_RESULTS, "false") |
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.
You may put this logic into QueryOptionsUtils
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.
It is equivalent to Boolean.parseBoolean(queryOptions.get(QueryOptionKey.DROP_RESULTS))
Codecov Report
@@ Coverage Diff @@
## master #10419 +/- ##
=============================================
- Coverage 70.32% 24.43% -45.90%
+ Complexity 6105 49 -6056
=============================================
Files 2055 2028 -27
Lines 111389 110638 -751
Branches 16939 16850 -89
=============================================
- Hits 78337 27034 -51303
- Misses 27566 80780 +53214
+ Partials 5486 2824 -2662
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1538 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
LGTM otherwise
public static boolean isDropResultsEnabled(final JsonNode request) { | ||
boolean dropResults = false; | ||
|
||
if (request.has(CommonConstants.Broker.Request.QUERY_OPTIONS)) { | ||
String queryOptions = request.get(CommonConstants.Broker.Request.QUERY_OPTIONS).asText(); | ||
Map<String, String> optionsFromString = RequestUtils.getOptionsFromString(queryOptions); | ||
if (Boolean.parseBoolean(optionsFromString.get(CommonConstants.Broker.Request.QueryOptionKey.DROP_RESULTS))) { | ||
dropResults = true; | ||
} | ||
} | ||
|
||
return dropResults; | ||
} |
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.
We want to take the Map
here instead of the JsonNode
. The logic of extracting the map should be on the caller side. Also suggest rename it
public static boolean isDropResultsEnabled(final JsonNode request) { | |
boolean dropResults = false; | |
if (request.has(CommonConstants.Broker.Request.QUERY_OPTIONS)) { | |
String queryOptions = request.get(CommonConstants.Broker.Request.QUERY_OPTIONS).asText(); | |
Map<String, String> optionsFromString = RequestUtils.getOptionsFromString(queryOptions); | |
if (Boolean.parseBoolean(optionsFromString.get(CommonConstants.Broker.Request.QueryOptionKey.DROP_RESULTS))) { | |
dropResults = true; | |
} | |
} | |
return dropResults; | |
} | |
public static boolean shouldDropResults(Map<String, String> queryOptions) { | |
return Boolean.parseBoolean(queryOptions.get(CommonConstants.Broker.Request.QueryOptionKey.DROP_RESULTS)); | |
} |
This PR adds a parameter to queryOptions to drop the resultTable from the response. This mode can be used to troubleshoot a customer's query (which may have sensitive data in the result) using metadata only.
Manual testing report:
Added integration tests as well.