fix(datastore): cascade deleted for nested Has Many

[iartemiev]

Description of changes

Fixes bug when deleting a record of a model with > 1 nested Has Many descendants
Ex:

type Blog @model {
  id: String!
  posts: [Post] @hasMany
}

type Post @model {
  id: String!
  comments: [Comment] @hasMany
}

type Comment @model {
  id: String!
}

Behavior before the change in this PR:

1. await DataStore.delete(Blog, 'some-id')
2. deleteTraverse retrieves all Post records from the client database that are related to the Blog record and recursively calls deleteTraverse, passing the retrieved post records as POJOs (link to code)
3. since Post has a Has Many relationship to Comment, deleteTraverse attempts to retrieve the related Comment records, but first has to extract PK metadata from the posts. Since the posts are POJOs, instead of model instances, they don't contain the necessary data and cause the error in the linked issue.

Note: If the child model does not have descendant relationships defined in the schema (e.g. if Post did not have a Has Many relationship to Comment) we wouldn't attempt to retrieve its descendants in step 3. so the bug would not show up with a relational graph of depth 2. Only 3+.

The change in this PR instantiates models from the records retrieved in step 2 and passes the models in the recursive call, which fixes the bug.

Issue #, if available

#10866

Description of how you validated changes

• Red/green unit test
• Manual testing

Checklist

• PR description included
• yarn test passes
• Tests are changed or added
• Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[codecov-commenter]

Codecov Report

Merging #10880 (e9f38ef) into main (76193f3) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #10880      +/-   ##
==========================================
+ Coverage   81.57%   81.59%   +0.02%     
==========================================
  Files         198      198              
  Lines       19584    19603      +19     
  Branches     4227     4230       +3     
==========================================
+ Hits        15975    15996      +21     
+ Misses       3318     3316       -2     
  Partials      291      291              

Impacted Files	Coverage Δ
packages/datastore/__tests__/commonAdapterTests.ts	98.20% <100.00%> (+0.04%)	⬆️
packages/datastore/__tests__/helpers.ts	84.56% <100.00%> (+0.03%)	⬆️
...tastore/src/storage/adapter/AsyncStorageAdapter.ts	94.03% <100.00%> (+0.08%)	⬆️
.../datastore/src/storage/adapter/IndexedDBAdapter.ts	88.95% <100.00%> (+0.27%)	⬆️
packages/datastore/src/sync/processors/mutation.ts	82.15% <0.00%> (+0.46%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[svidgen]

I think I see why this fixes the problem. But, I have one concern, noted in diff.

I'm also very curious whether this problem becomes simpler if handled outside the adapter. I'm thinking we should have an adapter-independent DeleteTraversal class or util function, and the adapters should just accept a list of items to delete (transactionally if supported, for the efficiency's sake if nothing else).

[iartemiev]

I'm also very curious whether this problem becomes simpler if handled outside the adapter. I'm thinking we should have an adapter-independent DeleteTraversal class or util function, and the adapters should just accept a list of items to delete (transactionally if supported, for the efficiency's sake if nothing else).

Abstracting out parts of the storage adapter implementations would avoid repetition, but the change in this PR would still apply, i.e. we'd still need to instantiate the models at some point to get the PK metadata.

[svidgen]

I'm also very curious whether this problem becomes simpler if handled outside the adapter. I'm thinking we should have an adapter-independent DeleteTraversal class or util function, and the adapters should just accept a list of items to delete (transactionally if supported, for the efficiency's sake if nothing else).

Abstracting out parts of the storage adapter implementations would avoid repetition, but the change in this PR would still apply.

Woops. I posted a half-completed thought here! 🧠 Yes. I agree! And this is also outside of scope for the PR anyway. Seeing redundant changes and yet another "not relevant for SQLiteAdapter" test bumps the urgency level of some refactoring here. So, I wanted to plant the seed and looking for a +/- reaction to the idea. If it inspired a followup, I wouldn't be opposed. I also wouldn't oppose to adding an item to the backlog.

[iartemiev]

That's fair. I think we should merge this to unblock folks that are depending on it first (provided there aren't any issues with the PR). But I'll create a backlog task to refactor. It'll be a non-trivial effort because despite a lot of high level similarity, there are many adapter-specific code differences that we'll need to find a way to express elegantly.

[svidgen]

Thanks for your patience explaining this to me. Does the suggested comment below on the new test make sense?

[david-mcafee]

LGTM!