Skip to content

Commit

Permalink
fix: rewrite assoc chains if intermediate assoc is not fk (#715)
Browse files Browse the repository at this point in the history
in one case a ref `genre.parent.ID` was mistaken for a foreign key
access because `ID` is fk of `parent`. But because `parent` is not fk of
`genre`, it is actually not an fk access.
Hence, the reference actually needs to point to the correct join node.
  • Loading branch information
patricebender authored Jun 28, 2024
1 parent 82c4dbe commit 3873f9a
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 6 deletions.
4 changes: 2 additions & 2 deletions db-service/lib/cqn4sql.js
Original file line number Diff line number Diff line change
Expand Up @@ -1573,7 +1573,7 @@ function cqn4sql(originalQuery, model) {
transformedFrom.args.push(f)
}
})
return { transformedFrom }
return { transformedFrom, transformedWhere: existingWhere }
} else if (from.SELECT) {
transformedFrom = transformSubquery(from)
if (from.as) {
Expand All @@ -1583,7 +1583,7 @@ function cqn4sql(originalQuery, model) {
// select from anonymous query, use artificial alias
transformedFrom.as = Object.keys(originalQuery.sources)[0]
}
return { transformedFrom }
return { transformedFrom, transformedWhere: existingWhere }
} else {
return _transformFrom()
}
Expand Down
8 changes: 6 additions & 2 deletions db-service/lib/infer/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -1013,9 +1013,11 @@ function infer(originalQuery, model) {
}
return true
}
if (assoc && assoc.keys?.some(key => key.ref.every((step, j) => column.ref[i + j] === step))) {
if (assoc) {
// foreign key access without filters never join relevant
return false
if (assoc.keys?.some(key => key.ref.every((step, j) => column.ref[i + j] === step))) return false
// <assoc>.<anotherAssoc>.<…> is join relevant as <anotherAssoc> is not fk of <assoc>
return true
}
if (link.definition.target && link.definition.keys) {
if (column.ref[i + 1] || assoc) fkAccess = false
Expand Down Expand Up @@ -1198,6 +1200,8 @@ function infer(originalQuery, model) {
firstStepIsEntity = true
if (arg.$refLinks.length === 1) return `${cur.definition.name}`
return `${cur.definition.name}`
} else if (cur.definition.SELECT) {
return `${cur.definition.as}`
}
const dot = i === 1 && firstStepIsEntity ? ':' : '.' // divide with colon if first step is entity
return res !== '' ? res + dot + cur.definition.name : cur.definition.name
Expand Down
12 changes: 10 additions & 2 deletions db-service/test/cds-infer/negative.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ describe('negative', () => {
let query = CQL`SELECT from (select from bookshop.Books { ID as FooID }) as Foo { ID }`
expect(() => _inferred(query)).to.throw(/"ID" not found in the elements of "Foo"/)
})
it("reference in group by can't be found in alias of subquery", () => {
let query = CQL`SELECT from (select from bookshop.Books { ID as FooID }) as Foo group by Foo.ID`
expect(() => _inferred(query)).to.throw(/"ID" not found in "Foo"/)
})

it('intermediate step in path expression is not found', () => {
let query = CQL`SELECT from bookshop.Books as Foo { Foo.notExisting.sub.boz }`
Expand Down Expand Up @@ -397,11 +401,15 @@ describe('negative', () => {
})
it('rejects 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.throw(/Only foreign keys of author can be accessed in infix filter, but found name/,) // revisit: better error location ""bookshop.Books:author"
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', () => {
let query = CQL`SELECT from bookshop.Books where exists author.books[coAuthorUnmanaged.name = 'John Doe']`
expect(() => _inferred(query)).to.throw(/Unexpected unmanaged association coAuthorUnmanaged in filter expression of books/,) // revisit: better error location ""bookshop.Books:author"
expect(() => _inferred(query)).to.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
20 changes: 20 additions & 0 deletions db-service/test/cqn4sql/assocs2joins.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,26 @@ describe('Unfolding Association Path Expressions to Joins', () => {
`)
})

it('properly rewrite association chains if intermediate assoc is not fk', () => {
// this issue came up for ref: [genre.parent.ID] because "ID" is fk of "parent"
// but "parent" is not fk of "genre"
const q = CQL`SELECT from (select genre from bookshop.Books) as book {
genre { name }
} group by genre.parent.ID, genre.parent.name`
const qx = CQL`
SELECT from (select Books.genre_ID from bookshop.Books as Books) as book
left join bookshop.Genres as genre on genre.ID = book.genre_ID
left join bookshop.Genres as parent on parent.ID = genre.parent_ID
{
(SELECT
genre2.name
from bookshop.Genres as genre2
where book.genre_ID = genre2.ID) as genre
} group by parent.ID, parent.name`
const res = cqn4sql(q, model)
expect(JSON.parse(JSON.stringify(res))).to.deep.eql(qx)
})

// some notes for later:
// what if only field we fetch from assoc target is virtual? -> make join, but don't fetch anything (?)
})
Expand Down

0 comments on commit 3873f9a

Please sign in to comment.