Skip to content

Commit

Permalink
fix: only expand partial foreign key if explicitly requested (#916)
Browse files Browse the repository at this point in the history 
If someone explicitly requested only a partial foreign key (e.g. in an
expand: `toAssocWithStructuredKey { toStructuredKey { second } }`) we
must not flatten all other parts of the foreign key, but only the
requested, partial key.
  • Loading branch information
@patricebender
patricebender authored Dec 9, 2024
1 parent dd2da3a commit 96911ad
Show file tree
Hide file tree
Showing 3 changed files with 135 additions and 62 deletions.
138 changes: 77 additions & 61 deletions db-service/lib/cqn4sql.js
View file
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
View file
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
View file
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

0 comments on commit 96911ad

Please sign in to comment.