graphql2/error-stamper misses some global errors

[holyjak]

When I run this query:

(parser {}
          [{[:mygql.subscription/phoneNumber "98765432"]
            [:mygql.SubscriptionData/roles]}])

Pathom gets this response:

#:com.wsscode.pathom.diplomat.http{
  :status 200, 
  :body {:data nil, 
         :errors [{:path ["_mygql_subscription_phoneNumber_98765432"], 
                   :data nil, :errorType "AUTHENTICATION", :errorInfo nil, 
                   :message "INVALID_OR_EXPIRED_AUTH_TOKEN"}]}, ...}

but the result the user gets is

{[:mygql.subscription/phoneNumber "98765432"] {}}

i.e. the error is swallowed instead of being made into a reader error.

It should be caught and propagated by com.wsscode.pathom.connect.graphql2/error-stamper but it is not due to this part:

  (let [path' (mapv #(cond
                       (p/ident? %)
                       (demung (namespace (first %))) ; <----

When called, the input errors is {["_subscription_phoneNumber_98765432"] ...}, path is [[:subscription/phoneNumber 98765432]], but path' becomes only [subscription] instead of ["_subscription_phoneNumber_98765432"]. The error is thus not considered to be a "local error" and is ignored.

A fix would be I guess to replace (namespace (first %))) with (com.wsscode.pathom.graphql/ident->alias %) but I assume that the current code is there for a reason and is the right way to do it in other circumstances.

I'd be more than happy to send a PR if you can guide me to as when to use (namespace (first %))) vs. ident->alias.

Follow-up error

When I fix error-stamper to detect the error, it will cause another error in pull-idents (called from graphql-resolve to process the result of (parser-item ...) because it will be called with data like this: {[:subscription/phoneNumber "98765432"] :com.wsscode.pathom.core/reader-error}] and it will fail with

Parser Error: Don't know how to create ISeq from: clojure.lang.Keyword {}
when trying to (into x v) where x={}, v=::p/reader-error

So something needs to be changed there as well.

[wilkerlucio]

Great debugging on this, your reasoning makes sense, if you send a PR with the fix I'll be glad to merge it. So seems we need to fix the error-stamper and the parser-item. Please let me know if there is contextual info you need. About the ident->alias part, I don't remember, you are probably making more use of that more than me now, so you can just use your judgment on how to go there.

[holyjak]

Hi, thank you for the kind words!

If I fix this then I will break the current tests, here:

pathom/test/com/wsscode/pathom/connect/graphql2_test.cljc

Line 327 in 7f5ebe0

:path ["customer" "creditCardAccount"]

the test assumes that a query for _customer_customer_id_123: customer(...) will return an error with :path ["customer" "creditCardAccount"] while in my case the path would be ["_customer_customer_id_123" "creditCardAccount"]. So the question is: is the test wrong, based on false assumptions? Or can both cases occur, perhaps depending on the GraphQL implementation?

From the spec:

If the error happens in an aliased field, the path to the error should use the aliased name, since it represents a path in the response, not in the query.

This example in the spec shows that the current code would be correct if we did not use aliases. So perhaps the question is: do we always create aliases when idents / parametrized fields are involved? I guess the answer is here:

pathom/src/com/wsscode/pathom/graphql.cljc

Line 108 in 7f5ebe0

::index (ident->alias key))

but I do not understand the data and code well enough to draw a firm conclusion.

Examples of the expected behavior

Two examples from my practice:

1. An alias

(parser {} [{[:channelapi.subscription/phoneNumber "98765432"]
               [:channelapi.SubscriptionData/roles]}])
;;=> 
  ;{[:channelapi.subscription/phoneNumber "98765432"] {},
  ; ::p/errors {[[:channelapi.subscription/phoneNumber "98765432"]]
  ;             {:path ["_subscription_phoneNumber_98765432"],
  ;              :message "INVALID_OR_EXPIRED_AUTH_TOKEN", ...}}}

2. Without an alias

(parser {} [{:channelapi/subscriptions
               [{[:channelapi.invoice/id "fakeid"] [:channelapi.Invoice/invoiceNumber]}]}])
;;=> 
  ;{::p/errors {[:channelapi/subscriptions]
  ;             {:path ["subscriptions"],
  ;              :message "INVALID_OR_EXPIRED_AUTH_TOKEN", ...}}}