fix: add missing LEFT JOIN for relation fields in groupBy orderByForRecords (#18798)
## Summary
Fixes "missing FROM-clause entry for table" SQL error when using
`orderByForRecords` with a relation field (e.g. `{ company: { name:
"AscNullsFirst" } }`) in a `groupBy` query.
### Root cause
This is a regression from #18005 (Feb 17). That PR correctly removed a
duplicate `applyOrderToBuilder()` call on the groupBy subquery (which
was conflicting with the `ROW_NUMBER() OVER (... ORDER BY ...)` window
function), but in doing so it also removed the LEFT JOINs that
`applyOrderToBuilder` was adding for relation fields.
After that change, ordering relied solely on `getOrderByRawSQL()` inside
`applyPartitionByToBuilder()`, which builds raw SQL for the window
function but never added the required JOINs. Scalar field ordering (e.g.
`name`, `position`) kept working since those don't need JOINs, but
relation field ordering (e.g. `company.name`) broke.
### Fix
- `getOrderByRawSQL` now returns `relationJoins` alongside the SQL
string so callers get the join info they need
- `applyPartitionByToBuilder` in `GroupByWithRecordsService` adds the
required LEFT JOINs before building the `ROW_NUMBER()` window function,
mirroring the pattern used by `applyOrderToBuilder` in the `findMany`
path
## Test plan
- [x] Manually tested locally with a GraphQL query matching the
customer's failing pattern (`orderByForRecords: [{ company: { name:
"AscNullsFirst" } }]`)
- [x] Added integration tests for ascending and descending relation
field ordering in `group-by-with-records-resolver.integration-spec.ts`
- [x] Typecheck passes
- [x] Lint passes
- [x] CI green
This commit is contained in:
+4
-3
@@ -13,6 +13,7 @@ import { GraphqlQueryOrderGroupByParser } from 'src/engine/api/graphql/graphql-q
|
||||
import {
|
||||
GraphqlQueryOrderFieldParser,
|
||||
type OrderByClause,
|
||||
type RelationJoinInfo,
|
||||
} from 'src/engine/api/graphql/graphql-query-runner/graphql-query-parsers/graphql-query-order/graphql-query-order.parser';
|
||||
import {
|
||||
GraphqlQuerySelectedFieldsParser,
|
||||
@@ -172,7 +173,7 @@ export class GraphqlQueryParser {
|
||||
orderBy: ObjectRecordOrderBy | OrderByWithGroupBy,
|
||||
objectNameSingular: string,
|
||||
isForwardPagination = true,
|
||||
): string {
|
||||
): { orderByRawSQL: string; relationJoins: RelationJoinInfo[] } {
|
||||
const parseResult = this.orderFieldParser.parse(
|
||||
orderBy as ObjectRecordOrderBy,
|
||||
objectNameSingular,
|
||||
@@ -208,11 +209,11 @@ export class GraphqlQueryParser {
|
||||
|
||||
const orderByRawSQLString = orderByRawSQLClauseArray.join(', ');
|
||||
|
||||
const orderByCompleteSQLClause = isNonEmptyString(orderByRawSQLString)
|
||||
const orderByRawSQL = isNonEmptyString(orderByRawSQLString)
|
||||
? `ORDER BY ${orderByRawSQLString}`
|
||||
: '';
|
||||
|
||||
return orderByCompleteSQLClause;
|
||||
return { orderByRawSQL, relationJoins: parseResult.relationJoins };
|
||||
}
|
||||
|
||||
public applyGroupByOrderToBuilder(
|
||||
|
||||
+20
-4
@@ -264,12 +264,28 @@ export class GroupByWithRecordsService {
|
||||
flatFieldMetadataMaps,
|
||||
);
|
||||
|
||||
const orderByRawSQL = graphqlQueryParser.getOrderByRawSQL(
|
||||
orderByForRecords,
|
||||
flatObjectMetadata.nameSingular,
|
||||
);
|
||||
const { orderByRawSQL, relationJoins } =
|
||||
graphqlQueryParser.getOrderByRawSQL(
|
||||
orderByForRecords,
|
||||
flatObjectMetadata.nameSingular,
|
||||
);
|
||||
|
||||
if (isNonEmptyString(orderByRawSQL)) {
|
||||
const existingJoinAliases = new Set(
|
||||
queryBuilder.expressionMap.joinAttributes.map(
|
||||
(joinAttribute) => joinAttribute.alias.name,
|
||||
),
|
||||
);
|
||||
|
||||
for (const joinInfo of relationJoins) {
|
||||
if (!existingJoinAliases.has(joinInfo.joinAlias)) {
|
||||
queryBuilder.leftJoin(
|
||||
`${flatObjectMetadata.nameSingular}.${joinInfo.joinAlias}`,
|
||||
joinInfo.joinAlias,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
return queryBuilder.addSelect(
|
||||
`ROW_NUMBER() OVER (PARTITION BY ${groupByExpressions} ${orderByRawSQL})`,
|
||||
'record_row_number',
|
||||
|
||||
+133
@@ -565,5 +565,138 @@ describe('basic group-by with records', () => {
|
||||
expect(thursdayNewGroup.edges[0].node.name).toBe('Opportunity 3');
|
||||
expect(thursdayNewGroup.edges[1].node.name).toBe('Opportunity 2');
|
||||
});
|
||||
|
||||
it('sorts by relation field (company name) in ascending order', async () => {
|
||||
const response = await makeGraphqlAPIRequest({
|
||||
query: gql`
|
||||
query OpportunitiesGroupBy(
|
||||
$groupBy: [OpportunityGroupByInput!]!
|
||||
$filter: OpportunityFilterInput
|
||||
$orderByForRecords: [OpportunityOrderByInput!]
|
||||
$limit: Int
|
||||
) {
|
||||
opportunitiesGroupBy(
|
||||
groupBy: $groupBy
|
||||
filter: $filter
|
||||
orderByForRecords: $orderByForRecords
|
||||
limit: $limit
|
||||
) {
|
||||
groupByDimensionValues
|
||||
__typename
|
||||
edges {
|
||||
node {
|
||||
stage
|
||||
name
|
||||
company {
|
||||
id
|
||||
name
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
`,
|
||||
variables: {
|
||||
groupBy: [
|
||||
{
|
||||
stage: true,
|
||||
},
|
||||
],
|
||||
orderByForRecords: [
|
||||
{
|
||||
company: {
|
||||
name: 'AscNullsFirst',
|
||||
},
|
||||
},
|
||||
],
|
||||
filter: FILTER_2020,
|
||||
limit: 20,
|
||||
},
|
||||
});
|
||||
|
||||
expect(response.body.errors).toBeUndefined();
|
||||
expect(response.body.data).toBeDefined();
|
||||
|
||||
const groups = response.body.data.opportunitiesGroupBy;
|
||||
|
||||
const newGroup = groups.find((group: any) =>
|
||||
group.groupByDimensionValues.includes('NEW'),
|
||||
);
|
||||
|
||||
expect(newGroup).toBeDefined();
|
||||
expect(newGroup.edges).toHaveLength(3);
|
||||
|
||||
// Opp 1 and 2 belong to Company 1, Opp 3 to Company 2
|
||||
// Ascending by company name: Company 1 < Company 2
|
||||
expect(newGroup.edges[0].node.company.name).toBe('Company 1');
|
||||
expect(newGroup.edges[1].node.company.name).toBe('Company 1');
|
||||
expect(newGroup.edges[2].node.company.name).toBe('Company 2');
|
||||
});
|
||||
|
||||
it('sorts by relation field (company name) in descending order', async () => {
|
||||
const response = await makeGraphqlAPIRequest({
|
||||
query: gql`
|
||||
query OpportunitiesGroupBy(
|
||||
$groupBy: [OpportunityGroupByInput!]!
|
||||
$filter: OpportunityFilterInput
|
||||
$orderByForRecords: [OpportunityOrderByInput!]
|
||||
$limit: Int
|
||||
) {
|
||||
opportunitiesGroupBy(
|
||||
groupBy: $groupBy
|
||||
filter: $filter
|
||||
orderByForRecords: $orderByForRecords
|
||||
limit: $limit
|
||||
) {
|
||||
groupByDimensionValues
|
||||
__typename
|
||||
edges {
|
||||
node {
|
||||
stage
|
||||
name
|
||||
company {
|
||||
id
|
||||
name
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
`,
|
||||
variables: {
|
||||
groupBy: [
|
||||
{
|
||||
stage: true,
|
||||
},
|
||||
],
|
||||
orderByForRecords: [
|
||||
{
|
||||
company: {
|
||||
name: 'DescNullsLast',
|
||||
},
|
||||
},
|
||||
],
|
||||
filter: FILTER_2020,
|
||||
limit: 20,
|
||||
},
|
||||
});
|
||||
|
||||
expect(response.body.errors).toBeUndefined();
|
||||
expect(response.body.data).toBeDefined();
|
||||
|
||||
const groups = response.body.data.opportunitiesGroupBy;
|
||||
|
||||
const newGroup = groups.find((group: any) =>
|
||||
group.groupByDimensionValues.includes('NEW'),
|
||||
);
|
||||
|
||||
expect(newGroup).toBeDefined();
|
||||
expect(newGroup.edges).toHaveLength(3);
|
||||
|
||||
// Descending by company name: Company 2 > Company 1
|
||||
expect(newGroup.edges[0].node.company.name).toBe('Company 2');
|
||||
expect(newGroup.edges[1].node.company.name).toBe('Company 1');
|
||||
expect(newGroup.edges[2].node.company.name).toBe('Company 1');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user