fix(order by): reject non-fk traversals of own columns in order by

db-service/lib/infer/index.js

@@ -334,17 +334,38 @@ function infer(originalQuery, model) {
       // link $refLinks -> special name resolution rules for orderBy
       orderBy.forEach(token => {
         let $baseLink
+        let rejectJoinRelevantPath
         // first check if token ref is resolvable in query elements
         if (columns) {
-          const e = queryElements[token.ref?.[0]]
-          const isAssocExpand = e?.$assocExpand // expand on structure can be addressed
-          if (e && !isAssocExpand) $baseLink = { definition: { elements: queryElements }, target: inferred }
+          const firstStep = token.ref?.[0].id || token.ref?.[0]
+          const tokenPointsToQueryElements = columns.some(c => {
+            const columnName = c.as || c.flatName || c.ref?.at(-1).id || c.ref?.at(-1) || c.func
+            return columnName === firstStep
+          })
+          const needsElementsOfQueryAsBase =
+            tokenPointsToQueryElements &&
+            queryElements[token.ref?.[0]] &&
+            /* expand on structure can be addressed */ !queryElements[token.ref?.[0]].$assocExpand
+
+          // if the ref points into the query itself and follows an exposed association
+          // to a non-fk column, we must reject the ref, as we can't join with the queries own results
+          rejectJoinRelevantPath = needsElementsOfQueryAsBase
+          if (needsElementsOfQueryAsBase) $baseLink = { definition: { elements: queryElements }, target: inferred }
         } else {
           // fallback to elements of query source
           $baseLink = null
         }
 
         inferQueryElement(token, false, $baseLink)
+        if (token.isJoinRelevant && rejectJoinRelevantPath) {
+          // reverse the array, find the last association and calculate the index of the association in non-reversed order
+          const assocIndex =
+            token.$refLinks.length - 1 - token.$refLinks.reverse().findIndex(link => link.definition.isAssociation)
+
+          throw new Error(
+            `Can follow managed association “${token.ref[assocIndex].id || token.ref[assocIndex]}” only to the keys of its target, not to “${token.ref[assocIndex + 1].id || token.ref[assocIndex + 1]}”`,
+          )
+        }
       })
     }
 
@@ -467,7 +488,7 @@ function infer(originalQuery, model) {
      */
 
     function inferQueryElement(column, insertIntoQueryElements = true, $baseLink = null, context) {
-      const { inExists, inExpr, inNestedProjection, inCalcElement, baseColumn } = context || {}
+      const { inExists, inExpr, inCalcElement, baseColumn, inInfixFilter } = context || {}
       if (column.param || column.SELECT) return // parameter references are only resolved into values on execution e.g. :val, :1 or ?
       if (column.args) column.args.forEach(arg => inferQueryElement(arg, false, $baseLink, context)) // e.g. function in expression
       if (column.list) column.list.forEach(arg => inferQueryElement(arg, false, $baseLink, context))
@@ -604,10 +625,15 @@ function infer(originalQuery, model) {
               inferQueryElement(token, false, column.$refLinks[i], {
                 inExists: skipJoinsForFilter,
                 inExpr: !!token.xpr,
+                inInfixFilter: true,
               })
             } else if (token.func) {
               token.args?.forEach(arg =>
-                inferQueryElement(arg, false, column.$refLinks[i], { inExists: skipJoinsForFilter, inExpr: true }),
+                inferQueryElement(arg, false, column.$refLinks[i], {
+                  inExists: skipJoinsForFilter,
+                  inExpr: true,
+                  inInfixFilter: true,
+                }),
               )
             }
           })
@@ -668,7 +694,7 @@ function infer(originalQuery, model) {
          * @param {CSN.Element} assoc if this is an association, the next step must be a foreign key of the element.
          */
         function rejectNonFkAccess(assoc) {
-          if (!inNestedProjection && !inCalcElement && assoc.target) {
+          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
@@ -678,7 +704,7 @@ function infer(originalQuery, model) {
               )
             // 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`)
+              throw new Error(`Only foreign keys of "${assoc.name}" can be accessed in infix filter, not "${nextStep}"`)
           }
         }
       })
@@ -727,12 +753,14 @@ function infer(originalQuery, model) {
       function resolveInline(col, namePrefix = col.as || col.flatName) {
         const { inline, $refLinks } = col
         const $leafLink = $refLinks[$refLinks.length - 1]
-        if(!$leafLink.definition.target && !$leafLink.definition.elements) {
-          throw new Error(`Unexpected “inline” on “${col.ref.map(idOnly)}”; can only be used after a reference to a structure, association or table alias`)
+        if (!$leafLink.definition.target && !$leafLink.definition.elements) {
+          throw new Error(
+            `Unexpected “inline” on “${col.ref.map(idOnly)}”; can only be used after a reference to a structure, association or table alias`,
+          )
         }
         let elements = {}
         inline.forEach(inlineCol => {
-          inferQueryElement(inlineCol, false, $leafLink, { inExpr: true, inNestedProjection: true, baseColumn: col })
+          inferQueryElement(inlineCol, false, $leafLink, { inExpr: true, baseColumn: col })
           if (inlineCol === '*') {
             const wildCardElements = {}
             // either the `.elements´ of the struct or the `.elements` of the assoc target
@@ -783,8 +811,10 @@ function infer(originalQuery, model) {
       function resolveExpand(col) {
         const { expand, $refLinks } = col
         const $leafLink = $refLinks?.[$refLinks.length - 1] || inferred.SELECT.from.$refLinks.at(-1) // fallback to anonymous expand
-        if(!$leafLink.definition.target && !$leafLink.definition.elements) {
-          throw new Error(`Unexpected “expand” on “${col.ref.map(idOnly)}”; can only be used after a reference to a structure, association or table alias`)
+        if (!$leafLink.definition.target && !$leafLink.definition.elements) {
+          throw new Error(
+            `Unexpected “expand” on “${col.ref.map(idOnly)}”; can only be used after a reference to a structure, association or table alias`,
+          )
         }
         const target = getDefinition($leafLink.definition.target)
         if (target) {
@@ -807,7 +837,7 @@ function infer(originalQuery, model) {
             if (e === '*') {
               elements = { ...elements, ...$leafLink.definition.elements }
             } else {
-              inferQueryElement(e, false, $leafLink, { inExpr: true, inNestedProjection: true })
+              inferQueryElement(e, false, $leafLink, { inExpr: true })
               if (e.expand) elements[e.as || e.flatName] = resolveExpand(e)
               if (e.inline) elements = { ...elements, ...resolveInline(e) }
               else elements[e.as || e.flatName] = e.$refLinks ? e.$refLinks[e.$refLinks.length - 1].definition : e

db-service/test/cds-infer/negative.test.js

@@ -368,23 +368,23 @@ describe('negative', () => {
 
     it('expand on `.items` not possible', () => {
       expect(() => _inferred(CQL`SELECT from bookshop.SoccerPlayers { name, emails { address } }`, model)).to.throw(
-        "Unexpected “expand” on “emails”; can only be used after a reference to a structure, association or table alias"
+        'Unexpected “expand” on “emails”; can only be used after a reference to a structure, association or table alias',
       )
     })
     it('expand on scalar not possible', () => {
       expect(() => _inferred(CQL`SELECT from bookshop.SoccerPlayers { name { address } }`, model)).to.throw(
-        "Unexpected “expand” on “name”; can only be used after a reference to a structure, association or table alias"
+        'Unexpected “expand” on “name”; can only be used after a reference to a structure, association or table alias',
       )
     })
 
     it('inline on `.items` not possible', () => {
       expect(() => _inferred(CQL`SELECT from bookshop.SoccerPlayers { name, emails.{ address } }`, model)).to.throw(
-        "Unexpected “inline” on “emails”; can only be used after a reference to a structure, association or table alias"
+        'Unexpected “inline” on “emails”; can only be used after a reference to a structure, association or table alias',
       )
     })
     it('inline on scalar not possible', () => {
       expect(() => _inferred(CQL`SELECT from bookshop.SoccerPlayers { name.{ address } }`, model)).to.throw(
-        "Unexpected “inline” on “name”; can only be used after a reference to a structure, association or table alias"
+        'Unexpected “inline” on “name”; can only be used after a reference to a structure, association or table alias',
       )
     })
   })
@@ -406,4 +406,20 @@ describe('negative', () => {
       ).to.throw(/Only foreign keys of "author" can be accessed in infix filter/)
     })
   })
+
+  describe('order by', () => {
+    it('reject join relevant path via queries own columns', () => {
+      let query = CQL`SELECT from bookshop.Books  {
+        ID,
+        author,
+        coAuthor as co
+      }
+      order by
+        Books.author,
+        co.name`
+      expect(() => {
+        cqn4sql(query, model)
+      }).to.throw(/Can follow managed association “co” only to the keys of its target, not to “name”/)
+    })
+  })
 })

db-service/test/cqn4sql/flattening.test.js

@@ -651,6 +651,27 @@ describe('Flattening', () => {
         co_ID
       `)
     })
+    it('same as above but navigation to foreign key in order by', () => {
+      let query = cqn4sql(
+        CQL`SELECT from bookshop.Books  {
+          ID,
+          author,
+          coAuthor as co
+        }
+        order by
+          Books.author,
+          co.ID`,
+        model,
+      )
+      expect(query).to.deep.equal(CQL`SELECT from bookshop.Books as Books {
+        Books.ID,
+        Books.author_ID,
+        Books.coAuthor_ID as co_ID
+      } order by
+        Books.author_ID,
+        co_ID
+      `)
+    })
 
     it('rejects managed association with multiple FKs in ORDER BY clause', () => {
       expect(() =>
@@ -834,5 +855,4 @@ describe('Flattening', () => {
       )
     })
   })
-
 })

db-service/test/cqn4sql/table-alias.test.js

@@ -545,6 +545,23 @@ describe('table alias access', () => {
       let query = cqn4sql(input, model)
       expect(query).to.deep.equal(CQL`SELECT from bookshop.Books as Books { func() as func } order by func`)
     })
+
+    it('do not try to resolve ref in columns if columns consists of star', () => {
+      let input = CQL`SELECT from bookshop.SimpleBook { * } order by author.name`
+      let query = cqn4sql(input, model)
+      const expected = CQL`SELECT from bookshop.SimpleBook as SimpleBook left join bookshop.Authors as author on author.ID = SimpleBook.author_ID
+      { SimpleBook.ID, SimpleBook.title, SimpleBook.author_ID } order by author.name`
+      expect(query).to.deep.equal(expected)
+    })
+    // doesnt work, can't join with the query source itself
+    it.skip('same as above but author is explicit column', () => {
+      let input = CQL`SELECT from bookshop.SimpleBook { *, author } order by author.name`
+      let query = cqn4sql(input, model)
+      const expected = CQL`SELECT from bookshop.SimpleBook as SimpleBook left join bookshop.Authors as author on author.ID = SimpleBook.author_ID
+      { SimpleBook.ID, SimpleBook.title, SimpleBook.author_ID } order by author.name`
+      expect(query).to.deep.equal(expected)
+    })
+
   })
 
   describe('replace usage of implicit aliases in subqueries', () => {