model associations: refactor + missing use cases #153

drochetti · 2019-11-24T22:35:51Z

refactor model association logic

some cleanup and documentation added

Notes:

this is still work in progress, but enables basic cases of lazy loading of one-to-many associations
more cleanup, documentation and tests coming soon

lazy collection capable of loading model associations

Notes:

create a custom Collection that is capable of loading a model association on demand
these changes are needed in order for codegen to generate the correct Swift code

Notes:

still work in progress
new DSL for configuring associations: belongsTo, hasManny and hasOne
custom collection for lazy loading in progress

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

**Notes:** - still work in progess - new DSL for configuring associations: `belongsTo`, `hasManny` and `hasOne` - custom collection for lazy loading in progress

…ionships

**Notes:** - create a custom `Collection` that is capable of loading a model association on demand

…ionships

**Notes:** - this is still work in progress, but enables basic cases of lazy loading of one-to-many associations - more cleanup, documentation and tests coming soon

**Notes:** - temporarily set the iOS platform to 13.0 until we make Combine an optional/weak dependency

**Notes:** - change the mutability strategy for model types. Make the properties that can be mutated to `var` and rely on `struct` value type to keep references unique

**Notes:** - after saving, return the recently stored model so users can consume a fresh instance with lazy associations correctly bound

**Notes:** - made `DataStoreResult` compatible with the platform standard `Result<Success, Failure>` - added integration layer between `List<ModelType>` and Combine - added small layer to integrate `DataStoreResult` and Combine

**Notes:** - SQL statements with multiple tables require columns aliases so column names from different tables don't collide - main table in a SQL statement is always aliases to "root" - this strategy might need some refactoring for more complex queries

…iations

drochetti · 2019-11-25T10:03:34Z

Amplify/Categories/DataStore/Model/ModelField+Association.swift

+            case .belongsTo(let associatedKey),
+                 .hasOne(let associatedKey),
+                 .hasMany(let associatedKey):
+                let key = associatedKey?.stringValue ?? associatedModel.modelName


note to self: add camelCase handling to modelName

drochetti · 2019-11-25T10:05:05Z

AmplifyPlugins.podspec

@@ -17,7 +17,7 @@ Pod::Spec.new do |s|
  s.homepage     = 'http://aws.amazon.com/mobile/sdk'
  s.license      = 'Apache License, Version 2.0'
  s.author       = { 'Amazon Web Services' => 'amazonwebservices' }
-  s.platform     = :ios, '11.0'
+  s.platform     = :ios, '13.0'


this is meant to be temporary: right now apps won't compile because our code that consumes Combine is not fully protected by platform constraints.

Sorry about that--I've got it on my list to fix :(

no worries. I just changed it for now so I can build it on the sample app

drochetti · 2019-11-25T10:06:52Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/QueryPredicate+SQLite.swift

+        if tokens.count == 1 {
+            tokens.insert("root", at: 0)
+        }


right now root is added to top-level columns of a SQL statement. We need a more resilient and flexible aliasing logic. Both Hibernate and RoR ActiveRecord generate random names so they don't collapse with user named properties, we should look into that.

drochetti · 2019-11-25T10:09:41Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/Statement+Model.swift

+        let keyName = (propertyPrefix ?? "") + schema.primaryKey.sqlName
+        let indexOfId = columns.firstIndex(of: keyName)
+        if let index = indexOfId, let id = row[index] as? String, let cached = cache[id] {
+            return cached
+        }


this gave a huge performance boost to SQL result set -> model conversion. I believe this is still need more testing and most likely there are more performance improvements to be made, but I would say that so far so good. A query of 1300 rows, including a joint table with 50 rows took less than 80ms to de-serialize (as opposed to 200ms of the previous implementation).

That said, I'm not 100% happy with this logic, readability and caching strategy

#157 created to track it

palpatim · 2019-11-25T16:07:54Z

Amplify/Categories/DataStore/DataStoreCallback.swift

-    case error(_ error: DataStoreError)
-}
+/// - seealso: [DataStoreCallback](#DataStoreCallback)
+public typealias DataStoreResult<Success> = Result<Success, DataStoreError>


Thanks for simplifying this

np, I didn't know about Result when I created this. Luckily DataStoreResult always had the same semantics as Result<Success, Failure>, so the change was straightforward. The only impact was changing the enum cases everywhere.

palpatim · 2019-11-25T16:11:21Z

Amplify/Categories/DataStore/Model/Collection/List+Model.swift

+
+    // MARK: - ExpressibleByArrayLiteral
+
+    required convenience public init(arrayLiteral elements: Element...) {


What does it mean to instantiate a list from an array? I assume it's for the use case of creating a model with associations in memory, and persisting it after creation?

It means the List<Model> holds no reference to an association. The idea is that even though it has no reference to an association, therefore it looks like an ordinary array, it will hold pagination information (like limit, offset and totalCount). However, we haven't defined the pagination API on DataStore.query yet. Also, in the future it can have "cascading" capabilities.

That said, I need to finalize documentation for this and update the PR.

palpatim · 2019-11-25T16:16:04Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/ModelSchema+SQLite.swift

@@ -57,7 +57,7 @@ extension ModelField: SQLColumn {
    }

    var isForeignKey: Bool {
-        isRelationshipOwner
+        isAssociationOwner


What does this mean for a many-many association?

it means it's the association table. Detecting many-to-many right now is not perfect. We don't support "implicit" many-to-many (i.e. association table/model is defined by us for the user implicitly). Right now users have to create their own association model (like PostEditor defined in the docs here https://aws-amplify.github.io/docs/cli-toolchain/graphql#usage-2)

**Notes:** - moved some test bootstrap logic to a common parent class - added docs to the `List<ModelType>` - added tests for difference usage (async vs sync) of `List<ModelType>`

…iations

drochetti · 2019-11-26T00:38:02Z

Amplify/Categories/Predictions/Models/Landmark.swift

 // Inc. or its affiliates. All Rights Reserved.
 //
 // SPDX-License-Identifier: Apache-2.0
 //

+import CoreGraphics


note: the lack of this import currently breaks the build. I suppose this is a known issue

@kneekey23, @royjit are you aware of this?

* master: API Predicate to GraphQL filter variables (#150) model associations: refactor + missing use cases (#153) Basic unit tests for comprehend and translate (#160) CoreML logic to Detect faces (#155) API Category - Updates to APIError and GraphQLResponse (#133) Initial commit for unit test for translate service behavior (#158) bug fix change (#152)

…mplify#153)

* feat(Auth): Adding List WebAuthn API * feat(Auth): Adding associate and delete WebAuthn credentials APIs * Addding missing transports array in the credentials payload * Adding friendlyName to AuthWebAuthnCredential * Adding excludedCredentials to avoid multiple PassKeys for the same device * Adding pagination support in the list API * Renaming CredentialPayload to CredentialRegistrationPayload * Addressing PR comments

* feat(auth): adding support for email mfa (#3892) * feat(auth): adding support for email mfa * fix swift lint warning * worked on a review comment * adding integration tests wave 1 * integration tests wave 2 * integration tests wave 3 * Add test setup instructions wave 4 * Add edge case * update readme to include graphQL details * chore: initial commit to add sdk with passwordless models * chore: model update * feat(Auth): Adding WebAuthn APIs (#153) * feat(Auth): Adding List WebAuthn API * feat(Auth): Adding associate and delete WebAuthn credentials APIs * Addding missing transports array in the credentials payload * Adding friendlyName to AuthWebAuthnCredential * Adding excludedCredentials to avoid multiple PassKeys for the same device * Adding pagination support in the list API * Renaming CredentialPayload to CredentialRegistrationPayload * Addressing PR comments * feat(auth): add passwordless sign with otp (#151) * feat(auth): add passwordless OTP implementation * add fallback password and password srp flows * add web auth n states * modifying states * feat(Auth): Adding WebAuthn support to signIn and confirmSignIn APIs (#155) * feat(auth): add passwordless OTP implementation * add fallback password and password srp flows * add web auth n states * modifying states * feat(Auth): Implementing signIn with WebAuthn * Adding support for a presentation anchor in sign in and confirm sign in options * Fixing errors * Addressing PR comments * fix build error --------- Co-authored-by: Harshdeep Singh <6162866+harsh62@users.noreply.github.com> * fix: Fixing build issue when iOS 18/macOS 15 are not installed * feat(WebAuthn): Adding support for retrying a confirmSignIn with WebAuthn request, if the first one fails (#158) * feat(auth): add support for passwordless sign up and auto sign in (#160) * add autoSignIn() category API definitions (#152) * add autoSignIn() category API definitions * add sign up step for auto sign in * add state machine changes for autoSignIn() and signUp() (#154) * add autoSignIn() category API definitions * add sign up step for auto sign in * add state machine changes * add events and update resolvers * update sign up events and resolvers * add updates to resolver for auto sign in * update confirm sign up flow and debug code * Address review comments --------- Co-authored-by: Harsh <6162866+harsh62@users.noreply.github.com> * update auto sign state machine events and resolver (#157) * update auto sign state machine events and resolver * Address review comments * update sign up and auto sign in unit tests (#159) * update sign up and auto sign in unit tests * add auto sign in tests and refactor existing tests * Add more service error tests * Address review changes --------- Co-authored-by: Harsh <6162866+harsh62@users.noreply.github.com> * chore: fix building of unit tests after sign up rebase * feat(auth): adding passwordless sign in preferred flows (#162) * feat(auth): add passwordless preferred flow * adding confirm device and device srp flows to user auth * update message * worked on review comments * update * chore(auth): add more auto sign in and sign up state machine/e2e unit tests (#161) * chore(auth): add more auto sign in and sign up state machine/e2e unit tests * Address review comments * chore: updated SDK and models * chore: update integration test host app * fix: Fixing build errors in watchOS/tvOS due to missing prechecks. * feat(auth): adding an initial passwordless integration test with resources defined (#163) * chore: update no-auth API's in the resolver * chore: Updating to the renamed WebAuthn APIs (#164) * chore: Updating to the renamed WebAuthn APIs * Fixing unit tests * chore: Adding unit tests for the WebAuthn APIs Tasks (#165) * test: Adding AssociateWebAuthn unit tests * test: Adding ListWebAuthnCredentials unit tests * test: Adding DeleteWebAuthnCredential unit tests * chore: simplifying how webauthn errors are handled * adressing PR comments * chore(auth): add integration tests for passwordless signup and auto sign in (#166) * chore(auth): add integration tests for passwordless signup and auto sign in * remove unused code * refactor code * chore: add integration tests for sign in flows (#168) * chore: add integration tests for sign in flows * Update AuthSignInWithPasswordUsingUserAuthTests.swift * Add more integration tests * update * chore: update sdk to use the latest models * test: Adding integration tests for WebAuthn APIs (#169) * test: Adding integration tests for WebAuthn APIs * chore: Adding webauthn integration workflow * Refactoring the code to remove unnecesary waits and make it more easy to read * fix: Fixing service errors being reported as .unknown when sign in fails (#170) * fix: Fixing service errors being reported as .unknown when sign in fails. Also adding proper WebAuthn cases to the AWSCognitoAuthError enum. * addressing PR comment * fix(auth): fix resolvers and tasks for auto sign in when state machine is in signing in state (#172) * fix(auth): fix resolvers and tasks for auto sign in when state machine is in signin in state * fix indentation * feat: Adding visionOS support to the WebAuthn APIs (#171) * chore: using the latest version aws sdk * chore: update changes needed for the sdk updated * chore: fix swiftlint errors (#3921) * chore: running passwordless integration tests on GEN2 backend * chore: update test target for watchOS * chore: fix OTP integration tests * chore: update more integration tests --------- Co-authored-by: Sebastian Villena <97059974+ruisebas@users.noreply.github.com> Co-authored-by: Abhash Kumar Singh <thisisabhash@gmail.com>

drochetti added 5 commits November 22, 2019 02:36

refactor model association logic

e259156

**Notes:** - still work in progess - new DSL for configuring associations: `belongsTo`, `hasManny` and `hasOne` - custom collection for lazy loading in progress

Merge remote-tracking branch 'origin/master' into feature/model-relat…

e4f6712

…ionships

lazy collection capable of loading model associations

1be62af

**Notes:** - create a custom `Collection` that is capable of loading a model association on demand

Merge remote-tracking branch 'origin/master' into feature/model-relat…

151f56a

…ionships

some cleanup and documentation added

857fc72

**Notes:** - this is still work in progress, but enables basic cases of lazy loading of one-to-many associations - more cleanup, documentation and tests coming soon

drochetti requested review from palpatim, wooj2 and lawmicha November 24, 2019 22:35

drochetti self-assigned this Nov 24, 2019

drochetti added the datastore Issues related to the DataStore category label Nov 24, 2019

drochetti and others added 8 commits November 24, 2019 23:12

fix missing dependency on podspec

ae59ab2

**Notes:** - temporarily set the iOS platform to 13.0 until we make Combine an optional/weak dependency

model mutability

2a4a37c

**Notes:** - change the mutability strategy for model types. Make the properties that can be mutated to `var` and rely on `struct` value type to keep references unique

improve nonUniqueResult error

dc8b66f

query model after saving

a93ac3a

**Notes:** - after saving, return the recently stored model so users can consume a fresh instance with lazy associations correctly bound

add targetName to field DSL

2bedf74

DataStoreResult cleanup and better combine support

c279df3

**Notes:** - made `DataStoreResult` compatible with the platform standard `Result<Success, Failure>` - added integration layer between `List<ModelType>` and Combine - added small layer to integrate `DataStoreResult` and Combine

Merge remote-tracking branch 'origin/master' into feature/model-assoc…

eae0369

…iations

drochetti commented Nov 25, 2019

View reviewed changes

drochetti mentioned this pull request Nov 25, 2019

Improve SQL Result Set to Model conversion #157

Closed

palpatim reviewed Nov 25, 2019

View reviewed changes

drochetti added 2 commits November 25, 2019 16:24

add missing docs and more tests

2cfbc08

**Notes:** - moved some test bootstrap logic to a common parent class - added docs to the `List<ModelType>` - added tests for difference usage (async vs sync) of `List<ModelType>`

Merge remote-tracking branch 'origin/master' into feature/model-assoc…

470074e

…iations

drochetti commented Nov 26, 2019

View reviewed changes

palpatim approved these changes Nov 26, 2019

View reviewed changes

drochetti merged commit 18e9c10 into master Nov 26, 2019

palpatim deleted the feature/model-associations branch December 3, 2019 17:21

waleedbutt pushed a commit to hassan31/amplify-swift that referenced this pull request Sep 18, 2024

fix: Fixing video not being mirrored in the 'Get Ready' screen (aws-a…

75a7c3c

…mplify#153)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

model associations: refactor + missing use cases #153

model associations: refactor + missing use cases #153

drochetti commented Nov 24, 2019 •

edited

Loading

drochetti Nov 25, 2019

drochetti Nov 25, 2019

palpatim Nov 25, 2019

drochetti Nov 26, 2019

drochetti Nov 25, 2019

drochetti Nov 25, 2019 •

edited

Loading

drochetti Nov 25, 2019

palpatim Nov 25, 2019

drochetti Nov 25, 2019

palpatim Nov 25, 2019

drochetti Nov 25, 2019

palpatim Nov 25, 2019

drochetti Nov 25, 2019

drochetti Nov 26, 2019

palpatim Nov 26, 2019


		// MARK: - ExpressibleByArrayLiteral

		required convenience public init(arrayLiteral elements: Element...) {

model associations: refactor + missing use cases #153

model associations: refactor + missing use cases #153

Conversation

drochetti commented Nov 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drochetti Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drochetti commented Nov 24, 2019 •

edited

Loading

drochetti Nov 25, 2019 •

edited

Loading