fix(polymorphism): include submodel relations when reading a concrete model #1112
Conversation
WalkthroughWalkthroughThe update introduces enhancements to the delegate system, focusing on improved handling of model includes and data merging. It involves adding new methods for recursive inclusion and assembly of model data, refining the merge logic with Changes
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.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/runtime/src/enhancements/delegate.ts (8 hunks)
- tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (2 hunks)
Additional comments: 4
tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (3)
- 204-216: The use of spread operators (
...r) andexpect.objectContainingfor assertions is a good practice for making the tests more flexible and maintainable. However, ensure that the spread properties fromrdo not unintentionally override the explicitly set properties in the expectation object, which could lead to false positives in tests.- 231-232: Using
expect.objectContainingfor nested object assertions is a good practice as it allows for more precise and flexible tests. This approach is particularly useful when dealing with complex data structures and ensures that the tests focus on the relevant properties.- 201-222: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-3]
The imports and setup at the beginning of the file are correctly structured, ensuring that all necessary modules and utilities are available for the tests. This setup is crucial for the tests to run smoothly and interact with the database as expected.
packages/runtime/src/enhancements/delegate.ts (1)
- 3-3: The import of
ArrayMergeOptionsfromdeepmergeis correctly added to support custom array merging strategies in thedeepmergefunction calls. This is essential for handling complex data structures that may involve arrays of objects, ensuring that the merge behavior is consistent with the application's requirements.
| private injectConcreteIncludeRecursively(model: string, selectInclude: any) { | ||
| const modelInfo = getModelInfo(this.options.modelMeta, model); | ||
| if (!modelInfo) { | ||
| return; | ||
| } | ||
|
|
||
| // get sub models of this model | ||
| const subModels = Object.values(this.options.modelMeta.models).filter((m) => | ||
| m.baseTypes?.includes(modelInfo.name) | ||
| ); | ||
|
|
||
| for (const subModel of subModels) { | ||
| // include sub model relation field | ||
| const subRelationName = this.makeAuxRelationName(subModel); | ||
| if (selectInclude.select) { | ||
| selectInclude.include = { [subRelationName]: {}, ...selectInclude.select }; | ||
| delete selectInclude.select; | ||
| } else { | ||
| selectInclude.include = { [subRelationName]: {}, ...selectInclude.include }; | ||
| } | ||
| this.injectConcreteIncludeRecursively(subModel.name, selectInclude.include[subRelationName]); | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of injectConcreteIncludeRecursively method correctly addresses the objective of recursively including submodel relations. It efficiently filters submodels based on the current model and modifies the selectInclude argument to include these relations. This method is crucial for ensuring that all relevant submodel data is fetched when interacting with a concrete model in a polymorphic hierarchy.
However, consider adding error handling or validation to ensure that the model and selectInclude arguments meet expected conditions before proceeding with the logic. This can prevent potential runtime errors and improve the robustness of the method.
+ if (!model || typeof selectInclude !== 'object') {
+ throw new Error('Invalid arguments provided to injectConcreteIncludeRecursively');
+ }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.
| private injectConcreteIncludeRecursively(model: string, selectInclude: any) { | |
| const modelInfo = getModelInfo(this.options.modelMeta, model); | |
| if (!modelInfo) { | |
| return; | |
| } | |
| // get sub models of this model | |
| const subModels = Object.values(this.options.modelMeta.models).filter((m) => | |
| m.baseTypes?.includes(modelInfo.name) | |
| ); | |
| for (const subModel of subModels) { | |
| // include sub model relation field | |
| const subRelationName = this.makeAuxRelationName(subModel); | |
| if (selectInclude.select) { | |
| selectInclude.include = { [subRelationName]: {}, ...selectInclude.select }; | |
| delete selectInclude.select; | |
| } else { | |
| selectInclude.include = { [subRelationName]: {}, ...selectInclude.include }; | |
| } | |
| this.injectConcreteIncludeRecursively(subModel.name, selectInclude.include[subRelationName]); | |
| } | |
| } | |
| private injectConcreteIncludeRecursively(model: string, selectInclude: any) { | |
| if (!model || typeof selectInclude !== 'object') { | |
| throw new Error('Invalid arguments provided to injectConcreteIncludeRecursively'); | |
| } | |
| const modelInfo = getModelInfo(this.options.modelMeta, model); | |
| if (!modelInfo) { | |
| return; | |
| } | |
| // get sub models of this model | |
| const subModels = Object.values(this.options.modelMeta.models).filter((m) => | |
| m.baseTypes?.includes(modelInfo.name) | |
| ); | |
| for (const subModel of subModels) { | |
| // include sub model relation field | |
| const subRelationName = this.makeAuxRelationName(subModel); | |
| if (selectInclude.select) { | |
| selectInclude.include = { [subRelationName]: {}, ...selectInclude.select }; | |
| delete selectInclude.select; | |
| } else { | |
| selectInclude.include = { [subRelationName]: {}, ...selectInclude.include }; | |
| } | |
| this.injectConcreteIncludeRecursively(subModel.name, selectInclude.include[subRelationName]); | |
| } | |
| } |
| const fieldValue = entity[field.name]; | ||
| if (field.isDataModel) { | ||
| if (Array.isArray(fieldValue)) { | ||
| result[field.name] = fieldValue.map((item) => this.assembleHierarchy(field.type, item)); | ||
| result[field.name] = fieldValue.map((item) => this.assembleUp(field.type, item)); | ||
| } else { | ||
| result[field.name] = this.assembleHierarchy(field.type, fieldValue); | ||
| result[field.name] = this.assembleUp(field.type, fieldValue); | ||
| } | ||
| } else { | ||
| result[field.name] = fieldValue; |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1092-1130]
The assembleUp method effectively merges data from base models upwards, ensuring that fields from base models are included in the current model's data structure. This is essential for correctly representing the complete data of an entity in a polymorphic model hierarchy.
One potential improvement is to ensure that the method does not inadvertently modify the original entity object passed as an argument. Consider creating a deep copy of entity at the beginning of the method to avoid side effects.
+ const entityCopy = deepcopy(entity);
- const baseData = entity[baseRelationName];
+ const baseData = entityCopy[baseRelationName];| private assembleDown(model: string, entity: any) { | ||
| const result: any = {}; | ||
| let curr = result; | ||
| const modelInfo = getModelInfo(this.options.modelMeta, model, true); | ||
|
|
||
| if (fieldModelPos > forModelPos) { | ||
| // walk down hierarchy | ||
| for (let i = forModelPos + 1; i <= fieldModelPos; i++) { | ||
| const rel = this.makeAuxRelationName(inheritStack[i]); | ||
| curr[rel] = {}; | ||
| curr = curr[rel]; | ||
| if (modelInfo.discriminator) { | ||
| // model is a delegate, merge sub model fields | ||
| const subModelName = entity[modelInfo.discriminator]; | ||
| if (subModelName) { | ||
| const subModel = getModelInfo(this.options.modelMeta, subModelName, true); | ||
| const subRelationName = this.makeAuxRelationName(subModel); | ||
| const subData = entity[subRelationName]; | ||
| if (subData && typeof subData === 'object') { | ||
| const subAssembled = this.assembleDown(subModel.name, subData); | ||
| Object.assign(result, subAssembled); | ||
| } | ||
| } | ||
| } else { | ||
| // walk up hierarchy | ||
| for (let i = forModelPos - 1; i >= fieldModelPos; i--) { | ||
| const rel = this.makeAuxRelationName(inheritStack[i]); | ||
| curr[rel] = {}; | ||
| curr = curr[rel]; | ||
| } | ||
|
|
||
| for (const field of Object.values(modelInfo.fields)) { | ||
| if (field.name in entity) { | ||
| const fieldValue = entity[field.name]; | ||
| if (field.isDataModel) { | ||
| if (Array.isArray(fieldValue)) { | ||
| result[field.name] = fieldValue.map((item) => this.assembleDown(field.type, item)); | ||
| } else { | ||
| result[field.name] = this.assembleDown(field.type, fieldValue); | ||
| } | ||
| } else { | ||
| result[field.name] = fieldValue; | ||
| } |
There was a problem hiding this comment.
The assembleDown method correctly assembles data from submodels downwards, ensuring that fields from submodels are included in the current model's data structure. This complements the assembleUp method and is crucial for accurately representing the complete data of an entity in a polymorphic model hierarchy.
Similar to the assembleUp method, consider ensuring that the original entity object is not modified. Using a deep copy of entity at the beginning of the method can help avoid unintended side effects.
+ const entityCopy = deepcopy(entity);
- const subData = entity[subRelationName];
+ const subData = entityCopy[subRelationName];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.
| private assembleDown(model: string, entity: any) { | |
| const result: any = {}; | |
| let curr = result; | |
| const modelInfo = getModelInfo(this.options.modelMeta, model, true); | |
| if (fieldModelPos > forModelPos) { | |
| // walk down hierarchy | |
| for (let i = forModelPos + 1; i <= fieldModelPos; i++) { | |
| const rel = this.makeAuxRelationName(inheritStack[i]); | |
| curr[rel] = {}; | |
| curr = curr[rel]; | |
| if (modelInfo.discriminator) { | |
| // model is a delegate, merge sub model fields | |
| const subModelName = entity[modelInfo.discriminator]; | |
| if (subModelName) { | |
| const subModel = getModelInfo(this.options.modelMeta, subModelName, true); | |
| const subRelationName = this.makeAuxRelationName(subModel); | |
| const subData = entity[subRelationName]; | |
| if (subData && typeof subData === 'object') { | |
| const subAssembled = this.assembleDown(subModel.name, subData); | |
| Object.assign(result, subAssembled); | |
| } | |
| } | |
| } else { | |
| // walk up hierarchy | |
| for (let i = forModelPos - 1; i >= fieldModelPos; i--) { | |
| const rel = this.makeAuxRelationName(inheritStack[i]); | |
| curr[rel] = {}; | |
| curr = curr[rel]; | |
| } | |
| for (const field of Object.values(modelInfo.fields)) { | |
| if (field.name in entity) { | |
| const fieldValue = entity[field.name]; | |
| if (field.isDataModel) { | |
| if (Array.isArray(fieldValue)) { | |
| result[field.name] = fieldValue.map((item) => this.assembleDown(field.type, item)); | |
| } else { | |
| result[field.name] = this.assembleDown(field.type, fieldValue); | |
| } | |
| } else { | |
| result[field.name] = fieldValue; | |
| } | |
| private assembleDown(model: string, entity: any) { | |
| const entityCopy = deepcopy(entity); | |
| const result: any = {}; | |
| const modelInfo = getModelInfo(this.options.modelMeta, model, true); | |
| if (modelInfo.discriminator) { | |
| // model is a delegate, merge sub model fields | |
| const subModelName = entity[modelInfo.discriminator]; | |
| if (subModelName) { | |
| const subModel = getModelInfo(this.options.modelMeta, subModelName, true); | |
| const subRelationName = this.makeAuxRelationName(subModel); | |
| const subData = entityCopy[subRelationName]; | |
| if (subData && typeof subData === 'object') { | |
| const subAssembled = this.assembleDown(subModel.name, subData); | |
| Object.assign(result, subAssembled); | |
| } | |
| } | |
| } | |
| for (const field of Object.values(modelInfo.fields)) { | |
| if (field.name in entity) { | |
| const fieldValue = entity[field.name]; | |
| if (field.isDataModel) { | |
| if (Array.isArray(fieldValue)) { | |
| result[field.name] = fieldValue.map((item) => this.assembleDown(field.type, item)); | |
| } else { | |
| result[field.name] = this.assembleDown(field.type, fieldValue); | |
| } | |
| } else { | |
| result[field.name] = fieldValue; | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/runtime/src/enhancements/delegate.ts (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/runtime/src/enhancements/delegate.ts
Summary by CodeRabbit