Skip to content

Commit

Permalink
Merge pull request #343 from walmartlabs/20201016-error-null-prop
Browse files Browse the repository at this point in the history 
Propogate null values in non-nullable fields up, as per the specification
  • Loading branch information
@hlship
hlship committed Oct 23, 2020
2 parents 2689e9d + 3b8e4b5 commit 9788454
Show file tree
Hide file tree
Showing 14 changed files with 265 additions and 192 deletions.
7 changes: 6 additions & 1 deletion CHANGES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,12 @@ per [the GraphQL specification](http://spec.graphql.org/June2018/#sec-Root-Opera
Added function `com.walmartlabs.lacinia.executor/selection` which provides access to
the details about the selection, including directives and nested selections.

Fixed problems with the literal values `true`, `false`, and `null` when parsing Schema Definition Language files.
Fixed an issue where a Schema Definition Language that contained
the literal values `true`, `false`, or `null` would fail to parse.

Lacinia now correctly conforms to the GraphQL specification related to
[Errors and Non-Nullability](https://spec.graphql.org/June2018/#sec-Errors-and-Non-Nullability).

## 0.37.0 -- 30 Jun 2020

Added new function `com.walmartlabs.lacinia.util/inject-streamers`, used to
Expand Down
14 changes: 14 additions & 0 deletions dev-resources/nullability.edn
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{:objects
{:Employer
{:fields
{:id {:type (non-null ID)}
:name {:type (non-null String)}}}
:User
{:fields
{:id {:type (non-null ID)}
:employer {:type :Employer}}}
:Query
{:fields
{:user {:type (non-null :User)
:args {:id {:type (non-null ID)}}
:resolve :query/user}}}}}
8 changes: 8 additions & 0 deletions perf/benchmarks.csv
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,3 +201,11 @@ date,commit,kind,parse,exec
20200928,e59b13b1,basic,0.16147504635584137,0.30589205388127855
20200928,e59b13b1,basic-vars,0.20434602015355086,0.30665841021825396
20200928,e59b13b1,errors,0.08698684070294785,6.678822322916667
20201022,01acf0f2,introspection,0.7854722372685186,3.849969166666667
20201022,01acf0f2,basic,0.17675072231686542,0.29987493457497616
20201022,01acf0f2,basic-vars,0.21031775741890962,0.36457497083333334
20201022,01acf0f2,errors,0.0972129609151573,6.786947344444445
20201022,ee438d25,introspection,0.6967846942528736,3.624546781609196
20201022,ee438d25,basic,0.1707877615039282,0.2857015922865014
20201022,ee438d25,basic-vars,0.20820654642618572,0.29815964183381094
20201022,ee438d25,errors,0.08834569788878976,6.6670127500000005
1 change: 0 additions & 1 deletion project.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,4 @@
:jvm-opts ["-Xmx1g" "-XX:-OmitStackTraceInFastThrow"]
:test2junit-output-dir ~(or (System/getenv "CIRCLE_TEST_REPORTS") "target/test2junit")
:codox {:source-uri "https://github.com/walmartlabs/lacinia/blob/master/{filepath}#L{line}"
:source-paths ["src"] ; and not vendor-src
:metadata {:doc/format :markdown}})
113 changes: 30 additions & 83 deletions src/com/walmartlabs/lacinia/executor.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
[com.walmartlabs.lacinia.selector-context :as sc]
[com.walmartlabs.lacinia.tracing :as tracing]
[com.walmartlabs.lacinia.constants :as constants]
[com.walmartlabs.lacinia.protocols :as p])
[com.walmartlabs.lacinia.selection :as selection])
(:import
(clojure.lang PersistentQueue)))

Expand Down Expand Up @@ -95,7 +95,7 @@
(:concrete-type? field-selection)
(-> field-selection :field-definition :resolve)

:let [field-name (p/field-name field-selection)]
:let [field-name (selection/field-name field-selection)]

(nil? resolved-type)
(throw (ex-info "Sanity check: value type tag is nil on abstract type."
Expand Down Expand Up @@ -125,10 +125,9 @@
[execution-context field-selection]
(try
(let [*resolver-tracing (:*resolver-tracing execution-context)
arguments (p/arguments field-selection)
arguments (selection/arguments field-selection)
container-value (:resolved-value execution-context)
{:keys [context]} execution-context
schema (get context constants/schema-key)
{:keys [context schema]} execution-context
resolved-type (:resolved-type execution-context)
resolve-context (assoc context
:com.walmartlabs.lacinia/container-type-name resolved-type
Expand Down Expand Up @@ -159,7 +158,7 @@
(catch Throwable t
(let [field-name (get-in field-selection [:field-definition :qualified-name])
{:keys [location]} field-selection
arguments (p/arguments field-selection)]
arguments (selection/arguments field-selection)]
(throw (ex-info (str "Exception in resolver for "
(q field-name)
": "
Expand All @@ -182,70 +181,19 @@
;; accumulates timing data during execution.
;; *extensions is an atom containing a map, if non-empty, it is added to the result map as :extensions
;; path is used when reporting errors
[context resolved-value resolved-type *errors *warnings *extensions *resolver-tracing path])

(defn ^:private null-to-nil
[v]
(cond
(vector? v)
(map null-to-nil v)

(= ::null v)
nil

:else
v))

(defn ^:private propogate-nulls
"When all values for a selected value are ::null, it is replaced with
::null (if non-nullable) or nil (if nullable).
Otherwise, the selected values are a mix of real values and ::null, so replace
the ::null values with nil."
[non-nullable? selected-value]
(cond
;; This sometimes happens when a field returns multiple scalars:
(not (map? selected-value))
selected-value

(and (seq selected-value)
(every? (fn [[_ v]] (= v ::null))
selected-value))
(if non-nullable? ::null nil)

:else
(map-vals null-to-nil selected-value)))
;; schema is the compiled schema (obtained from the parsed query)
[context resolved-value resolved-type *errors *warnings *extensions *resolver-tracing path schema])

(defrecord ^:private ResultTuple [alias value])

(defn ^:private apply-field-selection
[execution-context field-selection]
(let [{:keys [alias]} field-selection
non-nullable-field? (-> field-selection :field-definition :type :kind (= :non-null))
null-collapser (get-in field-selection [:field-definition :null-collapser])
resolver-result (resolve-and-select execution-context field-selection)]
(transform-result resolver-result
(fn [resolved-field-value]
(let [sub-selection (cond
(and non-nullable-field?
(nil? resolved-field-value))
::null

;; child field was non-nullable and resolved to null,
;; but parent is nullable so let's null parent
(and (= resolved-field-value ::null)
(not non-nullable-field?))
nil

(map? resolved-field-value)
(propogate-nulls non-nullable-field? resolved-field-value)

;; TODO: We also support sets
(vector? resolved-field-value)
(mapv #(propogate-nulls non-nullable-field? %) resolved-field-value)

:else
resolved-field-value)]
(->ResultTuple alias sub-selection))))))
(->ResultTuple alias (null-collapser resolved-field-value))))))

(defn ^:private maybe-apply-fragment
[execution-context fragment-selection concrete-types]
Expand Down Expand Up @@ -337,12 +285,12 @@
Accumulates errors in the execution context as a side-effect."
[execution-context selection]
(let [is-fragment? (not= :field (p/selection-kind selection))
(let [is-fragment? (not= :field (selection/selection-kind selection))
;; When starting to execute a field, add the current alias (or field name) to the path.
execution-context' (if is-fragment?
execution-context
(update execution-context :path conj (p/alias-name selection)))
sub-selections (p/selections selection)
(update execution-context :path conj (selection/alias-name selection)))
sub-selections (selection/selections selection)

apply-errors (fn [selection-context sc-key ec-atom-key]
(when-let [errors (get selection-context sc-key)]
Expand Down Expand Up @@ -383,10 +331,9 @@
selector (if is-fragment?
schema/floor-selector
(or (-> selection :field-definition :selector)
(let [field-name (p/field-name selection)]
(let [field-name (selection/field-name selection)]
(-> execution-context'
:context
(get constants/schema-key)
:schema
(get resolved-type)
:fields
(get field-name)
Expand Down Expand Up @@ -416,8 +363,8 @@
(if @*pass-through-exceptions
(throw t)
(let [{:keys [location]} selection
arguments (p/arguments selection)
qualified-name (p/qualified-name selection)]
arguments (selection/arguments selection)
qualified-name (selection/qualified-name selection)]
(throw (ex-info (str "Exception processing resolved value for "
(q qualified-name)
": "
Expand Down Expand Up @@ -486,6 +433,7 @@
;; For subscriptions, the :resolved-value will be set to a non-nil value before
;; executing the query.
execution-context (map->ExecutionContext {:context context'
:schema (get parsed-query constants/schema-key)
:*errors *errors
:*warnings *warnings
:*resolver-tracing *resolver-tracing
Expand All @@ -503,20 +451,19 @@
(execute-nested-selections execution-context enabled-selections))]
(resolve/on-deliver! operation-result
(fn [selected-data]
(let [data (propogate-nulls false selected-data)]
(let [errors (seq @*errors)
warnings (seq @*warnings)
extensions @*extensions]
(resolve/deliver! result-promise
(cond-> {:data data}
(seq extensions) (assoc :extensions extensions)
*resolver-tracing
(tracing/inject-tracing timing-start
(::tracing/parsing parsed-query)
(::tracing/validation context)
@*resolver-tracing)
errors (assoc :errors (distinct errors))
warnings (assoc-in [:extensions :warnings] (distinct warnings)))))))))
(let [errors (seq @*errors)
warnings (seq @*warnings)
extensions @*extensions]
(resolve/deliver! result-promise
(cond-> {:data (schema/collapse-nulls-in-map selected-data)}
(seq extensions) (assoc :extensions extensions)
*resolver-tracing
(tracing/inject-tracing timing-start
(::tracing/parsing parsed-query)
(::tracing/validation context)
@*resolver-tracing)
errors (assoc :errors (distinct errors))
warnings (assoc-in [:extensions :warnings] (distinct warnings))))))))
(catch Throwable t
(resolve/deliver! result-promise t))))]

Expand Down
26 changes: 14 additions & 12 deletions src/com/walmartlabs/lacinia/parser.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
[com.walmartlabs.lacinia.resolve :as resolve]
[com.walmartlabs.lacinia.parser.query :as qp]
[com.walmartlabs.lacinia.tracing :as tracing]
[com.walmartlabs.lacinia.protocols :as p]
[com.walmartlabs.lacinia.selection :as selection]
[flatland.ordered.map :refer [ordered-map]])
(:import
(clojure.lang ExceptionInfo)))
Expand Down Expand Up @@ -727,11 +727,11 @@

(defrecord ^:private Directive [directive-name effector arguments arguments-extractor]

p/Directive
selection/Directive

(directive-type [_] directive-name)

p/Arguments
selection/Arguments

(arguments [_] arguments))

Expand Down Expand Up @@ -776,6 +776,8 @@

:field-name :__typename

:null-collapser identity

:resolve (fn [context _ _]
(-> context
:com.walmartlabs.lacinia/container-type-name
Expand All @@ -787,40 +789,40 @@
alias field-name selections directives arguments
location locations root-value-type]

p/QualifiedName
selection/QualifiedName

(qualified-name [_] (:qualified-name field-definition))

p/SelectionSet
selection/SelectionSet

(selection-kind [_] :field)

(selections [_] selections)

p/FieldSelection
selection/FieldSelection

(field-name [_] field-name)

(alias-name [_] alias)

(root-value-type [_] root-value-type)

p/Arguments
selection/Arguments
(arguments [_] arguments)

p/Directives
selection/Directives

(directives [_]
(nil-map (group-by :directive-name directives))))

(defrecord ^:private InlineFragment [selection-type selections directives
location locations concrete-types]

p/Directives
selection/Directives

(directives [_] directives)

p/SelectionSet
selection/SelectionSet

(selection-kind [_] :inline-fragment)

Expand All @@ -829,13 +831,13 @@
(defrecord ^:private NamedFragment [selection-type directives selections
location locations concrete-types]

p/SelectionSet
selection/SelectionSet

(selection-kind [_] :named-fragment)

(selections [_] selections)

p/Directives
selection/Directives

(directives [_] directives))

Expand Down

0 comments on commit 9788454

Please sign in to comment.