Skip to content
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

feat(datastore): support of custom primary key #1641

Merged
merged 10 commits into from
Jul 20, 2022

Conversation

HuiSF
Copy link
Member

@HuiSF HuiSF commented May 26, 2022

!!Do not merge until required versions of amplify-ios and amplify-android are released.

Recommend to review by commit.

PR checks will fail as required amplify-ios and amplify-android interface changes do not exist yet.

Issue #, if available:

#1426

Description of changes:

  • Interface changes to support custom primary key
  • native implementation to integration custom primary key functionality of amplify-ios and amplify-android
  • relevant unit tests
  • relevant integration tests

Pending Changes

  • [ ] GraphQL API model helper user case (out of scope)
  • model association with custom primary key
  • complete integration test suites covering model association with custom primary key

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@HuiSF HuiSF requested a review from a team as a code owner May 26, 2022 23:38
@HuiSF HuiSF marked this pull request as draft May 26, 2022 23:39
@HuiSF HuiSF marked this pull request as ready for review June 29, 2022 16:19
dnys1
dnys1 previously approved these changes Jun 29, 2022
@@ -30,7 +30,6 @@ class FlutterSubscriptionDataProcessedEvent(
"modelName" to model.modelName,
"element" to mapOf(
"syncMetadata" to mapOf(
"id" to syncMetadata.id,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this important to include?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With custom primary key, id is no longer a mandatory field.

Model identifier (either transition single id field, or a composite key formed by multiple model fields) can always be determined by schema + model fields values. In addition, this field has not been used any where. So this field can be safely removed.

fun toValueMap(): Map<String, Any?> {
return mapOf(
"id" to this.metadata.model.id,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question -

Isn't this important to include?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

packages/amplify_core/lib/src/types/models/model.dart Outdated Show resolved Hide resolved
.modelSchema(schema)
.serializedData(serializedModelData)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing the order here matters which is why you've made the change throughout? If that's the case, can we possibly create a wrapper class/method that abstracts away the need to remember this? It seems like an easy thing to miss.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's due to builder step interface change in amplify-android.
We don't need to remember the order, compiler will let us know if the order is wrong. So we don't need a wrapper either.

packages/amplify_core/lib/src/types/models/model.dart Outdated Show resolved Hide resolved
@HuiSF HuiSF force-pushed the feat/custom-primary-key branch 2 times, most recently from cc9babb to 1a50262 Compare July 8, 2022 20:25
poojamat
poojamat previously approved these changes Jul 9, 2022
dnys1
dnys1 previously approved these changes Jul 15, 2022
@HuiSF HuiSF dismissed stale reviews from dnys1 and poojamat via bdbe950 July 19, 2022 21:44
- CRLF to LF
- Source code reformatting and fixing linter issues
@HuiSF HuiSF force-pushed the feat/custom-primary-key branch 3 times, most recently from b9afc16 to 90f1a44 Compare July 19, 2022 23:18
dnys1
dnys1 previously approved these changes Jul 20, 2022
packages/amplify_core/lib/src/types/models/model.dart Outdated Show resolved Hide resolved
- Add ModelIndex ModelIdentifier interface
- Make Model non-abstract class to prevent unnecessary breaking changes
- Add indexes to model schema
- Add query by model identifier interface
dnys1
dnys1 previously approved these changes Jul 20, 2022
Copy link
Member

@haverchuck haverchuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one comment about mavenLocal, otherwise LGTM.

@HuiSF HuiSF merged commit 3eaa2ac into aws-amplify:main Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants