-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Fix/i18n locale cache #13003
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/i18n locale cache #13003
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Major refactoring to make the metadata cache system locale-aware, ensuring proper internationalization throughout the application's data model handling.
- Introduced new
WithAuthContext
methods in core services (ObjectMetadataService
,FieldMetadataService
) to properly propagate locale information through metadata operations - Added locale parameters to cache keys in
workspace-cache-storage.service.ts
to ensure language-specific caching of GraphQL type definitions - Updated DataLoader payload types to include locale property, ensuring proper type safety for internationalized metadata
- Implemented fallback to 'en' locale when undefined, with proper TypeScript typing in
auth-context.type.ts
- Enhanced middleware service to extract and propagate locale from request headers through the authentication context
15 files reviewed, 10 comments
Edit PR Review Bot Settings | Greptile
@@ -47,12 +47,14 @@ export type FieldMetadataLoaderPayload = { | |||
export type IndexMetadataLoaderPayload = { | |||
workspaceId: string; | |||
objectMetadata: Pick<ObjectMetadataInterface, 'id'>; | |||
locale?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using keyof typeof APP_LOCALES for consistency with FieldMetadataLoaderPayload
locale?: string; | |
locale?: keyof typeof APP_LOCALES; |
@@ -148,6 +148,7 @@ export class WorkspaceDatasourceFactory { | |||
await this.workspaceMetadataCacheService.getExistingOrRecomputeMetadataMaps( | |||
{ | |||
workspaceId, | |||
locale: 'en', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Extract 'en' as a named constant to improve maintainability
{ | ||
locale: context.req.headers['x-locale'], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Duplicate locale extraction from context, consider extracting to a shared variable
}; | ||
|
||
export type IndexFieldMetadataLoaderPayload = { | ||
workspaceId: string; | ||
objectMetadata: Pick<ObjectMetadataInterface, 'id'>; | ||
indexMetadata: Pick<IndexMetadataInterface, 'id'>; | ||
locale?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Should use same type as other locale definitions for consistency
locale?: string; | |
locale?: keyof typeof APP_LOCALES; |
dataSourceId, | ||
workspaceId, | ||
}, | ||
{ locale: 'en', workspace: { id: workspaceId } as Workspace }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Avoid type assertion for workspace. Create proper workspace object or use type guard
@@ -92,11 +93,13 @@ export class WorkspaceSchemaFactory { | |||
authContext.workspace.id, | |||
metadataVersion, | |||
typeDefs, | |||
authContext.locale ?? 'en', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Extract authContext.locale ?? 'en' into a constant since it's used multiple times
request.locale = request.headers['x-locale'] as string; // Added locale from request headers | ||
data.locale = request.headers['x-locale'] as string; // Also add to AuthContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using a constant for the 'x-locale' header key instead of repeating the string literal
await this.objectMetadataService.createOneWithAuthContext( | ||
{ | ||
nameSingular: camelCase(localTableNameSingular), | ||
namePlural: camelCase(localTableNamePlural), | ||
labelSingular: camelToTitleCase(camelCase(localTableBaseName)), | ||
labelPlural: camelToTitleCase(plural(camelCase(localTableBaseName))), | ||
description: 'Remote table', | ||
dataSourceId: dataSourceMetadataId, | ||
workspaceId: workspaceId, | ||
icon: 'IconPlug', | ||
isRemote: true, | ||
primaryKeyColumnType: distantTableIdColumn.udtName, | ||
primaryKeyFieldMetadataSettings: mapUdtNameToFieldSettings( | ||
distantTableIdColumn.udtName, | ||
), | ||
} satisfies CreateObjectInput, | ||
{ locale: 'en', workspace: { id: workspaceId } as Workspace }, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider parameterizing the default locale ('en') as a constant to maintain consistency and ease future updates
distantTableIdColumn.udtName, | ||
), | ||
} satisfies CreateObjectInput, | ||
{ locale: 'en', workspace: { id: workspaceId } as Workspace }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Avoid type assertion with 'as Workspace', use proper workspace object creation
{ | ||
name: columnName, | ||
label: camelToTitleCase(columnName), | ||
description: 'Field of remote', | ||
type: mapUdtNameToFieldType(columnType), | ||
workspaceId: workspaceId, | ||
objectMetadataId: objectMetadataId, | ||
isRemoteCreation: true, | ||
isNullable: true, | ||
icon: 'IconPlug', | ||
settings: mapUdtNameToFieldSettings(columnType), | ||
} satisfies CreateFieldInput, | ||
{ locale: 'en', workspace: { id: workspaceId } as Workspace }, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Extract auth context creation into a helper method since it's used multiple times with same values
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:40226 This environment will automatically shut down when the PR is closed or after 5 hours. |
Thanks @MaxwellAt, I will look into that shortly. It's indeed quite an annoying bug |
This commit addresses a bug where the data model was being served in an incorrect language (e.g., Dutch) despite the workspace preference being set to English. The root cause was that the GraphQL metadata cache was not considering the user's locale when storing and retrieving data. Changes made: - Modified `WorkspaceCacheStorageService` to include the `locale` in the cache keys for metadata-related entries (`MetadataObjectMetadataMaps`, `GraphQLTypeDefs`, `GraphQLUsedScalarNames`, `ORMEntitySchemas`). - Updated `WorkspaceMetadataCacheService` to pass the `locale` to `WorkspaceCacheStorageService` methods when recomputing and retrieving metadata. - Added a `locale` property to `AuthContext` to carry the user's language preference throughout the request context. - In `MiddlewareService`, the `locale` is now extracted from the `x-locale` request header and bound to the `AuthContext` and the request object. This ensures the correct `locale` is available when metadata is cached or retrieved. - Ensured `flushVersionedMetadata` in `WorkspaceCacheStorageService` can clear locale-specific cache entries. These changes ensure that metadata is cached and retrieved based on the user's selected language, preventing language mismatches.
…type/lint issues This commit introduces locale awareness to the GraphQL metadata cache and resolves subsequent type-checking and linting errors in the package. The core problem was that the metadata cache was not locale-aware, leading to incorrect language display for users. Key changes include: - Modified to include LANG=pt_BR.UTF-8 LANGUAGE=pt_BR:pt:en LC_CTYPE="pt_BR.UTF-8" LC_NUMERIC="pt_BR.UTF-8" LC_TIME="pt_BR.UTF-8" LC_COLLATE="pt_BR.UTF-8" LC_MONETARY="pt_BR.UTF-8" LC_MESSAGES="pt_BR.UTF-8" LC_PAPER="pt_BR.UTF-8" LC_NAME="pt_BR.UTF-8" LC_ADDRESS="pt_BR.UTF-8" LC_TELEPHONE="pt_BR.UTF-8" LC_MEASUREMENT="pt_BR.UTF-8" LC_IDENTIFICATION="pt_BR.UTF-8" LC_ALL= in cache keys for , , , and . - Updated to accept and pass the LANG=pt_BR.UTF-8 LANGUAGE=pt_BR:pt:en LC_CTYPE="pt_BR.UTF-8" LC_NUMERIC="pt_BR.UTF-8" LC_TIME="pt_BR.UTF-8" LC_COLLATE="pt_BR.UTF-8" LC_MONETARY="pt_BR.UTF-8" LC_MESSAGES="pt_BR.UTF-8" LC_PAPER="pt_BR.UTF-8" LC_NAME="pt_BR.UTF-8" LC_ADDRESS="pt_BR.UTF-8" LC_TELEPHONE="pt_BR.UTF-8" LC_MEASUREMENT="pt_BR.UTF-8" LC_IDENTIFICATION="pt_BR.UTF-8" LC_ALL= parameter during metadata recomputation. - Passed to methods in . - Added to the type. - Extracted LANG=pt_BR.UTF-8 LANGUAGE=pt_BR:pt:en LC_CTYPE="pt_BR.UTF-8" LC_NUMERIC="pt_BR.UTF-8" LC_TIME="pt_BR.UTF-8" LC_COLLATE="pt_BR.UTF-8" LC_MONETARY="pt_BR.UTF-8" LC_MESSAGES="pt_BR.UTF-8" LC_PAPER="pt_BR.UTF-8" LC_NAME="pt_BR.UTF-8" LC_ADDRESS="pt_BR.UTF-8" LC_TELEPHONE="pt_BR.UTF-8" LC_MEASUREMENT="pt_BR.UTF-8" LC_IDENTIFICATION="pt_BR.UTF-8" LC_ALL= from in and assigned it to and . To address the resulting typecheck and lint issues: - Extended the Express interface in to include custom properties (, , , , , , , , LANG=pt_BR.UTF-8 LANGUAGE=pt_BR:pt:en LC_CTYPE="pt_BR.UTF-8" LC_NUMERIC="pt_BR.UTF-8" LC_TIME="pt_BR.UTF-8" LC_COLLATE="pt_BR.UTF-8" LC_MONETARY="pt_BR.UTF-8" LC_MESSAGES="pt_BR.UTF-8" LC_PAPER="pt_BR.UTF-8" LC_NAME="pt_BR.UTF-8" LC_ADDRESS="pt_BR.UTF-8" LC_TELEPHONE="pt_BR.UTF-8" LC_MEASUREMENT="pt_BR.UTF-8" LC_IDENTIFICATION="pt_BR.UTF-8" LC_ALL=). - Removed conflicting file from directory. - Updated to correctly include . - Replaced directives with statements or removed them where global augmentation was handled by . - Corrected type usage and assignments to handle and cases consistently. - Re-added missing import in . - Increased Node.js memory limit for TypeScript compilation to prevent out-of-memory errors. - Resolved linting errors in by using where appropriate. These changes ensure the metadata cache is now locale-aware, and the package compiles and lints without errors, maintaining type safety and code quality.
4d3eaf9
to
4e98ca9
Compare
This commit addresses the type-checking and linting errors introduced during the implementation of locale-aware metadata caching. Key changes include: - Refactored and to introduce methods for , , and operations, ensuring proper and LANG=pt_BR.UTF-8 LANGUAGE=pt_BR:pt:en LC_CTYPE="pt_BR.UTF-8" LC_NUMERIC="pt_BR.UTF-8" LC_TIME="pt_BR.UTF-8" LC_COLLATE="pt_BR.UTF-8" LC_MONETARY="pt_BR.UTF-8" LC_MESSAGES="pt_BR.UTF-8" LC_PAPER="pt_BR.UTF-8" LC_NAME="pt_BR.UTF-8" LC_ADDRESS="pt_BR.UTF-8" LC_TELEPHONE="pt_BR.UTF-8" LC_MEASUREMENT="pt_BR.UTF-8" LC_IDENTIFICATION="pt_BR.UTF-8" LC_ALL= propagation. - Updated all call sites for these methods across resolvers, services, and factories to use the new versions, passing the LANG=pt_BR.UTF-8 LANGUAGE=pt_BR:pt:en LC_CTYPE="pt_BR.UTF-8" LC_NUMERIC="pt_BR.UTF-8" LC_TIME="pt_BR.UTF-8" LC_COLLATE="pt_BR.UTF-8" LC_MONETARY="pt_BR.UTF-8" LC_MESSAGES="pt_BR.UTF-8" LC_PAPER="pt_BR.UTF-8" LC_NAME="pt_BR.UTF-8" LC_ADDRESS="pt_BR.UTF-8" LC_TELEPHONE="pt_BR.UTF-8" LC_MEASUREMENT="pt_BR.UTF-8" LC_IDENTIFICATION="pt_BR.UTF-8" LC_ALL= from the or a default value ('en') where appropriate. - Corrected type usage in various services to properly handle the object and LANG=pt_BR.UTF-8 LANGUAGE=pt_BR:pt:en LC_CTYPE="pt_BR.UTF-8" LC_NUMERIC="pt_BR.UTF-8" LC_TIME="pt_BR.UTF-8" LC_COLLATE="pt_BR.UTF-8" LC_MONETARY="pt_BR.UTF-8" LC_MESSAGES="pt_BR.UTF-8" LC_PAPER="pt_BR.UTF-8" LC_NAME="pt_BR.UTF-8" LC_ADDRESS="pt_BR.UTF-8" LC_TELEPHONE="pt_BR.UTF-8" LC_MEASUREMENT="pt_BR.UTF-8" LC_IDENTIFICATION="pt_BR.UTF-8" LC_ALL= property. - Updated Dataloader payload types (, , ) to include the LANG=pt_BR.UTF-8 LANGUAGE=pt_BR:pt:en LC_CTYPE="pt_BR.UTF-8" LC_NUMERIC="pt_BR.UTF-8" LC_TIME="pt_BR.UTF-8" LC_COLLATE="pt_BR.UTF-8" LC_MONETARY="pt_BR.UTF-8" LC_MESSAGES="pt_BR.UTF-8" LC_PAPER="pt_BR.UTF-8" LC_NAME="pt_BR.UTF-8" LC_ADDRESS="pt_BR.UTF-8" LC_TELEPHONE="pt_BR.UTF-8" LC_MEASUREMENT="pt_BR.UTF-8" LC_IDENTIFICATION="pt_BR.UTF-8" LC_ALL= property. - Adjusted to correctly include LANG=pt_BR.UTF-8 LANGUAGE=pt_BR:pt:en LC_CTYPE="pt_BR.UTF-8" LC_NUMERIC="pt_BR.UTF-8" LC_TIME="pt_BR.UTF-8" LC_COLLATE="pt_BR.UTF-8" LC_MONETARY="pt_BR.UTF-8" LC_MESSAGES="pt_BR.UTF-8" LC_PAPER="pt_BR.UTF-8" LC_NAME="pt_BR.UTF-8" LC_ADDRESS="pt_BR.UTF-8" LC_TELEPHONE="pt_BR.UTF-8" LC_MEASUREMENT="pt_BR.UTF-8" LC_IDENTIFICATION="pt_BR.UTF-8" LC_ALL= in cache keys for GraphQL type definitions and scalar names. - Resolved all linting issues, primarily related to import order and formatting, by running NX Running target lint for project twenty-server and 3 tasks it depends on: > nx run twenty-shared:generateBarrels [existing outputs match the cache, left as is] > nx run twenty-shared:build [existing outputs match the cache, left as is] > nx run twenty-emails:build [existing outputs match the cache, left as is] > nx run twenty-server:lint:ci --fix [existing outputs match the cache, left as is] Linting "twenty-server"... ✔ All files pass linting NX Successfully ran target lint for project twenty-server and 3 tasks it depends on Nx read the output from the cache instead of running the command for 4 out of 4 tasks.. These changes ensure that the package now compiles and lints without errors, maintaining type safety and code quality while supporting locale-aware metadata handling.
…es and fix setup-test.ts This commit addresses type-checking errors in integration tests related to undeclared global variables like and . Key changes include: - Created to declare global types for test environment variables (, , , , , , ). - Included in the array of to ensure these global declarations are recognized by the TypeScript compiler. - Added a type assertion () to in to resolve a error related to type assignment. These changes ensure that the integration tests compile without errors, improving the stability and maintainability of the test suite.
8788312
to
dd9069e
Compare
- Add package resolution for @envelop/core to ^5.0.2 to fix version compatibility - @envelop/on-resolve v4.1.1 requires @envelop/core ^5.0.2 but project had v4.0.3 - This resolves the '(0 , core_1.mapMaybePromise) is not a function' runtime error - GraphQL integration tests now pass successfully Fixes #issue-number # Conflicts: # yarn.lock
… críticos de metadata cache e object metadata service, evitando inconsistência de dados mínimos em seeds, migrations e testes de integração
34c7844
to
3265753
Compare
Hi @MaxwellAt, thanks for opening a PR on this part. Modifying the engine itself can be quite tricky! I have investigated your PR and figured out that it's not really touching the right level. I've opened another PR and explained in the description where changes should happen: #13067 I'm going to close this one but I really appreciate the effort, feel free to work on any good first issue |
In this PR, I'm fixing a bug introduced in recent performance work on the cache. Bug context: #12865 Related PR opened by a contributor: #13003 ## Root cause We cache all objectMetadataItems at graphql level : see `useCachedMetadata` hook: - instead of going through the regular resolvers, we direlcty load data from the cache. However this data must be localized regarding labels and descriptions In a precedent refactoring, we introduced the notion of locale in the cache key. However, the user locale was not properly taken into account as we did not have the information in this hook. ## Fix 1. **Introduce locale in userWorkspace entity**. The locale is stored on workspaceMember in each postgres workspaceSchema (workspace_xxx) which is the alter ego of userWorkspace in postgres core schema. Note that we can't store it in user as a user can be part of multiple workspaces (the locale already there must be seen as a default for this user), and we cannot rely on workspaceMember as we would need to query the workspaceSchema in the authentication layer which we want to avoid for performance reasons. 2. During request hydration from token (containing the userWorkspaceId), we fetch the userWorkspace and store it in the Request (this impact both AuthContext and Request interface) 3. Leverage userWorkspace.locale in the useCachedMetadata hook ## Additional notes There is no need to change the way we store and retrieve the object-metadata-maps object itself which is different from the graphql layer cache. object-metadadata-maps are not localized
In this PR, I'm fixing a bug introduced in recent performance work on the cache. Bug context: twentyhq#12865 Related PR opened by a contributor: twentyhq#13003 ## Root cause We cache all objectMetadataItems at graphql level : see `useCachedMetadata` hook: - instead of going through the regular resolvers, we direlcty load data from the cache. However this data must be localized regarding labels and descriptions In a precedent refactoring, we introduced the notion of locale in the cache key. However, the user locale was not properly taken into account as we did not have the information in this hook. ## Fix 1. **Introduce locale in userWorkspace entity**. The locale is stored on workspaceMember in each postgres workspaceSchema (workspace_xxx) which is the alter ego of userWorkspace in postgres core schema. Note that we can't store it in user as a user can be part of multiple workspaces (the locale already there must be seen as a default for this user), and we cannot rely on workspaceMember as we would need to query the workspaceSchema in the authentication layer which we want to avoid for performance reasons. 2. During request hydration from token (containing the userWorkspaceId), we fetch the userWorkspace and store it in the Request (this impact both AuthContext and Request interface) 3. Leverage userWorkspace.locale in the useCachedMetadata hook ## Additional notes There is no need to change the way we store and retrieve the object-metadata-maps object itself which is different from the graphql layer cache. object-metadadata-maps are not localized
Pull Request Title: fix(i18n): Resolve typecheck and lint issues after
locale-aware changes
Pull Request Description:
This PR addresses and resolves the type-checking (typecheck) and
linting issues that arose after implementing changes to make the
metadata cache locale-aware.
Original Problem:
The data model was being displayed in an incorrect language (e.g.,
Dutch) despite the workspace preference being set to English,
indicating that the GraphQL metadata cache was not considering the
user's locale when storing and retrieving data.
Implemented Solutions:
ObjectMetadataService and FieldMetadataService for createOne,
updateOne, and createMany operations, ensuring proper AuthContext and
locale propagation.
across resolvers, services, and factories to use the new
WithAuthContext versions, passing the locale from the AuthContext or
a default value ('en') where appropriate.
(IndexMetadataLoaderPayload, FieldMetadataLoaderPayload,
IndexFieldMetadataLoaderPayload) were updated to include the locale
property.
correctly include locale in cache keys for GraphQL type definitions
and scalar names.
undefined
Handling: Added fallback (?? 'en') for authContext.localewhere a string type was expected.