fix(datastore): eager loaded associations depth #520

drochetti · 2020-06-04T13:18:11Z

Issue #503

The associations fetching code (inner/left joins) was only fetch one level deep, causing issues when decoding into Swift structs that had required properties that were missing from the SQL result set.

This change not only simplifies a lot the result set parsing, but also adds recursion to find all required associations of a given model to make sure data contracts will be fulfilled when using Swift decode.

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

…ecoding

royjit · 2020-06-05T17:24:23Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/SQLStatement+Select.swift

+    let columnMapping: ColumnMapping
+    let bindings: [Binding?]
+
+    // TODO remove additionalStatements once sorting support is added to DataStore


Maybe create a github issue for this TODO to track?

royjit · 2020-06-05T17:50:17Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/SQLStatement+Select.swift

+    let bindings: [Binding?]
+
+    // TODO remove additionalStatements once sorting support is added to DataStore
+    static func get(from modelType: Model.Type,


Should we rename to something different than 'get'?

sure thing, suggestions?

can this just be the constructor for SelectStatementMetadata?

make metaData(from:), statementMetaData(from:) or why not an init method?

can this just be the constructor for SelectStatementMetadata?

using the constructor is tricky because I have to execute the entire logic inside the constructor (no other functions, etc) because I need to initialize any struct property.

That's why I went with the static factory function, otherwise the constructor becomes a mess.

I was also thinking .selectStatementMetadata(from:)

it's an internal API, so I renamed to .metadata(), feel free to change that later as it has no impact on customers.

royjit · 2020-06-05T18:17:06Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/SQLStatement+Select.swift

+
+    /// Walk through the associations recursively to generate join statements.
+    ///
+    /// Implementation note: this should be revisited once we define support


We are doing eager loading now right? Let us add that to the comment.

wooj2 · 2020-06-05T20:24:44Z

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

+internal extension Dictionary where Key == String, Value == Any? {
+
+    /// Utility to create a `NSMutableDictionary` from a Swift `Dictionary<String, Any?>`.
+    func mutableCopy() -> NSMutableDictionary {


I think this name should be something like nsMutableCopy... because we are converting a data structure (dictionary) to an NS data structure?

sure... but doesn't the return type make that clear?

wooj2 · 2020-06-05T20:26:26Z

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

+    ///   - value: the value to be set
+    ///   - key: the key as a `String`
+    func updateValue(_ value: Value?, forKey key: String) {
+        setObject(value as Any, forKey: key as NSString)


If value is nil, i'm not sure if this works, because I don't think NSDictionaries can hold nil (i believe in obj-c we'd have to set it to [NSNull null]?

Good call, fixing it!

wooj2 · 2020-06-05T20:30:23Z

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

+    /// - Parameters:
+    ///   - value: the value to be set
+    ///   - keyPath: the key path as a `String` (e.g. "nested.key")
+    func updateValue(_ value: Value?, forKeyPath keyPath: String) {


Hmm stupid question: what if we have something like nested..key. will keyComponents be 2 or 3? Maybe there's some extra sanity checks on the data that is being passed in here?

that's an invalid keyPath. We could add sanity checks, but it wouldn't make a difference because this is used in a controlled environment. The keyPath passed here is created by SelectStatement, is select statement creates an invalid key, those tests will fail. If tests don't catch it, the SQL operation will fail before it gets here.

but good question, I don't know how https://developer.apple.com/documentation/objectivec/nsobject/1418139-setvalue behaves in those cases. It would be good to mimic its behavior

wooj2 · 2020-06-05T20:49:59Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/SQLStatement+Select.swift

+                joinStatements.append("""
+                \(joinType) join \(associatedTableName) as "\(alias)"
+                  on \(associatedColumn) = \(foreignKeyName)
+                """)


Being ultra paranoid, do we care to filter out names of tables that might impact these statements. Like... it would be bad if someone had a table called Null or as (or something malicious)... maybe something to do for another PR.

they are escaped with double quotes, so you can even pass a SQL statement there and nothing would happen.

that said, the names here are a result of the models, so only valid Swift identifiers can be passed here.

royjit · 2020-06-05T20:28:52Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/SQLStatement+Select.swift

+    let bindings: [Binding?]
+
+    // TODO remove additionalStatements once sorting support is added to DataStore
+    static func get(from modelType: Model.Type,


make metaData(from:), statementMetaData(from:) or why not an init method?

royjit · 2020-06-05T20:32:20Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/SQLStatement+Select.swift

+         paginationInput: QueryPaginationInput? = nil,
+         additionalStatements: String? = nil) {
+        self.modelType = modelType
+        self.metadata = .get(from: modelType,


A lot happens here in the init method, and I think this code path is sync with the developers code Amplify.DataStore.query. Is this an expensive logic that can affect the query api?

In future we should add some performance test around this.

+1 to performance benchmarks

Is this an expensive logic that can affect the query api?

No, it's not expensive. It generates a string based on a Swift struct. Of course there are tons of optimizations we could do, like caching the queries, etc. But as of now I have no concerns.

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/SQLStatement+Select.swift

lawmicha · 2020-06-05T21:03:41Z

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

    /// - Returns: an array of `Model` of the specified type
-    func convert<M: Model>(to modelType: M.Type) throws -> [M]
+    func convert<M: Model>(to modelType: M.Type,
+                           using statement: SelectStatement) throws -> [M]


can just pass over ColumnMapping instead of SelectStatement

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

lawmicha · 2020-06-05T21:21:17Z

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

+                let associations = schema.fields.values.filter {
+                    $0.isArray && $0.hasAssociation
+                }
+                let prefix = field.name.replacingOccurrences(of: field.name, with: "")


what is the prefix here? isn't it just ""?

it depends on how many levels deep you are in the object graph. It starts with "", it becomes "post." then "post.blog."

…ecoding

drochetti added 2 commits June 4, 2020 06:13

fix(datastore): eager loaded associations depth - fixes #503

ff84753

Merge remote-tracking branch 'origin/master' into fix/sql-statement-d…

077b662

…ecoding

drochetti added the datastore Issues related to the DataStore category label Jun 4, 2020

drochetti added 2 commits June 4, 2020 12:17

add documentation

dc3d007

Merge remote-tracking branch 'origin/master' into fix/sql-statement-d…

c2deecd

…ecoding

drochetti marked this pull request as ready for review June 4, 2020 19:27

drochetti requested review from lawmicha and wooj2 June 4, 2020 19:27

drochetti added 3 commits June 4, 2020 13:22

remove ConvertCache references

aa76580

Merge remote-tracking branch 'origin/master' into fix/sql-statement-d…

8cf9738

…ecoding

wrap sql alias in quotes

d564e68

royjit reviewed Jun 5, 2020

View reviewed changes

update changelog

1bee870

wooj2 reviewed Jun 5, 2020

View reviewed changes

royjit reviewed Jun 5, 2020

View reviewed changes

lawmicha reviewed Jun 5, 2020

View reviewed changes

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/SQLStatement+Select.swift Outdated Show resolved Hide resolved

lawmicha reviewed Jun 5, 2020

View reviewed changes

drochetti added 2 commits June 5, 2020 15:06

address PR feedback

4d82d76

Merge remote-tracking branch 'origin/master' into fix/sql-statement-d…

fa9a41e

…ecoding

drochetti requested review from royjit, lawmicha and wooj2 June 5, 2020 22:12

lawmicha approved these changes Jun 5, 2020

View reviewed changes

drochetti merged commit de04410 into master Jun 5, 2020

drochetti deleted the fix/sql-statement-decoding branch June 5, 2020 23:05

drochetti mentioned this pull request Jun 24, 2020

Improve SQL Result Set to Model conversion #157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(datastore): eager loaded associations depth #520

fix(datastore): eager loaded associations depth #520

drochetti commented Jun 4, 2020 •

edited

Loading

royjit Jun 5, 2020

royjit Jun 5, 2020 •

edited

Loading

drochetti Jun 5, 2020

lawmicha Jun 5, 2020

royjit Jun 5, 2020

drochetti Jun 5, 2020

lawmicha Jun 5, 2020

drochetti Jun 5, 2020

royjit Jun 5, 2020

wooj2 Jun 5, 2020

drochetti Jun 5, 2020

wooj2 Jun 5, 2020

drochetti Jun 5, 2020

wooj2 Jun 5, 2020

drochetti Jun 5, 2020

wooj2 Jun 5, 2020 •

edited

Loading

drochetti Jun 5, 2020

royjit Jun 5, 2020

royjit Jun 5, 2020 •

edited

Loading

drochetti Jun 5, 2020 •

edited

Loading

lawmicha Jun 5, 2020

lawmicha Jun 5, 2020

drochetti Jun 5, 2020

fix(datastore): eager loaded associations depth #520

fix(datastore): eager loaded associations depth #520

Conversation

drochetti commented Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

royjit Jun 5, 2020 • 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

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

wooj2 Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

royjit Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

drochetti Jun 5, 2020 • 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

drochetti commented Jun 4, 2020 •

edited

Loading

royjit Jun 5, 2020 •

edited

Loading

wooj2 Jun 5, 2020 •

edited

Loading

royjit Jun 5, 2020 •

edited

Loading

drochetti Jun 5, 2020 •

edited

Loading