Skip to content

Conversation

@bsod90
Copy link
Member

@bsod90 bsod90 commented Dec 6, 2024

fixes an issue when processing queries with member expressions

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Issue Reference this PR resolves

[For example #12]

Description of Changes Made (if issue reference is not provided)

[Description goes here]

@bsod90 bsod90 requested a review from a team as a code owner December 6, 2024 02:28
@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Dec 6, 2024
if (hasExpressionsInQuery) {
rewrittenQuery = this.evalMemberExpressionsInQuery(rewrittenQuery);
if (this.hasExpressionsInQuery(rewrittenQuery)) {
rewrittenQuery = this.parseMemberExpressionsInQuery(rewrittenQuery);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not yet checked yet what can applyRowLevelSecurity introduce

But parseMemberExpressionsInQuery is not enough anyway.
There's three states of member expressions:

  1. MemberExpression
    Object with expression method, that takes cubes as symbols and returns SQL

  2. ParsedMemberExpression
    Object with expression as array of strings. Essentially JSON-compatible way of storing MemberExpression

  3. Just a string with JSON-serialzed ParsedMemberExpression.

Schema compiler expects only MemberExpression.
parseMemberExpressionsInQuery turns strings in proper places in query to ParsedMemberExpression.
evalMemberExpressionsInQuery turns ParsedMemberExpression to MemberExpression in those places.

So, depending on what applyRowLevelSecurity can add either only evalMemberExpressionsInQuery, or both of parseMemberExpressionsInQuery + evalMemberExpressionsInQuery are necessary.

Copy link
Contributor

@mcheshkov mcheshkov Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expression: () => '1 = 0',
That's MemberExpression, with expression as method, returning SQL. For this case no need to add any additional calls after applyRowLevelSecurity

String member expression would look like something this: query.segments.push('{"cubeName":"foo","expr":"1=0"}');
This would require to add both calls to parse and eval

Copy link
Member Author

@bsod90 bsod90 Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I've removed all my previous comments and just added the eval. Looks like it should be fine now

const normalizedQuery = normalizeQuery(currentQuery, persistent);
let normalizedQuery = normalizeQuery(currentQuery, persistent);

if (hasExpressionsInQuery) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add comment about why early/second call is necessary. I'll probably forget about this whole issue next week 😅

@bsod90 bsod90 force-pushed the dap_sql_pushdown_fix branch from 75f8708 to b7bd8b6 Compare December 6, 2024 17:07
@bsod90 bsod90 changed the title fix(api-gateway): make sure DAP works sql pushdown fix(api-gateway): make sure DAP works with sql pushdown Dec 6, 2024
fixes an issue when processing queries with member expressions
@bsod90 bsod90 force-pushed the dap_sql_pushdown_fix branch from b7bd8b6 to 33c1659 Compare December 6, 2024 17:22
@bsod90 bsod90 requested a review from mcheshkov December 6, 2024 22:17
@bsod90 bsod90 merged commit 23695b2 into master Dec 9, 2024
28 checks passed
@bsod90 bsod90 deleted the dap_sql_pushdown_fix branch December 9, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:community Contribution from Cube.js community members.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants