Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes a runtime error that occurs when deep-merging fragments #462

Closed
wants to merge 4 commits into from
Closed

Fixes a runtime error that occurs when deep-merging fragments #462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4932394
Add failing tests for deep-merge'in fragment error
namenu Jun 25, 2024
11d5c68
Fix failing test
namenu Jun 25, 2024
1602b0c
add tests for deep-merge
namenu Jun 25, 2024
0550585
Add another test
namenu Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/com/walmartlabs/lacinia/internal_utils.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,11 +407,19 @@
nil
coll))

(defn- null?
[v]
(or (nil? v)
(= v :com.walmartlabs.lacinia.schema/null)))

(defn deep-merge
"Merges two maps together. Later map override earlier.
If a key is sequential, then each element in the list is merged."
[left right]
(cond
(or (null? left) (null? right))
Copy link
Contributor Author

@namenu namenu Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I observed that left/rigt can be ::null or nil.
It depends on whether the object that catches bubbled-up error is non-null or not.

Test case in L169 covers this.

:com.walmartlabs.lacinia.schema/null

(and (map? left) (map? right))
(merge-with deep-merge left right)

Expand Down
143 changes: 122 additions & 21 deletions test/com/walmartlabs/lacinia/executor_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@
(ns com.walmartlabs.lacinia.executor-test
"Tests for errors and exceptions inside field resolvers, and for the exception converter."
(:require
[clojure.test :refer [deftest is]]
[clojure.test :refer [deftest is testing]]
[com.walmartlabs.lacinia.resolve :refer [resolve-as]]
[com.walmartlabs.test-utils :refer [execute]]
[com.walmartlabs.lacinia.schema :as schema]))

(deftest deep-merge-on-error
(def compiled-schema
(let [test-schema {:interfaces
{:Node
{:fields {:id {:type '(non-null String)}}}}
Expand All @@ -36,30 +36,45 @@
:resolve (fn [_ _ _]
"Hello, World!")}}}

:Author
:PublicDomainPost
{:implements [:Node]
:fields {:id {:type '(non-null String)}
:name {:type '(non-null String)
:resolve (fn [_ _ _]
"John Doe")}
:absurd {:type '(non-null String)
:author {:type :Author ;; Author is nullable
:resolve (fn [_ _ _] nil)}
:title {:type 'String
:resolve (fn [_ _ _]
(resolve-as nil {:message "This field can't be resolved."}))}}}}
"Epic of Gilgamesh")}}}

:Author
{:implements [:Node]
:fields {:id {:type '(non-null String)}
:name {:type '(non-null String)
:resolve (fn [_ _ _]
"John Doe")}
:alwaysNull {:type 'String
:resolve (fn [_ _ _]
nil)}
:alwaysFail {:type '(non-null String)
:resolve (fn [_ _ _]
(resolve-as nil {:message "This field can't be resolved."}))}}}}

:queries
{:node {:type '(non-null :Node)
:args {:id {:type '(non-null String)}}
:resolve (fn [ctx args v]
(let [{:keys [episode]} args]
(schema/tag-with-type {:id "1000"} :Post)))}}}
compiled-schema (schema/compile test-schema)]
:resolve (fn [_ctx args _v]
(let [{:keys [id]} args]
(case id
"1000" (schema/tag-with-type {:id id} :Post)
"2000" (schema/tag-with-type {:id id} :PublicDomainPost))))}}}]
(schema/compile test-schema)))

(is (= {:data nil,
:errors [{:message "This field can't be resolved.", :locations [{:line 4, :column 5}], :path [:node :author :absurd]}]}
(execute compiled-schema "
(deftest deep-merge-on-error
(is (= {:data nil,
:errors [{:message "This field can't be resolved.", :locations [{:line 4, :column 5}], :path [:node :author :alwaysFail]}]}
(execute compiled-schema "
fragment PostFragment on Post {
author {
absurd
alwaysFail
}
}
query MyQuery {
Expand All @@ -74,12 +89,12 @@ query MyQuery {
}
}")))

(is (= {:data nil,
:errors [{:message "This field can't be resolved.", :locations [{:line 4, :column 5}], :path [:node :author :absurd]}]}
(execute compiled-schema "
(is (= {:data nil,
:errors [{:message "This field can't be resolved.", :locations [{:line 4, :column 5}], :path [:node :author :alwaysFail]}]}
(execute compiled-schema "
fragment PostFragment on Post {
author {
absurd
alwaysFail
}
}
query MyQuery {
Expand All @@ -92,4 +107,90 @@ query MyQuery {
}
id
}
}")))))
}")))

(testing "when non-null field is resolved to nil, deep-merge should return nil"
(is (= {:data nil,
:errors [{:message "This field can't be resolved.",
:locations [{:line 13, :column 5}],
:path [:node :author :alwaysFail]}]}
(execute compiled-schema "
query MyQuery {
node(id: \"1000\") {
... on Post {
id
...PostFragment
}
}
}

fragment PostFragment on Post {
author {
alwaysFail
}
...PostFragment2
}

fragment PostFragment2 on Post {
author {
name
}
}
")))

(is (= {:data nil,
:errors [{:message "This field can't be resolved.",
:locations [{:line 14, :column 5}],
:path [:node :author :alwaysFail]}]}
(execute compiled-schema "
query MyQuery {
node(id: \"1000\") {
... on Post {
id
...PostFragment
}
}
}

fragment PostFragment on Post {
...PostFragment2
author {
alwaysFail
}
}

fragment PostFragment2 on Post {
author {
name
}
}
")))

(testing "Nullable parent (PublicDomainPost) with failing non-null child (Author)"
(is (= {:data {:node {:id "2000", :author nil}}}
(execute compiled-schema "
query MyQuery {
node(id: \"2000\") {
... on PublicDomainPost {
id
...PostFragment
}
}
}

fragment PostFragment on PublicDomainPost {
...PostFragment2
author {
alwaysFail
}
}

fragment PostFragment2 on PublicDomainPost {
author {
name
}
}
"))))))

(comment
(deep-merge-on-error))
57 changes: 55 additions & 2 deletions test/com/walmartlabs/lacinia/internal_utils_tests.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@

(ns com.walmartlabs.lacinia.internal-utils-tests
(:require
[clojure.test :refer [deftest is]]
[com.walmartlabs.lacinia.internal-utils :refer [assoc-in! update-in!]]
[clojure.test :refer [deftest testing is]]
[com.walmartlabs.lacinia.internal-utils :refer [assoc-in! update-in! deep-merge]]
[clojure.string :as str])
(:import
(clojure.lang ExceptionInfo)))
Expand Down Expand Up @@ -63,3 +63,56 @@
:map {:name {:type String}}
:more-keys (:description)}
(ex-data e)))))

(deftest test-deep-merge
(testing "Basic map merge"
(is (= (deep-merge {:a 1} {:b 2})
{:a 1, :b 2}))
(is (= (deep-merge {:a 1} {:a 2})
{:a 2})))

(testing "Nested map merge"
(is (= (deep-merge {:a {:b 1}} {:a {:c 2}})
{:a {:b 1, :c 2}}))
(is (= (deep-merge {:a {:b 1}} {:a {:b 2}})
{:a {:b 2}})))

(testing "Mixed map and sequential"
(is (thrown-with-msg? ExceptionInfo #"unable to deep merge"
(deep-merge {:a 1} [1 2 3])))
(is (thrown-with-msg? ExceptionInfo #"unable to deep merge"
(deep-merge [1 2 3] {:a 1}))))

#_(testing "Merge with nil values"
(is (thrown-with-msg? ExceptionInfo #"unable to deep merge" (deep-merge {:a 1} nil)
{:a 1}))
(is (thrown-with-msg? ExceptionInfo #"unable to deep merge" (deep-merge nil {:a 1})
{:a 1})))

(testing "Merging with :com.walmartlabs.lacinia.schema/null values"
(is (= (deep-merge {:a 1} :com.walmartlabs.lacinia.schema/null)
:com.walmartlabs.lacinia.schema/null))
(is (= (deep-merge :com.walmartlabs.lacinia.schema/null {:a 1})
:com.walmartlabs.lacinia.schema/null))
(is (= (deep-merge :com.walmartlabs.lacinia.schema/null :com.walmartlabs.lacinia.schema/null)
:com.walmartlabs.lacinia.schema/null)))

(testing "Merging with empty maps"
(is (= (deep-merge {} {:a 1})
{:a 1}))
(is (= (deep-merge {:a 1} {})
{:a 1})))

(testing "Complex nested structures"
(is (= (deep-merge {:a {:b [1 2]}} {:a {:b [3 4]}})
{:a {:b [3 4]}}))
(is (= (deep-merge {:a [{:b 1} {:c 2}]} {:a [{:d 3} {:e 4}]})
{:a [{:b 1, :d 3} {:c 2, :e 4}]})))

(testing "Nested :com.walmartlabs.lacinia.schema/null values"
(is (= (deep-merge {:a {:b :com.walmartlabs.lacinia.schema/null}} {:a {:b 1}})
{:a {:b :com.walmartlabs.lacinia.schema/null}}))
(is (= (deep-merge {:a {:b 1}} {:a {:b :com.walmartlabs.lacinia.schema/null}})
{:a {:b :com.walmartlabs.lacinia.schema/null}}))
(is (= (deep-merge {:a {:b :com.walmartlabs.lacinia.schema/null, :c 2}} {:a {:b 1, :c :com.walmartlabs.lacinia.schema/null}})
{:a {:b :com.walmartlabs.lacinia.schema/null, :c :com.walmartlabs.lacinia.schema/null}}))))
Loading