[WIP] Allow objects parameters in mutations #252

prevostc · 2016-05-28T12:04:52Z

This is a work in progress to add support for object parameters in mutations and thus support reindex.io default mutation format.

I did succeed in making all tests pass BUT the fix is obviously wrong so I reach out here to get feedback on which other part of the project should be breaking so I can add some tests there and create a proper fix.

I also reused a duplicated test.

apollo-cla · 2016-05-28T12:04:52Z

@prevostc: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

stubailo · 2016-05-28T15:07:27Z

I think the way to do this is to use valueFromAST from the graphql utils: https://github.com/graphql/graphql-js/blob/master/src/utilities/valueFromAST.js

Basically, what you're doing here is using the AST nodes for the object in the store, but we need to use the actual values.

prevostc · 2016-05-28T19:37:34Z

All right, I did find how to use that function but I can't find a way to get the right GraphQLType. It seems like I need a GraphQLSchema to call getType on it but I'm not sure.

helfer · 2016-05-28T19:53:57Z

@prevostc Since the goal of your PR is to support objects as inputs to mutations, I think what you're looking for is GraphQLInputObjectType. If that's the case, you won't need a schema, because this is a generic type that you can import from GraphQL.

prevostc · 2016-05-28T20:25:11Z

@helfer sorry I didn't explained the whole story, I do need a GraphQLInputObjectType as the second argument of valueFromAST but I may need an instance of it because if I try to call it with valueFromAST(value, GraphQLInputObjectType) I get this compilation error:

Argument of type 'typeof GraphQLInputObjectType' is not assignable to parameter of type 'GraphQLScalarType | GraphQLEnumType | GraphQLInputObjectType | GraphQLList | GraphQLNonNull'.

I did some research and the GraphQLSchema has a way to retrieve defined types, but this might not be the right way to get the corresponding graphql type.

helfer · 2016-05-28T21:06:56Z

Oh, yes, of course you need an instance.

Getting the type from the schema works just fine if you have the schema, but we're trying not to require having the schema on the client, because it means either doing an extra round-trip, or making the build process more complicated.

Since in Apollo client mutations with input objects as variables currently don't do any validation, it probably makes most sense to do the same with inline input objects, i.e. just parse the thing without bothering to check the types at all.

To achieve that, you can copy the valueFromAST function and modify it so it doesn't require instances for InputObjectType and ListType. Then you just call modifiedValueFromAST(astValue, variables) without having to pass the type.

Does that make sense?

prevostc · 2016-05-28T22:38:15Z

Thanks @helfer for the guidance. After some heavy code reading, I did not copied the valueFromAST because it relied on type.parseLiteral and as you mentioned I don't have a type object to work with.

Instead I reworked the existing storeKeyNameFromField code to be called recursively on ObjectValue like valueFromAST do for nested objects.

I'm not sure that this new function should be named valueFromAST though as the signature is not the same on both packages, this might be confusing.

helfer · 2016-05-29T01:24:19Z

That's great! valueFromAST is definitely not the right name, as it's already taken. Just give it another name that describes what it does.

prevostc · 2016-05-29T11:35:06Z

@helfer done \o/ Should I do something else ?

stubailo · 2016-05-30T19:09:11Z

@prevostc one thing you should do is sign the CLA - it lets us make sure that people are contributing code that they own.

stubailo · 2016-05-30T19:09:48Z

typings/browser/globals/graphql/index.d.ts

We don't need this, do we?

Without this declaration, the ts compilation fails. As for the comment line, I tried to use the same convention already present in the file that is to have a section prefixed with the corresponding file name.

stubailo · 2016-05-30T19:37:51Z

I would really prefer to have a few more tests here though - just changing that one doesn't really cover a lot of space of possible arguments. We could just add tests for valueToObjectRepresentation directly, without running any other code.

helfer · 2016-06-02T00:32:43Z

@prevostc Any update on this? I'd like to merge it as soon as there are some more tests and you've signed the CLA.

prevostc · 2016-06-02T06:46:05Z

@helfer I will be able to add more tests by Monday. I will add more Object to string representation tests, including all scalar types, aliases, 2 or 3 levels of nested object, arrays, arrays of objects and edge cases like nulls and cycling references.

helfer · 2016-06-02T20:50:31Z

@prevostc Perfect, I think it shows up as signed now.

I don't think you need to test cycles, since the AST gets parsed from a query string, which cannot possibly have any cycles. Everything else sounds good. 2 levels of nesting should be enough.

stubailo · 2016-06-03T23:20:25Z

src/data/storeUtils.ts

+      return nestedArgArrayObj[name.value];
+    });
+  } else {
+    throw new Error(`For inline arguments, only scalar and Object types are supported.


Probably this should just say enum arguments are not supported, since I think we have covered literally everything else: http://facebook.github.io/graphql/#Value

I added Enum support in the next commit and reworked the error message to be more generic.

stubailo · 2016-06-06T19:58:45Z

I'd request that we add tests that ensure that arguments are serialized the same way if they are passed via inline AST or variables.

prevostc · 2016-06-07T23:10:39Z

@stubailo only numbers are not handled the same way, integers are not cast to numbers when inlined. fixing this broke some tests (see below), I don't know what's the right fix here.

1) writing to the store properly normalizes a aliased fields with arguments:

      AssertionError: expected { Object (abcd) } to deeply equal { Object (abcd) }
      + expected - actual

         "abcd": {
           "id": "abcd"
           "nullField": [null]
           "numberField": 5
      -    "stringField({"arg":1})": "The arg was 1!"
      -    "stringField({"arg":2})": "The arg was 2!"
      +    "stringField({"arg":"1"})": "The arg was 1!"
      +    "stringField({"arg":"2"})": "The arg was 2!"
         }
       }

2) client can allow the store to be rehydrated from the server:

      AssertionError: expected { Object (apollo) } to deeply equal { Object (apollo) }
      + expected - actual

         "apollo": {
           "data": {
             "ROOT_QUERY": {
               "allPeople({"first":"1"})": "ROOT_QUERY.allPeople({\"first\":\"1\"})"
      +        "allPeople({"first":1})": "ROOT_QUERY.allPeople({\"first\":1})"
             }```

stubailo · 2016-06-07T23:59:24Z

Hmm I think we should make integers always cast to numbers and update the tests. Strings in graphql literals always have quotes, so it shouldn't be ambiguous I think!

stubailo · 2016-06-07T23:59:54Z

By the way, thanks for putting so much time into the PR, this is really great stuff.

helfer · 2016-06-09T15:40:32Z

test/QueryManager.ts

@@ -979,10 +979,10 @@ describe('QueryManager', () => {
    });
  });

-  it('runs a mutation and puts the result in the store', () => {
+  it('runs a mutation with object parameters and puts the result in the store', () => {


It's not clear what exactly this test is supposed to do. Originally it was to test that the result of a mutation is properly put in the store. We should keep the old test and create an additional one for object values.

This test was a duplicate from the test at line 938, I just reused it instead of dropping it and recreating it.

Ah, yes, I didn't see that.

helfer · 2016-06-09T15:50:23Z

@prevostc This is shaping up very nicely, thanks a lot for all your work!

My only nitpick is with the tests. Currently, there's just one unit large test that presumably covers all of your changes. If this test fails in the future, it will be very hard to determine what exactly isn't working. Instead of writing one big test, we generally try to test each bit of functionality separately with a smaller test.

So you should write a separate test for each branch of the valueToObjectRepresentation function:

for a scalar
for a number
for an object
for a variable
for a missing variable
for a list
for an unknown kind (making sure the correct error is thrown)

You can do that by importing the valueToObjectRepresentation function directly, and then making sure the output is what it should be.

The big test you wrote is more like an integration test, so it's a perfectly good test that you should keep, but it should run after the smaller unit tests for the valueToObjectRepresentation. That way it will be much easier to know what's broken when a test fails.

helfer · 2016-06-11T00:09:41Z

@prevostc Just give me a nudge when you think it's ready to be reviewed again, ok?

prevostc · 2016-06-11T00:47:43Z

@helfer I added a unit test that cover each case individually. I did the "data provider" manually, maybe there is a best way :s

helfer · 2016-06-11T01:10:21Z

Ah, ok, I wasn't sure if you were still working on it or not, so I didn't take a look yet. It will have to be rebased before we can merge, but I can take a look first.

helfer · 2016-06-13T19:19:49Z

@prevostc The new tests look great! 👍 This is 99% ready to merge, just some tiny things to fix.

I took a look at the coverage report, and it seems there are still two lines that aren't tested at all. You can see them here: https://coveralls.io/builds/6534889/source?filename=src%2Fdata%2FstoreUtils.ts

Can you add two tests that cover these lines? One is for a missing variable, the other is for unsupported inline type (just enum, I believe).

Also, please rebase your PR so it can be merged without conflict. If you're not sure how to do that, just message me on the apollo slack.

stubailo · 2016-06-14T18:35:38Z

This PR is amazing.

prevostc · 2016-06-14T18:41:37Z

@helfer Done :) I also squashed all commits so you can keep a clean commit history too.

helfer · 2016-06-14T20:52:55Z

Awesome, amazing work, thank you very much!

PS: For next time it's easier if you don't squash the commits, because we can do that with the "squash and merge" option in github. It's easier to review the changes if it's not squashed.

prevostc · 2016-06-14T20:57:22Z

Oups, sry. Thanks for you careful reviews and your time!

helfer · 2016-06-14T21:03:31Z

No worries, thanks a lot for the PR, it's really great! It would be really cool if you want to be an active contributor! I can add you to our team, so you can work on branches directly on this repo. How does that sound?

danielbuechele · 2016-06-21T16:49:16Z

I am wondering how the suggested workaround with variables was working before this PR. I have the code below, but I am getting an error.

mutation: gql`
    mutation(
        $title: String!,
        $status: ItemStatus!,
    ) {
        item(item: {
            title: $title,
            status: $status,
        }) {
            id
        }
    }
`,
variables: {
    title: raw.title,
    status: raw.status,
},

stubailo · 2016-06-21T16:50:36Z

I am getting an error.

What does the error say?

danielbuechele · 2016-06-21T16:52:36Z

Uncaught Error: For inline arguments, only scalar types are supported. To use Enum or Object types, please pass them as variables.
I am using [email protected]
In my case ItemStatus is an enum type, which I want to set.

prevostc · 2016-06-26T01:44:54Z

This issue have been fixed by the merged PR but as a quickfix you could pass the item field as a variable (might not work):

mutation: gql`
    mutation(
        $title: String!,
        $status: ItemStatus!,
    ) {
        item(item: $item) {
            id
        }
    }
`,
variables: {
    item: {
        title: raw.title,
        status: raw.status,
    }
},

stubailo · 2016-06-26T06:09:38Z

@prevostc has the right workaround. Since item is an object, you had to pass the whole object as a variable.

Imperative read and write docs

Fix broken link in simple-example

stubailo reviewed May 30, 2016
View reviewed changes

zol added the in progress by contributor label Jun 2, 2016

stubailo reviewed Jun 3, 2016
View reviewed changes

helfer reviewed Jun 9, 2016
View reviewed changes

Allow inline object parameters

089f132

prevostc force-pushed the master branch from ae6741f to 089f132 Compare June 14, 2016 18:39

helfer merged commit 9e8ce8c into apollographql:master Jun 14, 2016

helfer removed the in progress by contributor label Jun 14, 2016

jbaxleyiii pushed a commit that referenced this pull request Oct 17, 2017

Merge pull request #252 from apollographql/read-write

4c5a610

Imperative read and write docs

jbaxleyiii pushed a commit that referenced this pull request Oct 18, 2017

Merge pull request #252 from stevenhao/patch-1

37db0c5

Fix broken link in simple-example

github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023

[WIP] Allow objects parameters in mutations #252

[WIP] Allow objects parameters in mutations #252

Conversation

prevostc commented May 28, 2016 • edited Loading

apollo-cla commented May 28, 2016

stubailo commented May 28, 2016

prevostc commented May 28, 2016

helfer commented May 28, 2016

prevostc commented May 28, 2016

helfer commented May 28, 2016

prevostc commented May 28, 2016

helfer commented May 29, 2016

prevostc commented May 29, 2016

stubailo commented May 30, 2016

stubailo May 30, 2016

Choose a reason for hiding this comment

prevostc May 30, 2016

Choose a reason for hiding this comment

stubailo commented May 30, 2016

helfer commented Jun 2, 2016

prevostc commented Jun 2, 2016 • edited Loading

helfer commented Jun 2, 2016

stubailo Jun 3, 2016

Choose a reason for hiding this comment

prevostc Jun 4, 2016

Choose a reason for hiding this comment

stubailo commented Jun 6, 2016

prevostc commented Jun 7, 2016 • edited by Poincare Loading

stubailo commented Jun 7, 2016

stubailo commented Jun 7, 2016

helfer Jun 9, 2016

Choose a reason for hiding this comment

prevostc Jun 9, 2016

Choose a reason for hiding this comment

helfer Jun 9, 2016

Choose a reason for hiding this comment

helfer commented Jun 9, 2016

helfer commented Jun 11, 2016

prevostc commented Jun 11, 2016

helfer commented Jun 11, 2016

helfer commented Jun 13, 2016

stubailo commented Jun 14, 2016

prevostc commented Jun 14, 2016

helfer commented Jun 14, 2016 • edited Loading

prevostc commented Jun 14, 2016 • edited Loading

helfer commented Jun 14, 2016

danielbuechele commented Jun 21, 2016 • edited Loading

stubailo commented Jun 21, 2016

danielbuechele commented Jun 21, 2016 • edited Loading

prevostc commented Jun 26, 2016

stubailo commented Jun 26, 2016

prevostc commented May 28, 2016 •

edited

Loading

prevostc commented Jun 2, 2016 •

edited

Loading

prevostc commented Jun 7, 2016 •

edited by Poincare

Loading

helfer commented Jun 14, 2016 •

edited

Loading

prevostc commented Jun 14, 2016 •

edited

Loading

danielbuechele commented Jun 21, 2016 •

edited

Loading

danielbuechele commented Jun 21, 2016 •

edited

Loading