fix: add ClearAll dataloader method #9975
Conversation
Signed-off-by: Matt Krick <[email protected]>
WalkthroughThe updates streamline and enhance the flexibility of data-loader handling in the server package. Key changes include clearing all organisation user data instead of individual user data, introducing a more generic approach to the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (2)
Additional comments not posted (20)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (3)
packages/server/dataloader/customLoaderMakers.ts (3)
88-91: Update the function call to include thedependsOnargument.The function
commentCountByDiscussionIdhas been updated to accept a newdependsOnparameter. However, the call to this function inpackages/server/graphql/types/Discussion.tsdoes not include this argument, which could lead to issues.
- File:
packages/server/graphql/types/Discussion.ts
- Line: Return statement using
dataLoader.get('commentCountByDiscussionId').load(discussionId)Please update the function call to include the
dependsOnargument.Analysis chain
LGTM! Verify correct usage of the
dependsOnparameter.The changes introduce a
dependsOnparameter to register dependencies for the data loader. Ensure that all calls to this function correctly provide thedependsOnargument.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `commentCountByDiscussionId` provide the `dependsOn` argument. # Test: Search for the function usage. Expect: All function calls include the `dependsOn` argument. rg --type typescript -A 2 $'commentCountByDiscussionId'Length of output: 89
Script:
#!/bin/bash # Description: Verify all function calls to `commentCountByDiscussionId` provide the `dependsOn` argument. # Test: Search for the function usage. Expect: All function calls include the `dependsOn` argument. rg --type ts -A 2 $'commentCountByDiscussionId'Length of output: 523
510-511: ThedependsOnargument is missing in the function call tomeetingTemplatesByTeamId.The function call in
archiveTeam.tsneeds to be updated to include thedependsOnargument, as per the new function signature.
- File:
packages/server/graphql/mutations/archiveTeam.ts- Line:
const teamTemplates = await dataLoader.get('meetingTemplatesByTeamId').load(teamId)Please update the function call to provide the
dependsOnargument.Analysis chain
LGTM! Verify correct usage of the
dependsOnparameter.The changes introduce a
dependsOnparameter to register dependencies for the data loader. Ensure that all calls to this function correctly provide thedependsOnargument.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `meetingTemplatesByTeamId` provide the `dependsOn` argument. # Test: Search for the function usage. Expect: All function calls include the `dependsOn` argument. rg --type typescript -A 2 $'meetingTemplatesByTeamId'Length of output: 87
Script:
#!/bin/bash # Description: Verify all function calls to `meetingTemplatesByTeamId` provide the `dependsOn` argument. # Test: Search for the function usage. Expect: All function calls include the `dependsOn` argument. rg -A 2 'meetingTemplatesByTeamId'Length of output: 672
587-590: MissingdependsOnArgument in Function CallThe function
taskIdsByTeamAndGitHubRepois being called without the requireddependsOnargument in the following location:
packages/server/graphql/types/Task.ts, lines surrounding the usage:const taskIds = await dataLoader .get('taskIdsByTeamAndGitHubRepo') .load({teamId, nameWithOwner})Please ensure that the
dependsOnargument is correctly provided in this function call.Analysis chain
LGTM! Verify correct usage of the
dependsOnparameter.The changes introduce a
dependsOnparameter to register dependencies for the data loader. Ensure that all calls to this function correctly provide thedependsOnargument.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `taskIdsByTeamAndGitHubRepo` provide the `dependsOn` argument. # Test: Search for the function usage. Expect: All function calls include the `dependsOn` argument. rg --type typescript -A 2 $'taskIdsByTeamAndGitHubRepo'Length of output: 89
Script:
#!/bin/bash # Description: Verify all function calls to `taskIdsByTeamAndGitHubRepo` provide the `dependsOn` argument. # Test: Search for the function usage. Expect: All function calls include the `dependsOn` argument. rg --type ts -A 2 $'taskIdsByTeamAndGitHubRepo'Length of output: 577
Script:
#!/bin/bash # Description: Inspect the surrounding lines of the identified usage of `taskIdsByTeamAndGitHubRepo` to confirm whether the `dependsOn` argument is missing. # Test: Search for the function usage and include more context lines. Expect: All function calls include the `dependsOn` argument. rg --type ts -A 5 -B 5 $'taskIdsByTeamAndGitHubRepo' packages/server/graphql/types/Task.tsLength of output: 654
| ? inPrimaryLoaders | ||
| : [inPrimaryLoaders] | ||
| primaryLoaders.forEach((primaryLoader) => { | ||
| ;(this.dependentLoaders[primaryLoader] ??= []).push(loaderName) |
There was a problem hiding this comment.
Avoid assignments in expressions.
The assignment within the expression can be confusing and should be avoided.
- ;(this.dependentLoaders[primaryLoader] ??= []).push(loaderName)
+ if (!this.dependentLoaders[primaryLoader]) {
+ this.dependentLoaders[primaryLoader] = [];
+ }
+ this.dependentLoaders[primaryLoader].push(loaderName);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ;(this.dependentLoaders[primaryLoader] ??= []).push(loaderName) | |
| if (!this.dependentLoaders[primaryLoader]) { | |
| this.dependentLoaders[primaryLoader] = []; | |
| } | |
| this.dependentLoaders[primaryLoader].push(loaderName); |
Tools
Biome
[error] 111-111: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
| @@ -97,6 +96,7 @@ const addUser = async (orgIds: string[], user: IUser, dataLoader: DataLoaderWork | |||
| tier: organization.tier | |||
| }) | |||
| }) | |||
| dataLoader.clearAll('organizationUsers') | |||
There was a problem hiding this comment.
now we don't have to wipe every single dataloader that could have been used.
we just wipe the primary key datalaoder & it wipes all dependents.
| export interface DataLoaderInstance { | ||
| get<LoaderName extends Loaders>(loaderName: LoaderName): TypedDataLoader<LoaderName> | ||
| // The RethinkDB logic is a leaky abstraction! It will be gone soon & this will be generic enough to put in its own package | ||
| interface GenericDataLoader<TLoaders, TPrimaryLoaderNames> { |
There was a problem hiding this comment.
this is basically TypedDataLoader, but the old version took 350ms to load types, this takes ~150ms
| parent: RootDataLoader, | ||
| dependsOn: RegisterDependsOn | ||
| ) => { | ||
| dependsOn('comments') |
There was a problem hiding this comment.
custom dataloaders can register with a primary key loader.
when a mutation calls dataLoader.clearAll('comments') this dataloader, if initialized, will get wiped, too.
| return (parent: RootDataLoader) => { | ||
| const primaryLoader = parent.get(primaryLoaderKey) as DataLoader<PrimaryKeyT, T, PrimaryKeyT> | ||
| return (parent: RootDataLoader, dependsOn: RegisterDependsOn) => { | ||
| dependsOn(primaryLoaderKey) |
There was a problem hiding this comment.
foreignKey dataloaders are automatically tagged as dependents of the pk loaders
Description
Curious for your feedback on this pattern @Dschoordsch @nickoferrall.
The problem:
After the DB is mutated, devs have to call
clearAllon every dataloader that could have been used & is now stale. Adding a new dataloader requires auditing all existing DB mutations to make sure calls to clear it exist.Proposed solution:
Immediately after a database mutation, wipe out any dataloaders that depend on that table
Summary by CodeRabbit
New Features
samlfunction for fetching SAML-related data.Improvements
organizationUsersentries.Refactor
normalizeResultsto improve performance.Bug Fixes
rethinkForeignKeyLoaderto ensure accurate data loading and caching.