Merge pull request #336 from walmartlabs/20200225-default-schema-names

Follow the specification for default names of operation types

CHANGES.md

@@ -4,6 +4,10 @@ Optional request tracing is now designed to be compatible with Apollo GraphQL's
 
 New support for [Apollo GraphQL Federation](https://www.apollographql.com/docs/apollo-server/federation/introduction/).
 
+The default objects names for storing operations are now `Query`, `Mutation`,
+and `Subscription`, and these must be objects (not unions), as
+per [the GraphQL specification](http://spec.graphql.org/June2018/#sec-Root-Operation-Types).
+
 Added function `com.walmartlabs.lacinia.executor/selection` which provides access to 
 the details about the selection, including directives and nested selections.
 

dev-resources/introspection.edn

@@ -1,6 +1,6 @@
 {:data
  {:__schema
-  {:queryType {:name "QueryRoot"},
+  {:queryType {:name "Query"},
    :mutationType nil,
    :types
    [{:kind :SCALAR,
@@ -36,8 +36,8 @@
      :enumValues nil,
      :possibleTypes nil}
     {:kind :OBJECT,
-     :name "QueryRoot",
-     :description "Root of all queries.",
+     :name "Query",
+     :description nil,
      :fields
      [{:name "droid",
        :description nil,

dev-resources/sample_schema.sdl

@@ -28,7 +28,7 @@ input Character {
   episodes: [episode]
 }
 
-type Query {
+type MyQuery {
   in_episode(episode: episode = NEWHOPE) : [CharacterOutput] @Trace
 }
 
@@ -46,10 +46,12 @@ type Subscription {
   new_character(episodes: [episode!]!) : CharacterOutput
 }
 
-union Queries = Query | OtherQuery
+# This doesn't make much sense but is a remnant from when unions were allowed for schema operation types
+# (Jun 2018 spec makes it clear they must be objects, not unions). */
+union Queries = MyQuery | OtherQuery
 
 schema {
-  query: Queries
+  query: MyQuery
   mutation: Mutation
   subscription: Subscription
 }

dev-resources/simple-federation.sdl

@@ -7,8 +7,6 @@ type Query {
     user_by_id(id: Int!) : User
 }
 
-schema { query: Query }
-
 type Account @key(fields: "acct_number") {
   acct_number: String!
   name: String!

docs/_examples/fed/external.gql

@@ -12,5 +12,3 @@ type Product @key(fields: "upc") {
 type Query {
     productByUpc(upc: String!) : Product
 }
-
-schema { query: Query }

docs/_examples/fed/internal.gql

@@ -6,5 +6,3 @@ type User @key(fields: "id") {
 type Query {
   userById(id:String!) : User
 }
-
-schema { query: Query }

docs/federation/implementation.rst

@@ -15,7 +15,7 @@ is a list of the ``_Entities`` union, therefore each value :doc:`must be tagged
 
 .. sidebar:: Return type
 
-    Return type here is just like a normal field resolver that returns GraphQL list; it may be seq of values, or a
+    Return type here is just like a normal field resolver that returns a GraphQL list; it may be seq of values, or a
     :doc:`ResolverResult </resolve/resolve-as>` that delivers such a seq.
 
 ``resolve-products-internal`` does the same for ``Product`` representations, but since this is the

docs/federation/index.rst

@@ -40,9 +40,10 @@ For example:
 which fields are needed to bridge relationships between the different services.
 
 Instead, federation allows the Apollo GraphQL gateway to merge together the three individual services into one composite service.
-The client is only needs access to the single gateway enpoint, and is free to make complex queries that
+The client only needs access to the single gateway endpoint, and is free to make complex queries that
 span from ``User`` to ``Review`` to ``Product`` seamlessly;
-the gateway service is responsible for communicating to the implementing services.
+the gateway service is responsible for communicating with the implementing services and merging together the final
+response.
 
 A GraphQL type (or interface) that can span services this way is called an `entity`.
 
@@ -52,7 +53,8 @@ backwards compatible, just as with a traditional GraphQL schema.
 
 In the other schemas, the ``User`` type is `external`; just a kind of stub for ``User`` is defined in the schemas for
 the products and reviews service. The full type, and the stub, must agree on fields that define the primary key
-for the entity. 
+for the entity. However, the stub can be extended by the other services to add new fields, surfacing data and relationships
+owned by the particular service.
 
 .. toctree::
     :hidden:

docs/federation/internal.rst

@@ -17,13 +17,3 @@ When federation is enabled, a
 including ``@key``, which defines the primary key, or primary keys, for the entity.
 
 The above example would be the schema for a users service that can be extended by other services.
-
-.. warning::
-
-   Lacinia's default name for the object containing queries is ``QueryRoot``, whereas the default for Apollo and 
-   `most other GraphQL libraries <https://graphql.org/learn/schema/#the-query-and-mutation-types>`_ is ``Query``.  
-
-   Because of this, and because `Apollo doesn't do the right thing here <https://github.com/apollographql/apollo-server/issues/4554>`_,
-   it is necessary to override Lacinia's default by adding ``schema { query : Query }``.
-
-   The same holds true for mutations, which must be on a type named ``Mutation``.

docs/federation/reps.rst

@@ -17,13 +17,13 @@ to pass to the products service, using information from the ``@key`` directive:
     {"__typename": "User",
      "id": "124c41"}
 
-This representation is JSON, and is be passed to an implementing service's ``_entities`` query, which is automaticaly added
+This representation is JSON, and is passed to an implementing service's ``_entities`` query, which is automaticaly added
 to the implementing service's schema by Lacinia:
 
 .. literalinclude:: /_examples/fed/schema.gql
 
 The ``_Entity`` union will contain all entities, internal or external, in the local schema; for the products service, this
-will be ``User`` and ``Product``.
+will be ``User`` (external) and ``Product`` (internal).
 
 The ``_entities`` query exists to convert some number of representations (here, as scalar type ``_Any``) into entities
 (either stub entities or full entities).

perf/benchmarks.csv

@@ -197,3 +197,7 @@ date,commit,kind,parse,exec
 20200928,0cfdbcfb,basic,0.16301182026666666,0.1713143711001642
 20200928,0cfdbcfb,basic-vars,0.199482251984127,0.1727440829159711
 20200928,0cfdbcfb,errors,0.08740605913978494,1.9187714722222227
+20200928,e59b13b1,introspection,0.6457643805031447,3.56976855952381
+20200928,e59b13b1,basic,0.16147504635584137,0.30589205388127855
+20200928,e59b13b1,basic-vars,0.20434602015355086,0.30665841021825396
+20200928,e59b13b1,errors,0.08698684070294785,6.678822322916667

src/com/walmartlabs/lacinia/executor.clj

@@ -433,9 +433,14 @@
                                  (let [final-result (resolve-promise)]
                                    (resolve/on-deliver! field-resolver-result
                                                         (fn receive-resolved-value-from-field [resolved-value]
-                                                          (resolve/on-deliver! (process-resolved-value resolved-value)
-                                                                               (fn deliver-selection-for-field [resolved-value]
-                                                                                 (resolve/deliver! final-result resolved-value)))))
+                                                          ;; This is for a specific case, where a parent resolver returns a map whose value
+                                                          ;; is also a ResolverResult; in that case, unwrap one layer further before calling
+                                                          ;; process-resolved-value.
+                                                          (if (resolve/is-resolver-result? resolved-value)
+                                                            (resolve/on-deliver! resolved-value receive-resolved-value-from-field)
+                                                            (resolve/on-deliver! (process-resolved-value resolved-value)
+                                                                                 (fn deliver-selection-for-field [resolved-value]
+                                                                                   (resolve/deliver! final-result resolved-value))))))
                                    final-result))]
 
     ;; For fragments, we start with a single value and it passes right through to

src/com/walmartlabs/lacinia/federation.clj

@@ -124,7 +124,7 @@
   [schema sdl entity-resolvers]
   (let [entity-names (find-entity-names schema)
         entities-resolver (entities-resolver-factory entity-names entity-resolvers)
-        query-root (get-in schema [:roots :query] :QueryRoot)]
+        query-root (get-in schema [:roots :query] :Query)]
     (prevent-collision schema [:unions :_Entity])
     (prevent-collision schema [:objects query-root :fields :_service])
     (prevent-collision schema [:objects query-root :fields :_entities])

src/com/walmartlabs/lacinia/schema.clj

@@ -1477,67 +1477,32 @@
 
 (defn ^:private add-root
   "Adds a root object for 'extra' operations (e.g., the :queries map in the input schema)."
-  [compiled-schema object-name object-description fields]
-  (assoc compiled-schema object-name
-         (map->Type {:category :object
-                     :type-name object-name
-                     :description object-description
-                     :fields fields})))
-
-(defn ^:private merge-root
-  "Used after the compile-type stage, to merge together the root objects, one possibly provided
-  via the input schema :roots map, and the other built from the :queries, :mutations, or :subscriptions
-  maps (the 'extra object').
-
-  The object-name is the name provided in the :roots map; it may not exist (normally, this is because
-  it gets a default name such as `QueryRoot`), in which case it is created."
-  [compiled-schema root-name extra-object-name object-name]
-  (let [root-object-def (get compiled-schema object-name)
-        extra-object-def (get compiled-schema extra-object-name)
-        merge-into (fn [fields more-fields]
-                     (reduce-kv (fn [m k v]
-                                  (when (contains? m k)
-                                    (throw (ex-info (format "Name collision compiling schema. %s %s conflicts with %s."
-                                                            (-> root-name name str/capitalize)
-                                                            (-> v :qualified-name q)
-                                                            (q (get-in m [k :qualified-name])))
-                                                    {:field-name k
-                                                     :operation root-name})))
-                                  (assoc m k v))
-                                fields
-                                more-fields))]
-    (cond-let
-
-      (nil? root-object-def)
-      ;; Copy the extra object over as the missing root object.
-      (let [extra-object-def (get compiled-schema extra-object-name)]
-        (assoc compiled-schema object-name
-               (assoc extra-object-def :type-name object-name)))
-
-      :let [category (:category root-object-def)]
-
-      (= :object category)
-      ;; Merge in the extra fields into the actual object
-      (update-in compiled-schema [object-name :fields] merge-into (:fields extra-object-def))
-
-      (= :union category)
-      ;; Lacinia's execution and introspection code is built on the idea of a single root object type for
-      ;; each type of operation. To keep that working, we copy the fields of each member object in
-      ;; the union into the extra object and then designate the extra object the root.
-      (let [reduce* (fn [val f coll] (reduce f val coll))]
-        (-> compiled-schema
-            (update-in [extra-object-name :fields] reduce* (fn [fields member-type-name]
-                                                             (merge-into fields (get-in compiled-schema [member-type-name :fields])))
-                       (:members root-object-def))
-            (assoc-in [::roots root-name] extra-object-name)))
+  [compiled-schema object-name operation-key fields]
+  (cond-let
+    :let [existing (get compiled-schema object-name)]
 
-      :else
-      (throw (ex-info (format "Type %s (a %s operation root) must be a union or object, not %s."
-                              (q object-name)
-                              (name root-name)
-                              (name category))
-                      {:type root-name
-                       :category category})))))
+    (nil? existing)
+    (assoc compiled-schema object-name
+           (map->Type {:category :object
+                       :type-name object-name
+                       :fields fields}))
+
+    (empty? fields)
+    compiled-schema
+
+    ;; Ok, so here's the less fun case - the object is already in the schema and there are fields defiend on that object
+    ;; there but there are also fields separately ... say, due to Introspection adding queries.
+    :else
+    (let [merged-fields (reduce-kv (fn [m k v]
+                                     (when (contains? m k)
+                                       (throw (ex-info (format "Name collision compiling schema: %s already exists with value from %s."
+                                                               (q (qualified-name object-name k))
+                                                               operation-key)
+                                                       {:field-name k})))
+                                     (assoc m k v))
+                                   (:fields existing)
+                                   fields)]
+      (assoc-in compiled-schema [object-name :fields] merged-fields))))
 
 (defn ^:private compile-directive-defs
   [schema directive-defs]
@@ -1616,9 +1581,9 @@
                             (merge default-scalar-transformers)
                             (map-vals #(assoc % :category :scalar)))
         {:keys [query mutation subscription]
-         :or {query :QueryRoot
-              mutation :MutationRoot
-              subscription :SubscriptionRoot}} (map-vals as-keyword (:roots schema))
+         :or {query :Query
+              mutation :Mutation
+              subscription :Subscription}} (map-vals as-keyword (:roots schema))
         defaulted-subscriptions (->> schema
                                      :subscriptions
                                      (map-vals #(if-not (:resolve %)
@@ -1634,14 +1599,14 @@
         (xfer-types (:objects schema) :object)
         (xfer-types (:interfaces schema) :interface)
         (xfer-types (:input-objects schema) :input-object)
-        (add-root :__Queries "Root of all queries." (:queries schema))
-        (add-root :__Mutations "Root of all mutations." (:mutations schema))
-        (add-root :__Subscriptions "Root of all subscriptions." defaulted-subscriptions)
+        (add-root query :queries (:queries schema))
+        (add-root mutation :mutations (:mutations schema))
+        (add-root subscription :subscriptions defaulted-subscriptions)
         (as-> s
               (map-vals #(compile-type % s) s))
-        (merge-root :query :__Queries query)
-        (merge-root :mutation :__Mutations mutation)
-        (merge-root :subscription :__Subscriptions subscription)
+        ;(merge-root :query :__Queries query)
+        ;(merge-root :mutation :__Mutations mutation)
+        ;(merge-root :subscription :__Subscriptions subscription)
         (compile-directive-defs (:directive-defs schema))
         (prepare-and-validate-interfaces)
         (prepare-and-validate-objects :object options)

test/com/walmartlabs/input_objects_test.clj

@@ -36,8 +36,8 @@
     (is (= {:data {:create_game "nil"}}
            (execute schema "mutation { create_game }")))
 
-    (is (= {:errors [{:extensions {:argument :__Mutations/create_game.game_data
-                                   :field-name :__Mutations/create_game
+    (is (= {:errors [{:extensions {:argument :Mutation/create_game.game_data
+                                   :field-name :Mutation/create_game
                                    :missing-key :id
                                    :required-keys [:id]
                                    :schema-type :game_template}
@@ -48,8 +48,8 @@
 
     ;; TODO: Missing some needed context from above
 
-    (is (= {:errors [{:extensions {:argument :__Mutations/create_game.game_data
-                                   :field-name :__Mutations/create_game
+    (is (= {:errors [{:extensions {:argument :Mutation/create_game.game_data
+                                   :field-name :Mutation/create_game
                                    :missing-key :id
                                    :required-keys [:id]
                                    :schema-type :game_template}
@@ -88,16 +88,16 @@
            (execute schema
                     "{ search(filter: {terms: \"lego\", max_count: 5}) }")))
 
-    (is (= {:errors [{:extensions {:argument :__Queries/search.filter
-                                   :field-name :__Queries/search
+    (is (= {:errors [{:extensions {:argument :Query/search.filter
+                                   :field-name :Query/search
                                    :schema-type :Filter}
                       :locations [{:column 3
                                    :line 1}]
                       :message "Exception applying arguments to field `search': For argument `filter', input object contained unexpected key `term'."}]}
            (execute schema
                     "{ search(filter: {term: \"lego\", max_count: 5}) }")))
 
-    (is (= {:errors [{:extensions {:argument :__Queries/search.filter
+    (is (= {:errors [{:extensions {:argument :Query/search.filter
                                    :field-name :term
                                    :input-object-fields [:max_count
                                                          :terms]
@@ -118,6 +118,9 @@
     "Field `Insect/legs' references unknown type `Six'."
     {:field-name :Insect/legs
      :schema-types {:input-object [:Insect]
+                    :object [:Mutation
+                             :Query
+                             :Subscription]
                     :scalar [:Boolean
                              :Float
                              :ID
@@ -133,7 +136,7 @@
     "Field `Web/spider' is type `Creepy', input objects may only be used as field arguments."
     {:field-name :Web/spider
      :schema-types {:scalar [:Boolean :Float :ID :Int :String],
-                    :object [:MutationRoot :QueryRoot :SubscriptionRoot :Web],
+                    :object [:Mutation :Query :Subscription :Web],
                     :input-object [:Creepy]}}
     (schema/compile
       {:input-objects
@@ -149,9 +152,9 @@
   (expect-exception
     "Field `Turtle/friend' is type `Hare', input objects may only contain fields that are scalar, enum, or input object."
     {:field-name :Turtle/friend
-     :schema-types  {:scalar [:Boolean :Float :ID :Int :String],
-                     :object [:Hare :MutationRoot :QueryRoot :SubscriptionRoot],
-                     :input-object [:Turtle]}}
+     :schema-types {:scalar [:Boolean :Float :ID :Int :String],
+                    :object [:Hare :Mutation :Query :Subscription],
+                    :input-object [:Turtle]}}
     (schema/compile
       {:input-objects
        {:Turtle {:fields {:friend {:type :Hare}}}}