Fork me on GitHub
#pathom
<
2018-12-20
>
Alex H20:12:56

I've read through the documentation, and part of the code, and I just can't get an exception to actually be thrown when I throw it within a mutation. ::pp/fail-fast? doesn't do anything, the explicit error handler doesn't do anything. It seems to me like the code in pp/parser always does try/catch for a mutation, and so I always get a ::pp/error value instead of the exception being thrown.

Alex H20:12:30

In particular, this bit - the call (action) is what throws (down the line is the user-provided mutation of pathom connect), and that always get caught.

value (case type
                          :call
                          (do
                            (assert mutate "Parse mutation attempted but no :mutate function supplied")
                            (let [{:keys [action]} (mutate env key params)]
                              (if action
                                (try
                                  (action)
                                  (catch #?(:clj Throwable :cljs :default) e
                                    {::error e})))))

wilkerlucio20:12:22

there were changes with the parallel that made things more complected around error catching, I didn't tried but I guess the fail fast thing is broken, can you please file an issue for it?

wilkerlucio20:12:57

and just wondering, for what purpose are you trying to get the extension to explode? its for debug it?

Alex H20:12:35

actually, I use the exceptions for validation, to be caught later in a ring handler, to format as a special exception response

wilkerlucio20:12:38

humm, pathom is more focused on partial failure modes, its intended to never explode entirely

wilkerlucio20:12:57

you can have a request with multiple mutations and only some fails, pathom is designed to get something back to the user, so it never fails entirelyu

Alex H20:12:40

well, yes, and I don't disagree. However, the straight-forward migration path for me is to have an overall error response

Alex H20:12:59

I've raised https://github.com/wilkerlucio/pathom/issues/69 regarding the ::pp/fail-fast? behaviour.

Alex H20:12:59

I'm only doing one mutation at a time, so the difference is mostly academic anyway. It's just easier for my client-side bits to recognize the error when it comes in that special format, rather than as a special result value in a normal mutation result.

Alex H20:12:43

it's mostly just showing an overall error message, effectively.

wilkerlucio20:12:06

cool, the reader part might need some more work, but I think to fail fast mutations will be an easy fix

wilkerlucio20:12:25

I'll probably be able to get to it by the weekend, thanks for the report

wilkerlucio21:12:15

ah, I just tough a simple thing you can do to make it explode until then

wilkerlucio21:12:33

you can create a plugin around everything, look at mutation responses and trigger an error if you find any. makes sense?

Alex H21:12:53

yes, I was already looking into doing something like that

Alex H21:12:25

On a related note, it also seems that the error-handler-plugin is not called at all for errors in mutations

Alex H21:12:14

so if I was hoping to format it in some special way (not necessarily rethrowing), I don't see any way of doing that other than then going some other plugin that replaces the error values with (other) error values (or throws - as you suggested).

wilkerlucio21:12:19

I think mutate-async is trapping it

wilkerlucio21:12:36

you can write a custom mutate-async, its a relative small fn

wilkerlucio21:12:41

probably the fix will be changing it

Alex H21:12:52

hm, I'm not using async parsers at all

wilkerlucio21:12:53

this is the current impl:

wilkerlucio21:12:54

(defn mutate-async
  "Async mutate function to integrate connect mutations to pathom parser."
  [{::keys [indexes mutate-dispatch mutation-join-globals]
    :keys  [query]
    :or    {mutation-join-globals []}
    :as    env} sym' input]
  (if-let [{::keys [sym]} (get-in indexes [::index-mutations sym'])]
    (let [env (assoc-in env [:ast :key] sym)]
      {:action #(go-catch
                  (let [res (<?maybe (mutate-dispatch (assoc env ::source-mutation sym') input))]
                    (if query
                      (merge (select-keys res mutation-join-globals)
                             (<? (p/join (atom res) env)))
                      res)))})
    (throw (ex-info "Mutation not found" {:mutation sym'}))))

wilkerlucio21:12:20

humm, let me check the parallel parser

wilkerlucio21:12:15

are you using parallel parser right? do your reads need async?

Alex H21:12:50

(def parser
  (p/parser {::p/env     {::p/reader [p/map-reader pc/reader2 pc/open-ident-reader p/env-placeholder-reader]
                          ::p/placeholder-prefixes #{">"}
                          ::p/process-error process-error
                          ::p/fail-fast? true}
             ::p/mutate  pc/mutate
             ::p/plugins [(pc/connect-plugin {::pc/register app-registry})
                          p/request-cache-plugin
                          p/trace-plugin
                          p/error-handler-plugin]}))

wilkerlucio21:12:22

weird, the old parser should work fine with fail-fast and error handler plugin

Alex H21:12:56

how? that code segment I pasted in the bug report shows an unconditional try-catch around it, no?

wilkerlucio21:12:16

yeah, I'm just wondering, because I was thinking of a problem somewhere else

wilkerlucio21:12:10

ok, I'm gonna have to leave in a bit, sorry can't dig on this now, the regular parser and mutation parts could be interesting if you wanna investigate, by the weekend I'll have adequated time to look into

wilkerlucio21:12:07

oh, just read the code you posted on the issue, that makes sense, I gotta remember why I add that there

Alex H21:12:21

seems like only the parallel-parser does the right thing w.r.t. calling process-error

wilkerlucio16:12:18

@U8Q71JK0W just released pathom 2.2.5, this removes the try catch on the parser level, please let me know if that works properly for you, thanks for the patience for this one 🙂