-
Notifications
You must be signed in to change notification settings - Fork 200
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(datastore): eager loaded associations depth #520
Changes from 8 commits
ff84753
077b662
dc3d007
c2deecd
aa76580
8cf9738
d564e68
1bee870
4d82d76
fa9a41e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,78 +9,49 @@ import Amplify | |
import Foundation | ||
import SQLite | ||
|
||
/// Represents a `select` SQL statement associated with a `Model` instance and | ||
/// optionally composed by a `ConditionStatement`. | ||
struct SelectStatement: SQLStatement { | ||
|
||
let modelType: Model.Type | ||
let conditionStatement: ConditionStatement? | ||
let paginationInput: QueryPaginationInput? | ||
|
||
// TODO remove this once sorting support is added to DataStore | ||
// Used by plugin to order and limit results for system table queries | ||
let additionalStatements: String? | ||
let namespace = "root" | ||
|
||
init(from modelType: Model.Type, | ||
predicate: QueryPredicate? = nil, | ||
paginationInput: QueryPaginationInput? = nil, | ||
additionalStatements: String? = nil) { | ||
self.modelType = modelType | ||
|
||
var conditionStatement: ConditionStatement? | ||
if let predicate = predicate { | ||
let statement = ConditionStatement(modelType: modelType, | ||
predicate: predicate, | ||
namespace: namespace[...]) | ||
conditionStatement = statement | ||
} | ||
self.conditionStatement = conditionStatement | ||
self.paginationInput = paginationInput | ||
self.additionalStatements = additionalStatements | ||
} | ||
|
||
var stringValue: String { | ||
/// Support data structure used to hold information about `SelectStatement` than | ||
/// can be used later to parse the results. | ||
struct SelectStatementMetadata { | ||
|
||
typealias ColumnMapping = [String: (ModelSchema, ModelField)] | ||
|
||
let statement: String | ||
let columnMapping: ColumnMapping | ||
let bindings: [Binding?] | ||
|
||
// TODO remove additionalStatements once sorting support is added to DataStore | ||
static func get(from modelType: Model.Type, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we rename to something different than 'get'? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure thing, suggestions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this just be the constructor for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was also thinking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's an internal API, so I renamed to |
||
predicate: QueryPredicate? = nil, | ||
paginationInput: QueryPaginationInput? = nil, | ||
additionalStatements: String? = nil) -> SelectStatementMetadata { | ||
let rootNamespace = "root" | ||
let schema = modelType.schema | ||
let fields = schema.columns | ||
let tableName = schema.name | ||
var columnMapping: ColumnMapping = [:] | ||
var columns = fields.map { field -> String in | ||
return field.columnName(forNamespace: namespace) + " " + field.columnAlias() | ||
columnMapping.updateValue((schema, field), forKey: field.name) | ||
return field.columnName(forNamespace: rootNamespace) + " as " + field.columnAlias() | ||
} | ||
|
||
// eager load many-to-one/one-to-one relationships | ||
var joinStatements: [String] = [] | ||
for foreignKey in schema.foreignKeys { | ||
let associatedModelType = foreignKey.requiredAssociatedModel | ||
let associatedSchema = associatedModelType.schema | ||
let associatedTableName = associatedModelType.schema.name | ||
|
||
// columns | ||
let alias = foreignKey.name | ||
let associatedColumn = associatedSchema.primaryKey.columnName(forNamespace: alias) | ||
let foreignKeyName = foreignKey.columnName(forNamespace: "root") | ||
|
||
// append columns from relationships | ||
columns += associatedSchema.columns.map { field -> String in | ||
return field.columnName(forNamespace: alias) + " " + field.columnAlias(forNamespace: alias) | ||
} | ||
|
||
let joinType = foreignKey.isRequired ? "inner" : "left outer" | ||
|
||
joinStatements.append(""" | ||
\(joinType) join \(associatedTableName) as \(alias) | ||
on \(associatedColumn) = \(foreignKeyName) | ||
""") | ||
} | ||
let (joinedColumns, joinStatements, joinedColumnMapping) = joins(from: schema) | ||
columns += joinedColumns | ||
columnMapping.merge(joinedColumnMapping) { _, new in new } | ||
|
||
var sql = """ | ||
select | ||
\(joinedAsSelectedColumns(columns)) | ||
from \(tableName) as root | ||
from \(tableName) as "\(rootNamespace)" | ||
\(joinStatements.joined(separator: "\n")) | ||
""".trimmingCharacters(in: .whitespacesAndNewlines) | ||
|
||
if let conditionStatement = conditionStatement { | ||
var bindings: [Binding?] = [] | ||
if let predicate = predicate { | ||
let conditionStatement = ConditionStatement(modelType: modelType, | ||
predicate: predicate, | ||
namespace: rootNamespace[...]) | ||
bindings.append(contentsOf: conditionStatement.variables) | ||
sql = """ | ||
\(sql) | ||
where 1 = 1 | ||
|
@@ -101,12 +72,81 @@ struct SelectStatement: SQLStatement { | |
\(paginationInput.sqlStatement) | ||
""" | ||
} | ||
return SelectStatementMetadata(statement: sql, | ||
columnMapping: columnMapping, | ||
bindings: bindings) | ||
} | ||
|
||
/// Tuple that represents a pair of `columns` and `statements` | ||
private typealias JoinStatements = ([String], [String], ColumnMapping) | ||
|
||
/// Walk through the associations recursively to generate join statements. | ||
/// | ||
/// Implementation note: this should be revisited once we define support | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are doing eager loading now right? Let us add that to the comment. |
||
/// for explicit `eager` vs `lazy` associations. | ||
private static func joins(from schema: ModelSchema) -> JoinStatements { | ||
drochetti marked this conversation as resolved.
Show resolved
Hide resolved
|
||
var columns: [String] = [] | ||
var joinStatements: [String] = [] | ||
var columnMapping: ColumnMapping = [:] | ||
|
||
func visitAssociations(node: ModelSchema, namespace: String = "root") { | ||
for foreignKey in node.foreignKeys { | ||
let associatedModelType = foreignKey.requiredAssociatedModel | ||
let associatedSchema = associatedModelType.schema | ||
let associatedTableName = associatedModelType.schema.name | ||
|
||
// columns | ||
let alias = namespace == "root" ? foreignKey.name : "\(namespace).\(foreignKey.name)" | ||
let associatedColumn = associatedSchema.primaryKey.columnName(forNamespace: alias) | ||
let foreignKeyName = foreignKey.columnName(forNamespace: namespace) | ||
|
||
// append columns from relationships | ||
columns += associatedSchema.columns.map { field -> String in | ||
columnMapping.updateValue((associatedSchema, field), forKey: "\(alias).\(field.name)") | ||
return field.columnName(forNamespace: alias) + " as " + field.columnAlias(forNamespace: alias) | ||
} | ||
|
||
let joinType = foreignKey.isRequired ? "inner" : "left outer" | ||
|
||
joinStatements.append(""" | ||
\(joinType) join \(associatedTableName) as "\(alias)" | ||
on \(associatedColumn) = \(foreignKeyName) | ||
""") | ||
Comment on lines
+114
to
+117
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
visitAssociations(node: associatedModelType.schema, | ||
namespace: alias) | ||
} | ||
} | ||
visitAssociations(node: schema) | ||
|
||
return (columns, joinStatements, columnMapping) | ||
} | ||
|
||
} | ||
|
||
/// Represents a `select` SQL statement associated with a `Model` instance and | ||
/// optionally composed by a `ConditionStatement`. | ||
struct SelectStatement: SQLStatement { | ||
|
||
let modelType: Model.Type | ||
let metadata: SelectStatementMetadata | ||
|
||
init(from modelType: Model.Type, | ||
predicate: QueryPredicate? = nil, | ||
paginationInput: QueryPaginationInput? = nil, | ||
additionalStatements: String? = nil) { | ||
self.modelType = modelType | ||
self.metadata = .get(from: modelType, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A lot happens here in the init method, and I think this code path is sync with the developers code In future we should add some performance test around this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to performance benchmarks
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. |
||
predicate: predicate, | ||
paginationInput: paginationInput, | ||
additionalStatements: additionalStatements) | ||
} | ||
|
||
return sql | ||
var stringValue: String { | ||
metadata.statement | ||
} | ||
|
||
var variables: [Binding?] { | ||
return conditionStatement?.variables ?? [] | ||
metadata.bindings | ||
} | ||
|
||
} | ||
|
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.
Maybe create a github issue for this TODO to track?