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

fix: only expand partial foreign key if explicitly requested #916

Merged
merged 10 commits into from
Dec 9, 2024
138 changes: 77 additions & 61 deletions db-service/lib/cqn4sql.js
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,10 @@ function cqn4sql(originalQuery, model) {

let baseName
if (col.ref.length >= 2) {
baseName = col.ref.slice(col.ref[0] === tableAlias ? 1 : 0, col.ref.length - 1).join('_')
baseName = col.ref
.map(idOnly)
.slice(col.ref[0] === tableAlias ? 1 : 0, col.ref.length - 1)
.join('_')
}

let columnAlias = col.as || (col.isJoinRelevant ? col.flatName : null)
Expand Down Expand Up @@ -983,7 +986,10 @@ function cqn4sql(originalQuery, model) {
let baseName
if (col.ref.length >= 2) {
// leaf might be intermediate structure
baseName = col.ref.slice(col.ref[0] === tableAlias ? 1 : 0, col.ref.length - 1).join('_')
baseName = col.ref
.map(idOnly)
.slice(col.ref[0] === tableAlias ? 1 : 0, col.ref.length - 1)
.join('_')
}
const flatColumns = getFlatColumnsFor(col, { baseName, tableAlias })
/**
Expand Down Expand Up @@ -1167,10 +1173,9 @@ function cqn4sql(originalQuery, model) {
else if (element.virtual === true) return []
else if (!isJoinRelevant && flatName) baseName = flatName
else if (isJoinRelevant) {
const leaf = column.$refLinks[column.$refLinks.length - 1]
const leaf = column.$refLinks.at(-1)
leafAssoc = [...column.$refLinks].reverse().find(link => link.definition.isAssociation)
let elements
elements = leafAssoc.definition.elements || leafAssoc.definition.foreignKeys
let elements = leafAssoc.definition.elements || leafAssoc.definition.foreignKeys
if (elements && leaf.definition.name in elements) {
element = leafAssoc.definition
baseName = getFullName(leafAssoc.definition)
Expand Down Expand Up @@ -1210,68 +1215,76 @@ function cqn4sql(originalQuery, model) {

if (element.keys) {
const flatColumns = []
element.keys.forEach(fk => {
const fkElement = getElementForRef(fk.ref, getDefinition(element.target))
let fkBaseName
if (!leafAssoc || leafAssoc.onlyForeignKeyAccess)
fkBaseName = `${baseName}_${fk.as || fk.ref[fk.ref.length - 1]}`
// e.g. if foreign key is accessed via infix filter - use join alias to access key in target
else fkBaseName = fk.ref[fk.ref.length - 1]
const fkPath = [...csnPath, fk.ref[fk.ref.length - 1]]
if (fkElement.elements) {
// structured key
Object.values(fkElement.elements).forEach(e => {
let alias
if (columnAlias) {
const fkName = fk.as
? `${fk.as}_${e.name}` // foreign key might also be re-named: `assoc { id as foo }`
: `${fk.ref.join('_')}_${e.name}`
alias = `${columnAlias}_${fkName}`
for (const k of element.keys) {
// if only one part of a foreign key is requested, only flatten the partial key
const keyElement = getElementForRef(k.ref, getDefinition(element.target))
const flattenThisForeignKey =
!$refLinks || // the association is passed as element, not as ref --> flatten full foreign key
element === $refLinks.at(-1).definition || // the association is the leaf of the ref --> flatten full foreign key
keyElement === $refLinks.at(-1).definition // the foreign key is the leaf of the ref --> only flatten this specific foreign key
if (flattenThisForeignKey) {
const fkElement = getElementForRef(k.ref, getDefinition(element.target))
let fkBaseName
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)]
if (fkElement.elements) {
// structured key
for (const e of Object.values(fkElement.elements)) {
let alias
if (columnAlias) {
const fkName = k.as
? `${k.as}_${e.name}` // foreign key might also be re-named: `assoc { id as foo }`
: `${k.ref.join('_')}_${e.name}`
alias = `${columnAlias}_${fkName}`
}
flatColumns.push(
...getFlatColumnsFor(
e,
{ baseName: fkBaseName, columnAlias: alias, tableAlias },
[...fkPath],
excludeAndReplace,
isWildcard,
),
)
}
} else if (fkElement.isAssociation) {
// assoc as key
flatColumns.push(
...getFlatColumnsFor(
e,
{ baseName: fkBaseName, columnAlias: alias, tableAlias },
[...fkPath],
fkElement,
{ baseName, columnAlias, tableAlias },
csnPath,
excludeAndReplace,
isWildcard,
),
)
})
} else if (fkElement.isAssociation) {
// assoc as key
flatColumns.push(
...getFlatColumnsFor(
fkElement,
{ baseName, columnAlias, tableAlias },
csnPath,
excludeAndReplace,
isWildcard,
),
)
} else {
// leaf reached
let flatColumn
if (columnAlias) {
// if the column has an explicit alias AND the orignal ref
// directly resolves to the foreign key, we must not append the fk name to the column alias
// e.g. `assoc.fk as FOO` => columns.alias = FOO
// `assoc as FOO` => columns.alias = FOO_fk
let columnAliasWithFlatFk
if (!(column.as && fkElement === column.$refLinks?.at(-1).definition))
columnAliasWithFlatFk = `${columnAlias}_${fk.as || fk.ref.join('_')}`
flatColumn = { ref: [fkBaseName], as: columnAliasWithFlatFk || columnAlias }
} else flatColumn = { ref: [fkBaseName] }
if (tableAlias) flatColumn.ref.unshift(tableAlias)

// in a flat model, we must assign the foreign key rather than the key in the target
const flatForeignKey = getDefinition(element.parent.name)?.elements[fkBaseName]

setElementOnColumns(flatColumn, flatForeignKey || fkElement)
Object.defineProperty(flatColumn, '_csnPath', { value: csnPath, writable: true })
flatColumns.push(flatColumn)
} else {
// leaf reached
let flatColumn
if (columnAlias) {
// if the column has an explicit alias AND the original ref
// directly resolves to the foreign key, we must not append the fk name to the column alias
// e.g. `assoc.fk as FOO` => columns.alias = FOO
// `assoc as FOO` => columns.alias = FOO_fk
let columnAliasWithFlatFk
if (!(column.as && fkElement === column.$refLinks?.at(-1).definition))
columnAliasWithFlatFk = `${columnAlias}_${k.as || k.ref.join('_')}`
flatColumn = { ref: [fkBaseName], as: columnAliasWithFlatFk || columnAlias }
} else flatColumn = { ref: [fkBaseName] }
if (tableAlias) flatColumn.ref.unshift(tableAlias)

// in a flat model, we must assign the foreign key rather than the key in the target
const flatForeignKey = getDefinition(element.parent.name)?.elements[fkBaseName]

setElementOnColumns(flatColumn, flatForeignKey || fkElement)
Object.defineProperty(flatColumn, '_csnPath', { value: csnPath, writable: true })
flatColumns.push(flatColumn)
}
}
})
}
return flatColumns
} else if (element.elements) {
const flatRefs = []
Expand Down Expand Up @@ -1304,7 +1317,7 @@ function cqn4sql(originalQuery, model) {

function getReplacement(from) {
return from?.find(replacement => {
const nameOfExcludedColumn = replacement.as || replacement.ref?.[replacement.ref.length - 1] || replacement
const nameOfExcludedColumn = replacement.as || replacement.ref?.at(-1) || replacement
return nameOfExcludedColumn === element.name
})
}
Expand Down Expand Up @@ -1588,7 +1601,10 @@ function cqn4sql(originalQuery, model) {
if (leaf.definition.parent.kind !== 'entity')
// we need the base name
return getFlatColumnsFor(leaf.definition, {
baseName: def.ref.slice(0, def.ref.length - 1).join('_'),
baseName: def.ref
.map(idOnly)
.slice(0, def.ref.length - 1)
.join('_'),
tableAlias,
})
return getFlatColumnsFor(leaf.definition, { tableAlias })
Expand Down
29 changes: 28 additions & 1 deletion db-service/test/cqn4sql/expand.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1050,6 +1050,29 @@ describe('Expands with aggregations are special', () => {
const res = cqn4sql(q, model)
expect(JSON.parse(JSON.stringify(res))).to.deep.equal(qx)
})

it('aggregation with mulitple path steps', () => {
const q = CQL`SELECT from bookshop.Intermediate {
ID,
toAssocWithStructuredKey { toStructuredKey { second } }
} group by toAssocWithStructuredKey.toStructuredKey.second`

const qx = CQL`SELECT from bookshop.Intermediate as Intermediate
left join bookshop.AssocWithStructuredKey as toAssocWithStructuredKey
on toAssocWithStructuredKey.ID = Intermediate.toAssocWithStructuredKey_ID
{
Intermediate.ID,
(SELECT from DUMMY {
(SELECT from DUMMY {
toAssocWithStructuredKey.toStructuredKey_second as second
}) as toStructuredKey
}) as toAssocWithStructuredKey
} group by toAssocWithStructuredKey.toStructuredKey_second`
qx.SELECT.columns[1].SELECT.from = null
qx.SELECT.columns[1].SELECT.columns[0].SELECT.from = null
const res = cqn4sql(q, model)
expect(JSON.parse(JSON.stringify(res))).to.deep.equal(qx)
})
it.skip('simple aggregation expand ref wrapped in func', () => {
// TODO: how to detect the nested ref?
const q = CQL`SELECT from bookshop.Books {
Expand All @@ -1062,6 +1085,7 @@ describe('Expands with aggregations are special', () => {
(SELECT from DUMMY { toLower(author.name) as name }) as author
} group by author.name`
qx.SELECT.columns[1].SELECT.from = null

const res = cqn4sql(q, model)
expect(JSON.parse(JSON.stringify(res))).to.deep.equal(qx)
})
Expand All @@ -1079,6 +1103,7 @@ describe('Expands with aggregations are special', () => {
const res = cqn4sql(q, cds.compile.for.nodejs(model))
expect(JSON.parse(JSON.stringify(res))).to.deep.equal(qx)
})

it('aggregation with structure', () => {
const q = CQL`SELECT from bookshop.Authors as Authors {
ID,
Expand Down Expand Up @@ -1260,7 +1285,9 @@ describe('Expands with aggregations are special', () => {
author[name='King'] { name }
} group by author.name`

expect(() => cqn4sql(q, model)).to.throw(`The expanded column "author[{"ref":["name"]},"=",{"val":"King"}].name" must be part of the group by clause`)
expect(() => cqn4sql(q, model)).to.throw(
`The expanded column "author[{"ref":["name"]},"=",{"val":"King"}].name" must be part of the group by clause`,
)
})
it('expand path with filter must be an exact match in group by (2)', () => {
const q = CQL`SELECT from bookshop.Books {
Expand Down
30 changes: 30 additions & 0 deletions db-service/test/cqn4sql/flattening.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,36 @@ describe('Flattening', () => {
`)
})

it('if only partial foreign key is accessed, only the requested key is flattened', () => {
const q = CQL`SELECT from bookshop.Intermediate {
ID
} group by toAssocWithStructuredKey.toStructuredKey.second`

const qx = CQL`SELECT from bookshop.Intermediate as Intermediate
left join bookshop.AssocWithStructuredKey as toAssocWithStructuredKey
on toAssocWithStructuredKey.ID = Intermediate.toAssocWithStructuredKey_ID
{
Intermediate.ID
} group by toAssocWithStructuredKey.toStructuredKey_second`
const res = cqn4sql(q, model)
expect(JSON.parse(JSON.stringify(res))).to.deep.equal(qx)
})
// TODO: currently no-op in runtime (those structured elements are flat), however this should work
it.skip('if only partial, structured foreign key is accessed, only the requested key is flattened', () => {
const q = CQL`SELECT from bookshop.Intermediate {
ID
} group by toAssocWithStructuredKey.toStructuredKey.struct.mid.leaf`

const qx = CQL`SELECT from bookshop.Intermediate as Intermediate
left join bookshop.AssocWithStructuredKey as toAssocWithStructuredKey
on toAssocWithStructuredKey.ID = Intermediate.toAssocWithStructuredKey_ID
{
Intermediate.ID
} group by toAssocWithStructuredKey.toStructuredKey_struct_mid_leaf`
const res = cqn4sql(q, model)
expect(JSON.parse(JSON.stringify(res))).to.deep.equal(qx)
})

it('rejects managed associations in expressions in GROUP BY clause (1)', () => {
expect(() => cqn4sql(CQL`SELECT from bookshop.Books { ID } GROUP BY 2*author`, model)).to.throw(
/An association can't be used as a value in an expression/,
Expand Down
Loading