From bd9d6adb7365066598a13e21a8fc01598fc7a793 Mon Sep 17 00:00:00 2001 From: Wilker Lucio Date: Tue, 3 Dec 2024 00:39:47 -0300 Subject: [PATCH] WIP (BROKEN STATE, DO NOT USE THIS COMMIT) - improving error messages --- .../com/wsscode/pathom3/connect/planner.cljc | 9 - .../com/wsscode/pathom3/connect/runner.cljc | 78 +++++- .../wsscode/pathom3/connect/runner/async.cljc | 10 +- .../pathom3/connect/runner/parallel.cljc | 6 +- src/main/com/wsscode/pathom3/error.cljc | 23 +- .../wsscode/pathom3/connect/runner_test.cljc | 262 ++++++++++++++---- 6 files changed, 278 insertions(+), 110 deletions(-) diff --git a/src/main/com/wsscode/pathom3/connect/planner.cljc b/src/main/com/wsscode/pathom3/connect/planner.cljc index 1cf06fd6..800f4dd6 100644 --- a/src/main/com/wsscode/pathom3/connect/planner.cljc +++ b/src/main/com/wsscode/pathom3/connect/planner.cljc @@ -575,15 +575,6 @@ op-name (update-in [::index-resolver->nodes op-name] coll/sconj node-id)))) -(defn create-and [graph env node-ids] - (if (= 1 (count node-ids)) - (get-node graph (first node-ids)) - (let [{and-node-id ::node-id - :as and-node} (new-node env {})] - (-> graph - (include-node and-node) - (add-node-branches and-node-id ::run-and node-ids))))) - (defn create-root-and [graph env node-ids] (if (= 1 (count node-ids)) (set-root-node graph (first node-ids)) diff --git a/src/main/com/wsscode/pathom3/connect/runner.cljc b/src/main/com/wsscode/pathom3/connect/runner.cljc index 5f760aa2..11508df6 100644 --- a/src/main/com/wsscode/pathom3/connect/runner.cljc +++ b/src/main/com/wsscode/pathom3/connect/runner.cljc @@ -467,7 +467,45 @@ ::pcp/foreign-ast (::pcp/foreign-ast node)} input-data)) -(defn wait-batch-check +(defn input-missing-error-message [nodes attr] + (if (> (count nodes) 1) + (str "- Attribute " attr " was expected to be returned from resolvers " + (str/join ", " (distinct (map ::pco/op-name nodes))) + " but all of them failed to provide it.") + (str "- Attribute " attr " was expected to be returned from resolver " (::pco/op-name (first nodes)) " but it failed to provide it."))) + +(defn input-missing-detail-OR + [{::pcp/keys [graph]} or-node attr] + (let [resolver-nodes (->> (pcp/node-successors graph (::pcp/node-id or-node)) + (map #(pcp/get-node graph %)) + (filter #(and + (contains? (::pcp/expects %) attr) + (::pco/op-name %))))] + (input-missing-error-message resolver-nodes attr))) + +(defn input-missing-detail + [{::pcp/keys [graph] :as env} node attr] + (tap> ["G" graph]) + (if-let [missed-parent (->> (pcp/node-ancestors graph (::pcp/node-id node)) + (map #(pcp/get-node graph %)) + (filter #(contains? (::pcp/expects %) attr)) + first)] + (if (::pcp/run-or missed-parent) + (input-missing-detail-OR env missed-parent attr) + (input-missing-error-message [missed-parent] attr)) + (str "Can't find parent node that should provide attribute " attr ", this is likely a bug in Pathom, please report it."))) + +(defn missing-input-exception [env node missing] + (ex-info + (str "Resolver " (::pco/op-name node) " can't be called due to missing required dependencies:\n" + (str/join "\n" (map #(input-missing-detail env node %) (keys missing)))) + {::attributes-missing missing})) + +(defn input-missing-check + "This will verify if all dependencies required by a resolver are satisfied. + + One special case here might be a dependency from a batch process that's pending to run. In this case, + we return a special block of data that tells Pathom to wait for the batch to run and try it again." [env node entity input input+opts] (let [missing-all (pfsd/missing-from-data entity input+opts) wait-batch? (and missing-all @@ -480,7 +518,7 @@ (wait-batch-response env node) missing - ::node-error))) + (throw (missing-input-exception env node missing))))) (defn invoke-resolver-from-node "Evaluates a resolver using node information. @@ -493,6 +531,7 @@ {::pco/keys [op-name] ::pcp/keys [input] :as node}] + (tap> ["invoke" op-name]) (let [resolver (pci/resolver env op-name) {::pco/keys [op-name batch? cache? cache-store optionals] :or {cache? true} @@ -508,10 +547,10 @@ _ (merge-node-stats! env node {::resolver-run-start-ms (time/now-ms)}) response (try - (let [batch-check (wait-batch-check env node entity input input+opts)] + (let [missing-check (input-missing-check env node entity input input+opts)] (cond - batch-check - batch-check + missing-check + missing-check batch? (if-let [x (p.cache/cache-find resolver-cache* [op-name input-data params])] @@ -528,6 +567,7 @@ (report-resolver-error env node e))) finish (time/now-ms) response (validate-response! env node response)] + (tap> ["response" op-name response]) (merge-node-stats! env node (cond-> {::resolver-run-finish-ms finish} (not (::batch-hold response)) @@ -645,14 +685,14 @@ [{::keys [node-run-stats*]} {::pcp/keys [node-id]} taken-path-id] (refs/gswap! node-run-stats* update-in [node-id ::taken-paths] coll/vconj taken-path-id)) -(defn fail-or-error [or-node errors] +(defn or-error-exception [or-node errors] (ex-info (str "All paths from an OR node failed. Expected: " (::pcp/expects or-node) "\n" (str/join "\n" (mapv ex-message errors))) {:errors errors})) (defn handle-or-error [env or-node res] - (let [error (fail-or-error or-node (::or-option-error res))] + (let [error (or-error-exception or-node (::or-option-error res))] (mark-node-error-with-plugins env or-node error) (fail-fast env error))) @@ -677,16 +717,17 @@ (let [picked-node-id (choose-path env or-node nodes) node-id (if (contains? nodes picked-node-id) picked-node-id - (do + (let [actual-node (first nodes)] (l/warn ::event-invalid-chosen-path {:expected-one-of nodes :chosen-attempt picked-node-id - :actual-used (first nodes)}) - (first nodes)))] + :actual-used actual-node}) + actual-node))] (add-taken-path! env or-node node-id) (let [res (try (run-node! env (pcp/get-node graph node-id)) (catch #?(:clj Throwable :cljs :default) e + {::or-option-error e}))] (cond (::batch-hold res) @@ -704,7 +745,7 @@ (::batch-hold res) res - (and (::or-option-error res) + (and (seq (::or-option-error res)) (not (or-expected-optional? env or-node))) (handle-or-error env or-node res) @@ -830,9 +871,18 @@ (doseq [ast (::pcp/mutations graph)] (invoke-mutation! env ast))) +(defn entity-missing-attribute-details + [{::pcp/keys [graph] + ::keys [node-run-stats*]} + attr] + (let [nodes (->> (get-in graph [::pcp/index-attrs attr]) + (filter #(get-in @node-run-stats* [% ::node-done?])) + (map #(pcp/get-node graph %)))] + (input-missing-error-message nodes attr))) + (defn check-entity-requires! "Verify if entity contains all required keys from graph index-ast. This is - shallow check (don't visit nested entities)." + a shallow check (don't visit nested entities)." [{::pcp/keys [graph] :as env}] (let [entity (p.ent/entity env) @@ -847,8 +897,8 @@ (fail-fast env (ex-info (str "Required attributes missing" (p.path/at-path-string env) - ": " - (pr-str (vec (keys missing)))) + ":\n" + (str/join "\n" (map #(entity-missing-attribute-details env %) (keys missing)))) {::attributes-missing missing ::p.error/phase ::execute ::p.error/cause ::p.error/attribute-missing}))))) diff --git a/src/main/com/wsscode/pathom3/connect/runner/async.cljc b/src/main/com/wsscode/pathom3/connect/runner/async.cljc index d9f365f1..d670843f 100644 --- a/src/main/com/wsscode/pathom3/connect/runner/async.cljc +++ b/src/main/com/wsscode/pathom3/connect/runner/async.cljc @@ -223,10 +223,12 @@ resolver-cache* (get env cache-store) _ (pcr/merge-node-stats! env node {::pcr/resolver-run-start-ms (time/now-ms)}) - batch-check (pcr/wait-batch-check env node entity input input+opts) + missing-check (try + (pcr/input-missing-check env node entity input input+opts) + (catch #?(:clj Throwable :cljs :default) e (p/rejected e))) response (-> (cond - batch-check - batch-check + missing-check + missing-check batch? (if-let [x (p.cache/cache-find resolver-cache* [op-name input-data params])] @@ -327,7 +329,7 @@ (::pcr/batch-hold res) res - (and (::pcr/or-option-error res) + (and (seq (::pcr/or-option-error res)) (not (pcr/or-expected-optional? env or-node))) (pcr/handle-or-error env or-node res) diff --git a/src/main/com/wsscode/pathom3/connect/runner/parallel.cljc b/src/main/com/wsscode/pathom3/connect/runner/parallel.cljc index 15b855d2..33d53b8f 100644 --- a/src/main/com/wsscode/pathom3/connect/runner/parallel.cljc +++ b/src/main/com/wsscode/pathom3/connect/runner/parallel.cljc @@ -314,8 +314,8 @@ resolver-cache* (get env cache-store)] (pcr/merge-node-stats! env node {::pcr/resolver-run-start-ms (time/now-ms)}) - (p/let [response (-> (if (pfsd/missing-from-data entity input) - ::pcr/node-error + (p/let [response (-> (if-let [missing (pfsd/missing-from-data entity input)] + (p/create (fn [_ reject] (reject (pcr/missing-input-exception env node missing)))) (cond batch? (if-let [x (p.cache/cache-find resolver-cache* [op-name input-data params])] @@ -409,7 +409,7 @@ (::pcr/batch-hold res) res - (and (::pcr/or-option-error res) + (and (seq (::pcr/or-option-error res)) (not (pcr/or-expected-optional? env or-node))) (pcr/handle-or-error env or-node res) diff --git a/src/main/com/wsscode/pathom3/error.cljc b/src/main/com/wsscode/pathom3/error.cljc index f5a422dd..1e4e57ba 100644 --- a/src/main/com/wsscode/pathom3/error.cljc +++ b/src/main/com/wsscode/pathom3/error.cljc @@ -118,24 +118,7 @@ (defn error-stack [err] (gobj/get err "stack"))) -(defn datafy-processor-error* [env] - (-> env - (select-keys [::error-data - ::error-message - ::error-stack - ::pcp/graph - :com.wsscode.pathom3.connect.runner/processor-error? - ;:com.wsscode.pathom3.connect.runner/processor-error-parent-env - :com.wsscode.pathom3.entity-tree/entity-tree - :com.wsscode.pathom3.path/path]) - (coll/update-if - :com.wsscode.pathom3.connect.runner/processor-error-parent-env - datafy-processor-error*))) - (defn datafy-processor-error [^Throwable err] - (let [env (ex-data err)] - (if (some-> env :com.wsscode.pathom3.connect.runner/processor-error?) - (datafy-processor-error* env) - {::error-message (ex-message err) - ::error-data (ex-data err) - ::error-stack (error-stack err)}))) + {::error-message (ex-message err) + ::error-data (ex-data err) + ::error-stack (error-stack err)}) diff --git a/test/com/wsscode/pathom3/connect/runner_test.cljc b/test/com/wsscode/pathom3/connect/runner_test.cljc index 786c38c1..dce209d8 100644 --- a/test/com/wsscode/pathom3/connect/runner_test.cljc +++ b/test/com/wsscode/pathom3/connect/runner_test.cljc @@ -400,7 +400,7 @@ (check (=> {:com.wsscode.pathom3.connect.runner/attribute-errors {:a {:com.wsscode.pathom3.error/cause :com.wsscode.pathom3.error/node-errors, - :com.wsscode.pathom3.error/node-error-details {1 {:com.wsscode.pathom3.error/cause :com.wsscode.pathom3.error/attribute-missing}}}}} + :com.wsscode.pathom3.error/node-error-details {1 {:com.wsscode.pathom3.error/cause ::p.error/node-exception}}}}} res)))) (testing "ending values" @@ -637,67 +637,46 @@ [(pco/? :err)] {:ex/message "Resolver -unqualified/err--const-fn exception: error"}))) - (testing "resolver missing response" - (check-all-runners-ex - (pci/register - (pco/resolver 'foo - {::pco/output [:foo]} - (fn [_ _] {}))) - {} - [:foo] - {:ex/message "Required attributes missing: [:foo]" - :ex/data {::pcr/attributes-missing {:foo {}}}}) + (testing "bug report #120" + (check-all-runners + (-> (pci/register [(pco/resolver 'dashboard-chartObjects-1 + {::pco/input [:portfolioKey :appKey :data-source/friendlyName] + ::pco/output [{:dashboard [:dashboard/chartObjects]}]} + (fn [_ _] + {:dashboard {:dashboard/chartObjects []}})) - (check-all-runners-ex - (pci/register - (pco/resolver 'foo - {::pco/output [:foo]} - (fn [_ _] {}))) - {:container {}} - [{:container [:foo]}] - {:ex/message "Required attributes missing at path [:container]: [:foo]" - :ex/data {::pcr/attributes-missing {:foo {}}}}) + (pco/resolver 'dashboard-chartObjects-2 + {::pco/input [:portfolioKey :appKey {:data-sources [:data-source/friendlyName]}] + ::pco/output [{:dashboard [:dashboard/chartObjects]}]} + (fn [_ _] + {:dashboard {:dashboard/chartObjects []}})) - (testing "bug report #120" - (check-all-runners - (-> (pci/register [(pco/resolver 'dashboard-chartObjects-1 - {::pco/input [:portfolioKey :appKey :data-source/friendlyName] - ::pco/output [{:dashboard [:dashboard/chartObjects]}]} - (fn [_ _] - {:dashboard {:dashboard/chartObjects []}})) - - (pco/resolver 'dashboard-chartObjects-2 - {::pco/input [:portfolioKey :appKey {:data-sources [:data-source/friendlyName]}] - ::pco/output [{:dashboard [:dashboard/chartObjects]}]} - (fn [_ _] - {:dashboard {:dashboard/chartObjects []}})) - - (pco/resolver 'data-source - {::pco/input [:portfolioKey :appKey] - ::pco/output [:appName :platform]} - (fn [_ _] - {:appName nil :platform nil})) - - (pco/resolver 'data-sources - {::pco/input [:portfolioKey] - ::pco/output [{:data-sources [:portfolioKey :appKey :appName :platform]}] - ::pco/cache-store :thirty-sec-ttl-cache*} - (fn [_ _] - {:data-sources []})) - - (pco/resolver 'data-source-friendlyName - {::pco/input [:appName] - ::pco/output [:data-source/friendlyName]} - (fn [_ _] - {:data-source/friendlyName "appName"}))])) - {:portfolioKey "portfolioKey", :appKey "appKey"} - [:data-source/friendlyName {:dashboard [:dashboard/chartObjects]}] - {:portfolioKey "portfolioKey", - :appKey "appKey", - :appName nil, - :platform nil, - :data-source/friendlyName "appName", - :dashboard {:dashboard/chartObjects []}}))) + (pco/resolver 'data-source + {::pco/input [:portfolioKey :appKey] + ::pco/output [:appName :platform]} + (fn [_ _] + {:appName nil :platform nil})) + + (pco/resolver 'data-sources + {::pco/input [:portfolioKey] + ::pco/output [{:data-sources [:portfolioKey :appKey :appName :platform]}] + ::pco/cache-store :thirty-sec-ttl-cache*} + (fn [_ _] + {:data-sources []})) + + (pco/resolver 'data-source-friendlyName + {::pco/input [:appName] + ::pco/output [:data-source/friendlyName]} + (fn [_ _] + {:data-source/friendlyName "appName"}))])) + {:portfolioKey "portfolioKey", :appKey "appKey"} + [:data-source/friendlyName {:dashboard [:dashboard/chartObjects]}] + {:portfolioKey "portfolioKey", + :appKey "appKey", + :appName nil, + :platform nil, + :data-source/friendlyName "appName", + :dashboard {:dashboard/chartObjects []}})) (testing "mutations" (let [err (ex-info "Fail fast" {})] @@ -760,6 +739,169 @@ (m/equals {:err {:com.wsscode.pathom3.error/cause :com.wsscode.pathom3.error/attribute-unreachable}})}))))) +(deftest run-graph!-fail-resolver-missing-data + (testing "single resolver graph" + (check-all-runners-ex + (pci/register + (pco/resolver 'my-resolver + {::pco/output [:foo]} + (fn [_ _] {}))) + {} + [:foo] + {:ex/message (str + "Required attributes missing:\n" + "- Attribute :foo was expected to be returned from resolver my-resolver but it failed to provide it.") + :ex/data {::pcr/attributes-missing {:foo {}}}}) + + (testing "at path" + (check-all-runners-ex + (pci/register + (pco/resolver 'my-resolver + {::pco/output [:foo]} + (fn [_ _] {}))) + {:container {}} + [{:container [:foo]}] + {:ex/message (str + "Required attributes missing at path [:container]:\n" + "- Attribute :foo was expected to be returned from resolver my-resolver but it failed to provide it.") + :ex/data {::pcr/attributes-missing {:foo {}}}})) + + (testing "multiple options" + (check-all-runners-ex + (pci/register + [(pco/resolver 'a1 + {::pco/output [:a]} + (fn [_ _] + {})) + + (pco/resolver 'a2 + {::pco/output [:a]} + (fn [_ _] + {}))]) + {} + [:a] + {:ex/message (str + "Required attributes missing:\n" + "- Attribute :a was expected to be returned from resolvers a2, a1 but all of them failed to provide it.") + :ex/data {::pcr/attributes-missing {:a {}}}}))) + + (testing "multiple attributes" + (check-all-runners-ex + (pci/register + (pco/resolver 'my-resolver + {::pco/output [:foo :bar]} + (fn [_ _] {}))) + {} + [:foo :bar] + {:ex/message (str + "Required attributes missing:\n" + "- Attribute :foo was expected to be returned from resolver my-resolver but it failed to provide it.\n" + "- Attribute :bar was expected to be returned from resolver my-resolver but it failed to provide it.") + :ex/data {::pcr/attributes-missing {:foo {} :bar {}}}}) + + (check-all-runners-ex + (pci/register + [(pco/resolver 'my-resolver-foo + {::pco/output [:foo]} + (fn [_ _] {})) + + (pco/resolver 'my-resolver-bar + {::pco/output [:bar]} + (fn [_ _] {}))]) + {} + [:foo :bar] + {:ex/message (str + "Required attributes missing:\n" + "- Attribute :foo was expected to be returned from resolver my-resolver-foo but it failed to provide it.\n" + "- Attribute :bar was expected to be returned from resolver my-resolver-bar but it failed to provide it.") + :ex/data {::pcr/attributes-missing {:foo {} :bar {}}}})) + + (testing "missing dependency" + (testing "missed attribute comes from resolver node" + (check-all-runners-ex + (pci/register + [(pco/resolver 'a + {::pco/output [:a]} + (fn [_ _] {})) + + (pco/resolver 'b + {::pco/input [:a] + ::pco/output [:b]} + (fn [_ _] {}))]) + {} + [:b] + {:ex/message (str + "Resolver b exception: Resolver b can't be called due to missing required dependencies:\n" + "- Attribute :a was expected to be returned from resolver a but it failed to provide it.") + :ex/data {::pcr/attributes-missing {:a {}}}})) + + (testing "missed on indirect dependency" + (check-all-runners-ex + (pci/register + [(pco/resolver 'ab + {::pco/output [:a :b]} + (fn [_ _] {:a 1})) + + (pco/resolver 'c + {::pco/input [:a] + ::pco/output [:c]} + (fn [_ _] {:c 2})) + + (pco/resolver 'd + {::pco/input [:b :c] + ::pco/output [:d]} + (fn [_ _] {}))]) + {} + [:d] + {:ex/message (str + "Resolver d exception: Resolver d can't be called due to missing required dependencies:\n" + "- Attribute :b was expected to be returned from resolver ab but it failed to provide it.") + :ex/data {::pcr/attributes-missing {:b {}}}})) + + (testing "missed attribute comes from OR node" + (check-all-runners-ex + (pci/register + [(pco/resolver 'a1 + {::pco/output [:a]} + (fn [_ _] {})) + + (pco/resolver 'a2 + {::pco/output [:a]} + (fn [_ _] {})) + + (pco/resolver 'b + {::pco/input [:a] + ::pco/output [:b]} + (fn [_ _] {}))]) + {} + [:b] + {:ex/message (str + "Resolver b exception: Resolver b can't be called due to missing required dependencies:\n" + "- Attribute :a was expected to be returned from resolvers a1, a2 but all of them failed to provide it.") + :ex/data {::pcr/attributes-missing {:a {}}}})) + + (testing "missed attribute comes from AND node" + (comment + (run-graph + (pci/register + [(pco/resolver 'a + {::pco/output [:a]} + (fn [_ _] + {})) + + (pco/resolver 'b + {::pco/output [:b]} + (fn [_ _] + {})) + + (pco/resolver 'c + {::pco/input [:a :b] + ::pco/output [:c]} + (fn [_ _] + {}))]) + {} + [:c]))))) + (deftest run-graph!-final-test (testing "map value" (is (graph-response? (pci/register geo/registry)