[frontend] Use api/utils for loading Impala data #3263
Conversation
| @@ -187,8 +192,10 @@ export const post = <T, U = unknown>( | |||
| const { cancelToken, cancel } = getCancelToken(); | |||
| let completed = false; | |||
|
|
|||
| const encodeData = options?.qsEncodeData == undefined || options?.qsEncodeData; | |||
There was a problem hiding this comment.
Why the undefined check and the ||?
There was a problem hiding this comment.
undefined check, to enabled qsEncodeData by default as expected by the existing calls to post
There was a problem hiding this comment.
Ah. I see that now. So... In your case you want to prevent qsEncodeData? I assumed the opposite, sorry...
We should have a unified approach to whether or not the parameters should be encoded and this is related to the actual lib we use for the API calls and we shouldn't expose that through the options ideally...
Could you adjust the code elsewhere not to encode?
There was a problem hiding this comment.
The alternative would be to change the type of "data" to URLSearchParams and adjust all the other code that calls this function.
There was a problem hiding this comment.
... or, if you're indeed using URLSearchParams.. const encode = !(data instanceof URLSearchParams)
There was a problem hiding this comment.
@sreenaths as far as the Hue UI is concerned it is a Hue API endpoint. The UI shouldn't know about the implementation details (proxy).
There was a problem hiding this comment.
I agree that the APIs should be consistent. But I think we are from from that today. The reason I was so confused about this default encoding is that it is not used by the file browser. So the file browsing APIs are expecting unencoded post data.. which I think makes sense. So what we probably should do is to change the APIs that do expect form data to be encoded, unless there is a good reason for them to do that in which case we have to support both scenarios.
There was a problem hiding this comment.
@sreenaths That is obviously not a task for this PR :-)
There was a problem hiding this comment.
I see now that the Hue API endpoint for the QP is using json.loads(request.body) here. Searching through the Hue codebase it seems to be the only API endpoint that expects json directly from request.body and it feels reasonable to support this.
How about renaming options.qsEncodeData to options.sendJsonBody for now? Later we should look into making jsonBody the default and adjust where it breaks.
There was a problem hiding this comment.
We can do that, although naming it options.sendJsonBody can be a bit confusing if the data is just a simple (non-json) string that we don't want to url encode. Since this function will continue to encode the post data by default using qs.stringify I think the originally suggested options.qsEncodeData is also good for now:
const {qsEncodeData = true } = options;
But this was enlightening! I agree that when we (or a future AI) later harmonize the HUE APIs the default should be not to encode the post data.
| axiosInstance | ||
| .post<T & DefaultApiResponse>(url, qs.stringify(data), { | ||
| .post<T & DefaultApiResponse>(url, encodeData ? qs.stringify(data) : data, { |
There was a problem hiding this comment.
You could use the option right away here, i.e. options?.qsEncodeData ? qs.stringify...
There was a problem hiding this comment.
Because of the undefined check, the conditional statement would be lengthy.
| apiError = new ApiError('error message 2\nerror details\nerror details'); | ||
| expect(apiError.message).toEqual('error message 2'); | ||
| expect(apiError.details).toEqual('error details\nerror details'); |
There was a problem hiding this comment.
Shouldn't these tests should be moved to utils.test.ts?
There was a problem hiding this comment.
Should I rename desktop/core/src/desktop/js/apps/jobBrowser/commons/api-utils/api.ts to desktop/core/src/desktop/js/apps/jobBrowser/commons/api/utils.ts?
There was a problem hiding this comment.
I'm thinking ApiError should be generic and belongs to our global utils.. Or is it very specific to the QP? In which case QueryProcessorApiError could be a better name.
There was a problem hiding this comment.
Implementation of ApiError is generic, nothing specific to QP, I can move it into api/utils.ts
There was a problem hiding this comment.
NIce work @sreenaths sreenaths!
Sorry for the lengthy discussion in your PR :-)
| @@ -187,8 +192,10 @@ export const post = <T, U = unknown>( | |||
| const { cancelToken, cancel } = getCancelToken(); | |||
| let completed = false; | |||
|
|
|||
| const encodeData = options?.qsEncodeData == undefined || options?.qsEncodeData; | |||
There was a problem hiding this comment.
We can do that, although naming it options.sendJsonBody can be a bit confusing if the data is just a simple (non-json) string that we don't want to url encode. Since this function will continue to encode the post data by default using qs.stringify I think the originally suggested options.qsEncodeData is also good for now:
const {qsEncodeData = true } = options;
But this was enlightening! I agree that when we (or a future AI) later harmonize the HUE APIs the default should be not to encode the post data.
|
@JohanAhlen & @bjornalm Thank you so much for the review. |
What changes were proposed in this pull request?
Use api/utils for loading Impala data. Remove the customer api implementation.
The change is based on the review comments at #3226 (comment)
How was this patch tested?