From 033fc07fdec10ef37c0bcdbd3df7d2d24b296da3 Mon Sep 17 00:00:00 2001 From: Patrice Bender Date: Thu, 12 Sep 2024 17:45:51 +0200 Subject: [PATCH] check also for expand and exists --- db-service/lib/cqn4sql.js | 80 +++++++++++++------------ db-service/lib/infer/join-tree.js | 7 +-- db-service/lib/utils.js | 27 +++++++++ db-service/test/cqn4sql/keyless.test.js | 42 ++++++++++--- 4 files changed, 108 insertions(+), 48 deletions(-) create mode 100644 db-service/lib/utils.js diff --git a/db-service/lib/cqn4sql.js b/db-service/lib/cqn4sql.js index 21e929ac9..96c32fb59 100644 --- a/db-service/lib/cqn4sql.js +++ b/db-service/lib/cqn4sql.js @@ -4,6 +4,7 @@ const cds = require('@sap/cds') const infer = require('./infer') const { computeColumnsToBeSearched } = require('./search') +const { prettyPrintRef } = require('./utils') /** * For operators of , this is replaced by comparing all leaf elements with null, combined with and. @@ -133,14 +134,14 @@ function cqn4sql(originalQuery, model) { const keys = Object.values(queryTarget.elements).filter(e => e.key === true) const primaryKey = { list: [] } keys - .filter(k => !k.virtual) // e.g. draft column `isActiveEntity` is virtual and key - .forEach(k => { - // cqn4sql will add the table alias to the column later, no need to add it here - subquery.SELECT.columns.push({ ref: [k.name] }) + .filter(k => !k.virtual) // e.g. draft column `isActiveEntity` is virtual and key + .forEach(k => { + // cqn4sql will add the table alias to the column later, no need to add it here + subquery.SELECT.columns.push({ ref: [k.name] }) - // add the alias of the main query to the list of primary key references - primaryKey.list.push({ ref: [transformedFrom.as, k.name] }) - }) + // add the alias of the main query to the list of primary key references + primaryKey.list.push({ ref: [transformedFrom.as, k.name] }) + }) const transformedSubquery = cqn4sql(subquery, model) @@ -765,6 +766,9 @@ function cqn4sql(originalQuery, model) { function expandColumn(column) { let outerAlias let subqueryFromRef + const ref = column.$refLinks[0].definition.kind === 'entity' ? column.ref.slice(1) : column.ref + const assoc = column.$refLinks.at(-1) + ensureValidForeignKeys(assoc.definition, column.ref, 'expand') if (column.isJoinRelevant) { // all n-1 steps of the expand column are already transformed into joins // find the last join relevant association. That is the n-1 assoc in the ref path. @@ -784,24 +788,17 @@ function cqn4sql(originalQuery, model) { outerAlias = transformedQuery.SELECT.from.as subqueryFromRef = [ ...(transformedQuery.SELECT.from.ref || /* subq in from */ [transformedQuery.SELECT.from.target.name]), - ...(column.$refLinks[0].definition.kind === 'entity' ? column.ref.slice(1) : column.ref), + ...ref, ] } // this is the alias of the column which holds the correlated subquery - const columnAlias = - column.as || - (column.$refLinks[0].definition.kind === 'entity' - ? column.ref.slice(1).map(idOnly).join('_') // omit explicit table alias from name of column - : column.ref.map(idOnly).join('_')) + const columnAlias = column.as || ref.map(idOnly).join('_') // if there is a group by on the main query, all // columns of the expand must be in the groupBy if (transformedQuery.SELECT.groupBy) { - const baseRef = - column.$refLinks[0].definition.SELECT || column.$refLinks[0].definition.kind === 'entity' - ? column.ref.slice(1) - : column.ref + const baseRef = column.$refLinks[0].definition.SELECT || ref return _subqueryForGroupBy(column, baseRef, columnAlias) } @@ -1385,6 +1382,8 @@ function cqn4sql(originalQuery, model) { }”`, ) } + const { definition: fkSource } = next + ensureValidForeignKeys(fkSource, ref) whereExistsSubSelects.push(getWhereExistsSubquery(current, next, step.where, true, step.args)) } @@ -1586,10 +1585,7 @@ function cqn4sql(originalQuery, model) { if (!def.$refLinks) return def const leaf = def.$refLinks[def.$refLinks.length - 1] const first = def.$refLinks[0] - const tableAlias = getTableAlias( - def, - def.ref.length > 1 && first.definition.isAssociation ? first : $baseLink, - ) + const tableAlias = getTableAlias(def, def.ref.length > 1 && first.definition.isAssociation ? first : $baseLink) if (leaf.definition.parent.kind !== 'entity') // we need the base name return getFlatColumnsFor(leaf.definition, { @@ -1673,23 +1669,23 @@ function cqn4sql(originalQuery, model) { const refReverse = [...from.ref].reverse() const $refLinksReverse = [...transformedFrom.$refLinks].reverse() for (let i = 0; i < refReverse.length; i += 1) { - const stepLink = $refLinksReverse[i] + const current = $refLinksReverse[i] - let nextStepLink = $refLinksReverse[i + 1] + let next = $refLinksReverse[i + 1] const nextStep = refReverse[i + 1] // only because we want the filter condition - if (stepLink.definition.target && nextStepLink) { + if (current.definition.target && next) { const { where, args } = nextStep - if (isStructured(nextStepLink.definition)) { + if (isStructured(next.definition)) { // find next association / entity in the ref because this is actually our real nextStep const nextStepIndex = 2 + $refLinksReverse .slice(i + 2) .findIndex(rl => rl.definition.isAssociation || rl.definition.kind === 'entity') - nextStepLink = $refLinksReverse[nextStepIndex] + next = $refLinksReverse[nextStepIndex] } - let as = getLastStringSegment(nextStepLink.alias) + let as = getLastStringSegment(next.alias) /** * for an `expand` subquery, we do not need to add * the table alias of the `expand` host to the join tree @@ -1699,8 +1695,10 @@ function cqn4sql(originalQuery, model) { if (!(inferred.SELECT?.expand === true)) { as = getNextAvailableTableAlias(as) } - nextStepLink.alias = as - whereExistsSubSelects.push(getWhereExistsSubquery(stepLink, nextStepLink, where, false, args)) + next.alias = as + const { definition: fkSource } = current + ensureValidForeignKeys(fkSource, from.ref) + whereExistsSubSelects.push(getWhereExistsSubquery(current, next, where, false, args)) } } @@ -1745,6 +1743,14 @@ function cqn4sql(originalQuery, model) { } } + function ensureValidForeignKeys(fkSource, ref, kind = null) { + if (fkSource.keys && fkSource.keys.length === 0) { + const path = prettyPrintRef(ref, model) + if (kind === 'expand') throw new Error(`Can't expand “${fkSource.name}” as it has no foreign keys`) + throw new Error(`Path step “${fkSource.name}” of “${path}” has no foreign keys`) + } + } + function whereExistsSubqueries(whereExistsSubSelects) { if (whereExistsSubSelects.length === 1) return whereExistsSubSelects[0] whereExistsSubSelects.reduce((prev, cur) => { @@ -1855,7 +1861,7 @@ function cqn4sql(originalQuery, model) { result[i] = asXpr(xpr) continue } - if(lhs.args) { + if (lhs.args) { const args = calculateOnCondition(lhs.args) result[i] = { ...lhs, args } continue @@ -2272,13 +2278,6 @@ function cqn4sql(originalQuery, model) { } } -module.exports = Object.assign(cqn4sql, { - // for own tests only: - eqOps, - notEqOps, - notSupportedOps, -}) - function calculateElementName(token) { const nonJoinRelevantAssoc = [...token.$refLinks].findIndex(l => l.definition.isAssociation && l.onlyForeignKeyAccess) let name @@ -2366,3 +2365,10 @@ const refWithConditions = step => { return appendix ? step.id + appendix : step } const is_regexp = x => x?.constructor?.name === 'RegExp' // NOTE: x instanceof RegExp doesn't work in repl + +module.exports = Object.assign(cqn4sql, { + // for own tests only: + eqOps, + notEqOps, + notSupportedOps, +}) diff --git a/db-service/lib/infer/join-tree.js b/db-service/lib/infer/join-tree.js index 8121381ce..334dff2c7 100644 --- a/db-service/lib/infer/join-tree.js +++ b/db-service/lib/infer/join-tree.js @@ -1,5 +1,7 @@ 'use strict' +const { prettyPrintRef } = require('../utils') + // REVISIT: define following unknown types /** @@ -186,10 +188,7 @@ class JoinTree { const $refLink = col.$refLinks[i] // sanity check: error out if we can't produce a join if ($refLink.definition.keys && $refLink.definition.keys.length === 0) { - const path = col.ref.reduce((acc, curr, j) => { - if (j > 0) acc += '.' - return acc + `${curr.id ? curr.id + '[…]' : curr}` - }, '') + const path = prettyPrintRef(col.ref) throw new Error(`Path step “${$refLink.alias}” of “${path}” has no valid foreign keys`) } diff --git a/db-service/lib/utils.js b/db-service/lib/utils.js new file mode 100644 index 000000000..7e83a40a0 --- /dev/null +++ b/db-service/lib/utils.js @@ -0,0 +1,27 @@ +'use strict' + +/** + * Formats a ref array into a string representation. + * If the first step is an entity, the separator is a colon, otherwise a dot. + * + * @param {Array} ref - The reference array to be formatted. + * @param {Object} model - The model object containing definitions. + * @returns {string} The formatted string representation of the reference. + */ +function prettyPrintRef(ref, model = null) { + return ref.reduce((acc, curr, j) => { + if (j > 0) { + if (j === 1 && model?.definitions[ref[0]]?.kind === 'entity') { + acc += ':' + } else { + acc += '.' + } + } + return acc + `${curr.id ? curr.id + '[…]' : curr}` + }, '') +} + +// export the function to be used in other modules +module.exports = { + prettyPrintRef, +} diff --git a/db-service/test/cqn4sql/keyless.test.js b/db-service/test/cqn4sql/keyless.test.js index 27553d310..3484bfb2d 100644 --- a/db-service/test/cqn4sql/keyless.test.js +++ b/db-service/test/cqn4sql/keyless.test.js @@ -19,6 +19,7 @@ describe('keyless entities', () => { expect(() => cqn4sql(q, model)).to.throw( 'Path step “author” of “author[…].book[…].author.name” has no valid foreign keys', ) + // ok if explicit foreign key is used const qOk = SELECT.columns('ID').from(Books).where(`authorWithExplicitForeignKey[ID = 42].name LIKE 'King'`) expect(cqn4sql(qOk, model)).to.eql( CQL`SELECT Books.ID FROM Books as Books @@ -28,17 +29,44 @@ describe('keyless entities', () => { where authorWithExplicitForeignKey.name LIKE 'King'`, ) }) - - it.skip('scoped query leading to where exists subquery cant be constructed', () => { + it('scoped query leading to where exists subquery cant be constructed', () => { const q = SELECT.from('Books:author') - expect(() => cqn4sql(q, model)).to.throw() + expect(() => cqn4sql(q, model)).to.throw(`Path step “author” of “Books:author” has no foreign keys`) + + // ok if explicit foreign key is used + const qOk = SELECT.from('Books:authorWithExplicitForeignKey').columns('ID') + expect(cqn4sql(qOk, model)).to.eql( + CQL`SELECT authorWithExplicitForeignKey.ID FROM Authors as authorWithExplicitForeignKey + where exists ( + SELECT 1 from Books as Books where Books.authorWithExplicitForeignKey_ID = authorWithExplicitForeignKey.ID + )`, + ) }) - it.skip('where exists predicate cant be transformed to subquery', () => { + it('where exists predicate cant be transformed to subquery', () => { const q = SELECT.from('Books').where('exists author') - expect(() => cqn4sql(q, model)).to.throw() + expect(() => cqn4sql(q, model)).to.throw(`Path step “author” of “author” has no foreign keys`) + // ok if explicit foreign key is used + const qOk = SELECT.from('Books').columns('ID').where('exists authorWithExplicitForeignKey') + expect(cqn4sql(qOk, model)).to.eql( + CQL`SELECT Books.ID FROM Books as Books + where exists ( + SELECT 1 from Authors as authorWithExplicitForeignKey where authorWithExplicitForeignKey.ID = Books.authorWithExplicitForeignKey_ID + )`, + ) }) - it.skip('correlated subquery for expand cant be constructed', () => { + it('correlated subquery for expand cant be constructed', () => { const q = CQL`SELECT author { name } from Books` - expect(() => cqn4sql(q, model)).to.throw() + expect(() => cqn4sql(q, model)).to.throw(`Can't expand “author” as it has no foreign keys`) + // ok if explicit foreign key is used + const qOk = CQL`SELECT authorWithExplicitForeignKey { name } from Books` + expect(JSON.parse(JSON.stringify(cqn4sql(qOk, model)))).to.eql( + CQL` + SELECT + ( + SELECT authorWithExplicitForeignKey.name from Authors as authorWithExplicitForeignKey + where Books.authorWithExplicitForeignKey_ID = authorWithExplicitForeignKey.ID + ) as authorWithExplicitForeignKey + from Books as Books`, + ) }) })