Skip to content
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

fix: flattening of comparisons #938

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 27 additions & 22 deletions db-service/lib/cqn4sql.js
Original file line number Diff line number Diff line change
Expand Up @@ -1439,20 +1439,24 @@ function cqn4sql(originalQuery, model) {
// expand `struct = null | struct2`
const { definition } = token.$refLinks?.[token.$refLinks.length - 1] || {}
const next = tokenStream[i + 1]
if (allOps.some(([firstOp]) => firstOp === next) && (definition?.elements || definition?.keys)) {
const ops = [next]
let indexRhs = i + 2
let rhs = tokenStream[i + 2] // either another operator (i.e. `not like` et. al.) or the operand, i.e. the val | null
if (allOps.some(([, secondOp]) => secondOp === rhs)) {
ops.push(rhs)
rhs = tokenStream[i + 3]
indexRhs += 1
}
let rhs = tokenStream[i + 2] // either another operator (i.e. `not like` et. al.) or the operand, i.e. the val | null
const ops = [next]
let indexRhs = i + 2
if (allOps.some(([, secondOp]) => secondOp === rhs)) {
ops.push(rhs)
rhs = tokenStream[i + 3]
indexRhs += 1
}
if (
allOps.some(([firstOp]) => firstOp === next) &&
(isAssocOrStruct(definition) || isAssocOrStruct(rhs.$refLinks?.at(-1).definition))
) {
if (
isAssocOrStruct(rhs.$refLinks?.[rhs.$refLinks.length - 1].definition) ||
(isAssocOrStruct(rhs.$refLinks?.at(-1).definition) ||
rhs.val !== undefined ||
/* unary operator `is null` parsed as string */
rhs === 'null'
rhs === 'null') ||
(isAssocOrStruct(definition) && rhs)
) {
if (notSupportedOps.some(([firstOp]) => firstOp === next))
throw new Error(`The operator "${next}" is not supported for structure comparison`)
Expand All @@ -1462,8 +1466,7 @@ function cqn4sql(originalQuery, model) {
i = indexRhs // jump to next relevant index
}
} else {
// reject associations in expression, except if we are in an infix filter -> $baseLink is set
assertNoStructInXpr(token, $baseLink)
assertNoStructInXpr(token)

let result = is_regexp(token?.val) ? token : copy(token) // REVISIT: too expensive! //
if (token.ref) {
Expand Down Expand Up @@ -1529,21 +1532,23 @@ function cqn4sql(originalQuery, model) {
* @returns {array}
*/
function expandComparison(token, operator, value, $baseLink = null) {
const { definition } = token.$refLinks[token.$refLinks.length - 1]
const lhs = token.$refLinks ? token : value
const rhs = token.$refLinks ? value : token
const { definition } = lhs.$refLinks.at(-1)
let flatRhs
const result = []
if (value.$refLinks) {
if (rhs.$refLinks) {
// structural comparison
flatRhs = flattenWithBaseName(value)
}

if (flatRhs) {
const flatLhs = flattenWithBaseName(token)
const flatLhs = flattenWithBaseName(lhs)
// make sure we can compare both structures
if (flatRhs.length !== flatLhs.length) {
throw new Error(
`Can't compare "${definition.name}" with "${
value.$refLinks[value.$refLinks.length - 1].definition.name
rhs.$refLinks.at(-1).definition.name
}": the operands must have the same structure`,
)
}
Expand All @@ -1569,12 +1574,12 @@ function cqn4sql(originalQuery, model) {
}
} else {
// compare with value
const flatLhs = flattenWithBaseName(token)
const flatLhs = flattenWithBaseName(lhs)
if (flatLhs.length > 1 && value.val !== null && value !== 'null')
throw new Error(`Can't compare structure "${token.ref.join('.')}" with value "${value.val}"`)
const boolOp = notEqOps.some(([f, s]) => operator[0] === f && operator[1] === s) ? 'or' : 'and'
flatLhs.forEach((column, i) => {
result.push(column, ...operator, value)
result.push(column, ...operator, rhs)
if (flatLhs[i + 1]) result.push(boolOp)
})
}
Expand All @@ -1595,11 +1600,11 @@ function cqn4sql(originalQuery, model) {
}
}

function assertNoStructInXpr(token, inInfixFilter = false) {
if (!inInfixFilter && token.$refLinks?.[token.$refLinks.length - 1].definition.target)
function assertNoStructInXpr(token) {
if (token.$refLinks?.at(-1).definition.target)
// REVISIT: let this through if not requested otherwise
rejectAssocInExpression()
if (isStructured(token.$refLinks?.[token.$refLinks.length - 1].definition))
if (isStructured(token.$refLinks?.at(-1).definition))
// REVISIT: let this through if not requested otherwise
rejectStructInExpression()

Expand Down
63 changes: 63 additions & 0 deletions db-service/test/cqn4sql/filter.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/**
* Transformations of infix filter expressions
* TODO: Move cluttered tests on filters to this file
*/
'use strict'

const cqn4sql = require('../../lib/cqn4sql')
const cds = require('@sap/cds')
const { expect } = cds.test
describe('filter expressions', () => {
let model
beforeAll(async () => {
model = await cds.load(__dirname + '/../bookshop/db/schema').then(cds.linked)
})

describe('on entity ref in from clause', () => {
it('flatten managed assoc; rhs is the assoc', () => {
expect(
cqn4sql(
CQL`
SELECT from bookshop.Books[ID = genre] { ID }
`,
model,
),
).to.deep.equal(
CQL`
SELECT from bookshop.Books as Books { Books.ID }
where Books.ID = Books.genre_ID
`,
)
})
it('flatten managed assoc lhs; rhs is val', () => {
expect(
cqn4sql(
CQL`
SELECT from bookshop.Books[genre = 1] { ID }
`,
model,
),
).to.deep.equal(
CQL`
SELECT from bookshop.Books as Books { Books.ID }
where Books.genre_ID = 1
`,
)
})
it('flatten managed assoc lhs; rhs is ref', () => {
expect(
cqn4sql(
CQL`
SELECT from bookshop.Books[genre = ID] { ID }
`,
model,
),
).to.deep.equal(
CQL`
SELECT from bookshop.Books as Books { Books.ID }
where Books.genre_ID = Books.ID
`,
)
})
})
})
6 changes: 3 additions & 3 deletions db-service/test/cqn4sql/flattening.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -826,9 +826,9 @@ describe('Flattening', () => {
// TODO if implementation is same for all clauses, we probably don't need all these tests
// relax for certain patterns -> see "Expressions in where clauses"

it('rejects managed associations in expressions in HAVING clause (1)', () => {
expect(() => cqn4sql(CQL`SELECT from bookshop.Books { ID } HAVING 2 = author`, model)).to.throw(
/An association can't be used as a value in an expression/,
it('flattens managed associations if compared to val', () => {
expect(cqn4sql(CQL`SELECT from bookshop.Books { ID } HAVING 2 = author`, model)).to.deep.equal(
CQL`SELECT from bookshop.Books as Books { Books.ID } HAVING 2 = Books.author_ID`,
)
})

Expand Down
2 changes: 1 addition & 1 deletion db-service/test/cqn4sql/tupleExpansion.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ describe('Structural comparison', () => {
expect(query).to.deep.equal(CQL(expectedQueryString))
})
})
// TODO (SMW) - should work ...

it('compare struct with assoc (2)', () => {
eqOps.forEach(op => {
const [first] = op
Expand Down
Loading