Change formatResult to return string instead of Date object for DATE_TIME#17407
Conversation
packages/twenty-server/src/engine/twenty-orm/utils/format-result.util.ts
Show resolved
Hide resolved
Greptile OverviewGreptile SummaryModified
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant QueryBuilder as WorkspaceSelectQueryBuilder
participant TypeORM
participant FormatResult as formatResult util
participant Backend
Client->>QueryBuilder: getOne() / getMany()
QueryBuilder->>TypeORM: Execute SQL query
TypeORM->>TypeORM: Transform DATE_TIME columns to Date objects
TypeORM-->>QueryBuilder: Raw result with Date objects
QueryBuilder->>FormatResult: formatResult(data, metadata)
FormatResult->>FormatResult: Filter DATE_TIME fields
FormatResult->>FormatResult: Check if value is Date object
FormatResult->>FormatResult: Convert Date to ISO string via toISOString()
FormatResult-->>QueryBuilder: Formatted result with ISO strings
QueryBuilder-->>Backend: Return consistent string datetimes
Backend-->>Client: String datetimes throughout
|
Weiko
left a comment
There was a problem hiding this comment.
LGTM
Hopefully we are not expecting those being Date objects somewhere in the codebase 🤞 (I'm pretty sure we don't)
There was a problem hiding this comment.
Pull request overview
This PR updates the formatResult utility so that DATE_TIME fields are consistently represented as ISO strings instead of Date objects, aligning backend behavior with consumers that expect string-based datetimes.
Changes:
- Added a post-processing pass in
formatResultthat locates allDATE_TIMEfields in the flat metadata and inspects corresponding values on the formatted result object. - Normalizes
DATE_TIMEfield values: leaves strings unchanged, convertsDateinstances to ISO strings, and throws an error for any other non-null/undefined type. - Keeps existing
DATEhandling intact while extending the utility’s behavior in a backward-compatible pattern with the DATE-only logic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const fieldMetadataItemsOfTypeDateTimeOnly = | ||
| getFlatFieldsFromFlatObjectMetadata( | ||
| flatObjectMetadata, | ||
| flatFieldMetadataMaps, | ||
| ).filter((field) => field.type === FieldMetadataType.DATE_TIME); | ||
|
|
||
| for (const dateTimeField of fieldMetadataItemsOfTypeDateTimeOnly) { | ||
| // @ts-expect-error legacy noImplicitAny | ||
| const rawUpdatedDateTime = newData[dateTimeField.name] as | ||
| | string | ||
| | Date | ||
| | null | ||
| | undefined; | ||
|
|
||
| if (!isDefined(rawUpdatedDateTime)) { | ||
| continue; | ||
| } | ||
|
|
||
| if (typeof rawUpdatedDateTime === 'string') { | ||
| // @ts-expect-error legacy noImplicitAny | ||
| newData[dateTimeField.name] = rawUpdatedDateTime; | ||
| } else if (rawUpdatedDateTime instanceof Date) { | ||
| const dateIsoString = rawUpdatedDateTime.toISOString(); | ||
|
|
||
| // @ts-expect-error legacy noImplicitAny | ||
| newData[dateTimeField.name] = dateIsoString; | ||
| } else { | ||
| throw new Error( | ||
| `Invalid DATE_TIME field "${dateTimeField.name}", value: "${rawUpdatedDateTime}", it should be a string or Date instance, (current type : ${typeof rawUpdatedDateTime}).`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
This new DATE_TIME normalization logic introduces several new behaviors (preserving string values, converting Date instances to ISO strings, and throwing on unexpected types) in a core utility, but there are no dedicated unit tests covering these cases. Given the existing pattern of unit tests for other Twenty ORM utils in utils/__tests__, it would be safer to add tests that (1) verify DATE_TIME fields passed as Date objects are converted to ISO strings, (2) verify string DATE_TIME values are left unchanged, and (3) assert that passing an invalid type (e.g., a number) causes the expected error to be thrown.
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:15119 This environment will automatically shut down when the PR is closed or after 5 hours. |
This PR fixes a bug that arises in `formatResult` following up the recent refactor for DATE_TIME : #17407 In the case of workflows, we pass a plain object to `formatResult` : ```ts { before: null, after: '2026-01-27T10:59:15.525Z' } ``` So a case has been added to handle this. @Weiko are we ok with this more restrictive else-if part in this util ? We could also just put a `continue` for unknown shapes.
…DATE_TIME` (twentyhq#17407) This PR modifies our broadly used `formatResult` util to counter act TypeORM transforming any date time to a `Date` object. Instead we return the ISO string for any `DATE_TIME`, this way we're not transporting Date object from one function to another in the backend. We do this because there was problems working with events utils that take string date time in parameters and received Date objects. As this is a recurring problem and because it's an opinionated choice from TypeORM, we chose to switch to string only in our codebase, from TypeORM's output to frontend.
This PR modifies our broadly used
formatResultutil to counter act TypeORM transforming any date time to aDateobject.Instead we return the ISO string for any
DATE_TIME, this way we're not transporting Date object from one function to another in the backend.We do this because there was problems working with events utils that take string date time in parameters and received Date objects.
As this is a recurring problem and because it's an opinionated choice from TypeORM, we chose to switch to string only in our codebase, from TypeORM's output to frontend.