groupBy unaware of additional columns resulting from using drafts and orderBy

[gicalin]

Description of erroneous behaviour

GET query in Hana-backed capire/orders service:

• http://localhost:4006/odata/v4/orders/Orders?$apply=filter(((IsActiveEntity%20eq%20false%20or%20SiblingEntity/IsActiveEntity%20eq%20null)))/groupby((buyer),aggregate(OrderNo%20with%20count%20as%20ordersCount))&$orderby=currency_code

Unexpected result:

{
  "error": {
  "message": "invalid column name: The column '__SELECT__.HASACTIVEENTITY' is invalid in the select list because the GROUP BY clause or an aggregation function does not contain it: line 1 col 229 (at pos 228)",
  "code": "260"
  }
}

Perhaps not the best solution but this patch seems to help:

Above patch code snippets as text:

        // patch: add to groupBy additional columns that are not in the groupBy clause but in the select clause
        for (const column of transformedQuery.SELECT.columns) {
          if (column.ref && !transformedGroupBy.some(e => e.ref?.length === column.ref.length && e.ref.every((p, i) => p === column.ref[i]))) {
            transformedGroupBy.push(column)
          }
        }
        // end patch
...
        // patch: add to groupBy additional columns that are not in the groupBy clause but in the orderBy clause
        for (const column of transformedQuery.SELECT.orderBy) {
          if (column.ref && transformedQuery.SELECT.groupBy && !transformedQuery.SELECT.groupBy.some(e => e.ref?.length === column.ref.length && e.ref.every((p, i) => p === column.ref[i]))) {
            transformedQuery.SELECT.groupBy.push(column)
          }
        }
        // end patch

Detailed steps to reproduce

1. git clone https://github.com/gicalin/cloud-cap-samples -b issues/1080
2. npm install
3. cd orders && cds deploy --to hana --profile hybrid
4. cds watch --profile hybrid
5. access http://localhost:4006/odata/v4/orders/Orders?$apply=filter(((IsActiveEntity%20eq%20false%20or%20SiblingEntity/IsActiveEntity%20eq%20null)))/groupby((buyer),aggregate(OrderNo%20with%20count%20as%20ordersCount))&$orderby=currency_code

Details about your project

@capire/orders	gicalin/cloud-cap-samples/tree/issues/1080
@cap-js/asyncapi	1.0.2
@cap-js/db-service	1.16.2
@cap-js/hana	1.5.2
@cap-js/openapi	1.1.2
@sap/cds	8.8.1
@sap/cds-compiler	5.2.0
@sap/cds-dk (global)	8.7.0
@sap/cds-fiori	1.2.7
@sap/cds-foss	5.0.1
@sap/cds-mtxs	2.5.0
@sap/eslint-plugin-cds	3.1.2
Node.js	v20.18.1
home	/Users/caling/dev/cap/cloud-cap-samples/node_modules/@sap/cds

[gicalin]

Looks like the patch I proposed is incomplete since this request still fails even if the earlier one doesn't:

• http://localhost:4006/odata/v4/orders/Orders?$apply=groupby((buyer),aggregate($count%20as%20ordersCount))&$orderby=currency_code,ordersCount%20desc

To fix this failure, the second part of my proposed patch could be:

        // patch: add to groupBy additional columns that are not in the groupBy clause but in the orderBy clause
        for (const column of transformedQuery.SELECT.orderBy) {
          if (column.ref && transformedQuery.SELECT.groupBy
            && !transformedQuery.SELECT.columns.some(c => c.func && c.as === column.ref[0]) // <- this is new
            && !transformedQuery.SELECT.groupBy.some(e => e.ref?.length === column.ref.length && e.ref.every((p, i) => p === column.ref[i]))) {
            transformedQuery.SELECT.groupBy.push(column)
          }
        }
        // end patch

Feel free to propose a more thorough solution so that such requests do not fail.

Thanks in advance, Georgel

[johannes-vogel]

please check whether it works if you omit the superfluous element currency_code in your order by clause. If the element is not specified in the group by clause, it must be ignored. Obviously, that does not happen yet in the OData layer but that's nothing the DB service should cover.

[gicalin]

Hi @johannes-vogel

I've checked and the request fails with the same error even if we omit the orderBy:

• http://localhost:4006/odata/v4/orders/Orders?$apply=filter(((IsActiveEntity%20eq%20false%20or%20SiblingEntity/IsActiveEntity%20eq%20null)))/groupby((buyer),aggregate(OrderNo%20with%20count%20as%20ordersCount))

Also, I cannot find any place in the OData specification that says either that 'superfluous elements should be omitted in orderBy clauses' or that 'if the element is not specified in the group by clause then it must be ignored'. I've looked here:

• https://docs.oasis-open.org/odata/odata/v4.02/csd01/part1-protocol/odata-v4.02-csd01-part1-protocol.html#SystemQueryOptionorderby
• https://docs.oasis-open.org/odata/odata-data-aggregation-ext/v4.0/cs01/odata-data-aggregation-ext-v4.0-cs01.html#_Toc378326304

Should you believe something should change in the OData layer instead of patching @cap-js/cds-dbs, can you please forward this issue to your colleagues responsible for the OData layer?

Thanks in advance, Georgel

P.S. I raised this issue with such seemingly artificial requests because we actually ran into these kind of issues while trying to upgrade to use cds 8 with the new Hana driver in our product.