Dataloader auto resolve #4776
Dataloader auto resolve #4776
Conversation
WalkthroughThe pull request introduces modifications across multiple files to enhance the handling of nullable fields within the data loader system. Key changes include updates to the documentation for the Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Wating for the rabbit review. Then switching to draft - needs good testing. |
![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 and nitpick comments (4)
src/core/dataloader/creators/base/data.loader.creator.base.options.ts (1)
32-53: Documentation looks great, consider minor enhancementThe documentation effectively explains the auto-resolution feature with clear examples. The structure flows well from explanation to example to override options.
Consider adding a brief example of what happens when
resolveToNull: false, to make the error case more explicit:// When resolveToNull: false // Returns EntityNotFoundException: "Could not load relation 'callout' for CalloutEntity for the given key"src/core/dataloader/utils/findByBatchIds.ts (1)
63-66: Consider type safety enhancementThe type assertion in the Map creation could be made more explicit.
- sortedResults.map<[string, TResult]>(result => [ + sortedResults.map<[string, TResult]>((result): [string, TResult] => [src/core/dataloader/utils/createTypedRelationLoader.ts (1)
Line range hint
1-52: Consider adding unit tests for null resolutionThe changes introduce new behavior for null resolution that should be tested.
Would you like me to help generate unit tests to verify the null resolution behavior?
src/domain/collaboration/callout/callout.resolver.fields.ts (1)
157-164: Consider extracting common user loading logicThe
createdByandpublishedBymethods share similar patterns for loading user data. Consider extracting this into a reusable helper method.+ private async loadUserField(userId: string | null, fieldName: string, loader: ILoader<IUser | null>): Promise<IUser | null> { + if (!userId) { + return null; + } + return loader.load(userId); + } async publishedBy( @Parent() callout: ICallout, @Loader(UserLoaderCreator) loader: ILoader<IUser | null> ): Promise<IUser | null> { - const publishedBy = callout.publishedBy; - if (!publishedBy) { - return null; - } - return loader.load(publishedBy); + return this.loadUserField(callout.publishedBy, 'publishedBy', loader); } async createdBy( @Parent() callout: ICallout, @Loader(UserLoaderCreator) loader: ILoader<IUser | null> ): Promise<IUser | null> { - const createdBy = callout.createdBy; - if (!createdBy) { - return null; - } - return loader.load(createdBy); + return this.loadUserField(callout.createdBy, 'createdBy', loader); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
src/core/dataloader/creators/base/data.loader.creator.base.options.ts(1 hunks)src/core/dataloader/decorators/data.loader.decorator.ts(2 hunks)src/core/dataloader/utils/createTypedRelationLoader.ts(1 hunks)src/core/dataloader/utils/createTypedSimpleLoader.ts(1 hunks)src/core/dataloader/utils/findByBatchIds.ts(2 hunks)src/core/dataloader/utils/findByBatchIdsSimple.ts(2 hunks)src/core/dataloader/utils/index.ts(1 hunks)src/domain/collaboration/callout/callout.resolver.fields.ts(2 hunks)src/domain/common/whiteboard/whiteboard.resolver.fields.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/core/dataloader/utils/index.ts
🧰 Additional context used
📓 Path-based instructions (8)
src/core/dataloader/creators/base/data.loader.creator.base.options.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/core/dataloader/utils/createTypedSimpleLoader.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/core/dataloader/utils/findByBatchIdsSimple.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/domain/common/whiteboard/whiteboard.resolver.fields.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/core/dataloader/decorators/data.loader.decorator.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/core/dataloader/utils/findByBatchIds.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/core/dataloader/utils/createTypedRelationLoader.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/domain/collaboration/callout/callout.resolver.fields.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
🔇 Additional comments (11)
src/core/dataloader/decorators/data.loader.decorator.ts (1)
9-9: LGTM: Improved type safety with default options
The change from optional parameter to required parameter with default value improves type safety while maintaining backward compatibility.
src/core/dataloader/utils/findByBatchIdsSimple.ts (2)
12-12: LGTM: Enhanced return type safety
The updated return type correctly reflects all possible outcomes: successful result, null for unresolved keys, or EntityNotFoundException.
28-36: LGTM: Improved error handling with context
The new resolveUnresolvedForKey function provides better error context by including:
- Entity class name in the error message
- ID of the unresolved entity
- Proper LogContext
src/core/dataloader/utils/createTypedSimpleLoader.ts (1)
27-27: LGTM: Consistent type definitions
The DataLoader return type correctly aligns with findByBatchIdsSimple's return type, ensuring type safety throughout the loading chain.
src/core/dataloader/utils/findByBatchIds.ts (3)
10-10: Type definitions properly updated for null support
The return type changes and imports align well with the PR objectives for nullable field support.
Also applies to: 14-14, 21-21
52-60: Error message enhancement looks good
The error message now includes the class name and key, making debugging easier. The null resolution logic is properly implemented based on the resolveToNull option.
36-44: Verify the impact of sorting implementation
The introduction of sorting results after the database query could affect performance with large datasets.
✅ Verification successful
Sorting implementation has minimal performance impact
The sorting implementation is lightweight and optimized:
- It uses DataLoader for batching and caching requests
- The
sorOutputByKeysimplementation uses lodash'ssortBywith a pre-computed index map - The sorting is necessary to maintain DataLoader's contract of returning results in the same order as the requested keys
- The DataLoader pattern itself helps in reducing database load by batching multiple requests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential large result sets in the codebase
rg -B 2 -A 2 'new DataLoader.*cache:\s*true'
Length of output: 46
Script:
#!/bin/bash
# Let's check where this findByBatchIds utility is used and analyze the data loading patterns
rg -B 2 -A 2 "findByBatchIds"
# Also check for DataLoader usage patterns
rg -B 2 -A 2 "new DataLoader"
# Let's look at the implementation of sorOutputByKeys
ast-grep --pattern 'function sorOutputByKeys'
Length of output: 11365
Script:
#!/bin/bash
# Let's look at the implementation of sorOutputByKeys with a different pattern
ast-grep --pattern 'const sorOutputByKeys'
# Also check the actual implementation file
rg -B 5 -A 5 "sorOutputByKeys"
# Let's see if there are any batch size limits defined
rg -B 2 -A 2 "batchSize|maxBatchSize"
Length of output: 11227
src/core/dataloader/utils/createTypedRelationLoader.ts (1)
37-37: Type definitions properly updated for DataLoader
The return type changes properly propagate through the DataLoader creation and findByBatchIds call, maintaining type safety.
Also applies to: 39-39
src/domain/common/whiteboard/whiteboard.resolver.fields.ts (2)
43-43: LGTM! Type-safe loader configuration
The updated loader type ILoader<IUser | null> correctly reflects that this field can resolve to null, matching the GraphQL schema definition.
54-54: LGTM! Simplified error handling
The direct return of loader.load(createdBy) aligns with the new auto-resolve feature and follows the coding guidelines by leveraging the global exception handler.
src/domain/collaboration/callout/callout.resolver.fields.ts (1)
129-129: LGTM! Type-safe loader configuration for publishedBy
The loader type ILoader<IUser | null> correctly reflects the nullable nature of the publishedBy field.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/core/dataloader/decorators/data.loader.decorator.ts (1)
19-19: Improve comment clarity for null resolution behaviorThe current comment could be more descriptive about when and why this behavior is applied.
-// as the default behaviour we resolve to null if the field is nullable +// Automatically determine if loader should resolve to null based on GraphQL field's nullability. +// This ensures consistent behavior with the GraphQL schema definition.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/core/dataloader/decorators/data.loader.decorator.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/core/dataloader/decorators/data.loader.decorator.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
🔇 Additional comments (3)
src/core/dataloader/decorators/data.loader.decorator.ts (3)
1-1: LGTM: Import added for GraphQL type checking
The import of isNonNullType is appropriate for the new nullability checking functionality.
10-10: LGTM: Improved type safety with default options
The addition of a default empty object for options parameter improves type safety while maintaining backward compatibility.
21-28:
Add null checks for GraphQL info object access
The code accesses info.parentType and info.fieldName without proper null checks. This could lead to runtime errors if the execution context is not properly set up.
Apply this diff to add proper null checks:
const info: GraphQLResolveInfo = context.getArgByIndex(3);
-if (info) {
+if (info?.parentType?.getFields) {
const fieldName = info.fieldName;
const field = info.parentType.getFields()[fieldName];
-
+ if (field?.type) {
options.resolveToNull = !isNonNullType(field.type);
+ }
}
todo: good testing
Quality of life feature. Will reduce bugs from forgotten configurations.
Dataloader now auto-resolves for nullable graphql fields as the default behaviour when resolving the unresolved result for a key.
This is useful when the result is expected to be null, and it's not an exceptional case.
The default behaviour is determined by the Loader decorator on the dataloader`s creation. It inspects the type of the field at runtime and determines if the dataloader should be able to resolve to null.
If the underlying graphql field, for which the dataloader is created, is nullable - the result can be resolved to null.
Example:
The
calloutis decorated as nullable, so the dataloader will auto-resolve tonullif the result is not found.You can override this behaviour by setting the
resolveToNullflag tofalse. That way the problematic values will always be resolved to errors.Summary by CodeRabbit
New Features
nullor exceptions when entities are not found.Bug Fixes
Documentation