mirror of
https://github.com/twentyhq/twenty.git
synced 2026-06-11 17:37:18 -04:00
fix(metadata): nestjs-query batched relation queries truncate results across parents (#21455)
## TL;DR
The metadata API silently drops relation rows whenever a batched
relation query exceeds the requested page size. A dev-seeded workspace
already has **558 fieldMetadata rows across 31 objects**, so
`objects(paging:{first:50}) { fields(paging:{first:500}) }` executes:
```sql
SELECT DISTINCT ... FROM core."fieldMetadata" fields
WHERE workspaceId = $1 AND objectMetadataId IN (...31 ids...)
LIMIT 501 OFFSET 0 -- no ORDER BY
```
…and returns exactly **501 of 558** fields — ~57 rows dropped, and
*which object loses which field is scan-order-dependent*. This is what
made `example-app-postcard` CI flap with "PostCard object missing field
X" (different X per run).
## Root cause
`@ptc-org/nestjs-query-typeorm`'s `batchQueryRelations` (the DataLoader
batch path behind every `@CursorConnection`) applies the **per-parent**
page size as a **single global LIMIT** on the batched query, then groups
rows per parent in memory. Any batch whose combined relation rows exceed
`first + 1` truncates arbitrary parents. This affects production
metadata reads, not just CI — any workspace with enough fields/objects
loses rows in `objects.fields`-style connections.
## Fix
Yarn patch on `@ptc-org/nestjs-query-typeorm@9.4.0` (same vehicle as the
existing `nestjs-query-graphql` patch):
- `RelationQueryBuilder.batchSelect`: only apply LIMIT/OFFSET when the
batch has a **single parent**; multi-parent batches stay **bounded**
with `parents × (offset + limit)` — the upper bound a correct per-parent
pager can ever need, so it cannot wrongly truncate while still guarding
against unbounded fetches on high-cardinality relations;
- `batchQueryRelations`: enforce paging **per parent** by slicing after
`mapRelations` (preserves the `first + 1` hasNextPage probe semantics).
## Verification
- On a frozen repro DB (postcard installed, 558 fields): unpatched
returns 501 fields with `postCard` missing `deliveredAt`; patched
returns **558/558** with the full `postCard` field set. Reproduced
identically on typeorm 0.3.20 and 0.3.26 — pre-existing bug, **not** a
typeorm regression (this unblocks the typeorm upgrade that was reverted
from #21448).
- Postcard install/uninstall stress loop: unpatched fails within 1–2
iterations; patched **12/12 green**.
- `npx nx typecheck twenty-server` clean, full `twenty-server` unit
suite green (5651 passed).
## Related
#21435 chases the **same CI symptom** (postcard randomly missing a
freshly synced field) at a different layer — a workspace-cache write
racing invalidation. The two are complementary: the repro behind this PR
survives a **cold server restart + `redis-cli FLUSHALL`** with all rows
intact in Postgres, which no cache race can explain — the truncation
happens on the DB read itself (`LIMIT 501` over 558 matching rows,
captured via `log_statement=all`). Both fixes are likely needed for the
postcard job to be fully reliable.
## Notes
Worth upstreaming to `@ptc-org/nestjs-query` eventually; the proper
upstream fix is per-parent windowed pagination (`ROW_NUMBER() OVER
(PARTITION BY parentId)`), but the in-memory per-parent slice is correct
and proportionate at metadata-API scale.
This commit is contained in:
@@ -0,0 +1,47 @@
|
||||
diff --git a/src/query/relation-query.builder.js b/src/query/relation-query.builder.js
|
||||
index 36deb6bd2758539241262e386da6ebed3b6cc838..d204807419cd641daf5cdf1ba408d48fd87a1cb2 100644
|
||||
--- a/src/query/relation-query.builder.js
|
||||
+++ b/src/query/relation-query.builder.js
|
||||
@@ -38,7 +38,20 @@ class RelationQueryBuilder {
|
||||
qb = this.filterQueryBuilder.applyRelationJoinsRecursive(qb, this.filterQueryBuilder.getReferencedRelationsWithAliasRecursive(this.relationRepo.metadata, query.filter, query.relations), query.relations);
|
||||
qb = this.filterQueryBuilder.applyFilter(qb, query.filter, qb.alias);
|
||||
qb = this.filterQueryBuilder.applySorting(qb, query.sorting, qb.alias);
|
||||
- qb = this.filterQueryBuilder.applyPaging(qb, query.paging);
|
||||
+ // Paging is per-parent, but this query is batched across all parents:
|
||||
+ // applying LIMIT here truncates the combined result set (and with no
|
||||
+ // ORDER BY, arbitrary parents silently lose rows). Only apply it when
|
||||
+ // the batch has a single parent; multi-parent batches are paged
|
||||
+ // per-parent in batchQueryRelations after mapRelations. The fetch
|
||||
+ // stays bounded with parents * (offset + limit) — the upper bound a
|
||||
+ // correct per-parent pager can need; a SQL-side per-parent quota
|
||||
+ // (ROW_NUMBER() OVER (PARTITION BY parent)) belongs upstream.
|
||||
+ if (entities.length === 1) {
|
||||
+ qb = this.filterQueryBuilder.applyPaging(qb, query.paging);
|
||||
+ }
|
||||
+ else if (query.paging?.limit !== undefined) {
|
||||
+ qb = qb.limit(entities.length * ((query.paging.offset ?? 0) + query.paging.limit));
|
||||
+ }
|
||||
if (this.relationRepo.metadata.deleteDateColumn?.propertyName && !withDeleted) {
|
||||
qb = qb.andWhere(`${qb.alias}.${this.relationRepo.metadata.deleteDateColumn.propertyName} IS NULL`);
|
||||
}
|
||||
diff --git a/src/services/relation-query.service.js b/src/services/relation-query.service.js
|
||||
index 91f7b4a83a5e0cdcd1b23b52fdf2fe6c587f7035..d36b7c3c06d5d76f86ba5fdd42b3be49a9dc4b8a 100644
|
||||
--- a/src/services/relation-query.service.js
|
||||
+++ b/src/services/relation-query.service.js
|
||||
@@ -167,7 +167,15 @@ class RelationQueryService {
|
||||
const results = new Map();
|
||||
for (const entity of entities) {
|
||||
const relations = relationQueryBuilder.relationMeta.mapRelations(entity, entityRelations.entities, entityRelations.raw);
|
||||
- results.set(entity, await assembler.convertToDTOs(relations));
|
||||
+ // batchSelect skips LIMIT/OFFSET for multi-parent batches (a global
|
||||
+ // limit truncates arbitrary parents), so enforce paging per-parent.
|
||||
+ const paging = convertedQuery.paging;
|
||||
+ const pagedRelations = paging && entities.length > 1
|
||||
+ ? relations.slice(paging.offset ?? 0, paging.limit !== undefined
|
||||
+ ? (paging.offset ?? 0) + paging.limit
|
||||
+ : undefined)
|
||||
+ : relations;
|
||||
+ results.set(entity, await assembler.convertToDTOs(pagedRelations));
|
||||
}
|
||||
return results;
|
||||
}
|
||||
@@ -69,7 +69,7 @@
|
||||
"@opentelemetry/sdk-metrics": "^2.0.0",
|
||||
"@ptc-org/nestjs-query-core": "^9.4.0",
|
||||
"@ptc-org/nestjs-query-graphql": "patch:@ptc-org/nestjs-query-graphql@npm%3A9.4.0#~/.yarn/patches/@ptc-org-nestjs-query-graphql-npm-9.4.0-8e6f7894e1.patch",
|
||||
"@ptc-org/nestjs-query-typeorm": "^9.4.0",
|
||||
"@ptc-org/nestjs-query-typeorm": "patch:@ptc-org/nestjs-query-typeorm@npm%3A9.4.0#~/.yarn/patches/@ptc-org-nestjs-query-typeorm-npm-9.4.0-ca3414967e.patch",
|
||||
"@react-email/render": "^1.2.3",
|
||||
"@sentry/nestjs": "^10.51.0",
|
||||
"@sentry/node": "^10.51.0",
|
||||
|
||||
25
yarn.lock
25
yarn.lock
@@ -16626,7 +16626,7 @@ __metadata:
|
||||
languageName: node
|
||||
linkType: hard
|
||||
|
||||
"@ptc-org/nestjs-query-typeorm@npm:^9.4.0":
|
||||
"@ptc-org/nestjs-query-typeorm@npm:9.4.0":
|
||||
version: 9.4.0
|
||||
resolution: "@ptc-org/nestjs-query-typeorm@npm:9.4.0"
|
||||
dependencies:
|
||||
@@ -16647,6 +16647,27 @@ __metadata:
|
||||
languageName: node
|
||||
linkType: hard
|
||||
|
||||
"@ptc-org/nestjs-query-typeorm@patch:@ptc-org/nestjs-query-typeorm@npm%3A9.4.0#~/.yarn/patches/@ptc-org-nestjs-query-typeorm-npm-9.4.0-ca3414967e.patch":
|
||||
version: 9.4.0
|
||||
resolution: "@ptc-org/nestjs-query-typeorm@patch:@ptc-org/nestjs-query-typeorm@npm%3A9.4.0#~/.yarn/patches/@ptc-org-nestjs-query-typeorm-npm-9.4.0-ca3414967e.patch::version=9.4.0&hash=c30501"
|
||||
dependencies:
|
||||
"@ptc-org/nestjs-query-core": "npm:9.4.0"
|
||||
camel-case: "npm:^4.1.2"
|
||||
lodash.filter: "npm:^4.6.0"
|
||||
lodash.merge: "npm:^4.6.2"
|
||||
lodash.omit: "npm:^4.5.0"
|
||||
reflect-metadata: "npm:0.2.2"
|
||||
tslib: "npm:^2.8.1"
|
||||
uuid: "npm:^10.0.0"
|
||||
peerDependencies:
|
||||
"@nestjs/common": ^9.0.0 || ^10.0.0 || ^11.0.0
|
||||
"@nestjs/typeorm": ^9.0.0 || ^10.0.0 || ^11.0.0
|
||||
class-transformer: ^0.5
|
||||
typeorm: ^0.3.15
|
||||
checksum: 10c0/93c81d7a787992a227be643ee03d211fabda9ecd30ffb0116c4d60244cde666c096ad7330d8969e1390d24f876e51ee11545033ea6e8b41cf656defe94c28899
|
||||
languageName: node
|
||||
linkType: hard
|
||||
|
||||
"@puppeteer/browsers@npm:2.3.0":
|
||||
version: 2.3.0
|
||||
resolution: "@puppeteer/browsers@npm:2.3.0"
|
||||
@@ -55079,7 +55100,7 @@ __metadata:
|
||||
"@opentelemetry/sdk-metrics": "npm:^2.0.0"
|
||||
"@ptc-org/nestjs-query-core": "npm:^9.4.0"
|
||||
"@ptc-org/nestjs-query-graphql": "patch:@ptc-org/nestjs-query-graphql@npm%3A9.4.0#~/.yarn/patches/@ptc-org-nestjs-query-graphql-npm-9.4.0-8e6f7894e1.patch"
|
||||
"@ptc-org/nestjs-query-typeorm": "npm:^9.4.0"
|
||||
"@ptc-org/nestjs-query-typeorm": "patch:@ptc-org/nestjs-query-typeorm@npm%3A9.4.0#~/.yarn/patches/@ptc-org-nestjs-query-typeorm-npm-9.4.0-ca3414967e.patch"
|
||||
"@react-email/render": "npm:^1.2.3"
|
||||
"@sentry/nestjs": "npm:^10.51.0"
|
||||
"@sentry/node": "npm:^10.51.0"
|
||||
|
||||
Reference in New Issue
Block a user