✨ feat(manager): Added support query with nested relations - #23
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the Datum framework by enabling the querying and loading of nested relations for entities. This change expands the data retrieval capabilities, allowing the system to traverse complex relation paths, which was previously not possible. The refactoring improves the maintainability and extensibility of the relation loading mechanism. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for querying nested relations by refactoring _fetchAndStitchRelations to recursively load relation paths, with a new _loadRelationPath helper method. However, a potential runtime exception has been identified in the HasMany relation processing logic due to an unsafe cast. This flaw could lead to a Denial of Service if query parameters are exposed to untrusted input. My review includes a suggestion to fix this to make the implementation more robust.
KiddoV
changed the title
|
Please add tests which bug you are fixing . @KiddoV . |
📝 WalkthroughWalkthroughRefactored eager-loading to build dot-separated relation paths and recursively load nested relations via a new async helper, handling BelongsTo and HasMany with early-return branches for missing relations, empty keys, or no related entities. Tests adjusted to use non-const constructors and cached relation maps; added a nested related-data query test. Changes
Sequence Diagram(s)sequenceDiagram
participant Manager
participant RelationTree
participant Adapter
participant Entities
Manager->>RelationTree: build dot-separated relation tree from withRelated
Manager->>Manager: call _loadRelationTree(rootNode, entities)
alt entities empty
Manager-->>Manager: return
else
Manager->>RelationTree: resolve relation on first entity
RelationTree-->>Manager: relation descriptor (BelongsTo/HasMany) or null
alt relation missing
Manager-->>Manager: log and return
else
Manager->>Adapter: query related records (by localKey or foreignKey)
Adapter-->>Entities: return related records
Manager->>Entities: setRaw on parent relations / group for HasMany
alt children exist and relatedEntities not empty
Manager->>Manager: recurse for each child node with relatedEntities
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
fb242c4 to
b147039
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/datum/lib/source/core/manager/datum_manager.dart`:
- Around line 1021-1078: This code only handles BelongsTo and HasMany, so HasOne
and ManyToMany paths never call setRaw or recurse; add branches for relation is
HasOne and relation is ManyToMany in the same conditional where
BelongsTo/HasMany are handled (use the same variables: relationName,
relatedManager, relatedEntities, source, userId) so they mirror
_getRelatedEntities() behavior: for HasOne query relatedManager with
foreignKey/localKey similar to HasMany but set a single related entity via
entity.relations[relationName]?.setRaw(related) and add that related to
relatedEntities; for ManyToMany perform the intermediate/pivot join query the
same way _getRelatedEntities() does (fetch related ids via pivot, then load
related entities) and setRaw the resulting list on each parent and add to
relatedEntities so eager loading covers HasOne and ManyToMany as well.
- Around line 1021-1076: The code assumes the primary key is always "id" when
stitching relations; update both BelongsTo and HasMany branches to use
relation.localKey consistently: in the BelongsTo branch build the lookup map
from fetched rows using (fetchedItem as
RelationalDatumEntity).toDatumMap()[relation.localKey] instead of e.id so keys
match the DatumQuery filter, and when reading the parent key use
entity.toDatumMap()[foreignKeyName] as now; in the HasMany branch compute
localKeyValues from entities.map((e) => e.toDatumMap()[relation.localKey]) and
when assigning related children use
grouped[entity.toDatumMap()[relation.localKey]] (ensure grouping keys come from
fetched.toDatumMap()[foreignKeyName] and remain strings) so all joins use
relation.localKey rather than hard-coded id when calling relatedManager.query,
building grouped maps, and calling entity.relations[relationName]?.setRaw.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23eafc04-f1bd-435b-9050-f88747179e7c
📒 Files selected for processing (1)
packages/datum/lib/source/core/manager/datum_manager.dart
| if (relation is BelongsTo) { | ||
| final foreignKeyName = relation.foreignKey; | ||
| final foreignKeyValues = entities.map((e) => e.toDatumMap()[foreignKeyName]).nonNulls.toSet().toList(); | ||
|
|
||
| for (final entity in entities) { | ||
| final related = relatedEntitiesByParentId[entity.id] ?? []; | ||
| (entity as RelationalDatumEntity).relations[relationName]?.setRaw(related); | ||
| } | ||
| if (foreignKeyValues.isEmpty) return; | ||
|
|
||
| final fetched = await relatedManager.query( | ||
| DatumQuery( | ||
| filters: [ | ||
| Filter(relation.localKey, FilterOperator.isIn, foreignKeyValues) | ||
| ], | ||
| ), | ||
| source: source, | ||
| userId: userId, | ||
| ); | ||
|
|
||
| final byId = {for (var e in fetched) e.id: e}; | ||
|
|
||
| for (final entity in entities) { | ||
| final fk = entity.toDatumMap()[foreignKeyName]; | ||
| final related = byId[fk]; | ||
|
|
||
| entity.relations[relationName]?.setRaw(related); | ||
|
|
||
| if (related is RelationalDatumEntity) { | ||
| relatedEntities.add(related); | ||
| } | ||
| } | ||
| } else if (relation is HasMany) { | ||
| final foreignKeyName = relation.foreignKey; | ||
| final localKeyValues = entities.map((e) => e.id).toSet().toList(); | ||
|
|
||
| if (localKeyValues.isEmpty) return; | ||
|
|
||
| final fetched = await relatedManager.query( | ||
| DatumQuery( | ||
| filters: [ | ||
| Filter(foreignKeyName, FilterOperator.isIn, localKeyValues) | ||
| ], | ||
| ), | ||
| source: source, | ||
| userId: userId, | ||
| ); | ||
|
|
||
| final grouped = <String, List<RelationalDatumEntity>>{}; | ||
|
|
||
| for (final entity in fetched) { | ||
| final parentId = (entity as RelationalDatumEntity).toDatumMap()[foreignKeyName]; | ||
|
|
||
| (grouped[parentId] ??= []).add(entity); | ||
| relatedEntities.add(entity); | ||
| } | ||
|
|
||
| for (final entity in entities) { | ||
| final related = grouped[entity.id] ?? []; | ||
| entity.relations[relationName]?.setRaw(related); | ||
| } | ||
| } |
There was a problem hiding this comment.
HasOne and ManyToMany relations currently fall through without loading.
This helper only handles BelongsTo and HasMany. If withRelated includes a HasOne or ManyToMany path, the method neither calls setRaw nor recurses into its children, so eager loading silently returns partial data. _getRelatedEntities() later in this file already supports both relation types, so this loader should match that coverage before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/datum/lib/source/core/manager/datum_manager.dart` around lines 1021
- 1078, This code only handles BelongsTo and HasMany, so HasOne and ManyToMany
paths never call setRaw or recurse; add branches for relation is HasOne and
relation is ManyToMany in the same conditional where BelongsTo/HasMany are
handled (use the same variables: relationName, relatedManager, relatedEntities,
source, userId) so they mirror _getRelatedEntities() behavior: for HasOne query
relatedManager with foreignKey/localKey similar to HasMany but set a single
related entity via entity.relations[relationName]?.setRaw(related) and add that
related to relatedEntities; for ManyToMany perform the intermediate/pivot join
query the same way _getRelatedEntities() does (fetch related ids via pivot, then
load related entities) and setRaw the resulting list on each parent and add to
relatedEntities so eager loading covers HasOne and ManyToMany as well.
| if (relation is BelongsTo) { | ||
| final foreignKeyName = relation.foreignKey; | ||
| final foreignKeyValues = entities.map((e) => e.toDatumMap()[foreignKeyName]).nonNulls.toSet().toList(); | ||
|
|
||
| for (final entity in entities) { | ||
| final related = relatedEntitiesByParentId[entity.id] ?? []; | ||
| (entity as RelationalDatumEntity).relations[relationName]?.setRaw(related); | ||
| } | ||
| if (foreignKeyValues.isEmpty) return; | ||
|
|
||
| final fetched = await relatedManager.query( | ||
| DatumQuery( | ||
| filters: [ | ||
| Filter(relation.localKey, FilterOperator.isIn, foreignKeyValues) | ||
| ], | ||
| ), | ||
| source: source, | ||
| userId: userId, | ||
| ); | ||
|
|
||
| final byId = {for (var e in fetched) e.id: e}; | ||
|
|
||
| for (final entity in entities) { | ||
| final fk = entity.toDatumMap()[foreignKeyName]; | ||
| final related = byId[fk]; | ||
|
|
||
| entity.relations[relationName]?.setRaw(related); | ||
|
|
||
| if (related is RelationalDatumEntity) { | ||
| relatedEntities.add(related); | ||
| } | ||
| } | ||
| } else if (relation is HasMany) { | ||
| final foreignKeyName = relation.foreignKey; | ||
| final localKeyValues = entities.map((e) => e.id).toSet().toList(); | ||
|
|
||
| if (localKeyValues.isEmpty) return; | ||
|
|
||
| final fetched = await relatedManager.query( | ||
| DatumQuery( | ||
| filters: [ | ||
| Filter(foreignKeyName, FilterOperator.isIn, localKeyValues) | ||
| ], | ||
| ), | ||
| source: source, | ||
| userId: userId, | ||
| ); | ||
|
|
||
| final grouped = <String, List<RelationalDatumEntity>>{}; | ||
|
|
||
| for (final entity in fetched) { | ||
| final parentId = (entity as RelationalDatumEntity).toDatumMap()[foreignKeyName]; | ||
|
|
||
| (grouped[parentId] ??= []).add(entity); | ||
| relatedEntities.add(entity); | ||
| } | ||
|
|
||
| for (final entity in entities) { | ||
| final related = grouped[entity.id] ?? []; | ||
| entity.relations[relationName]?.setRaw(related); |
There was a problem hiding this comment.
Honor relation.localKey when batching and stitching.
BelongsTo filters on relation.localKey but then looks up fetched rows by e.id, and HasMany hard-codes entity.id on both sides of the join. Any relation using a non-id local key will attach the wrong children/parent here.
Suggested fix
if (relation is BelongsTo) {
final foreignKeyName = relation.foreignKey;
final foreignKeyValues = entities.map((e) => e.toDatumMap()[foreignKeyName]).nonNulls.toSet().toList();
if (foreignKeyValues.isEmpty) return;
final fetched = await relatedManager.query(
DatumQuery(
filters: [
Filter(relation.localKey, FilterOperator.isIn, foreignKeyValues)
],
),
source: source,
userId: userId,
);
- final byId = {for (var e in fetched) e.id: e};
+ final byLocalKey = {
+ for (final e in fetched) e.toDatumMap()[relation.localKey]: e,
+ };
for (final entity in entities) {
final fk = entity.toDatumMap()[foreignKeyName];
- final related = byId[fk];
+ final related = byLocalKey[fk];
entity.relations[relationName]?.setRaw(related);
if (related is RelationalDatumEntity) {
relatedEntities.add(related);
}
}
} else if (relation is HasMany) {
final foreignKeyName = relation.foreignKey;
- final localKeyValues = entities.map((e) => e.id).toSet().toList();
+ final localKeyValues = entities
+ .map((e) => e.toDatumMap()[relation.localKey])
+ .nonNulls
+ .toSet()
+ .toList();
if (localKeyValues.isEmpty) return;
final fetched = await relatedManager.query(
DatumQuery(
filters: [
Filter(foreignKeyName, FilterOperator.isIn, localKeyValues)
],
),
source: source,
userId: userId,
);
- final grouped = <String, List<RelationalDatumEntity>>{};
+ final grouped = <Object?, List<RelationalDatumEntity>>{};
for (final entity in fetched) {
final parentId = (entity as RelationalDatumEntity).toDatumMap()[foreignKeyName];
(grouped[parentId] ??= []).add(entity);
relatedEntities.add(entity);
}
for (final entity in entities) {
- final related = grouped[entity.id] ?? [];
+ final parentKey = entity.toDatumMap()[relation.localKey];
+ final related = grouped[parentKey] ?? const <RelationalDatumEntity>[];
entity.relations[relationName]?.setRaw(related);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/datum/lib/source/core/manager/datum_manager.dart` around lines 1021
- 1076, The code assumes the primary key is always "id" when stitching
relations; update both BelongsTo and HasMany branches to use relation.localKey
consistently: in the BelongsTo branch build the lookup map from fetched rows
using (fetchedItem as RelationalDatumEntity).toDatumMap()[relation.localKey]
instead of e.id so keys match the DatumQuery filter, and when reading the parent
key use entity.toDatumMap()[foreignKeyName] as now; in the HasMany branch
compute localKeyValues from entities.map((e) =>
e.toDatumMap()[relation.localKey]) and when assigning related children use
grouped[entity.toDatumMap()[relation.localKey]] (ensure grouping keys come from
fetched.toDatumMap()[foreignKeyName] and remain strings) so all joins use
relation.localKey rather than hard-coded id when calling relatedManager.query,
building grouped maps, and calling entity.relations[relationName]?.setRaw.
|
@Shreemanarjun I added test. Please check! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/datum/test/core/relational_data_test.dart (2)
886-886: Test name is misleading.The test is named
'query supports nested relation field (author.name)'which implies filtering or selecting by a nested field, but the test actually verifies eager loading of nested relations viawithRelated. Consider renaming to accurately describe what's being tested.✏️ Suggested rename
- test('query supports nested relation field (author.name)', () async { + test('query eager-loads nested relations via withRelated', () async {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/datum/test/core/relational_data_test.dart` at line 886, Rename the test description to accurately reflect that it's verifying eager loading of nested relations via withRelated rather than querying/filtering by a nested field; update the test named 'query supports nested relation field (author.name)' to something like 'eager loads nested relation via withRelated (author.name)' or 'withRelated eagerly loads nested relation author.name' so it clearly references the use of withRelated in the test body.
920-924: Consider adding assertions on the nested author's data.The test verifies that
authoris not null but doesn't assert on the actual author data. Adding assertions onauthor.idandauthor.namewould strengthen the test by confirming the correct entity was loaded.💪 Proposed enhancement
final author = postsWithAuthors?.first.relations['author']?.value; expect(postsWithAuthors?.length, 1, reason: 'Should find 1 post'); expect(author, isNotNull, reason: 'Post author should exist'); + expect((author as User).id, testUser.id, reason: 'Author ID should match testUser'); + expect(author.name, 'Kiddo V', reason: 'Author name should match'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/datum/test/core/relational_data_test.dart` around lines 920 - 924, The test currently checks postsWithAuthors and that author is not null but doesn't assert the author's fields; update the assertions to verify the nested author's identity by asserting expected values for author.id and author.name (for example compare to the source user in usersWithPosts.first or to the specific expected literals used when creating the fixture). Locate the variables postsWithAuthors and author in the test (and the usersWithPosts fixture) and add assertEquals-like expectations for author.id and author.name to confirm the correct User entity was loaded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/datum/test/core/relational_data_test.dart`:
- Around line 870-885: The setUp block creating testUser and testPost is missing
a call to Datum.resetForTesting(), which risks test-state leakage; update the
setUp for this test group to call Datum.resetForTesting() before creating
testUser/testPost (i.e., invoke Datum.resetForTesting() at the start of the
setUp for this group so the singleton Datum is cleared before the fixtures are
created).
---
Nitpick comments:
In `@packages/datum/test/core/relational_data_test.dart`:
- Line 886: Rename the test description to accurately reflect that it's
verifying eager loading of nested relations via withRelated rather than
querying/filtering by a nested field; update the test named 'query supports
nested relation field (author.name)' to something like 'eager loads nested
relation via withRelated (author.name)' or 'withRelated eagerly loads nested
relation author.name' so it clearly references the use of withRelated in the
test body.
- Around line 920-924: The test currently checks postsWithAuthors and that
author is not null but doesn't assert the author's fields; update the assertions
to verify the nested author's identity by asserting expected values for
author.id and author.name (for example compare to the source user in
usersWithPosts.first or to the specific expected literals used when creating the
fixture). Locate the variables postsWithAuthors and author in the test (and the
usersWithPosts fixture) and add assertEquals-like expectations for author.id and
author.name to confirm the correct User entity was loaded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5814a0f-2340-4d3c-ae70-1ba239a748cb
📒 Files selected for processing (1)
packages/datum/test/core/relational_data_test.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/datum/test/core/relational_data_test.dart`:
- Around line 919-927: The test accesses usersWithPosts.first without checking
the query result, which can throw a RangeError; update the test around
userManager.query / DatumQuery to assert usersWithPosts is not empty (e.g.,
expect or an explicit check) before using usersWithPosts.first, and only then
derive postsWithAuthors and author from that first element so the failure
message is clear and avoids an obscure RangeError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a66dce85-6204-4e02-b298-53a917605435
📒 Files selected for processing (1)
packages/datum/test/core/relational_data_test.dart
2 participants
This PR add support for
querywith nested relations, that currently missing for Datum.Related issue #22
Summary by CodeRabbit