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: enable path expressions in infix filter after exists predicate #875

Merged
merged 17 commits into from
Nov 13, 2024
Merged
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
31 changes: 23 additions & 8 deletions db-service/lib/cqn4sql.js
Original file line number Diff line number Diff line change
Expand Up @@ -266,10 +266,10 @@ function cqn4sql(originalQuery, model) {
const id = localized(r.queryArtifact)
args.push({ ref: [r.args ? { id, args: r.args } : id], as: r.alias })
}
from = { join: 'left', args, on: [] }
from = { join: r.join || 'left', args, on: [] }
r.children.forEach(c => {
from = joinForBranch(from, c)
from = { join: 'left', args: [from], on: [] }
from = { join: c.join || 'left', args: [from], on: [] }
})
})
return from.args.length > 1 ? from : from.args[0]
Expand Down Expand Up @@ -309,7 +309,7 @@ function cqn4sql(originalQuery, model) {
}
if (node.children) {
node.children.forEach(c => {
lhs = { join: 'left', args: [lhs], on: [] }
lhs = { join: c.join || 'left', args: [lhs], on: [] }
lhs = joinForBranch(lhs, c)
})
}
Expand Down Expand Up @@ -2093,11 +2093,6 @@ function cqn4sql(originalQuery, model) {
const unmanagedOn = onCondFor(inWhere ? next : current, inWhere ? current : next, inWhere)
on.push(...(customWhere && hasLogicalOr(unmanagedOn) ? [asXpr(unmanagedOn)] : unmanagedOn))
}
// infix filter conditions are wrapped in `xpr` when added to the on-condition
if (customWhere) {
const filter = getTransformedTokenStream(customWhere, next)
on.push(...['and', ...(hasLogicalOr(filter) ? [asXpr(filter)] : filter)])
}

const subquerySource = assocTarget(nextDefinition) || nextDefinition
const id = localized(subquerySource)
Expand All @@ -2115,6 +2110,26 @@ function cqn4sql(originalQuery, model) {
],
where: on,
}
if (next.pathExpressionInsideFilter) {
SELECT.where = customWhere
const transformedExists = transformSubquery({ SELECT })
// infix filter conditions are wrapped in `xpr` when added to the on-condition
if (transformedExists.SELECT.where) {
on.push(
...[
'and',
...(hasLogicalOr(transformedExists.SELECT.where)
? [asXpr(transformedExists.SELECT.where)]
: transformedExists.SELECT.where),
],
)
}
transformedExists.SELECT.where = on
return transformedExists.SELECT
} else if (customWhere) {
const filter = getTransformedTokenStream(customWhere, next)
on.push(...['and', ...(hasLogicalOr(filter) ? [asXpr(filter)] : filter)])
}
return SELECT
}

Expand Down
93 changes: 55 additions & 38 deletions db-service/lib/infer/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,16 +184,13 @@ function infer(originalQuery, model) {
if (e.target) {
// only fk access in infix filter
const nextStep = ref[1]?.id || ref[1]
// no unmanaged assoc in infix filter path
if (!expandOrExists && e.on) {
const err = `Unexpected unmanaged association “${e.name}” in filter expression of “${$baseLink.definition.name}”`
throw new Error(err)
if (isNonForeignKeyNavigation(e, nextStep)) {
if (expandOrExists) {
Object.defineProperty($baseLink, 'pathExpressionInsideFilter', { value: true })
} else {
rejectNonFkNavigation(e, e.on ? $baseLink.definition.name : nextStep)
}
}
// no non-fk traversal in infix filter
if (!expandOrExists && nextStep && !isForeignKeyOf(nextStep, e))
throw new Error(
`Only foreign keys of “${e.name}” can be accessed in infix filter, but found “${nextStep}”`,
)
}
arg.$refLinks.push({ definition: e, target: definition })
// filter paths are flattened
Expand Down Expand Up @@ -226,7 +223,7 @@ function infer(originalQuery, model) {
// don't miss an exists within an expression
token.xpr.forEach(walkTokenStream)
} else {
attachRefLinksToArg(token, arg.$refLinks[i], existsPredicate)
attachRefLinksToArg(token, arg.$refLinks[i], existsPredicate || expandOrExists)
existsPredicate = false
}
}
Expand All @@ -235,6 +232,7 @@ function infer(originalQuery, model) {
}
i += 1
}
if ($baseLink?.pathExpressionInsideFilter) Object.defineProperty(arg, 'join', { value: 'inner' })
const { definition, target } = arg.$refLinks[arg.$refLinks.length - 1]
if (definition.value) {
// nested calculated element
Expand Down Expand Up @@ -542,9 +540,19 @@ function infer(originalQuery, model) {
const elements = getDefinition(definition.target)?.elements || definition.elements
if (elements && id in elements) {
const element = elements[id]
rejectNonFkAccess(element)
if (inInfixFilter) {
const nextStep = column.ref[1]?.id || column.ref[1]
if (isNonForeignKeyNavigation(element, nextStep)) {
if (inExists) {
Object.defineProperty($baseLink, 'pathExpressionInsideFilter', { value: true })
} else {
rejectNonFkNavigation(element, element.on ? $baseLink.definition.name : nextStep)
}
}
}
const resolvableIn = getDefinition(definition.target) || target
column.$refLinks.push({ definition: elements[id], target: resolvableIn })
const $refLink = { definition: elements[id], target: resolvableIn }
column.$refLinks.push($refLink)
} else {
stepNotFoundInPredecessor(id, definition.name)
}
Expand Down Expand Up @@ -593,7 +601,16 @@ function infer(originalQuery, model) {

const target = getDefinition(definition.target) || column.$refLinks[i - 1].target
if (element) {
if ($baseLink) rejectNonFkAccess(element)
if ($baseLink && inInfixFilter) {
const nextStep = column.ref[i + 1]?.id || column.ref[i + 1]
if (isNonForeignKeyNavigation(element, nextStep)) {
if (inExists) {
Object.defineProperty($baseLink, 'pathExpressionInsideFilter', { value: true })
} else {
rejectNonFkNavigation(element, element.on ? $baseLink.definition.name : nextStep)
}
}
}
const $refLink = { definition: elements[id], target }
column.$refLinks.push($refLink)
} else if (firstStepIsSelf) {
Expand Down Expand Up @@ -637,7 +654,7 @@ function infer(originalQuery, model) {
skipJoinsForFilter = true
} else if (token.ref || token.xpr) {
inferQueryElement(token, false, column.$refLinks[i], {
inExists: skipJoinsForFilter,
inExists: skipJoinsForFilter || inExists,
inExpr: !!token.xpr,
inInfixFilter: true,
})
Expand All @@ -646,7 +663,7 @@ function infer(originalQuery, model) {
applyToFunctionArgs(token.args, inferQueryElement, [
false,
column.$refLinks[i],
{ inExists: skipJoinsForFilter, inExpr: true, inInfixFilter: true },
{ inExists: skipJoinsForFilter || inExists, inExpr: true, inInfixFilter: true },
])
}
}
Expand Down Expand Up @@ -700,31 +717,11 @@ function infer(originalQuery, model) {
}
}
}

/**
* Check if the next step in the ref is foreign key of `assoc`
* if not, an error is thrown.
*
* @param {CSN.Element} assoc if this is an association, the next step must be a foreign key of the element.
*/
function rejectNonFkAccess(assoc) {
if (inInfixFilter && assoc.target) {
// only fk access in infix filter
const nextStep = column.ref[i + 1]?.id || column.ref[i + 1]
// no unmanaged assoc in infix filter path
if (!inExists && assoc.on) {
const err = `Unexpected unmanaged association “${assoc.name}” in filter expression of “${$baseLink.definition.name}”`
throw new Error(err)
}
// no non-fk traversal in infix filter in non-exists path
if (nextStep && !assoc.on && !isForeignKeyOf(nextStep, assoc))
throw new Error(
`Only foreign keys of “${assoc.name}” can be accessed in infix filter, but found “${nextStep}”`,
)
}
}
})

// we need inner joins for the path expressions inside filter expressions after exists predicate
if ($baseLink?.pathExpressionInsideFilter) Object.defineProperty(column, 'join', { value: 'inner' })

// ignore whole expand if target of assoc along path has ”@cds.persistence.skip”
if (column.expand) {
const { $refLinks } = column
Expand Down Expand Up @@ -1214,6 +1211,26 @@ function infer(originalQuery, model) {
}
}

/**
* Determines if a given association is a non-foreign key navigation.
*
* @param {Object} assoc - The association.
* @param {Object} nextStep - The next step in the navigation path.
* @returns {boolean} - Returns true if the next step is a non-foreign key navigation, otherwise false.
*/
function isNonForeignKeyNavigation(assoc, nextStep) {
if (!nextStep || !assoc.target) return false

return assoc.on || !isForeignKeyOf(nextStep, assoc)
}

function rejectNonFkNavigation(assoc, additionalInfo) {
if (assoc.on) {
throw new Error(`Unexpected unmanaged association “${assoc.name}” in filter expression of “${additionalInfo}”`)
}
throw new Error(`Only foreign keys of “${assoc.name}” can be accessed in infix filter, but found “${additionalInfo}”`)
}

/**
* Returns true if e is a foreign key of assoc.
* this function is also compatible with unfolded csn (UCSN),
Expand Down
1 change: 1 addition & 0 deletions db-service/lib/infer/join-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ class JoinTree {
// if no root node was found, the column is selected from a subquery
if (!node) return
while (i < col.ref.length) {
if(col.join === 'inner') node.join = 'inner'
const step = col.ref[i]
const { where, args } = step
const id = joinId(step, args, where)
Expand Down
16 changes: 11 additions & 5 deletions db-service/test/cds-infer/negative.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -399,17 +399,23 @@ describe('negative', () => {
/Only foreign keys of “author” can be accessed in infix filter, but found “name”/,
)
})
it('rejects non fk traversal in infix filter in where exists', () => {
it('does not reject non fk traversal in infix filter in where exists', () => {
let query = CQL`SELECT from bookshop.Books where exists author.books[author.name = 'John Doe']`
expect(() => _inferred(query)).to.not.throw(
/Only foreign keys of “author” can be accessed in infix filter, but found “name”/,
)
})
it('rejects non fk traversal in infix filter in where', () => {
let query = CQL`SELECT from bookshop.Books where author.books[author.name = 'John Doe'].title = 'foo'`
expect(() => _inferred(query)).to.throw(
/Only foreign keys of “author” can be accessed in infix filter, but found “name”/,
) // revisit: better error location ""bookshop.Books:author"
)
})
it('rejects unmanaged traversal in infix filter in where exists', () => {
it('does not reject unmanaged traversal in infix filter in where exists', () => {
let query = CQL`SELECT from bookshop.Books where exists author.books[coAuthorUnmanaged.name = 'John Doe']`
expect(() => _inferred(query)).to.throw(
expect(() => _inferred(query)).to.not.throw(
/Unexpected unmanaged association “coAuthorUnmanaged” in filter expression of “books”/,
) // revisit: better error location ""bookshop.Books:author"
)
})

it('rejects non fk traversal in infix filter in column', () => {
Expand Down
Loading