graphql queries verification broken with graphql-tag

[tomasAlabes]

Software versions

• OS: e.g. Mac OSX 10.11.5
• Consumer Pact library: e.g. Pact JS v7.0.4
• Node Version: 10.13.0

Expected behaviour

Related to #243

When using the graphql-tag package like in the graphql example:

const gql = require("graphql-tag");

const client = /* ... */

function query() {
	return client.query({
		query: gql`{ hello }`
	})
	.then(result => result.data);
};

Actual behaviour

With graphql-tag, when the query doesn't have variables, the post is done with

"body" : {
   "query" : "...",
   "variables": {}
}

and without operation:

"body" : {
   "query" : "...",
    "operationName": null
}

With the latest commit, now those attributes are not part of the body, hence the verification fails. As the tests are written with manual queries and not with the graphql-tag package, tests are sucessfull.

Steps to reproduce

Just remove the variables of the example in this repo and the example fails to verify.

Relevant log files

Matching keys and values are not shown
 {
   "body": {
+    "operationName": null
   }
 }

Matching keys and values are not shown

 {
   "body": {
+    "variables": {
+    }
   }
 }

[mefellows]

Oh my, I'm sorry!

I think we might need to take a step back here and think a bit more carefully.

If I understand correctly, I think this is the specific requirement needed:

1. Default variables/operationName to undefined
2. If variables, operationName ... are present (but may be null/empty) we should leave them in
3. If not present or undefined then remove them from the payload

Would this satisfy your requirement?

@ngbrown would this work for you also?

[tomasAlabes]

Hi @mefellows, thanks for answering. Absolutely no problem, thank you for putting in time on this!

I think it would work. Tests using graphql-tag will make any issue arise.

Also, I noticed that the queries done with graphql-tag are received by the mock server with the additional element __typename, making the verification fail.

Like:

person {
   name
   __typename
}

And the verification is against \s*person\s*{\s*name\s*}. That's failing for me too.

Thank you!

[mefellows]

No worries. Can you please put together a small gist/PR to demonstrate what you mean by the following:

Also, I noticed that the queries done with graphql-tag are received by the mock server with the additional element __typename, making the verification fail.

Using the example in the repository, it doesn't seem to pass that extra field through.

The test is using the actual client code, which constructs the query using graphql-tag. The expectation in the test is setup by hand, but that's because graphql-tag doesn't give us anything here that I'm aware of. Happy to be re-educated here.

I'm actually surprised at how many people seem to be using Pact with GraphQL - this gives some indication that it's worthwhile adding to the other libraries also (once we iron out these wrinkles!).

FWIW setting those fields to undefined and allowing null and {} to be marshalled seems to work. It's a bit of a "damned if you do, damned if you don't" situation - graphl-tag users will get a good experience and others will have to explicitly set null or {} or vice versa. I'll go for the audience with the most usage (or potentially create another slim API wrapper for each usage type).

[tomasAlabes]

I'll try to create something reproducing the __typename thing. It's related to Apollo's cache (see apollographql/apollo-client#1913), but it can be disabled.
I'm sorry if I can't give all the info together, as it's my 2nd day using pact-js so I'm still getting my head around it.

I'm actually surprised at how many people seem to be using Pact with GraphQL - this gives some indication that it's worthwhile adding to the other libraries also (once we iron out these wrinkles!).

Totally agree. When I'm finished creating the pact with pact-js, my idea is to consume it with pact-jvm. The graphql-java community is big and might benefit from this kind of dsl too.

It's a bit of a "damned if you do, damned if you don't" situation

It seems that way. But maybe a mode can be introduced that will change the default behavior or gives the implementation a different strategy.

[mefellows]

No worries at all. It'll just help us think through how we might be able to deliver this.

This being said, if your actual client query is sending through extra fields but is failing the pact test, then it really belongs in the expectation in the first place - the point of the test is to confirm what your code is doing is as expected.

I'll share this with other maintainers and get their thoughts. Any feedback on your use with Pact vs potential other alternatives would be great (I believe Apollo has a way of locking schemas on both client/server?).

In the meanwhile, I'll explore API options to support both graphql-tag and other use cases (it's just a small facade, so perhaps we can have two of them, or a switch as you say).

[ngbrown]

I'm not clear on why Pact need modes around the GraphQL DSL. I was using Pact with GraphQL queries before the DSL helper was added. It was working fine then, but I had to be careful to get the spaces exactly the same as what the request actually was. Now with the DSL helper, the spacing differences are ignored.

If the only change is to handle not having operationName set when it's not specified with a DSL call, then I'm fine with that (same as .withOperation(undefined)).

If it's to change the variable object again when .withVariable() isn't called, then I'm not fine. I can't tell what client @tomasAlabes is using from his code, but if it is sending an empty object as the variable, then I think he can specify the expectation .withVariable({}) to make it match.

[tomasAlabes]

Hi @ngbrown, I'm using Apollo's graphql-tag. As long as variables and operationName can be defined in the expectation, I think the approach @mefellows mentioned can be used with any client.
The mode might help to tailor the dsl to a specific client but I agree it is totally optional.

As the example in this repo uses graphql-tag, I was suggesting to use that too in the tests.

Thanks

[mefellows]

I think I'm going to leave the extra option out for now to reduce confusion / noise in the API itself, and ensure that only undefined properties are not represented in the output JSON. Pact is complicated enough already :)

[archanarachuri]

@tomasAlabes I'm facing same issue and I disabled __typename in the consumer tests when mocking mutation but now fails provider tests. Did you find a workaround for this?

● Verify graphql endpoint with pact testing › should adhere to pact contract

INFO: Reading pact at https://jardendigital.pact.dius.com.au/pacts/provider/contractservice/consumer/advisoronboarding/latest/master

Verifying a pact between advisoronboarding and contractservice

  A create contract request
    with POST /graphql
      returns a response which

        has status code 200

        has a matching body (FAILED - 1)

        includes headers

          "Content-Type" which equals "application/json; charset=utf-8" (FAILED - 2)


Failures:

  1) Verifying a pact between advisoronboarding and contractservice A create contract request with POST /graphql returns a response which has a matching body
     Failure/Error: expect(response_body).to match_term expected_response_body, diff_options, example

       Actual: {"data":{"createContract":{"contract":{"id":"ba2847f4-b58b-4c14-bdac-77b6cca032c0","name":null,"contractStatus":"DRAFT","clientId":"Testing","riskLevel":"LOW","managementBasis":null,"serviceLevel":null,"modificationHistory":{"dateOfCreation":"2019-11-07T10:14:13.000000+13:00","dateOfLastModification":"2019-11-07T10:14:13.000000+13:00"},"advisers":[],"primaryAdvisers":[],"state":"CURRENT"}}}}

       Diff
       --------------------------------------
       Key: - is expected 
            + is actual 
       Matching keys and values are not shown

          "data": {
            "createContract": {
              "contract": {
       -        "__typename": String
              }
            }
          }

       Description of differences
       --------------------------------------
       * Could not find key "__typename" (keys present are: id, name, contractStatus, clientId, riskLevel, managementBasis, serviceLevel, modificationHistory, advisers, primaryAdvisers, state) at $.data.createContract.contract

  2) Verifying a pact between advisoronboarding and contractservice A create contract request with POST /graphql returns a response which includes headers "Content-Type" which equals "application/json; charset=utf-8"
     Failure/Error: expect(header_value).to match_header(name, expected_header_value)
       Expected header "Content-Type" to equal "application/json; charset=utf-8", but was "application/json; charset=utf-8"


1 interaction, 1 failure

[tomasAlabes]

I solved it with the new ApolloGraphQLInteraction