GraphQLDocument Builder #309

lawmicha · 2020-02-05T22:24:46Z

This adds a GraphQLDocument Builder class which holds a list of decorators for intercepting the document to decorate it with graphql related components using Model related data

list of decorators

ConflictResolutionDecorator
DirectiveDecorator
ModelDecorator
ModelIdDecorator
PredicateDecorator

Usage

var documentBuilder = SingleDirectiveGraphQLDocumentBuilder(modelType: Post.self, operationType: .mutation)
documentBuilder.add(decorator: DirectiveDecorator(type: .update))
documentBuilder.add(decorator: ModelDecorator(model: post))
let document = documentBuilder.build()

The currently split between APIPlugin's use of GraphQLRequest and DataStore's are implemented in the GraphQLRequest extensions, I tried not to make many changes here other than consolidating the methods places which these methods live

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

palpatim

We're getting much closer. I have some concerns about a few things that I called out. In particular, I'd like to understand Selection Set handling better.

palpatim · 2020-02-06T21:59:01Z

AmplifyPlugins/Core/AWSPluginsCore/Model/Decorator/DirectiveDecorator.swift

+
+/// Holds all of the possible operations such as `.get`, `.list`, `.sync`, `create`, `update`, `delete`, `onCreate`,
+/// `onUpdate`, `onDelete` defined, for each GraphQL operation type (query, mutation, subscription)
+enum GraphQLOperationSubType {


What's the difference between an "operation type" and an "operation subtype"? Why is a "subtype" hierarchy in the DirectiveDecorator as opposed to being declared & managed with the "super type"?

Don't we have one or two representations of this concept already? I'm not sure exactly where, but I remember having to translate "mutation" between a couple of contexts.

I mainly used this to as a private property on DirectiveDecorator to hold one of the values of GraphQLQueryType, GraphQLSubscriptionType, or GraphQLMutationType. Since I'm just exposing 3 separate constructors for the DirectiveNameDecorator, this type can actually be removed completely.

ie. caller can only do one of the following anyways:

public init(queryType: GraphQLQueryType) public init(mutationType: GraphQLMutationType) public init(subscriptionType: GraphQLSubscriptionType)

AmplifyPlugins/Core/AWSPluginsCore/Model/Decorator/DirectiveDecorator.swift

AmplifyPlugins/Core/AWSPluginsCore/Model/Decorator/ModelDecorator.swift

AmplifyPlugins/Core/AWSPluginsCore/Model/GraphQLDocument/GraphQLQuery.swift

palpatim · 2020-02-06T22:12:07Z

AmplifyPlugins/Core/AWSPluginsCore/Model/GraphQLDocument/GraphQLSubscription.swift

+                name: String,
+                inputs: [GraphQLParameterName: GraphQLDocumentInput],
+                selectionSet: SelectionSet?) {
+        self.operationType = operationType


here and above: do we need to init with an OperationType if we're creating a GraphQLSubscription ? Wouldn't the OperationType be inferrable in this init?

the reason why there is an all parameter init is for the deep copy method created for the Decorators to return a new instance of the document after decorating

public func copy(operationType: GraphQLOperationType? = nil, name: String? = nil, inputs: [GraphQLParameterName: GraphQLDocumentInput]? = nil, selectionSet: SelectionSet? = nil) -> Self { return Self.init(operationType: operationType ?? self.operationType, name: name ?? self.name, inputs: inputs ?? self.inputs, selectionSet: selectionSet ?? self.selectionSet) }

palpatim · 2020-02-06T22:38:37Z

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/ModelSchema+GraphQL.swift

-    var graphQLName: String {
-        name
+    /// The GraphQL directive name translated from the GraphQL operation and model schema data
+    func graphQLName(operationSubType: GraphQLOperationSubType) -> String {


This feels like an appropriate use of an extension: taking a custom type and extending it with more custom behavior that doesn't necessarily represent the core functionality of the type.

ended up removing GraphQLOperationSubType and creating three extensions with parameters taking in GraphQLQueryType' GraphQLMutationType, etc

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/ModelSchema+GraphQL.swift

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/SelectionSet.swift

palpatim · 2020-02-06T22:46:56Z

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/InitialSync/InitialSyncOperation.swift

+        let request = GraphQLRequest<SyncQueryResult>.syncQuery(modelType: modelType,
+                                                                nextToken: nextToken,
+                                                                lastSync: lastSyncTime)


Much nicer!

palpatim · 2020-02-11T15:38:21Z

Amplify/Core/Support/String+Casing.swift

@@ -30,4 +30,9 @@ extension String {
    public func camelCased() -> String {
        return prefix(1).lowercased() + dropFirst()
    }
+


nit: rename file since it's no longer just "casing" :)

updating to String+Extensions (hmm, couldn't think of something better lol)

palpatim · 2020-02-11T15:39:06Z

AmplifyPlugins/API/AWSAPICategoryPluginFunctionalTests/AnyModelIntegrationTests.swift

@@ -50,7 +50,7 @@ class AnyModelIntegrationTests: XCTestCase {

        let callbackInvoked = expectation(description: "Callback invoked")
        var responseFromOperation: GraphQLResponse<AnyModel>?
-        _ = Amplify.API.mutate(ofAnyModel: anyPost, type: .create) { response in
+        _ = Amplify.API.mutate(of: anyPost, type: .create) { response in


I think we discussed at some point removing the of: so the API would be API.mutate(anyPost, type: .create) ?

ah yes, i still have those captured in the previous PR: https://github.com/aws-amplify/amplify-ios/blob/7db239ddb6e303fb9dad876033286f0c0ca8979a/Amplify/Categories/API/ClientBehavior/AmplifyAPICategory%2BGraphQLBehavior.swift

but would opt to do any of these in another PR. The change you see here is due to removing the mutate(ofAnyModel) API from Amplify.API. this set of test is disabled at the moment so i'm not sure if that's even the right call for the test. datastore uses the GraphQLRequest extensions so AnyModelIntegrationTests should actualy be removed. we test the sync/anymodel APIs in GraphQLSyncBasedIntegrationTests

AmplifyPlugins/Core/AWSPluginsCore/Model/Decorator/ConflictResolutionDecorator.swift

AmplifyPlugins/Core/AWSPluginsCore/Model/Decorator/ModelDecorator.swift

palpatim · 2020-02-11T15:49:54Z

AmplifyPlugins/Core/AWSPluginsCore/Model/GraphQLRequest/GraphQLRequest+Model.swift

@@ -98,7 +98,7 @@ extension GraphQLRequest {
        return GraphQLRequest<[M]>(document: document.stringValue,
                                   variables: document.variables,
                                   responseType: [M].self,
-                                   decodePath: document.name)
+                                   decodePath: document.name + ".items")


Let's add a backlog item to refactor decode path handling. It doesn't make sense for the query function at this level to know the details of pagination implementation

we have this bug here for fixing the model based list query API #291

palpatim · 2020-02-11T16:31:09Z

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/GraphQLDocumentnputValue.swift

@@ -10,7 +10,7 @@ import Foundation
 /// A container to hold either an object or a value, useful for storing document inputs and allowing manipulation at
 /// the first level of the object
 public enum GraphQLDocumentInputValue {
-    case value(Any)
+    case scalarOrString(Any)


As above: scalar should be sufficient--all non-optional types can be safely interpolated via String(describing:). However, what you really want is for the incoming value to be safely representable as a primitive type (as opposed to a nested JSON-like object), rather than relying on an internal String(describing:) implementation that may change without warning (hello, NSData!) so you could define some protocol, and some convenience extensions to make it trivial to pass primitives into the enum value:

public enum GraphQLDocumentInputValue { case value(GraphQLDocumentValueRepresentable) case object([String: GraphQLDocumentValueRepresentable]) } public protocol GraphQLDocumentValueRepresentable { var graphQLDocumentValue: String { get } } extension Bool: GraphQLDocumentValueRepresentable { public var graphQLDocumentValue: String { return "\(self)" } } extension Float: GraphQLDocumentValueRepresentable { public var graphQLDocumentValue: String { return "\(self)" } } extension Int: GraphQLDocumentValueRepresentable { public var graphQLDocumentValue: String { return "\(self)" } } extension String: GraphQLDocumentValueRepresentable { public var graphQLDocumentValue: String { return self } } ...etc

thanks for providing the example! i added this in with a few modifications:

Added extension Decimal: GraphQLDocumentValueRepresentable as we represent Double's as Decimal. from what i recall it something to do with rounding. current we don't have any scalar cases other than int and string, but left this in for now.

case object([String: GraphQLDocumentValueRepresentable]) doesn't quite work since if you take QueryPredicate to GraphQL input for example, the value can be infinitely nested. so the .object case is something recursive like

public enum GraphQLDocumentInputValue { case scalar(GraphQLDocumentValueRepresentable) case object([String: GraphQLDocumentInputValue]) }

currently still have it as [String: Any?] and will see if the above works out

so digging into this deeper, having .scalar(GraphQLDocumentValueRepresentable) seems to work well in this class. Adding it to the .object[String:GraphQLDocumentValueRepresentable] case doesn't work as it may contain arrays as well. then it will evolve to something similar to JSONValue.

palpatim · 2020-02-11T16:38:24Z

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/SelectionSet.swift

-public struct SelectionSet {
-    var fields: [SelectionSetField]
-    var type: SelectionSetType
+public class TreeNode<E> {


Put TreeNode in its own file

TreeNode is a public class, but its methods and properties are internal. Intentional?

Since we're naming a type, we should probably call it Tree instead of TreeNode

updating 1 and 3, for 2, Tree is pubic for the typealias SelectionSet as SelectionSet is used as a property on the SingleDirectiveGraphQLDocument public protocol. Everything else on the Tree is used internally.

palpatim · 2020-02-11T16:42:38Z

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/SelectionSet.swift

    }

-    mutating func append(_ selectionSetField: SelectionSetField) {
-        fields.append(selectionSetField)
+    func add(child: TreeNode) {


This has the non-obvious side-effect of mutating the child. Suggest you rename the method to make it more obvious to addChild(settingParentOf child:) or similar, and add a doc comment with a note. It's the right behavior--it's would be cumbersome to require call sites to do this explicitly--but we just want to make sure that it's obvious what's happening when we call the method.

thanks, updating! can go with your suggestion. I was also thinking of insert(settingParentOf child:)

- some remaining code clean up

palpatim

Question about Decimal, but otherwise LGTM

palpatim · 2020-02-12T15:31:44Z

Amplify/Core/Support/Tree.swift

+public class Tree<E> {
+    public var value: E
+    public var children: [Tree<E>] = []
+    public weak var parent: Tree<E>?


Thanks for remembering to make this weak

palpatim · 2020-02-12T15:34:51Z

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/GraphQLDocumentnputValue.swift

-    public init(type: String, value: GraphQLDocumentInputValue) {
-        self.type = type
-        self.value = value
+extension Decimal: GraphQLDocumentValueRepresentable {


I'm surprised we're using Decimal--I thought we had moved over to Float for our codegenned models? Do we at least need to add a conformance for Float, since it's a standard library, as opposed to a Foundation, type?

I believe we landed on Double because it can hold up to 64 bit. then i encountered some serialization issues, a Double with value 12.3 is serialized as "12.300000000001", which is why i started using Decimal (#159). so still not 100% confident in which is the right type to use as looking up more details on Decimal isn't giving me many results to resolve this. Maybe there to use Double and override the serialization logic on Doubles. I think the best test is to write an integration test which has a single model with all of the various types that we support.

Float in AppSync is currently returning .double as the ModelFieldType, which then gets translated to a Decimal and serialized. https://github.com/aws-amplify/amplify-ios/blob/461c3cfd8923fbbb6eb10b60495b0f4594879cf4/Amplify/Categories/DataStore/Model/ModelSchema%2BDefinition.swift#L56

opening an issue track adding test with all types that are supported, may be completed as part of @drochetti 's work or follow up task #314

lawmicha requested review from drochetti and palpatim February 5, 2020 22:24

lawmicha force-pushed the lawmicha/document-builder branch 3 times, most recently from 12a9711 to b6060f7 Compare February 6, 2020 07:23

GraphQLDocument Builder

31298c1

lawmicha force-pushed the lawmicha/document-builder branch from b6060f7 to 31298c1 Compare February 6, 2020 07:23

lawmicha marked this pull request as ready for review February 6, 2020 07:29

palpatim requested changes Feb 6, 2020

View reviewed changes

lawmicha mentioned this pull request Feb 10, 2020

Adding predicate to mutate for GraphQL condition input #272

Closed

Adding improvements to the SelectionSet structure

7b9d5be

lawmicha requested a review from palpatim February 11, 2020 16:18

palpatim reviewed Feb 11, 2020

View reviewed changes

- Moved Tree to Amplify

f3082c2

- some remaining code clean up

lawmicha force-pushed the lawmicha/document-builder branch from eb0624d to f3082c2 Compare February 11, 2020 21:33

clean up

600ab31

lawmicha requested a review from palpatim February 11, 2020 22:37

palpatim approved these changes Feb 12, 2020

View reviewed changes

lawmicha merged commit ad94773 into master Feb 12, 2020

lawmicha mentioned this pull request Feb 25, 2020

API Plugin Improvements #292

Closed

lawmicha deleted the lawmicha/document-builder branch April 27, 2020 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GraphQLDocument Builder #309

GraphQLDocument Builder #309

lawmicha commented Feb 5, 2020 •

edited

Loading

palpatim left a comment

palpatim Feb 6, 2020

lawmicha Feb 11, 2020 •

edited

Loading

palpatim Feb 6, 2020

lawmicha Feb 11, 2020

palpatim Feb 6, 2020

lawmicha Feb 11, 2020

palpatim Feb 6, 2020

lawmicha Feb 11, 2020

palpatim Feb 11, 2020

lawmicha Feb 11, 2020

palpatim Feb 11, 2020

lawmicha Feb 11, 2020

palpatim Feb 11, 2020

lawmicha Feb 11, 2020

palpatim Feb 11, 2020

lawmicha Feb 11, 2020 •

edited

Loading

lawmicha Feb 11, 2020

palpatim Feb 11, 2020

lawmicha Feb 11, 2020

palpatim Feb 11, 2020

lawmicha Feb 11, 2020 •

edited

Loading

palpatim left a comment

palpatim Feb 12, 2020

palpatim Feb 12, 2020 •

edited

Loading

lawmicha Feb 12, 2020 •

edited

Loading

lawmicha Feb 12, 2020

GraphQLDocument Builder #309

GraphQLDocument Builder #309

Conversation

lawmicha commented Feb 5, 2020 • edited Loading

palpatim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawmicha Feb 11, 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

lawmicha Feb 11, 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

lawmicha Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

palpatim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palpatim Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

lawmicha Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawmicha commented Feb 5, 2020 •

edited

Loading

lawmicha Feb 11, 2020 •

edited

Loading

lawmicha Feb 11, 2020 •

edited

Loading

lawmicha Feb 11, 2020 •

edited

Loading

palpatim Feb 12, 2020 •

edited

Loading

lawmicha Feb 12, 2020 •

edited

Loading