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

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

Merged
merged 4 commits into from
Jun 26, 2024
Merged

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

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))
:com.walmartlabs.lacinia.schema/null

Comment on lines +420 to +422
Copy link

Choose a reason for hiding this comment

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

그냥 요러케 고쳐보면 어떨까싶은 느낌이 있습니다.. nil또는 null을 반환하게..

(cond
    (null? left)
    left

    (null? right)
    right
...

null이 반환되는 경우에는 애초에 null이 들어왔다는건데, 이건 보통 필드리졸버가 null을 임시로 넣어두고 나중에 null-collapser로 nil처리가 될거같고요,
nil이 애초에 들어왔다면 이미 null-collapser를 통과한 값이거나 unauthorized 상황인거라 그냥 nil 반환해주면되는거 아닐까 싶은 생각인데.. 테스트를 좀해봐야. ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

#12

(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