Conversation
packages/twenty-server/src/engine/workspace-event-emitter/workspace-event-emitter.service.ts
Outdated
Show resolved
Hide resolved
Greptile OverviewGreptile SummaryThis PR enhances event payloads with nested relation data to match frontend query patterns. Previously, mutation results only included depth-0 fields; now events include depth-1 relations with label/image identifier fields for lighter payloads. Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant WEE as WorkspaceEventEmitter
participant WEES as WorkspaceEventEmitterService
participant RCSFH as RestToCommonSelectedFieldsHandler
participant GASF as getAllSelectableFields
participant PNRH as ProcessNestedRelationsHelper
participant DB as Database
WEE->>WEES: publish(workspaceEventBatch)
WEES->>WEES: publishToEventStreams()
alt Has Active Streams
WEES->>WEES: processStreamEvents()
loop For Each Event
WEES->>WEES: Filter restricted fields
WEES->>WEES: Match queries with RLS filters
end
alt Has Matched Events
WEES->>WEES: enrichEventBatchWithNestedRelations()
WEES->>RCSFH: computeFromDepth(depth=1, onlyUseLabelIdentifierFieldsInRelations=true)
RCSFH->>GASF: getAllSelectableFields(onlyUseLabelIdentifierFieldsInRelations=true)
GASF->>GASF: Filter to id, label, image identifier fields only
GASF-->>RCSFH: Filtered fields
RCSFH->>RCSFH: getRelationsAndRelationsSelectFields()
alt Is Junction Table or Activity Target
RCSFH->>RCSFH: Recurse with depth=1 for depth-2 relations
end
RCSFH-->>WEES: Selected fields structure
WEES->>PNRH: processNestedRelations(parentObjectRecords, relations)
PNRH->>DB: Query nested relations
DB-->>PNRH: Relation data
PNRH->>PNRH: Populate relations in records
PNRH-->>WEES: Enriched records
WEES->>WEES: publishToEventStream()
end
end
|
...enty-server/src/engine/api/rest/core/rest-to-common-args-handlers/selected-fields-handler.ts
Outdated
Show resolved
Hide resolved
| flatObjectMetadata.nameSingular === 'company' && | ||
| flatField.name === 'domainName' | ||
| ) { | ||
| return true; |
There was a problem hiding this comment.
I am surprised we never need it before. Looks a bit hacky TBH. Don't we have a better way to retrieve the label identifier?
There was a problem hiding this comment.
Indeed we already have it
There was a problem hiding this comment.
We have isImageIdentifierField in the frontend indeed. This could live in twenty-shared maybe
packages/twenty-server/src/engine/metadata-modules/role-target/types/user-workspace-role-map.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/workspace-event-emitter/workspace-event-emitter.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/workspace-event-emitter/workspace-event-emitter.service.ts
Outdated
Show resolved
Hide resolved
Weiko
left a comment
There was a problem hiding this comment.
Left some comments @lucasbordeau !
| flatObjectMetadata.nameSingular === 'company' && | ||
| flatField.name === 'domainName' | ||
| ) { | ||
| return true; |
There was a problem hiding this comment.
We have isImageIdentifierField in the frontend indeed. This could live in twenty-shared maybe
| import { type FlatFieldMetadata } from 'src/engine/metadata-modules/flat-field-metadata/types/flat-field-metadata.type'; | ||
| import { type FlatObjectMetadata } from 'src/engine/metadata-modules/flat-object-metadata/types/flat-object-metadata.type'; | ||
|
|
||
| export const checkIfFieldIsLabelIdentifier = ( |
There was a problem hiding this comment.
Could you check in the server where we do
flatObjectMetadata.labelIdentifierFieldMetadataId === and use that new util? 🙂
packages/twenty-server/src/engine/workspace-event-emitter/workspace-event-emitter.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/workspace-event-emitter/workspace-event-emitter.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/metadata-modules/role-target/types/user-workspace-role-map.ts
Show resolved
Hide resolved
| ...relationFieldSelectFields, | ||
| ...depth2RelationsSelectFields, | ||
| }; | ||
| } else if (flatFieldIsActivityTarget) { |
There was a problem hiding this comment.
Note: What about future many-to-many relations? I believe we are trying to achieve that very soon and the code is almost ready.
taskTarget/noteTarget are probably not migrated yet though but ideally this should be metadata driven instead of hardcoded
There was a problem hiding this comment.
Ok I will abstract this into a util for now, as it will probably be easier to refactor once we are there.
...enty-server/src/engine/api/rest/core/rest-to-common-args-handlers/selected-fields-handler.ts
Outdated
Show resolved
Hide resolved
b75f90b to
ea9329b
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances SSE (Server-Sent Events) to include related objects with their identifier fields, moving from depth-0 to depth-1 relations by default, with depth-2 for junction tables and activity targets.
Changes:
- Implemented nested relation enrichment for SSE events to match frontend query structure
- Refactored and consolidated field selection logic across REST and GraphQL APIs
- Added support for label identifier field filtering in relation queries
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/twenty-shared/src/metadata/index.ts | Exports new utilities for label and image identifier field checks |
| packages/twenty-shared/src/metadata/check-if-field-is-label-identifier.util.ts | New utility to check if a field is the label identifier |
| packages/twenty-shared/src/metadata/check-if-field-is-image-identifier.util.ts | New utility to check if a field is the image identifier |
| packages/twenty-server/src/engine/workspace-event-emitter/workspace-event-emitter.service.ts | Adds nested relations enrichment for SSE events |
| packages/twenty-server/src/engine/workspace-event-emitter/workspace-event-emitter.module.ts | Registers new dependencies for nested relation processing |
| packages/twenty-server/src/engine/workspace-event-emitter/tests/workspace-event-emitter.service.spec.ts | Comprehensive test coverage for nested relations enrichment |
| packages/twenty-server/src/engine/workspace-cache/types/workspace-cache-key.type.ts | Updates import path for UserWorkspaceRoleMap type |
| packages/twenty-server/src/engine/twenty-orm/storage/orm-workspace-context.storage.ts | Updates type reference for userWorkspaceRoleMap |
| packages/twenty-server/src/engine/twenty-orm/interfaces/workspace-internal-context.interface.ts | Updates type reference for userWorkspaceRoleMap |
| packages/twenty-server/src/engine/metadata-modules/role-target/types/user-workspace-role-map.ts | Creates shared type definition for UserWorkspaceRoleMap |
| packages/twenty-server/src/engine/metadata-modules/role-target/services/workspace-user-workspace-role-map-cache.service.ts | Imports UserWorkspaceRoleMap from shared location |
| packages/twenty-server/src/engine/core-modules/record-crud/services/common-api-context-builder.service.ts | Updates import path for getAllSelectableFields |
| packages/twenty-server/src/engine/api/rest/core/rest-to-common-args-handlers/utils/tests/get-all-selectable-fields.util.spec.ts | Removes old test file (moved) |
| packages/twenty-server/src/engine/api/rest/core/rest-to-common-args-handlers/rest-to-common-args-handlers.ts | Updates to use renamed CommonSelectFieldsHelper |
| packages/twenty-server/src/engine/api/rest/core/rest-to-common-args-handlers/tests/selected-fields-handler.spec.ts | Comprehensive tests for field selection with depth and relations |
| packages/twenty-server/src/engine/api/rest/core/handlers/rest-api-base.handler.ts | Updates to use renamed CommonSelectFieldsHelper |
| packages/twenty-server/src/engine/api/graphql/graphql-query-runner/group-by/services/group-by-with-records.service.ts | Updates import path for ProcessNestedRelationsHelper |
| packages/twenty-server/src/engine/api/graphql/graphql-query-runner/graphql-query-runner.module.ts | Updates import paths for relocated helpers |
| packages/twenty-server/src/engine/api/common/core-common-api.module.ts | Updates import paths for relocated helpers |
| packages/twenty-server/src/engine/api/common/common-select-fields/utils/get-should-recurse-into-relation.ts | New utility to determine if relation should be recursed |
| packages/twenty-server/src/engine/api/common/common-select-fields/utils/get-is-flat-field-a-junction-relation-field.ts | New utility to identify junction relation fields |
| packages/twenty-server/src/engine/api/common/common-select-fields/utils/get-is-flat-field-a-join-column.util.ts | New utility to identify join column fields |
| packages/twenty-server/src/engine/api/common/common-select-fields/utils/get-all-selectable-fields.util.ts | Enhanced with label identifier field filtering |
| packages/twenty-server/src/engine/api/common/common-select-fields/utils/tests/get-all-selectable-fields.util.spec.ts | Comprehensive tests for selectable fields logic |
| packages/twenty-server/src/engine/api/common/common-select-fields/common-select-fields-helper.ts | Renamed and enhanced with junction table handling |
| packages/twenty-server/src/engine/api/common/common-query-runners/common-base-query-runner.service.ts | Updates import path for ProcessNestedRelationsHelper |
| packages/twenty-server/src/engine/api/common/common-nested-relations-processor/process-nested-relations.helper.ts | Updates import path for V2 helper |
| packages/twenty-server/src/engine/api/common/common-args-handlers/common-query-selected-fields/common-selected-fields.handler.ts | Removed (functionality moved to common-select-fields-helper) |
| packages/twenty-front/src/modules/sse-db-event/hooks/useTriggerEventStreamCreation.ts | Improves error handling with try-catch wrapper |
| packages/twenty-front/src/modules/object-record/utils/computeOptimisticRecordFromInput.ts | Removes unnecessary error throwing for relation fields |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const isActivityRelationField = | ||
| flatField.name === 'note' || flatField.name === 'task'; |
There was a problem hiding this comment.
Hard-coded activity relation fields ('note', 'task') are duplicated across multiple utility functions. Consider extracting these to a shared constant to improve maintainability and reduce duplication.
| const flatFieldIsActivityTarget = | ||
| flatField.name === 'noteTargets' || flatField.name === 'taskTargets'; |
There was a problem hiding this comment.
Hard-coded activity target fields ('noteTargets', 'taskTargets') are duplicated. Consider extracting these to a shared constant to improve maintainability and reduce duplication.
| const flatFieldIsActivityTarget = | ||
| flatField.name === 'noteTargets' || flatField.name === 'taskTargets'; |
There was a problem hiding this comment.
Hard-coded activity target fields are duplicated across multiple files. Consider extracting these to a shared constant to improve maintainability.
| if ( | ||
| currentDepthLevelIsAJunctionTable && | ||
| recurseIntoJunctionTableRelations | ||
| ) { | ||
| const fieldIsJunctionRelation = getIsFlatFieldAJunctionRelationField({ | ||
| flatField, | ||
| }); | ||
|
|
||
| if (!fieldIsJunctionRelation) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The nested condition checks could be simplified by combining the logic. Consider checking both conditions at once or adding an early return when both conditions are true but the field is not a junction relation.
| if ( | |
| currentDepthLevelIsAJunctionTable && | |
| recurseIntoJunctionTableRelations | |
| ) { | |
| const fieldIsJunctionRelation = getIsFlatFieldAJunctionRelationField({ | |
| flatField, | |
| }); | |
| if (!fieldIsJunctionRelation) { | |
| continue; | |
| } | |
| const shouldFilterToJunctionRelations = | |
| currentDepthLevelIsAJunctionTable && recurseIntoJunctionTableRelations; | |
| if ( | |
| shouldFilterToJunctionRelations && | |
| !getIsFlatFieldAJunctionRelationField({ flatField }) | |
| ) { | |
| continue; |
| continue; | ||
| } | ||
|
|
||
| if (!isUndefined(recordInputFieldValue)) { |
There was a problem hiding this comment.
@charlesBochet I removed this because it makes no sense to check that for SSE events. I saw no problem removing it because the optimistic logic shouldn't crash if there is "too much" data.
Also this was developed while taking for granted that we have all corresponding relations in the front cache, which is not the case for SSE events.
| case 'EVENT_STREAM_ALREADY_EXISTS': { | ||
| set(shouldDestroyEventStreamState, true); | ||
| break; | ||
| const result = data as ExecutionResult<{ |
There was a problem hiding this comment.
@thomtrp I put everything into the message handler since this is the top-level handler that get called first.
- Errors can be in the
datanormal flow, so we have to handle them here. - If an error happens during processing the data, that is not caught here, the SSE client silently crashed and don't recover, it took me some time to figure this out, so I put a top-level try/catch here.
|
|
||
| @Injectable() | ||
| export class RestToCommonSelectedFieldsHandler { | ||
| export class CommonSelectFieldsHelper { |
There was a problem hiding this comment.
@Weiko For now I put it in api/common, but we could move it elsewhere (maybe in another PR though)
| @@ -0,0 +1,23 @@ | |||
| import { type Nullable } from '@/types'; | |||
|
|
|||
| export const checkIfFieldIsImageIdentifier = ( | |||
There was a problem hiding this comment.
I would replace this in twenty-front/ but in another PR to not bloat this one.
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:37494 This environment will automatically shut down when the PR is closed or after 5 hours. |
|
@thomtrp could you take over this one? :) |
|
Closing until @lucasbordeau's back from pto |
Fixes twentyhq/core-team-issues#2192
This PR implements what is necessary to re-create the query that we build on the frontend to obtain the returned object record from a mutation, but on the backend, which was only partially implemented for REST API.
Usually we want to have relations with only their id and label identifier field to have lighter payloads.
In the event we only had depth 0 fields, with this PR we have all events with depth 1 relations.
We have depth 2 for many-to-many cases, like updateOne or updateMany result :