Bug: GroupBy using column alias not working with SQL server

[Flusinerd]

Describe the bug
GroupBy does not work with aggregate queries, because of invalid column name, when using SQL-Server.

Have you read the Contributing Guidelines?

Yes

To Reproduce
Steps to reproduce the behavior:

1. Create a DTO + Module where aggregations are enabled
2. Query the aggregate query with groupBy set to fields of the DTO
3. DB exception is thrown, because column aliases are not valid for GROUP BY clauses.

Expected behavior
Aggregate query should not fail.
It should use the column name instead of the alias.

Screenshots

Desktop (please complete the following information):

• Node Version 16
• Nestjs-query Version 2.5.0

Additional context
GraphQL Query:

query GetProjectLineVersions($projectId: ID!) {
    lineAggregate(filter: {lineProjectId: {eq: $projectId}}) {
    groupBy {
      lineVersion
      pending
    }
  }
}

NestJS Exception:

[Nest] 78461  - 07/24/2023, 11:46:59 AM   ERROR [ExceptionsHandler] Error: Invalid column name 'GROUP_BY_lineVersion'.
QueryFailedError: Error: Invalid column name 'GROUP_BY_lineVersion'.
    at QueryFailedError.TypeORMError [as constructor] (/Users/tomnissing/THDS/nav-prod-monorepo/src/error/TypeORMError.ts:7:9)

Generated SQL:

SELECT
  "Line"."lineVersion" AS "GROUP_BY_lineVersion",
  "Line"."pending" AS "GROUP_BY_pending"
FROM
  "Lines" "Line"
WHERE
  (("Line"."lineProjectId" = '9B5D7BF2-D46E-EC11-9462-005056A2678B'))
  AND ("Line"."deletedAt" IS NULL)
GROUP BY
  "GROUP_BY_lineVersion",
  "GROUP_BY_pending"
ORDER BY
  "GROUP_BY_lineVersion" ASC,
  "GROUP_BY_pending" ASC

SQL that works:

SELECT
  "Line"."lineVersion" AS "GROUP_BY_lineVersion",
  "Line"."pending" AS "GROUP_BY_pending"
FROM
  "Lines" "Line"
WHERE
  (("Line"."lineProjectId" = '9B5D7BF2-D46E-EC11-9462-005056A2678B'))
  AND ("Line"."deletedAt" IS NULL)
GROUP BY
  lineVersion,
  pending
ORDER BY
  "GROUP_BY_lineVersion" ASC,
  "GROUP_BY_pending" ASC

[TriPSs]

You also have this issue if you also provide a count / sum?

[Flusinerd]

I'll be able to verify that tomorrow.

The problem is, that the SELECT gets executed after the group by. Therefore the alias is non existing at the time of the GROUP BY

Source: http://tinman.cs.gsu.edu/~raj/sql/node22.html

We just recently started transitioning from the old nestjs-query package to this.
This query used to work, so its some kind of regression.

[TriPSs]

Will see if I can try to reproduce this, there is not anything special that you are doing? As, in my own projects everything works and all tests also passed.

[Flusinerd]

No, not really. I have a custom typeorm service that extends from the libraries typeorm service, but that only overrides the update / create methods, the get methods are all inherited.

We are using Microsoft SQL Server 2019

[Flusinerd]

This is the resolvers definition:

resolvers: [
    {
      DTOClass: LineDto,
      UpdateDTOClass: UpdateLineDto,
      ServiceClass: LinesRelationQueryService,
      EntityClass: Line,
      enableAggregate: true,
      delete: { disabled: true },
      create: { disabled: true },
      guards,
    },
 ]

The DTO:

export class LineDto extends ResourceDto {
  @FilterableField(() => Int)
  lineVersion: number;

  @FilterableField({ nullable: false })
  pending: boolean;
  
  ...
}

[TriPSs]

I think the reason for this change was to support grouping by week/day/month, commit: 7ffeaf6, we can check to update this to the old behaviour for non date fields.

[TriPSs]

@Flusinerd would it be possible for you to tryout something?

Could you update src/query/aggregate.builder.ts the constructor to always set this.isPostgres = true, I think it should work than. If this is the case we can add SQL server there to.

[Flusinerd]

Hi, ty for coming back to this. I'll give it a try on Monday 😉

[TriPSs]

Sorry after checking a bit more the change is not going to make a difference.

I do now see the change what caused it, when implementing the grouping of dates (DAY, WEEK etc) we also changed how the group by is done so that it references the alias and we only needed the date logic on one place.

The fix could be that we just update that logic to not use the alias if the field is not a date field, but than the grouping of dates will not work for SQL server, or we update the complete logic to instead of using the alias for date and normal fields we use the normal fields and the query of the date for date fields as group by, this will than work for all languages.

What do you think?