New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(@aws-amplify/datastore-storage-adapter): SQLiteAdapter fails on required related models #10720
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…need to generate a pre-cpk variant
svidgen
commented
Nov 28, 2022
packages/datastore-storage-adapter/__tests__/SQLiteAdapter.test.ts
Outdated
Show resolved
Hide resolved
Adding some hope-fully helpful comments.
iartemiev
approved these changes
Nov 28, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏻
Codecov Report
@@ Coverage Diff @@
## main #10720 +/- ##
==========================================
+ Coverage 85.65% 85.71% +0.05%
==========================================
Files 196 196
Lines 18261 18335 +74
Branches 3892 3900 +8
==========================================
+ Hits 15642 15715 +73
- Misses 2543 2544 +1
Partials 76 76
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
dpilch
approved these changes
Nov 29, 2022
This was referenced Aug 5, 2023
Closed
Closed
Closed
Closed
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
Adding
manyToMany
tests revealed that the SQLiteAdapter was failing to handle required related models correctly. As it was loading from the database, the adapter was attempting to initialize records withnull
values for the required related-model fields, rather than allowing these fields to be lazy-loaded using the given FK's. This was causing operations against models with requiredbelongsTo
andhasOne
fields to throw errors, due to the fields being initializednull
.I'm not completely sure why these fields are present in the SQLite schema to begin with, so I'm leaving them in for now. However, this change fixes the issue by filtering related model fields out of the initialization entirely. Only the FK's should be populated.
Issue #, if available
#10718
Description of how you validated changes
Added unit tests. Red/Green'd the tests.
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.