fix(client-core): Fix for the issue with Generated SQL tab in playground #9675
Conversation
github-actions
bot
added
the
pr:community
|
@UsmanYasin Hey! Good catch! Thnx for fixing it! |
| @@ -627,7 +627,7 @@ class CubeApi { | |||
| query, | |||
| signal: options?.signal | |||
| }), | |||
| (response: any) => (Array.isArray(response) ? response.map((body) => new SqlQuery(body)) : new SqlQuery(response)), | |||
There was a problem hiding this comment.
TBH, I think it would be better to fix the original place that was changed. Because this changes the semantics.
return this.sqlQuery.sql;
}
There was a problem hiding this comment.
Reverted this change and implemented the fix in SqlQuery.ts file. Introduced this type SqlQueryWrapper = { sql: SqlData }; and changed the constructor argument and private sqlQuery variable type to SqlQueryWrapper instead of SqlData.
There was a problem hiding this comment.
Cool! Thnx!
Btw, have you tested it locally?)
There was a problem hiding this comment.
Tested by generating the dev Docker image, and the SQL Generated Tab appears to be working afterwards. I’m not entirely sure how to run all the tests locally or if there are any tests covering this class. I’ll check the tests and add some if they don’t already exist.
There was a problem hiding this comment.
That would be just awesome! I think we can add unit tests for this class for the future!
There was a problem hiding this comment.
Unit tests added for SqlQuery.ts
KSDaemon
changed the title
|
Hope it gets merged and fixed soon! |
|
Thnx for adding tests! |
…und (cube-js#9675) * fix: Suggested fix for the issue with Generated SQL tab in playground * fix: Revised fixed to keep the same semantics * fix: correct SqlQuery types and add unit tests --------- Co-authored-by: Usman Yasin <usman.yasin@rystadenergy.com>
Check List
Issue Reference this PR resolves
This PR fixes issue 9589
Description of Changes Made (if issue reference is not provided)
The generated SQL tab started crashing since v1.3.16 and it was linked to the PR 9562. There are multiple ways of fixing the issue so I have attempted the one with minimal changes. Since I have limited understanding of the codebase, happy to modify the PR if a different approach should be taken to fix the problem.