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.

Sign up for GitHub

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: for aggregated expand always set explicit alias #739

Merged
merged 6 commits into from
Jul 12, 2024
Merged

fix: for aggregated expand always set explicit alias #739

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d853a27
fix: for `expand` + `group by` use pk name instead of fk name
patricebender Jul 10, 2024
6282d57
improve scenario test
patricebender Jul 10, 2024
d760954
Merge branch 'main' into patrice/rename
patricebender Jul 11, 2024
edba66a
apply suggestions
patricebender Jul 12, 2024
00e5897
Merge branch 'main' into patrice/rename
patricebender Jul 12, 2024
67f70af
Merge branch 'patrice/rename' of https://github.com/cap-js/cds-dbs in…
patricebender Jul 12, 2024
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
38 changes: 18 additions & 20 deletions db-service/lib/cqn4sql.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ function cqn4sql(originalQuery, model) {
return
}

const tableAlias = getQuerySourceName(col)
const tableAlias = getTableAlias(col)
// re-adjust usage of implicit alias in subquery
if (col.$refLinks[0].definition.kind === 'entity' && col.ref[0] !== tableAlias) {
col.ref[0] = tableAlias
Expand Down Expand Up @@ -725,7 +725,7 @@ function cqn4sql(originalQuery, model) {
res.push(...getColumnsForWildcard(exclude, replace, col.as))
} else
res.push(
...getFlatColumnsFor(col, { columnAlias: col.as, tableAlias: getQuerySourceName(col) }, [], {
...getFlatColumnsFor(col, { columnAlias: col.as, tableAlias: getTableAlias(col) }, [], {
exclude,
replace,
}),
Expand Down Expand Up @@ -895,11 +895,12 @@ function cqn4sql(originalQuery, model) {
)
}

const columnCopy = Object.create(groupByRef)
if (expand.as) {
columnCopy.as = expand.as
}
const res = getFlatColumnsFor(columnCopy, { tableAlias: getQuerySourceName(columnCopy) })
const copy = Object.create(groupByRef)
// always alias for this special case, so that they nested element names match the expected result structure
// otherwise we'd get `author { <outer>.author_ID }`, but we need `author { <outer>.author_ID as ID }`
copy.as = expand.as || expand.ref.at(-1)
const tableAlias = getTableAlias(copy)
const res = getFlatColumnsFor(copy, { tableAlias })
res.forEach(c => {
elements[c.as || c.ref.at(-1)] = c.element
})
Expand Down Expand Up @@ -961,7 +962,7 @@ function cqn4sql(originalQuery, model) {
referredCol.nulls = col.nulls
col = referredCol
if (definition.kind === 'element') {
tableAlias = getQuerySourceName(col)
tableAlias = getTableAlias(col)
} else {
// we must replace the reference with the underlying expression
const { val, func, args, xpr } = col
Expand All @@ -973,7 +974,7 @@ function cqn4sql(originalQuery, model) {
}
}
} else {
tableAlias = getQuerySourceName(col) // do not prepend TA if orderBy column addresses element of query
tableAlias = getTableAlias(col) // do not prepend TA if orderBy column addresses element of query
}
const leaf = col.$refLinks[col.$refLinks.length - 1].definition
if (leaf.virtual === true) continue // already in getFlatColumnForElement
Expand All @@ -992,12 +993,9 @@ function cqn4sql(originalQuery, model) {
if (inOrderBy && flatColumns.length > 1)
throw new Error(`"${getFullName(leaf)}" can't be used in order by as it expands to multiple fields`)
flatColumns.forEach(fc => {
if (col.nulls)
fc.nulls = col.nulls
if (col.sort)
fc.sort = col.sort
if (fc.as)
delete fc.as
if (col.nulls) fc.nulls = col.nulls
if (col.sort) fc.sort = col.sort
if (fc.as) delete fc.as
})
res.push(...flatColumns)
} else {
Expand Down Expand Up @@ -1156,7 +1154,7 @@ function cqn4sql(originalQuery, model) {
if (column.val || column.func || column.SELECT) return [column]

const structsAreUnfoldedAlready = model.meta.unfolded?.includes('structs')
let { baseName, columnAlias, tableAlias } = names
let { baseName, columnAlias = column.as, tableAlias } = names
const { exclude, replace } = excludeAndReplace || {}
const { $refLinks, flatName, isJoinRelevant } = column
let leafAssoc
Expand Down Expand Up @@ -1199,7 +1197,7 @@ function cqn4sql(originalQuery, model) {
baseName = getFullName(replacedBy.$refLinks?.[replacedBy.$refLinks.length - 2].definition)
if (replacedBy.isJoinRelevant)
// we need to provide the correct table alias
tableAlias = getQuerySourceName(replacedBy)
tableAlias = getTableAlias(replacedBy)

if (replacedBy.expand) return [{ as: baseName }]

Expand Down Expand Up @@ -1488,7 +1486,7 @@ function cqn4sql(originalQuery, model) {
// hence we need to ignore the alias of the `$baseLink`
const lastAssoc =
token.isJoinRelevant && [...token.$refLinks].reverse().find(l => l.definition.isAssociation)
const tableAlias = getQuerySourceName(token, (!lastAssoc?.onlyForeignKeyAccess && lastAssoc) || $baseLink)
const tableAlias = getTableAlias(token, (!lastAssoc?.onlyForeignKeyAccess && lastAssoc) || $baseLink)
if ((!$baseLink || lastAssoc) && token.isJoinRelevant) {
let name = calculateElementName(token, getFullName)
result.ref = [tableAlias, name]
Expand Down Expand Up @@ -1585,7 +1583,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 = getQuerySourceName(
const tableAlias = getTableAlias(
def,
def.ref.length > 1 && first.definition.isAssociation ? first : $baseLink,
)
Expand Down Expand Up @@ -2194,7 +2192,7 @@ function cqn4sql(originalQuery, model) {
* the combined elements of the query
* @returns the source name which can be used to address the node
*/
function getQuerySourceName(node, $baseLink = null) {
function getTableAlias(node, $baseLink = null) {
if (!node || !node.$refLinks || !node.ref) {
throw new Error('Invalid node')
}
Expand Down
53 changes: 44 additions & 9 deletions db-service/test/cqn4sql/expand.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1044,7 +1044,7 @@ describe('Expands with aggregations are special', () => {

const qx = CQL`SELECT from bookshop.Books as Books left join bookshop.Authors as author on author.ID = Books.author_ID {
Books.ID,
(SELECT from DUMMY { author.name }) as author
(SELECT from DUMMY { author.name as name }) as author
} group by author.name`
qx.SELECT.columns[1].SELECT.from = null
const res = cqn4sql(q, model)
Expand All @@ -1058,7 +1058,12 @@ describe('Expands with aggregations are special', () => {

const qx = CQL`SELECT from bookshop.Authors as Authors left join bookshop.Books as books on books.author_ID = Authors.ID {
Authors.ID,
(SELECT from DUMMY { books.dedication_addressee_ID, books.dedication_text, books.dedication_sub_foo, books.dedication_dedication }) as books
(SELECT from DUMMY {
books.dedication_addressee_ID as dedication_addressee_ID,
books.dedication_text as dedication_text,
books.dedication_sub_foo as dedication_sub_foo,
books.dedication_dedication as dedication_dedication
}) as books
} group by books.dedication_addressee_ID, books.dedication_text, books.dedication_sub_foo, books.dedication_dedication`
qx.SELECT.columns[1].SELECT.from = null
const res = cqn4sql(q, model)
Expand All @@ -1072,12 +1077,42 @@ describe('Expands with aggregations are special', () => {

const qx = CQL`SELECT from bookshop.Books as Books left join bookshop.Authors as author on author.ID = Books.author_ID {
Books.ID,
(SELECT from DUMMY { author.name, Books.author_ID }) as author
(SELECT from DUMMY { author.name as name, Books.author_ID as ID }) as author
} group by author.name, Books.author_ID`
qx.SELECT.columns[1].SELECT.from = null
const res = cqn4sql(q, model)
expect(JSON.parse(JSON.stringify(res))).to.deep.equal(qx)
})
it('foreign key access renamed', () => {
const q = CQL`SELECT from bookshop.Books {
ID,
Books.author { name, ID as foo }
} group by author.name, author.ID`

const qx = CQL`SELECT from bookshop.Books as Books left join bookshop.Authors as author on author.ID = Books.author_ID {
Books.ID,
(SELECT from DUMMY { author.name as name, Books.author_ID as foo }) as author
} group by author.name, Books.author_ID`
qx.SELECT.columns[1].SELECT.from = null
const res = cqn4sql(q, model)
expect(JSON.parse(JSON.stringify(res))).to.deep.equal(qx)
})
it('non optimized foreign key access with filters', () => {
const q = CQL`SELECT from bookshop.Books {
ID,
Books.author[ID = 201] { name, ID }
} group by author[ID = 201].name, author[ID = 201].ID`

const qx = CQL`SELECT from bookshop.Books as Books
left join bookshop.Authors as author on author.ID = Books.author_ID and author.ID = 201
{
Books.ID,
(SELECT from DUMMY { author.name as name, author.ID as ID}) as author
} group by author.name, author.ID`
qx.SELECT.columns[1].SELECT.from = null
const res = cqn4sql(q, model)
expect(JSON.parse(JSON.stringify(res))).to.deep.equal(qx)
})
it('expand path with filter must be an exact match in group by', () => {
const q = CQL`SELECT from bookshop.Books {
Books.ID,
Expand All @@ -1087,7 +1122,7 @@ describe('Expands with aggregations are special', () => {
const qx = CQL`SELECT from bookshop.Books as Books
left join bookshop.Authors as author on author.ID = Books.author_ID and author.name = 'King' {
Books.ID,
(SELECT from DUMMY { author.name }) as author
(SELECT from DUMMY { author.name as name }) as author
} group by author.name`
qx.SELECT.columns[1].SELECT.from = null
const res = cqn4sql(q, model)
Expand All @@ -1106,8 +1141,8 @@ describe('Expands with aggregations are special', () => {
left join bookshop.Genres as genre on genre.ID = Books.genre_ID
{
Books.ID,
(SELECT from DUMMY { author.name }) as author,
(SELECT from DUMMY { genre.name }) as genre
(SELECT from DUMMY { author.name as name}) as author,
(SELECT from DUMMY { genre.name as name}) as genre
} group by author.name, genre.name`
qx.SELECT.columns[1].SELECT.from = null
qx.SELECT.columns[2].SELECT.from = null
Expand All @@ -1127,7 +1162,7 @@ describe('Expands with aggregations are special', () => {
Genres.ID,
(
SELECT from DUMMY {
(SELECT from DUMMY { parent2.name }) as parent
(SELECT from DUMMY { parent2.name as name }) as parent
}
) as parent,
} group by parent2.name`
Expand All @@ -1149,8 +1184,8 @@ describe('Expands with aggregations are special', () => {
Genres.ID,
(
SELECT from DUMMY {
(SELECT from DUMMY { parent2.name }) as parent,
parent.name
(SELECT from DUMMY { parent2.name as name}) as parent,
parent.name as name
}
) as parent,
} group by parent2.name, parent.name`
Expand Down
18 changes: 16 additions & 2 deletions test/scenarios/bookshop/read.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,27 @@ describe('Bookshop - Read', () => {
})

test('Books with groupby with path expression and expand result', async () => {
const res = await GET('/admin/Books?$apply=filter(title%20ne%20%27bar%27)/groupby((author/name),aggregate(price with sum as totalAmount))', admin)
const res = await GET(
'/admin/Books?$apply=filter(title%20ne%20%27bar%27)/groupby((author/name),aggregate(price with sum as totalAmount))',
admin,
)
expect(res.data.value.length).to.be.eq(4) // As there are two books which have the same author
})

test('same as above, with foreign key optimization', async () => {
const res = await GET('/admin/Books?$apply=filter(title%20ne%20%27bar%27)/groupby((author/name, author/ID),aggregate(price with sum as totalAmount))', admin)
const res = await GET(
'/admin/Books?$apply=filter(title%20ne%20%27bar%27)/groupby((author/name, author/ID),aggregate(price with sum as totalAmount))',
admin,
)
expect(res.data.value.length).to.be.eq(4) // As there are two books which have the same author
expect(
res.data.value.every(
item =>
Object.prototype.hasOwnProperty.call(item, 'author') &&
Object.prototype.hasOwnProperty.call(item.author, 'ID') && // foreign key is renamed to element name in target
!Object.prototype.hasOwnProperty.call(item.author, 'author_ID'),
),
).to.be.true
})

test('Path expression', async () => {
Expand Down
Loading