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

쿼리 복잡도 버그 수정 및 결과 값 수정 #16

Merged
merged 11 commits into from
Sep 5, 2024
Merged

쿼리 복잡도 버그 수정 및 결과 값 수정 #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b981d63
fix case of root query does not have selection
1e16miin Aug 30, 2024
32a7511
change tc
1e16miin Aug 30, 2024
3fa3943
warning to analyze result
1e16miin Aug 30, 2024
106fe42
nit
1e16miin Aug 30, 2024
02f0d90
add tc
1e16miin Aug 30, 2024
bf0b2e4
rename
1e16miin Sep 5, 2024
590bca1
nit
1e16miin Sep 5, 2024
8dfebd5
nit
1e16miin Sep 5, 2024
d4f9b32
refactor
1e16miin Sep 5, 2024
b592837
apply review
1e16miin Sep 5, 2024
40748ac
refactor
1e16miin Sep 5, 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: 6 additions & 2 deletions dev-resources/complexity-analysis-error.edn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@
:Seller
{:implements [:Node :User]
:fields {:id {:type (non-null ID)}
:name {:type (non-null String)}
:name {:type (non-null String)
:resolve :resolve-name}
:products
{:type (non-null :ProductConnection)
:args {:first {:type Int}}
Expand All @@ -83,4 +84,7 @@
:queries {:node
{:type :Node
:args {:id {:type (non-null ID)}}
:resolve :resolve-node}}}
:resolve :resolve-node}
:root
{:type :String
:resolve :resolve-root}}}
63 changes: 30 additions & 33 deletions src/com/walmartlabs/lacinia.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
[com.walmartlabs.lacinia.util :refer [as-error-map]]
[com.walmartlabs.lacinia.resolve :as resolve]
[com.walmartlabs.lacinia.tracing :as tracing]
[com.walmartlabs.lacinia.complexity-analysis :as complexity-analysis])
[com.walmartlabs.lacinia.query-analyzer :as query-analyzer])
(:import (clojure.lang ExceptionInfo)))

(defn ^:private as-errors
Expand All @@ -34,37 +34,32 @@

Returns a [[ResolverResult]] that will deliver the result map, or an exception."
{:added "0.16.0"}
([parsed-query variables context]
(execute-parsed-query-async parsed-query variables context nil))
([parsed-query variables context options]
{:pre [(map? parsed-query)
(or (nil? context)
(map? context))]}
(cond-let
:let [{:keys [::tracing/timing-start]} parsed-query
;; Validation phase encompasses preparing with query variables and actual validation.
;; It's somewhat all mixed together.
start-offset (tracing/offset-from-start timing-start)
start-nanos (System/nanoTime)
[prepared error-result] (try
[(parser/prepare-with-query-variables parsed-query variables)]
(catch Exception e
[nil (as-errors e)]))]

(some? error-result)
(resolve/resolve-as error-result)

:let [validation-errors (validator/validate prepared)]

(seq validation-errors)
(resolve/resolve-as {:errors validation-errors})

:else (let [complexity-warning (when (:max-complexity options)
(complexity-analysis/complexity-analysis prepared options))]
(executor/execute-query (assoc context constants/parsed-query-key prepared
:complexity-warning complexity-warning
::tracing/validation {:start-offset start-offset
:duration (tracing/duration start-nanos)}))))))
[parsed-query variables context]
{:pre [(map? parsed-query)
(or (nil? context)
(map? context))]}
(cond-let
:let [{:keys [::tracing/timing-start]} parsed-query
;; Validation phase encompasses preparing with query variables and actual validation.
;; It's somewhat all mixed together.
start-offset (tracing/offset-from-start timing-start)
start-nanos (System/nanoTime)
[prepared error-result] (try
[(parser/prepare-with-query-variables parsed-query variables)]
(catch Exception e
[nil (as-errors e)]))]

(some? error-result)
(resolve/resolve-as error-result)

:let [validation-errors (validator/validate prepared)]

(seq validation-errors)
(resolve/resolve-as {:errors validation-errors})

:else (executor/execute-query (assoc context constants/parsed-query-key prepared
::tracing/validation {:start-offset start-offset
:duration (tracing/duration start-nanos)}))))

(defn execute-parsed-query
"Prepares a query, by applying query variables to it, resulting in a prepared
Expand All @@ -81,7 +76,9 @@
{:keys [timeout-ms timeout-error]
:or {timeout-ms 0
timeout-error {:message "Query execution timed out."}}} options
execution-result (execute-parsed-query-async parsed-query variables context options)
context' (cond-> context
(:analyze-query options) query-analyzer/enable-query-analyzer)
Comment on lines +79 to +80
Copy link
Author

Choose a reason for hiding this comment

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

@namenu 리뷰를 반영하여,
lacinia.clj execute 함수를 호출할 때 options 값에 :analyze-query 가 trucy 하다면 ::query-analyzer/enable? 라는 key를 context에 true로 assoc 하도록 수정하였습니다.

execution-result (execute-parsed-query-async parsed-query variables context')
result (do
(resolve/on-deliver! execution-result *result)
;; Block on that deliver, then return the final result.
Expand Down
36 changes: 18 additions & 18 deletions src/com/walmartlabs/lacinia/executor.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,19 @@
(ns com.walmartlabs.lacinia.executor
"Mechanisms for executing parsed queries against compiled schemas."
(:require
[com.walmartlabs.lacinia.internal-utils
:refer [cond-let q to-message
deep-merge keepv get-nested]]
[flatland.ordered.map :refer [ordered-map]]
[com.walmartlabs.lacinia.select-utils :as su]
[com.walmartlabs.lacinia.resolve-utils :refer [transform-result aggregate-results]]
[com.walmartlabs.lacinia.schema :as schema]
[com.walmartlabs.lacinia.resolve :as resolve
:refer [resolve-as resolve-promise]]
[com.walmartlabs.lacinia.tracing :as tracing]
[com.walmartlabs.lacinia.constants :as constants]
[com.walmartlabs.lacinia.selection :as selection])
[com.walmartlabs.lacinia.internal-utils
:refer [cond-let q to-message
deep-merge keepv get-nested]]
[flatland.ordered.map :refer [ordered-map]]
[com.walmartlabs.lacinia.select-utils :as su]
[com.walmartlabs.lacinia.resolve-utils :refer [transform-result aggregate-results]]
[com.walmartlabs.lacinia.schema :as schema]
[com.walmartlabs.lacinia.resolve :as resolve
:refer [resolve-as resolve-promise]]
[com.walmartlabs.lacinia.tracing :as tracing]
[com.walmartlabs.lacinia.constants :as constants]
[com.walmartlabs.lacinia.selection :as selection]
[com.walmartlabs.lacinia.query-analyzer :as query-analyzer])
(:import (clojure.lang PersistentQueue)
(java.util.concurrent Executor)))

Expand Down Expand Up @@ -379,15 +380,14 @@
(let [parsed-query (get context constants/parsed-query-key)
{:keys [selections operation-type ::tracing/timing-start]} parsed-query
schema (get parsed-query constants/schema-key)
^Executor executor (::schema/executor schema)
complexity-warning (:complexity-warning context)]
^Executor executor (::schema/executor schema)]
(binding [resolve/*callback-executor* executor]
(let [enabled-selections (remove :disabled? selections)
*errors (atom [])
*warnings (if complexity-warning
(atom [complexity-warning])
(atom []))
*extensions (atom {})
*warnings (atom [])
*extensions (if (::query-analyzer/enable? context)
Copy link
Author

Choose a reason for hiding this comment

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

기존에는 execute 되기 전에 최대 복잡도를 초과하면 에러를 발생시킬 목적으로 validation 과 execute 사이에서 함수를 호출하였는데요.
execute 될 때 metadata인 extensions에만 넣어줄 거라면 execute 되는 지점에서 옵션이 true 라면 계산되어도 될것 같아서
executor.clj 내부로 함수호출을 변경하였습니다.

(atom {:analysis (query-analyzer/complexity-analysis parsed-query)})
(atom {}))
*resolver-tracing (when (::tracing/enabled? context)
(atom []))
context' (assoc context constants/schema-key schema)
Expand Down
15 changes: 9 additions & 6 deletions ...lmartlabs/lacinia/complexity_analysis.clj → ...om/walmartlabs/lacinia/query_analyzer.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
(ns com.walmartlabs.lacinia.complexity-analysis
(ns com.walmartlabs.lacinia.query-analyzer
(:require [com.walmartlabs.lacinia.selection :as selection]))

(defn ^:private list-args? [arguments]
Expand All @@ -10,7 +10,7 @@
[{:keys [arguments selections field-name leaf? fragment-name] :as selection} fragment-map]
(let [selection-kind (selection/selection-kind selection)]
(cond
;; If it's a leaf node or `pageInfo`, return nil.
;; If it's a leaf or `pageInfo`, return nil.
(or leaf? (= :pageInfo field-name))
nil

Expand Down Expand Up @@ -39,9 +39,12 @@
(+ n-nodes children-complexity))))

(defn complexity-analysis
[query {:keys [max-complexity] :as _options}]
[query]
(let [{:keys [fragments selections]} query
summarized-selections (mapcat #(summarize-selection % fragments) selections)
complexity (calculate-complexity (first summarized-selections))]
(when (> complexity max-complexity)
{:message (format "Over max complexity! Current number of resources to be queried: %s" complexity)})))
complexity (apply + (map calculate-complexity summarized-selections))]
{:complexity complexity}))

(defn enable-query-analyzer
[context]
(assoc context ::enable? true))
162 changes: 91 additions & 71 deletions test/com/walmartlabs/lacinia/complexity_analysis_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,107 +43,127 @@
[_ _ _]
nil)

(defn ^:private resolve-name
[_ _ _]
"name")

(defn ^:private resolve-rooot
[_ _ _]
nil)

(def ^:private schema
(utils/compile-schema "complexity-analysis-error.edn"
{:resolve-products resolve-products
:resolve-followings resolve-followings
:resolve-reviews resolve-reviews
:resolve-likers resolve-likers
:resolve-node resolve-node}))
:resolve-node resolve-node
:resolve-name resolve-name
:resolve-root resolve-rooot}))

(defn ^:private q [query variables]
(utils/simplify (execute schema query variables nil {:max-complexity 10})))
(utils/simplify (execute schema query variables nil {:analyze-query true})))

(deftest over-complexity-analysis
(deftest test-complexity-analysis
(testing "It is possible to calculate the complexity of a query in the Relay connection spec
by taking into account both named fragments and inline fragments."
(is (= {:data {:node nil}
:extensions {:warnings [{:message "Over max complexity! Current number of resources to be queried: 27"}]}}
:extensions {:analysis {:complexity 32}}}
(q "query ProductDetail($productId: ID){
node(id: $productId) {
... on Product {
...ProductLikersFragment
seller{
id
products(first: 5){
node(id: $productId) {
... on Product {
...ProductLikersFragment
seller{
id
products(first: 5){
edges{
node{
id
}
}
}
}
reviews(first: 5){
edges{
node{
id
author{
id
name
}
product{
id
}
}
}
}
}
reviews(first: 5){
edges{
node{
}
}
fragment ProductLikersFragment on Product {
likers(first: 10){
edges{
node{
... on Seller{
id
}
... on Buyer{
id
author{
id
}
}
}
}
}
}
}
fragment ProductLikersFragment on Product {
likers(first: 10){
edges{
node{
... on Seller{
}" {:productId "id"}))))
(testing "If no arguments are passed in the query, the calculation uses the default value defined in the schema."
(is (= {:data {:node nil}
:extensions {:analysis {:complexity 22}}}
(q "query ProductDetail($productId: ID){
node(id: $productId) {
... on Product {
...ProductLikersFragment
seller{
id
products(first: 5){
edges{
node{
id
}
}
}
}
... on Buyer{
id
reviews(first: 5){
edges{
node{
id
author{
id
}
}
}
}
}
}
}
}" {:productId "id"}))))
(testing "If no arguments are passed in the query, the calculation uses the default value defined in the schema."
(is (= {:data {:node nil}
:extensions {:warnings [{:message "Over max complexity! Current number of resources to be queried: 22"}]}}
(q "query ProductDetail($productId: ID){
node(id: $productId) {
... on Product {
...ProductLikersFragment
seller{
id
products(first: 5){
edges{
node{
id
}
}
}
}
reviews(first: 5){
edges{
node{
id
author{
id
}
}
}
}
}
}
}
fragment ProductLikersFragment on Product {
likers{
edges{
node{
... on Seller{
id
}
... on Buyer{
id
}
}
}
}
}" {:productId "id"})))))
fragment ProductLikersFragment on Product {
likers{
edges{
node{
... on Seller{
id
}
... on Buyer{
id
}
}
}
}
}" {:productId "id"}))))
(testing "If return type of root query is scala, then complexity is 0"
(is (= {:data {:root nil}
:extensions {:analysis {:complexity 0}}}
(q "query root{
root
}" nil)))))
Comment on lines +161 to +166
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼


(comment
(run-test over-complexity-analysis))
(run-test test-complexity-analysis))
Loading