include associations in the GraphQL query selected fields #164

drochetti · 2019-11-26T07:05:46Z

Notes:

added logic to handle associations when generating GraphQL documents
for Queries and Subscriptions
fixed value resolution for associations
added some tests to check the output of associated models queries

Issue #, if available:

Description of changes:

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

**Notes:** - added logic to handle associations when generating GraphQL documents for Queries and Subscriptions - fixed value resolution for associations - added some tests to check the output of associated models queries

drochetti · 2019-11-26T07:06:14Z

Amplify/Categories/Logging/LoggingCategory+ClientBehavior.swift

@@ -14,6 +14,10 @@ extension LoggingCategory: LoggingCategoryClientBehavior {
        plugin.logger(forCategory: category)
    }

+    public func logger(forCategory category: CategoryType) -> Logger {


@palpatim would like to get your thoughts on this

I had a similar thought. From #149:

/// Defines a `log` convenience property, and provides a default implementation that returns a Logger for a category /// name of `String(describing: self)` public protocol DefaultLogger { static var log: Logger { get } var log: Logger { get } } public extension DefaultLogger { static var log: Logger { Amplify.Logging.logger(forCategory: String(describing: self)) } var log: Logger { type(of: self).log } }

Then any type can get its own type-specific logger by declaring a no-op conformance to DefaultLogger.

Note that the current Logging subsystem is broken for types that want to use it during initialization, or during configuration in tests after an Amplify.reset(). Tracking that on #161.

Of course now that I look at my implementation, I realize I added DefaultLogger in AWSPluginsCore rather than AmplifyCore. sigh I'll move it to AmplifyCore in an upcoming PR, and ensure that each category also conforms.

Cool, thanks for the context. I'll rely on your changes for then

drochetti · 2019-11-26T07:06:58Z

AmplifyPlugins/API/AWSAPICategoryPlugin/Model/GraphQLDocument.swift

+                    fieldSet.append(indent + field.graphQLName)
+                }
+            }
+            fieldSet.append(indent + "__typename")


@palpatim is it safe to assume we should add __typename for queries and subscriptions (mutations override this logic)?

@drochetti Yes, I believe that is a safe assumption. Mutations will need the field as well, but I assume you're referring to overriding the assigment of __typename during the AnyModel flow?

drochetti · 2019-11-26T07:07:53Z

AmplifyPlugins/API/AWSAPICategoryPluginTests/Model/GraphQLDocumentTests.swift

+            id
+            content
+            createdAt
+            postId


point-of-interest: in mutations, the associated id (aka "foreign key") is passed

what i'm discovering right now is that the selectionSet should be the same for both mutations and queries in this case. the CreateComment can also return the same thing as GetComment.

mutation CreateComment($input:CreateCommentInput!){ createComment(input: $input) { id content createdAt post { id _version content createdAt draft rating title updatedAt __typename } } }

{ "input": { "content": "Content", "createdAt": "2019-11-25T00:35:01.746Z", "commentPostId": "123" } }

as in, the response for a CreateComment mutation is the same object as the response for a GetComment. this being the Post is eager loaded.

The question is, should the selection set be different on Mutations over Queries?

Let's take a look at if Post is required field on Comment and selection set does not specify the Post

mutation CreateComment($input:CreateCommentInput!){ createComment(input: $input) { id content createdAt } }

{ "input": { "content": "Content", "createdAt": "2019-11-25T00:35:01.746Z", "commentPostId": "123" } }

The Comment Model contains a required Post and data does not come back. this means it will fail to deserialize.

scenario 2a: Post is not a required field on the Comment, and selection set contains the Post. Deserialized Comment will contain post.

scenario 2b: Post is not required field on the Comment, and selection set does not contain Post. Deserialized Comment contains nil post field, which is fine.

drochetti · 2019-11-26T07:09:18Z

AmplifyPlugins/API/AWSAPICategoryPluginTests/Model/GraphQLDocumentTests.swift

+            id
+            content
+            createdAt
+            post {


point-of-interest: in queries (and subscriptions) the required associations are added to the selection set (otherwise decoding would fail). In the future we should discuss giving the control to users define which associates are included or not (eager vs lazy loading).

"required associations are added" to what depth?

That's a good question and the part I struggled with. Right now the depth is not specified. This logic will even break in case or circular references.

How to decode missing properties to a Struct that require them? I'm having a hard time keeping the data model types consistent but optimize for performance and different use cases of associations (lazy vs eager, required vs optional)

I think codegen specifies a limit of 2; we can take that as a guideline.

I created #168 to track it. I'll also look into @propertyWrapper. I've played with it when I was trying to define the "schema" definition without much success, but now I have more context on how it works, I might be successful in that endeavor.

palpatim · 2019-11-26T17:09:44Z

Amplify/Categories/Logging/LoggingCategory+ClientBehavior.swift

@@ -14,6 +14,10 @@ extension LoggingCategory: LoggingCategoryClientBehavior {
        plugin.logger(forCategory: category)
    }

+    public func logger(forCategory category: CategoryType) -> Logger {


I had a similar thought. From #149:

/// Defines a `log` convenience property, and provides a default implementation that returns a Logger for a category /// name of `String(describing: self)` public protocol DefaultLogger { static var log: Logger { get } var log: Logger { get } } public extension DefaultLogger { static var log: Logger { Amplify.Logging.logger(forCategory: String(describing: self)) } var log: Logger { type(of: self).log } }

Then any type can get its own type-specific logger by declaring a no-op conformance to DefaultLogger.

Note that the current Logging subsystem is broken for types that want to use it during initialization, or during configuration in tests after an Amplify.reset(). Tracking that on #161.

palpatim · 2019-11-26T17:12:27Z

AmplifyPlugins/API/AWSAPICategoryPlugin/Model/GraphQLDocument.swift

+                    fieldSet.append(indent + field.graphQLName)
+                }
+            }
+            fieldSet.append(indent + "__typename")


@drochetti Yes, I believe that is a safe assumption. Mutations will need the field as well, but I assume you're referring to overriding the assigment of __typename during the AnyModel flow?

palpatim · 2019-11-26T17:14:06Z

AmplifyPlugins/API/AWSAPICategoryPlugin/Model/GraphQLDocument+Mutation.swift

-        // All mutation documents should include typename, to support type-erased operations on the client
-        fields.append("__typename")
-
+        let fields = selectionSet


probably safe to remove this and just interpolate selectionSet below--not sure the variable adds clarity any more

palpatim · 2019-11-26T17:18:18Z

AmplifyPlugins/API/AWSAPICategoryPlugin/Model/GraphQLDocument.swift

+    /// - Note: Currently implementation assumes the most common and efficient queries.
+    /// Future APIs might allow user customization of the selected fields.
+    public var selectionSet: [String] {
+        var fieldSet = [String].init()


Avoid appending type information to variable names, except in cases where it is needed to disambiguate, e.g. during a transfomation. In this case, it's actively misleading since it's an Array, not a Set. :) I think the intent of the field is to store components of the selection set, so maybe selectionSetFields or similar?

Avoid explicit calls to .init, prefer initializers like var foo = [String]()

palpatim · 2019-11-26T17:21:01Z

AmplifyPlugins/API/AWSAPICategoryPlugin/Model/GraphQLDocument.swift

+
+        var indentSize = 0
+
+        func appendFields(_ fields: [ModelField]) {


Why a nested function? Seems like this could be a standalone static function

To make it a standalone function more work would be necessary to handle the recursive nature of the data structure. It seems that it would also require an inout argument.

I find that nested functions are a good fit for recursive logic. That or this is just my inner JavaScript side speaking

OK, I didn't trace through the flow in depth, so I'm happy to use nested functions as well. Us old JS hands gotta stick together. :)

palpatim · 2019-11-26T17:21:06Z

AmplifyPlugins/API/AWSAPICategoryPlugin/Model/GraphQLDocument.swift

+    ///
+    /// - Note: Currently implementation assumes the most common and efficient queries.
+    /// Future APIs might allow user customization of the selected fields.
+    public var selectionSet: [String] {


Why is this property public?

Why are you composing a formatted string, but returning the individual components instead of the final output?

The name selectionSet is also on the model, but that property which returns a list of field names, thus adding to the confusion.

Good call, no need

Yeah, I initially thought about returning an array of fields. But then struggled with nested properties. Then decided to return the formatted string, but then struggled with the final formatting of the document, since I don't know the indentation level of the document at this point.

I could make this a function as take parameters in order to produce the correct string output

or I could return with no indentation and the consuming code adds a padding to each line? Argh...

True, good call. I'll change.

Yeah, formatting is tricky. I think if you rename the property to "selectionSetComponents" or similar, and let the final document composer handle indentation, I'd find it eaiser to understand

palpatim · 2019-11-26T17:23:30Z

AmplifyPlugins/API/AWSAPICategoryPluginTests/Model/GraphQLDocumentTests.swift

+            id
+            content
+            createdAt
+            post {


"required associations are added" to what depth?

lawmicha · 2019-11-26T22:36:47Z

AmplifyPlugins/API/AWSAPICategoryPluginTests/Model/GraphQLDocumentTests.swift

+        query GetComment($id: ID!) {
+          getComment(id: $id) {
+            id
+            content
+            createdAt
+            post {
+              id
+              _version
+              content
+              createdAt
+              draft
+              rating
+              title
+              updatedAt
+              __typename
+            }
+            __typename


The issue i'm seeing for decoding Post exists here as well. Post contains List.
In this code, the response data from the service will then deserialize the data into a Comment which contains a Post and will fail to find the comments since we are not asking for it back in the selection set. List's decoder is never called. This is the same failure in a simpler case when we call Mutation CreatePost or Query GetPost. Both will return a Post data which we try to deserialize into the Post Model.

So we need to add to the selection set "comments"

Here's an example of my attempt in the AppSync console:
To do this, let's add "comments" in the selection set

mutation CreatePost($input:CreatePostInput!) { createPost(input: $input) { id title comments //this does not work, it needs at least one selection for comments } }

This works:

mutation CreatePost($input:CreatePostInput!) { createPost(input: $input) { id title comments { //items { // commented out as an example, that, in the future if we support user defined control, then we will probably generate the items with the selection set for Comment as well // id //} nextToken } } } returns: { "data": { "createPost": { "id": "84b1a5ca-7ead-48da-9b65-8df458d1f613", "title": "title", "comments": { "nextToken": null } } } }

some options

setting Post's List to optional like comments: List<Comment>? does work for decoding since the key does not exist in the response and it is not required in the Model so decoder is happy. Model gen needs to be updated. also datastore local store logic. developer experience is poor since they have to now check that the Post that is return contains comments that is not nil before operating on the List

Always set selection set to contain comments with nextToken, and in the future when we support user defined control, items gets added to the comments. This does eager loading but does not return any results from the service. i do not think this is what we want just to make decoder happy.

somehow, customize the decoding logic for the Post Model which contains comments that is a List. when decoding, when key does not exist for a required field like comments, we want to create List() that is just empty

I was hopping that List<ModelType> decoder would be called with nil, but since that's not the case (bummer) I would say let's go with #1. Make it optional and call it a day.

@lawmicha thanks for testing it and providing all the context, appreciate it

…ociations

lawmicha · 2019-11-27T01:44:31Z

AmplifyPlugins/API/AWSAPICategoryPlugin/Model/Model+GraphQL.swift

+            case .model:
+                input[name] = (value as? Model)?.id


i have updated this code to produce "commentPostId" and can take it forward

let fieldName = modelName.lowerCaseFirstLetter() + name input[fieldName] = (value as? Model)?.id

lawmicha · 2019-11-27T01:47:44Z

AmplifyPlugins/API/AWSAPICategoryPluginTests/Model/GraphQLDocumentTests.swift

    ///   - the subscription is of type `.onCreate`
    /// - Then:
    ///   - check if the generated GraphQL document is a valid subscription
-    ///     - it has a list of fields with no nested/connected models
+    ///     - it has a list of fields with no nested models
    func testOnCreateGraphQLSubscriptionFromSimpleModel() {


no action needed, adding note here:

onCreateComment should also return the Post like a CreateComment selection set

will take a look up UpdateComment/DeleteComment, this scenario is important for Comment containing a Post.

Will add integration testing around these

palpatim

LGTM. Retroactive Ship-It!

* master: include associations in the GraphQL query selected fields (#164) Re-add ModelSchema+Syncable.swift to project (#171) Remove stray references to Model+Syncable.swift (#170)

include associations in the GraphQL query selected fields

86cbea6

**Notes:** - added logic to handle associations when generating GraphQL documents for Queries and Subscriptions - fixed value resolution for associations - added some tests to check the output of associated models queries

drochetti added api Issues related to the API category datastore Issues related to the DataStore category labels Nov 26, 2019

drochetti requested review from palpatim and lawmicha November 26, 2019 07:05

drochetti self-assigned this Nov 26, 2019

drochetti commented Nov 26, 2019

View reviewed changes

palpatim reviewed Nov 26, 2019

View reviewed changes

palpatim mentioned this pull request Nov 26, 2019

Subscriptions, State machine for ReconcileAndLocalSaveOperation #149

Merged

lawmicha reviewed Nov 26, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into feature/graphql-ass…

87a6d26

…ociations

drochetti mentioned this pull request Nov 27, 2019

Define the depth of eager loaded associations #168

Closed

drochetti added 4 commits November 26, 2019 17:13

address feedback for handling graphql selection sets

f54a5d4

Merge remote-tracking branch 'origin/master' into feature/graphql-ass…

20da876

…ociations

fix tests after merge from master

5376027

Merge remote-tracking branch 'origin/master' into feature/graphql-ass…

c08f8af

…ociations

lawmicha reviewed Nov 27, 2019

View reviewed changes

drochetti merged commit bce59bc into master Nov 27, 2019

lawmicha reviewed Nov 27, 2019

View reviewed changes

palpatim reviewed Nov 27, 2019

View reviewed changes

palpatim deleted the feature/graphql-associations branch November 27, 2019 01:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

include associations in the GraphQL query selected fields #164

include associations in the GraphQL query selected fields #164

drochetti commented Nov 26, 2019

drochetti Nov 26, 2019

palpatim Nov 26, 2019

palpatim Nov 26, 2019

drochetti Nov 26, 2019

drochetti Nov 26, 2019

palpatim Nov 26, 2019

drochetti Nov 26, 2019

lawmicha Nov 27, 2019 •

edited

Loading

lawmicha Nov 27, 2019

drochetti Nov 26, 2019

palpatim Nov 26, 2019

drochetti Nov 26, 2019

palpatim Nov 26, 2019

drochetti Nov 27, 2019

palpatim Nov 26, 2019

palpatim Nov 26, 2019

palpatim Nov 26, 2019

palpatim Nov 26, 2019

palpatim Nov 26, 2019

drochetti Nov 26, 2019

palpatim Nov 26, 2019

palpatim Nov 26, 2019

drochetti Nov 26, 2019

palpatim Nov 26, 2019

palpatim Nov 26, 2019

lawmicha Nov 26, 2019 •

edited

Loading

lawmicha Nov 26, 2019 •

edited

Loading

drochetti Nov 27, 2019 •

edited

Loading

lawmicha Nov 27, 2019

lawmicha Nov 27, 2019

palpatim left a comment


		var indentSize = 0

		func appendFields(_ fields: [ModelField]) {

include associations in the GraphQL query selected fields #164

include associations in the GraphQL query selected fields #164

Conversation

drochetti commented Nov 26, 2019

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 Nov 27, 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

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 Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

lawmicha Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

drochetti Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palpatim left a comment

Choose a reason for hiding this comment

lawmicha Nov 27, 2019 •

edited

Loading

lawmicha Nov 26, 2019 •

edited

Loading

lawmicha Nov 26, 2019 •

edited

Loading

drochetti Nov 27, 2019 •

edited

Loading