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

fix(Model): retrieve correct associated target name #965

Merged
merged 22 commits into from
Jan 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,21 +31,13 @@ extension List {
internal func lazyLoad(_ completion: DataStoreCallback<Elements>) {

// if the collection has no associated field, return the current elements
guard let associatedId = self.associatedId,
let associatedField = self.associatedField else {
guard let associatedId = associatedId,
let associatedField = associatedField else {
completion(.success(elements))
return
}

// TODO: this is currently done by specific plugin implementations (API or DataStore)
// How to add this name resolution to Amplify?
let modelName = Element.modelName
var name = modelName.camelCased() + associatedField.name.pascalCased() + "Id"
if case let .belongsTo(_, targetName) = associatedField.association {
name = targetName ?? name
}

let predicate: QueryPredicate = field(name) == associatedId
let predicate: QueryPredicate = field(associatedField.name) == associatedId
ruiguoamz marked this conversation as resolved.
Show resolved Hide resolved
Amplify.DataStore.query(Element.self, where: predicate) {
switch $0 {
case .success(let elements):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,6 @@ class GraphQLConnectionScenario1Tests: XCTestCase {
wait(for: [getProjectAfterDeleteCompleted], timeout: TestCommonConstants.networkTimeout)
}

// TODO: This test will fail until https://github.com/aws-amplify/amplify-ios/pull/885 is merged in
func testListProjectsByTeamID() {
guard let team = createTeam(name: "name") else {
XCTFail("Could not create team")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,6 @@ class GraphQLConnectionScenario4Tests: XCTestCase {
wait(for: [getCommentAfterDeleteCompleted], timeout: TestCommonConstants.networkTimeout)
}

// TODO: This test will fail until https://github.com/aws-amplify/amplify-ios/pull/885 is merged in
func testListCommentsByPostID() {
guard let post = createPost(title: "title") else {
XCTFail("Could not create post")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ class GraphQLConnectionScenario5Tests: XCTestCase {
Amplify.reset()
}

// TODO: This test will fail until https://github.com/aws-amplify/amplify-ios/pull/885 is merged in
func testListPostEditorByPost() {
guard let post = createPost(title: "title") else {
XCTFail("Could not create post")
Expand Down Expand Up @@ -98,7 +97,6 @@ class GraphQLConnectionScenario5Tests: XCTestCase {
wait(for: [listPostEditorByPostIDCompleted], timeout: TestCommonConstants.networkTimeout)
}

// TODO: This test will fail until https://github.com/aws-amplify/amplify-ios/pull/885 is merged in
func testListPostEditorByUser() {
guard let post = createPost(title: "title") else {
XCTFail("Could not create post")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ extension GraphQLRequest: ModelSyncGraphQLRequestFactory {
operationType: .query)
documentBuilder.add(decorator: DirectiveNameDecorator(type: .sync))
if let predicate = predicate {
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter))
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter(for: modelSchema)))
}
documentBuilder.add(decorator: PaginationDecorator(limit: limit, nextToken: nextToken))
documentBuilder.add(decorator: ConflictResolutionDecorator(lastSync: lastSync))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,12 @@ extension GraphQLRequest: ModelGraphQLRequestFactory {
case .delete:
documentBuilder.add(decorator: ModelIdDecorator(id: model.id))
if let predicate = predicate {
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter))
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter(for: modelSchema)))
}
case .update:
documentBuilder.add(decorator: ModelDecorator(model: model))
if let predicate = predicate {
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter))
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter(for: modelSchema)))
}
}

Expand Down Expand Up @@ -206,7 +206,7 @@ extension GraphQLRequest: ModelGraphQLRequestFactory {
documentBuilder.add(decorator: DirectiveNameDecorator(type: .list))

if let predicate = predicate {
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter))
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter(for: modelType.schema)))
}

documentBuilder.add(decorator: PaginationDecorator())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,35 @@ import Amplify
public typealias GraphQLFilter = [String: Any]

protocol GraphQLFilterConvertible {
var graphQLFilter: GraphQLFilter { get }
func graphQLFilter(for modelSchema: ModelSchema?) -> GraphQLFilter
}

// Convert QueryPredicate to GraphQLFilter JSON, and GraphQLFilter JSON to GraphQLFilter
public struct GraphQLFilterConverter {

/// Serialize the translated GraphQL query variable object to JSON string.
/// - Warning: Although this has `public` access, it is intended for internal use and should not be used directly
/// by host applications. The behavior of this may change without warning.
public static func toJSON(_ queryPredicate: QueryPredicate,
modelSchema: ModelSchema,
options: JSONSerialization.WritingOptions = []) throws -> String {
let graphQLFilterData =
try JSONSerialization.data(withJSONObject: queryPredicate.graphQLFilter(for: modelSchema),
options: options)

guard let serializedString = String(data: graphQLFilterData, encoding: .utf8) else {
preconditionFailure("""
Could not initialize String from the GraphQL representation of QueryPredicate:
\(String(describing: graphQLFilterData))
""")
}

return serializedString
}

@available(*, deprecated, message: """
Use `toJSON(_:modelSchema:options)` instead. See https://github.com/aws-amplify/amplify-ios/pull/965 for more details.
""")
/// Serialize the translated GraphQL query variable object to JSON string.
public static func toJSON(_ queryPredicate: QueryPredicate,
options: JSONSerialization.WritingOptions = []) throws -> String {
Expand Down Expand Up @@ -47,11 +70,27 @@ public struct GraphQLFilterConverter {
/// Extension to translate a `QueryPredicate` into a GraphQL query variables object
extension QueryPredicate {

/// - Warning: Although this has `public` access, it is intended for internal use and should not be used directly
/// by host applications. The behavior of this may change without warning.
public func graphQLFilter(for modelSchema: ModelSchema?) -> GraphQLFilter {
Copy link
Contributor

@lawmicha lawmicha Jan 6, 2021

Choose a reason for hiding this comment

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

add doc comment for the public API, i see up above

/// Extension to translate a `QueryPredicate` into a GraphQL query variables object

perhaps this information can be on top of the method as well. just a suggestion since i recall the discussion regarding adding doc comment and warning, etc. something like

/// Retrieve the GraphQL representation of the `QueryPredicate`. This translates the `QueryPredicate` into a 
/// JSON object, useful for passing as the "filter" or "condition" input of the GraphQL variables. For example
/// <Query Predicate example to JSON string>
///
/// - Warning: Although this has `public` access, it is intended for internal use and should not be used directly
///   by host applications. The behavior of this may change without warning.

if let operation = self as? QueryPredicateOperation {
return operation.graphQLFilter(for: modelSchema)
} else if let group = self as? QueryPredicateGroup {
return group.graphQLFilter(for: modelSchema)
}

preconditionFailure(
"Could not find QueryPredicateOperation or QueryPredicateGroup for \(String(describing: self))")
Comment on lines +82 to +83
Copy link
Contributor

@lawmicha lawmicha Jan 6, 2021

Choose a reason for hiding this comment

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

no action needed just a thought exercise.. If developers use QueryPredicateConstant.all then it will fail. Is there a way for developers to get into this state?

  1. Calling API directly with a predicate .all and app will crash. This is probably okay since it is not supported to use .all
  2. Is there a scenario which is calling into DataStore, and then it gets synced to the API and fail here? If so, this may be a bigger concern, what should API do when they see a predicate that is ".all"?

Secondly, precondionFailure affects live apps as well. In one of my latest PR's, I've started using assert(false, "message") instead so that it crashes only during debug builds, and not on release builds

}

@available(*, deprecated, message: """
Use `graphQLFilter(for:)` instead. See https://github.com/aws-amplify/amplify-ios/pull/965 for more details.
""")
public var graphQLFilter: GraphQLFilter {
lawmicha marked this conversation as resolved.
Show resolved Hide resolved
if let operation = self as? QueryPredicateOperation {
return operation.graphQLFilter
return operation.graphQLFilter(for: nil)
} else if let group = self as? QueryPredicateGroup {
return group.graphQLFilter
return group.graphQLFilter(for: nil)
}

preconditionFailure(
Expand All @@ -60,30 +99,52 @@ extension QueryPredicate {
}

extension QueryPredicateOperation: GraphQLFilterConvertible {
var graphQLFilter: GraphQLFilter {
return [field: [self.operator.graphQLOperator: self.operator.value]]

func graphQLFilter(for modelSchema: ModelSchema?) -> GraphQLFilter {
let filterValue = [self.operator.graphQLOperator: self.operator.value]
guard let modelSchema = modelSchema else {
return [field: filterValue]
}
return [columnName(modelSchema): filterValue]
}

func columnName(_ modelSchema: ModelSchema) -> String {
guard let modelField = modelSchema.field(withName: field) else {
return field
ruiguoamz marked this conversation as resolved.
Show resolved Hide resolved
}
let defaultFieldName = modelSchema.name.camelCased() + field.pascalCased() + "Id"
switch modelField.association {
case .belongsTo(_, let targetName):
return targetName ?? defaultFieldName
case .hasOne(_, let targetName):
return targetName ?? defaultFieldName
default:
return field
}
}
}

extension QueryPredicateGroup: GraphQLFilterConvertible {
var graphQLFilter: GraphQLFilter {

func graphQLFilter(for modelSchema: ModelSchema?) -> GraphQLFilter {
let logicalOperator = type.rawValue
switch type {
case .and, .or:
var graphQLPredicateOperation = [logicalOperator: [Any]()]
predicates.forEach { predicate in
graphQLPredicateOperation[logicalOperator]?.append(predicate.graphQLFilter)
graphQLPredicateOperation[logicalOperator]?.append(predicate.graphQLFilter(for: modelSchema))
}
return graphQLPredicateOperation
case .not:
if let predicate = predicates.first {
return [logicalOperator: predicate.graphQLFilter]
return [logicalOperator: predicate.graphQLFilter(for: modelSchema)]
} else {
preconditionFailure("Missing predicate for \(String(describing: self)) with type: \(type)")
}
}
}
}

extension QueryOperator {
var graphQLOperator: String {
switch self {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class GraphQLListQueryTests: XCTestCase {
var documentBuilder = ModelBasedGraphQLDocumentBuilder(modelSchema: Post.schema, operationType: .query)
documentBuilder.add(decorator: DirectiveNameDecorator(type: .list))
documentBuilder.add(decorator: PaginationDecorator())
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter))
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter(for: Post.schema)))
let document = documentBuilder.build()
let expectedQueryDocument = """
query ListPosts($filter: ModelPostFilterInput, $limit: Int) {
Expand Down Expand Up @@ -115,14 +115,75 @@ class GraphQLListQueryTests: XCTestCase {
XCTAssertEqual(String(data: filterJSON!, encoding: .utf8), expectedFilterJSON)
}

func testComment4BelongsToPost4Success() {
let comment4 = Comment4.keys
let predicate = comment4.id == "comment4Id" && comment4.post == "post4Id"

var documentBuilder = ModelBasedGraphQLDocumentBuilder(modelSchema: Comment4.schema,
operationType: .query)
documentBuilder.add(decorator: DirectiveNameDecorator(type: .list))
documentBuilder.add(decorator: PaginationDecorator())
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter(for: Comment4.schema)))
let document = documentBuilder.build()
let expectedQueryDocument = """
query ListComment4s($filter: ModelComment4FilterInput, $limit: Int) {
listComment4s(filter: $filter, limit: $limit) {
items {
id
content
postID
__typename
}
nextToken
}
}
"""
XCTAssertEqual(document.name, "listComment4s")
XCTAssertEqual(document.stringValue, expectedQueryDocument)
guard let variables = document.variables else {
XCTFail("The document doesn't contain variables")
return
}
XCTAssertNotNil(variables["limit"])
XCTAssertEqual(variables["limit"] as? Int, 1_000)

guard let filter = variables["filter"] as? GraphQLFilter else {
XCTFail("variables should contain a valid filter")
return
}

// Test filter for a valid JSON format
let filterJSON = try? JSONSerialization.data(withJSONObject: filter,
options: .prettyPrinted)
XCTAssertNotNil(filterJSON)

let expectedFilterJSON = """
{
"and" : [
{
"id" : {
"eq" : "comment4Id"
}
},
{
"postID" : {
"eq" : "post4Id"
}
}
]
}
"""
XCTAssertEqual(String(data: filterJSON!, encoding: .utf8), expectedFilterJSON)
}

func testListGraphQLQueryFromSimpleModelWithSyncEnabled() {
let post = Post.keys
let predicate = post.id.eq("id") && (post.title.beginsWith("Title") || post.content.contains("content"))

var documentBuilder = ModelBasedGraphQLDocumentBuilder(modelSchema: Post.schema, operationType: .query)
documentBuilder.add(decorator: DirectiveNameDecorator(type: .list))
documentBuilder.add(decorator: PaginationDecorator())
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter))
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter(for: Post.schema)))
documentBuilder.add(decorator: ConflictResolutionDecorator())
let document = documentBuilder.build()
let expectedQueryDocument = """
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class GraphQLSyncQueryTests: XCTestCase {

var documentBuilder = ModelBasedGraphQLDocumentBuilder(modelSchema: Post.schema, operationType: .query)
documentBuilder.add(decorator: DirectiveNameDecorator(type: .sync))
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter))
documentBuilder.add(decorator: FilterDecorator(filter: predicate.graphQLFilter(for: Post.schema)))
documentBuilder.add(decorator: PaginationDecorator(limit: 100, nextToken: "token"))
documentBuilder.add(decorator: ConflictResolutionDecorator(lastSync: 123))
let document = documentBuilder.build()
Expand Down
Loading