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

feat: path expressions inside filter of scoped queries #991

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 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
53 changes: 36 additions & 17 deletions db-service/lib/cqn4sql.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@
transformedProp.where = getTransformedTokenStream(where)
}

const queryNeedsJoins = inferred.joinTree && !inferred.joinTree.isInitial
// Transform the from clause: association path steps turn into `WHERE EXISTS` subqueries.
// The already transformed `where` clause is then glued together with the resulting subqueries.
const { transformedWhere, transformedFrom } = getTransformedFrom(from || entity, transformedProp.where)
const queryNeedsJoins = inferred.joinTree && !inferred.joinTree.isInitial

if (inferred.SELECT) {
transformedQuery = transformSelectQuery(queryProp, transformedFrom, transformedWhere, transformedQuery)
Expand Down Expand Up @@ -173,7 +173,10 @@
if (columns) {
transformedQuery.SELECT.columns = getTransformedColumns(columns)
} else {
transformedQuery.SELECT.columns = getColumnsForWildcard(inferred.SELECT?.excluding)
if(inferred.correlateWith)
transformedQuery.SELECT.columns = ['*']
else
transformedQuery.SELECT.columns = getColumnsForWildcard(inferred.SELECT?.excluding)
}

// Like the WHERE clause, aliases from the SELECT list are not accessible for `group by`/`having` (in most DB's)
Expand Down Expand Up @@ -300,12 +303,20 @@
lhs.args.push(arg)
alreadySeen.set(nextAssoc.$refLink.alias, true)
if (nextAssoc.where) {
const filter = getTransformedTokenStream(nextAssoc.where, nextAssoc.$refLink)
lhs.on = [
...(hasLogicalOr(lhs.on) ? [asXpr(lhs.on)] : lhs.on),
'and',
...(hasLogicalOr(filter) ? [asXpr(filter)] : filter),
]
if(nextAssoc.$refLink.specialExistsSubquery) {
const target = getDefinition(nextAssoc.$refLink.definition.target)
const sub = SELECT.from(target).where(nextAssoc.where)
Object.defineProperty(sub, 'correlateWith', { value: {assoc: nextAssoc, alias: arg.as} })
const transformed = cqn4sql(sub, model)
console.log('transformed', transformed)

Check warning on line 311 in db-service/lib/cqn4sql.js

View workflow job for this annotation

GitHub Actions / Tests (22)

Unexpected console statement
} else {
const filter = getTransformedTokenStream(nextAssoc.where, nextAssoc.$refLink)
lhs.on = [
...(hasLogicalOr(lhs.on) ? [asXpr(lhs.on)] : lhs.on),
'and',
...(hasLogicalOr(filter) ? [asXpr(filter)] : filter),
]
}
}
if (node.children) {
node.children.forEach(c => {
Expand Down Expand Up @@ -1035,8 +1046,9 @@
* @returns {object} - The cqn4sql transformed subquery.
*/
function transformSubquery(q) {
if (q.outerQueries) q.outerQueries.push(inferred)
else {
if (q.outerQueries) {
q.outerQueries.push(inferred)
} else {
const outerQueries = inferred.outerQueries || []
outerQueries.push(inferred)
Object.defineProperty(q, 'outerQueries', { value: outerQueries })
Expand Down Expand Up @@ -1225,8 +1237,7 @@
if (flattenThisForeignKey) {
const fkElement = getElementForRef(k.ref, getDefinition(element.target))
let fkBaseName
if (!leafAssoc || leafAssoc.onlyForeignKeyAccess)
fkBaseName = `${baseName}_${k.as || k.ref.at(-1)}`
if (!leafAssoc || leafAssoc.onlyForeignKeyAccess) fkBaseName = `${baseName}_${k.as || k.ref.at(-1)}`
// e.g. if foreign key is accessed via infix filter - use join alias to access key in target
else fkBaseName = k.ref.at(-1)
const fkPath = [...csnPath, k.ref.at(-1)]
Expand Down Expand Up @@ -1478,8 +1489,7 @@
// reject associations in expression, except if we are in an infix filter -> $baseLink is set
assertNoStructInXpr(token, $baseLink)
// reject virtual elements in expressions as they will lead to a sql error down the line
if(definition?.virtual)
throw new Error(`Virtual elements are not allowed in expressions`)
if (definition?.virtual) throw new Error(`Virtual elements are not allowed in expressions`)

let result = is_regexp(token?.val) ? token : copy(token) // REVISIT: too expensive! //
if (token.ref) {
Expand Down Expand Up @@ -1721,8 +1731,12 @@
}

// only append infix filter to outer where if it is the leaf of the from ref
if (refReverse[0].where)
filterConditions.push(getTransformedTokenStream(refReverse[0].where, $refLinksReverse[0]))
if (refReverse[0].where) {
const whereIsTransformedLater =
inferred.joinTree && !inferred.joinTree.isInitial && (originalQuery.DELETE || originalQuery.UPDATE)
if (whereIsTransformedLater) filterConditions.push(refReverse[0].where)
else filterConditions.push(getTransformedTokenStream(refReverse[0].where, $refLinksReverse[0]))
}

if (existingWhere.length > 0) filterConditions.push(existingWhere)
if (whereExistsSubSelects.length > 0) {
Expand Down Expand Up @@ -2144,7 +2158,12 @@
}
if (next.pathExpressionInsideFilter) {
SELECT.where = customWhere
const transformedExists = transformSubquery({ SELECT })
const sub = { SELECT }
// table aliases usually have precedence over elements
// for `SELECT from bookshop.Authors[books.title LIKE '%POE%']:books`
// the outer queries alias would shadow the `books.title` path
Object.defineProperty(sub, 'noBreakout', { value: true })
const transformedExists = transformSubquery(sub)
// infix filter conditions are wrapped in `xpr` when added to the on-condition
if (transformedExists.SELECT.where) {
on.push(
Expand Down
110 changes: 76 additions & 34 deletions db-service/lib/infer/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,25 +44,18 @@ function infer(originalQuery, model) {

let $combinedElements

const sources = inferTarget(_.from || _.into || _.entity, {})
const joinTree = new JoinTree(sources)
// path expressions in from.ref.at(-1).where
// are collected here and merged once the joinTree is initialized
const mergeOnceJoinTreeIsInitialized = []

let sources = inferTarget(_.from || _.into || _.entity, {})
let joinTree = new JoinTree(sources)
const aliases = Object.keys(sources)
Object.defineProperties(inferred, {
// REVISIT: public, or for local reuse, or in cqn4sql only?
sources: { value: sources, writable: true },
target: {
value: aliases.length === 1 ? getDefinitionFromSources(sources, aliases[0]) : originalQuery,
writable: true,
}, // REVISIT: legacy?
})
// also enrich original query -> writable because it may be inferred again
Object.defineProperties(originalQuery, {
sources: { value: sources, writable: true },
target: {
value: aliases.length === 1 ? getDefinitionFromSources(sources, aliases[0]) : originalQuery,
writable: true,
},
})
if (mergeOnceJoinTreeIsInitialized.length) {
mergeOnceJoinTreeIsInitialized.forEach(arg => joinTree.mergeColumn(arg, originalQuery.outerQueries))
}

initializeQueryTargets(inferred, originalQuery, sources)
if (originalQuery.SELECT || originalQuery.DELETE || originalQuery.UPDATE) {
$combinedElements = inferCombinedElements()
/**
Expand Down Expand Up @@ -404,7 +397,7 @@ function infer(originalQuery, model) {
*/

function inferArg(arg, queryElements = null, $baseLink = null, context = {}) {
const { inExists, inXpr, inCalcElement, baseColumn, inInfixFilter, inQueryModifier, inFrom, dollarSelfRefs } = context
const { inExists, inXpr, inCalcElement, baseColumn, inInfixFilter, inQueryModifier, inFrom, dollarSelfRefs, atFromLeaf } = context
if (arg.param || arg.SELECT) return // parameter references are only resolved into values on execution e.g. :val, :1 or ?
if (arg.args) applyToFunctionArgs(arg.args, inferArg, [null, $baseLink, context])
if (arg.list) arg.list.forEach(arg => inferArg(arg, null, $baseLink, context))
Expand Down Expand Up @@ -456,10 +449,10 @@ function infer(originalQuery, model) {
if (inInfixFilter) {
const nextStep = arg.ref[1]?.id || arg.ref[1]
if (isNonForeignKeyNavigation(element, nextStep)) {
if (inExists) {
if (inExists || inFrom) {
Object.defineProperty($baseLink, 'pathExpressionInsideFilter', { value: true })
} else {
rejectNonFkNavigation(element, element.on ? $baseLink.definition.name : nextStep)
Object.defineProperty($baseLink, 'specialExistsSubquery', { value: true })
}
}
}
Expand All @@ -480,7 +473,7 @@ function infer(originalQuery, model) {
})
} else if (firstStepIsSelf) {
arg.$refLinks.push({ definition: { elements: queryElements }, target: { elements: queryElements } })
} else if (arg.ref.length > 1 && inferred.outerQueries?.find(outer => id in outer.sources)) {
} else if (!inferred.noBreakout && arg.ref.length > 1 && inferred.outerQueries?.find(outer => id in outer.sources)) {
// outer query accessed via alias
const outerAlias = inferred.outerQueries.find(outer => id in outer.sources)
arg.$refLinks.push({
Expand Down Expand Up @@ -520,7 +513,7 @@ function infer(originalQuery, model) {
if ($baseLink && inInfixFilter) {
const nextStep = arg.ref[i + 1]?.id || arg.ref[i + 1]
if (isNonForeignKeyNavigation(element, nextStep)) {
if (inExists) {
if (inExists || inFrom) {
Object.defineProperty($baseLink, 'pathExpressionInsideFilter', { value: true })
} else {
rejectNonFkNavigation(element, element.on ? $baseLink.definition.name : nextStep)
Expand Down Expand Up @@ -575,13 +568,19 @@ function infer(originalQuery, model) {
inXpr: !!token.xpr,
inInfixFilter: true,
inFrom,
atFromLeaf: inFrom && !arg.ref[i + 1],
})
} else if (token.func) {
if (token.args) {
applyToFunctionArgs(token.args, inferArg, [
false,
arg.$refLinks[i],
{ inExists: skipJoinsForFilter || inExists, inXpr: true, inInfixFilter: true, inFrom },
arg.$refLinks[i], {
inExists: skipJoinsForFilter || inExists,
inXpr: true,
inInfixFilter: true,
inFrom,
atFromLeaf: inFrom && !arg.ref[i + 1],
},
])
}
}
Expand Down Expand Up @@ -637,7 +636,18 @@ function infer(originalQuery, model) {
})

// we need inner joins for the path expressions inside filter expressions after exists predicate
if ($baseLink?.pathExpressionInsideFilter) Object.defineProperty(arg, 'join', { value: 'inner' })
if ($baseLink?.pathExpressionInsideFilter) {
Object.defineProperty(arg, 'join', { value: 'inner' })
if (inFrom && atFromLeaf && !inExists) {
// REVISIT: would it be enough to check the last assocs cardinality?
if(arg.$refLinks.some(link => link.definition.isAssociation && link.definition.is2many)) {
throw cds.error`Filtering via path expressions on to-many associations is not allowed at the leaf of a FROM clause. Use EXISTS predicates instead.`
}
// join tree not yet initialized
Object.defineProperty(arg, 'isJoinRelevant', { value: true })
mergeOnceJoinTreeIsInitialized.push(arg)
}
}

// ignore whole expand if target of assoc along path has ”@cds.persistence.skip”
if (arg.expand) {
Expand All @@ -657,6 +667,19 @@ function infer(originalQuery, model) {
? { ref: [...baseColumn.ref, ...arg.ref], $refLinks: [...baseColumn.$refLinks, ...arg.$refLinks] }
: arg
if (isColumnJoinRelevant(colWithBase)) {
if(originalQuery.correlateWith && joinTree.isInitial) {
// the very first assoc sets the alias of the correlated query
const firstAssoc = arg.$refLinks.find(link => link.definition.isAssociation)
const key = Object.keys(originalQuery.sources)[0];
const adjustedSource = {
[firstAssoc.alias]: originalQuery.sources[key]
}
sources = adjustedSource
// initializeQueryTargets(inferred, originalQuery, adjustedSource)
joinTree = new JoinTree(adjustedSource, originalQuery)
inferred.SELECT.from.as = firstAssoc.alias
$combinedElements = inferCombinedElements()
}
Object.defineProperty(arg, 'isJoinRelevant', { value: true })
joinTree.mergeColumn(colWithBase, originalQuery.outerQueries)
}
Expand Down Expand Up @@ -884,8 +907,7 @@ function infer(originalQuery, model) {
step[nestedProp].forEach(a => {
// reset sub path for each nested argument
// e.g. case when <path> then <otherPath> else <anotherPath> end
if(!a.ref)
subPath = { $refLinks: [...basePath.$refLinks], ref: [...basePath.ref] }
if (!a.ref) subPath = { $refLinks: [...basePath.$refLinks], ref: [...basePath.ref] }
mergePathsIntoJoinTree(a, subPath)
})
}
Expand All @@ -896,7 +918,7 @@ function infer(originalQuery, model) {
const calcElementIsJoinRelevant = isColumnJoinRelevant(p)
if (calcElementIsJoinRelevant) {
if (!calcElement.value.isJoinRelevant)
Object.defineProperty(step, 'isJoinRelevant', { value: true, writable: true, })
Object.defineProperty(step, 'isJoinRelevant', { value: true, writable: true })
joinTree.mergeColumn(p, originalQuery.outerQueries)
} else {
// we need to explicitly set the value to false in this case,
Expand Down Expand Up @@ -963,7 +985,7 @@ function infer(originalQuery, model) {
const exclude = _.excluding ? x => _.excluding.includes(x) : () => false

if (Object.keys(queryElements).length === 0 && aliases.length === 1) {
const { elements } = getDefinitionFromSources(sources, aliases[0])
const { elements } = getDefinitionFromSources(sources, Object.keys(sources)[0])
// only one query source and no overwritten columns
for (const k of Object.keys(elements)) {
if (!exclude(k)) {
Expand Down Expand Up @@ -1094,10 +1116,6 @@ function infer(originalQuery, model) {
return model.definitions[name]
}

function getDefinitionFromSources(sources, id) {
return sources[id].definition
}

/**
* Returns the csn path as string for a given column ref with sibling $refLinks
*
Expand All @@ -1120,6 +1138,30 @@ function infer(originalQuery, model) {
}
}

function getDefinitionFromSources(sources, id) {
return sources[id].definition
}

function initializeQueryTargets(inferred, originalQuery, sources) {
const aliases = Object.keys(sources)
Object.defineProperties(inferred, {
// REVISIT: public, or for local reuse, or in cqn4sql only?
sources: { value: sources, writable: true },
target: {
value: aliases.length === 1 ? getDefinitionFromSources(sources, aliases[0]) : originalQuery,
writable: true,
}, // REVISIT: legacy?
})
// also enrich original query -> writable because it may be inferred again
Object.defineProperties(originalQuery, {
sources: { value: sources, writable: true },
target: {
value: aliases.length === 1 ? getDefinitionFromSources(sources, aliases[0]) : originalQuery,
writable: true,
},
})
}

/**
* Determines if a given association is a non-foreign key navigation.
*
Expand Down
2 changes: 1 addition & 1 deletion db-service/test/cds-infer/negative.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ describe('negative', () => {
})

describe('infix filters', () => {
it('rejects non fk traversal in infix filter in from', () => {
it.skip('rejects non fk traversal in infix filter in from', () => {
expect(() => _inferred(CQL`SELECT from bookshop.Books[author.name = 'Kurt']`, model)).to.throw(
/Only foreign keys of “author” can be accessed in infix filter, but found “name”/,
)
Expand Down
59 changes: 59 additions & 0 deletions db-service/test/cqn4sql/DELETE.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -244,4 +244,63 @@ describe('DELETE', () => {
}
expect(query.DELETE).to.deep.equal(expected.DELETE)
})

describe('with path expressions', () => {
let forNodeModel
beforeAll(() => {
// subqueries reference flat author_ID, which is not part of client csn
forNodeModel = cds.compile.for.nodejs(JSON.parse(JSON.stringify(cds.model)))
})

it('inner joins for the path expression at the leaf of scoped queries', () => {
let query = DELETE.from('bookshop.Authors:books[genre.name = null]')
const transformed = cqn4sql(query, forNodeModel)

const subquery = cds.ql`
SELECT books.ID from bookshop.Books as books
inner join bookshop.Genres as genre on genre.ID = books.genre_ID
WHERE EXISTS (
SELECT 1 from bookshop.Authors as Authors where Authors.ID = books.author_ID
) and genre.name = null`
const expected = DELETE.from('bookshop.Books').alias('books2')
expected.DELETE.where = [{ list: [{ ref: ['books2', 'ID'] }] }, 'in', subquery]

expect(transformed).to.deep.equal(expected)
})

it('inner joins for the path expression at the leaf of scoped queries, two assocs', () => {
let query = DELETE.from('bookshop.Authors:books[genre.parent.name = null]')
const transformed = cqn4sql(query, forNodeModel)

const subquery = cds.ql`
SELECT books.ID from bookshop.Books as books
inner join bookshop.Genres as genre on genre.ID = books.genre_ID
inner join bookshop.Genres as parent on parent.ID = genre.parent_ID
WHERE EXISTS (
SELECT 1 from bookshop.Authors as Authors where Authors.ID = books.author_ID
) and parent.name = null`
const expected = DELETE.from('bookshop.Books').alias('books2')
expected.DELETE.where = [{ list: [{ ref: ['books2', 'ID'] }] }, 'in', subquery]

expect(transformed).to.deep.equal(expected)
})
it('inner joins for the path expression NOT at the leaf of scoped queries, two assocs', () => {
let query = DELETE.from(`bookshop.Authors[books.title = 'bar']:books[genre.parent.name = null]`).alias('MyBook')

const transformed = cqn4sql(query, forNodeModel)
const subquery = cds.ql`
SELECT MyBook.ID from bookshop.Books as MyBook
inner join bookshop.Genres as genre on genre.ID = MyBook.genre_ID
inner join bookshop.Genres as parent on parent.ID = genre.parent_ID
WHERE EXISTS (
SELECT 1 from bookshop.Authors as Authors
inner join bookshop.Books as books on books.author_ID = Authors.ID
where Authors.ID = MyBook.author_ID and books.title = 'bar'
) and parent.name = null`
const expected = DELETE.from('bookshop.Books').alias('MyBook2')
expected.DELETE.where = [{ list: [{ ref: ['MyBook2', 'ID'] }] }, 'in', subquery]

expect(transformed).to.deep.equal(expected)
})
})
})
Loading
Loading