Merge pull request #5694 from nicuveo/fix-schema-duplication

https://github.com/hasura/graphql-engine/pull/5694

server/src-lib/Hasura/GraphQL/Parser.hs

@@ -21,6 +21,7 @@ module Hasura.GraphQL.Parser
   , list
   , object
   , selectionSet
+  , safeSelectionSet
   , selectionSetObject
 
   , InputFieldsParser

server/src-lib/Hasura/GraphQL/Parser/Internal/Parser.hs

@@ -13,6 +13,7 @@ import qualified Data.HashMap.Strict.Extended  as M
 import qualified Data.HashMap.Strict.InsOrd    as OMap
 import qualified Data.HashSet                  as S
 import qualified Data.Text                     as T
+import qualified Data.List.Extended            as LE
 
 import           Control.Lens.Extended         hiding (enum, index)
 import           Data.Int                      (Int32, Int64)
@@ -679,6 +680,18 @@ selectionSet
   -> Parser 'Output m (OMap.InsOrdHashMap Name (ParsedSelection a))
 selectionSet name desc fields = selectionSetObject name desc fields []
 
+safeSelectionSet
+  :: (MonadError QErr n, MonadParse m)
+  => Name
+  -> Maybe Description
+  -> [FieldParser m a]
+  -> n (Parser 'Output m (OMap.InsOrdHashMap Name (ParsedSelection a)))
+safeSelectionSet name desc fields
+  | S.null duplicates = pure $ selectionSetObject name desc fields []
+  | otherwise         = throw500 $ "found duplicate fields in selection set: " <> T.intercalate ", " (unName <$> toList duplicates)
+  where
+    duplicates = LE.duplicates $ getName . fDefinition <$> fields
+
 -- Should this rather take a non-empty `FieldParser` list?
 -- See also Note [Selectability of tables].
 selectionSetObject

server/src-lib/Hasura/GraphQL/Schema.hs

@@ -152,11 +152,10 @@ buildGQLContext =
               ) |) [] (Map.toList allRemoteSchemas)
 
     let unauthenticatedContext :: m GQLContext
-        unauthenticatedContext = do
-          let gqlContext = GQLContext . finalizeParser <$>
-                unauthenticatedQueryWithIntrospection queryRemotes mutationRemotes
-          halfContext <- P.runSchemaT gqlContext
-          return $ halfContext $ finalizeParser <$> unauthenticatedMutation mutationRemotes
+        unauthenticatedContext = P.runSchemaT do
+          ucQueries   <- finalizeParser <$> unauthenticatedQueryWithIntrospection queryRemotes mutationRemotes
+          ucMutations <- fmap finalizeParser <$> unauthenticatedMutation mutationRemotes
+          pure $ GQLContext ucQueries ucMutations
 
         -- | The 'query' type of the remotes. TODO: also expose mutation
         -- remotes. NOT TODO: subscriptions, as we do not yet aim to support
@@ -309,8 +308,8 @@ query
   -> m (Parser 'Output n (OMap.InsOrdHashMap G.Name (QueryRootField UnpreparedValue)))
 query name allTables allFunctions allRemotes allActions nonObjectCustomTypes = do
   queryFieldsParser <- query' allTables allFunctions allRemotes allActions nonObjectCustomTypes
-  pure $ P.selectionSet name Nothing queryFieldsParser
-    <&> fmap (P.handleTypename (RFRaw . J.String . G.unName))
+  P.safeSelectionSet name Nothing queryFieldsParser
+    <&> fmap (fmap (P.handleTypename (RFRaw . J.String . G.unName)))
 
 subscription
   :: forall m n r
@@ -323,20 +322,20 @@ subscription allTables allFunctions asyncActions =
   query $$(G.litName "subscription_root") allTables allFunctions [] asyncActions mempty
 
 queryRootFromFields
-  :: forall n
-   . MonadParse n
+  :: forall n m
+   . (MonadError QErr m, MonadParse n)
   => [P.FieldParser n (QueryRootField UnpreparedValue)]
-  -> Parser 'Output n (OMap.InsOrdHashMap G.Name (QueryRootField UnpreparedValue))
+  -> m (Parser 'Output n (OMap.InsOrdHashMap G.Name (QueryRootField UnpreparedValue)))
 queryRootFromFields fps =
-      P.selectionSet $$(G.litName "query_root") Nothing fps
-  <&> fmap (P.handleTypename (RFRaw . J.String . G.unName))
+  P.safeSelectionSet $$(G.litName "query_root") Nothing fps
+    <&> fmap (fmap (P.handleTypename (RFRaw . J.String . G.unName)))
 
 emptyIntrospection
   :: forall m n
    . (MonadSchema n m, MonadError QErr m)
   => m [P.FieldParser n (QueryRootField UnpreparedValue)]
 emptyIntrospection = do
-  let emptyQueryP = queryRootFromFields @n []
+  emptyQueryP <- queryRootFromFields @n []
   introspectionTypes <- collectTypes (P.parserType emptyQueryP)
   let introspectionSchema = Schema
         { sDescription = Nothing
@@ -366,15 +365,14 @@ queryWithIntrospectionHelper
   -> Parser 'Output n (OMap.InsOrdHashMap G.Name (QueryRootField UnpreparedValue))
   -> m (Parser 'Output n (OMap.InsOrdHashMap G.Name (QueryRootField UnpreparedValue)))
 queryWithIntrospectionHelper basicQueryFP mutationP subscriptionP = do
-  let
-    basicQueryP = queryRootFromFields basicQueryFP
+  basicQueryP <- queryRootFromFields basicQueryFP
   emptyIntro <- emptyIntrospection
   allBasicTypes <- collectTypes $
     [ P.parserType basicQueryP
     , P.parserType subscriptionP
     ]
     ++ maybeToList (P.parserType <$> mutationP)
-  allIntrospectionTypes <- collectTypes (P.parserType (queryRootFromFields emptyIntro))
+  allIntrospectionTypes <- collectTypes . P.parserType =<< queryRootFromFields emptyIntro
   let allTypes = Map.unions
         [ allBasicTypes
         , Map.filterWithKey (\name _info -> name /= $$(G.litName "query_root")) allIntrospectionTypes
@@ -389,8 +387,8 @@ queryWithIntrospectionHelper basicQueryFP mutationP subscriptionP = do
         }
   let partialQueryFields =
         basicQueryFP ++ (fmap RFRaw <$> [schema partialSchema, typeIntrospection partialSchema])
-  pure $ P.selectionSet $$(G.litName "query_root") Nothing partialQueryFields
-    <&> fmap (P.handleTypename (RFRaw . J.String . G.unName))
+  P.safeSelectionSet $$(G.litName "query_root") Nothing partialQueryFields
+    <&> fmap (fmap (P.handleTypename (RFRaw . J.String . G.unName)))
 
 -- | Prepare the parser for query-type GraphQL requests, but with introspection
 --   for queries, mutations and subscriptions built in.
@@ -428,20 +426,20 @@ relayWithIntrospection
   -> [FunctionInfo]
   -> m (Parser 'Output n (OMap.InsOrdHashMap G.Name (QueryRootField UnpreparedValue)))
 relayWithIntrospection allTables allFunctions = do
-  nodeFP <- fmap (RFDB . QDBPrimaryKey) <$> nodeField
-  basicQueryFP <- relayQuery' allTables allFunctions
-  mutationP <- mutation allTables [] [] mempty
+  nodeFP        <- fmap (RFDB . QDBPrimaryKey) <$> nodeField
+  basicQueryFP  <- relayQuery' allTables allFunctions
+  mutationP     <- mutation allTables [] [] mempty
+  emptyIntro    <- emptyIntrospection
   let relayQueryFP = nodeFP:basicQueryFP
-      subscriptionP = P.selectionSet $$(G.litName "subscription_root") Nothing relayQueryFP
-                      <&> fmap (P.handleTypename (RFRaw . J.String. G.unName))
-      basicQueryP = queryRootFromFields relayQueryFP
-  emptyIntro <- emptyIntrospection
+  basicQueryP   <- queryRootFromFields relayQueryFP
+  subscriptionP <- P.safeSelectionSet $$(G.litName "subscription_root") Nothing relayQueryFP
+                   <&> fmap (fmap (P.handleTypename (RFRaw . J.String. G.unName)))
   allBasicTypes <- collectTypes $
     [ P.parserType basicQueryP
     , P.parserType subscriptionP
     ]
     ++ maybeToList (P.parserType <$> mutationP)
-  allIntrospectionTypes <- collectTypes (P.parserType (queryRootFromFields emptyIntro))
+  allIntrospectionTypes <- collectTypes . P.parserType =<< queryRootFromFields emptyIntro
   let allTypes = Map.unions
         [ allBasicTypes
         , Map.filterWithKey (\name _info -> name /= $$(G.litName "query_root")) allIntrospectionTypes
@@ -456,8 +454,8 @@ relayWithIntrospection allTables allFunctions = do
         }
   let partialQueryFields =
         nodeFP : basicQueryFP ++ (fmap RFRaw <$> [schema partialSchema, typeIntrospection partialSchema])
-  pure $ P.selectionSet $$(G.litName "query_root") Nothing partialQueryFields
-    <&> fmap (P.handleTypename (RFRaw . J.String . G.unName))
+  P.safeSelectionSet $$(G.litName "query_root") Nothing partialQueryFields
+    <&> fmap (fmap (P.handleTypename (RFRaw . J.String . G.unName)))
 
 -- | Prepare the parser for query-type GraphQL requests, but with introspection
 -- for queries, mutations and subscriptions built in.
@@ -471,8 +469,8 @@ unauthenticatedQueryWithIntrospection
   -> m (Parser 'Output n (OMap.InsOrdHashMap G.Name (QueryRootField UnpreparedValue)))
 unauthenticatedQueryWithIntrospection queryRemotes mutationRemotes = do
   let basicQueryFP = fmap (fmap RFRemote) queryRemotes
-      mutationP = unauthenticatedMutation mutationRemotes
-      subscriptionP = unauthenticatedSubscription @n
+  mutationP     <- unauthenticatedMutation mutationRemotes
+  subscriptionP <- unauthenticatedSubscription @n
   queryWithIntrospectionHelper basicQueryFP mutationP subscriptionP
 
 mutation
@@ -550,27 +548,26 @@ mutation allTables allRemotes allActions nonObjectCustomTypes = do
       ActionQuery -> pure Nothing
 
   let mutationFieldsParser = concat (catMaybes mutationParsers) <> catMaybes actionParsers <> fmap (fmap RFRemote) allRemotes
-  pure if null mutationFieldsParser
-       then Nothing
-       else Just $ P.selectionSet $$(G.litName "mutation_root") (Just $ G.Description "mutation root") mutationFieldsParser
-            <&> fmap (P.handleTypename (RFRaw . J.String . G.unName))
+  if null mutationFieldsParser
+  then pure Nothing
+  else fmap Just $ P.safeSelectionSet $$(G.litName "mutation_root") (Just $ G.Description "mutation root") mutationFieldsParser
+            <&> fmap (fmap (P.handleTypename (RFRaw . J.String . G.unName)))
 
 unauthenticatedMutation
-  :: forall n
-   . MonadParse n
+  :: forall n m
+   . (MonadError QErr m, MonadParse n)
   => [P.FieldParser n (RemoteSchemaInfo, G.Field G.NoFragments P.Variable)]
-  -> Maybe (Parser 'Output n (OMap.InsOrdHashMap G.Name (MutationRootField UnpreparedValue)))
+  -> m (Maybe (Parser 'Output n (OMap.InsOrdHashMap G.Name (MutationRootField UnpreparedValue))))
 unauthenticatedMutation allRemotes =
   let mutationFieldsParser = fmap (fmap RFRemote) allRemotes
   in if null mutationFieldsParser
-     then Nothing
-     else Just $ P.selectionSet $$(G.litName "mutation_root") Nothing mutationFieldsParser
-          <&> fmap (P.handleTypename (RFRaw . J.String . G.unName))
+     then pure Nothing
+     else fmap Just $ P.safeSelectionSet $$(G.litName "mutation_root") Nothing mutationFieldsParser
+          <&> fmap (fmap (P.handleTypename (RFRaw . J.String . G.unName)))
 
 unauthenticatedSubscription
-  :: forall n
-   . MonadParse n
-  => Parser 'Output n (OMap.InsOrdHashMap G.Name (QueryRootField UnpreparedValue))
+  :: forall n m. (MonadParse n, MonadError QErr m)
+  => m (Parser 'Output n (OMap.InsOrdHashMap G.Name (QueryRootField UnpreparedValue)))
 unauthenticatedSubscription =
- P.selectionSet $$(G.litName "subscription_root") Nothing []
- <&> fmap (P.handleTypename (RFRaw . J.String . G.unName))
+ P.safeSelectionSet $$(G.litName "subscription_root") Nothing []
+  <&> fmap (fmap (P.handleTypename (RFRaw . J.String . G.unName)))

server/tests-py/queries/schema/duplication/create_action_and_track_table_fail.yaml

@@ -0,0 +1,49 @@
+- description: Create custom object type for action return type
+  url: /v1/query
+  status: 200
+  query:
+    type: set_custom_types
+    args:
+      objects:
+      - name: UserId
+        fields:
+          - name: id
+            type: Int!
+
+- description: Create an action
+  url: /v1/query
+  status: 200
+  query:
+    type: create_action
+    args:
+      name: insert_user
+      definition:
+        kind: synchronous
+        arguments:
+        - name: email
+          type: Int!
+        - name: name
+          type: String!
+        output_type: UserId!
+        handler: http://127.0.0.1:5593/create-user
+
+- description: Track the table
+  url: /v1/query
+  status: 500
+  response:
+    path: "$.args"
+    code: unexpected
+    error: "found duplicate fields in selection set: insert_user"
+  query:
+    type: track_table
+    args:
+      table: user
+
+- description: Drop the custom types and action
+  url: /v1/query
+  status: 200
+  query:
+    type: drop_action
+    args:
+      name: insert_user
+      clear_data: true

server/tests-py/queries/schema/duplication/setup.yaml

@@ -0,0 +1,13 @@
+type: run_sql
+args:
+  sql: |
+    CREATE TABLE "user" (
+      id serial primary key,
+      email text,
+      name text
+    );
+    CREATE TABLE users (
+       id serial primary key,
+       email text,
+       name text
+    );

server/tests-py/queries/schema/duplication/teardown.yaml

@@ -0,0 +1,10 @@
+type: bulk
+args:
+- type: set_custom_types
+  args: {}
+- type: run_sql
+  args:
+    cascade: true
+    sql: |
+      DROP TABLE "user";
+      DROP TABLE users;

server/tests-py/queries/schema/duplication/track_table_and_create_action_fail.yaml

@@ -0,0 +1,44 @@
+- description: Track the "user" table
+  url: /v1/query
+  status: 200
+  response:
+    message: success
+  query:
+    type: track_table
+    args:
+      table: user
+
+- description: Create custom object type for action return type
+  url: /v1/query
+  status: 200
+  response:
+    message: success
+  query:
+    type: set_custom_types
+    args:
+      objects:
+        - name: UserId
+          fields:
+            - name: id
+              type: Int!
+
+- description: Create an action with a same name with a mutation that's created by tracking "user" table
+  url: /v1/query
+  status: 500
+  response:
+    path: "$.args"
+    code: unexpected
+    error: "found duplicate fields in selection set: insert_user"
+  query:
+    type: create_action
+    args:
+      name: insert_user
+      definition:
+        kind: synchronous
+        arguments:
+        - name: email
+          type: Int!
+        - name: name
+          type: String!
+        output_type: UserId!
+        handler: http://127.0.0.1:5593/create-user

server/tests-py/queries/schema/duplication/track_table_with_conflicting_custom_root_node_names_fail.yaml

@@ -0,0 +1,25 @@
+- description: Track the "user" table
+  url: /v1/query
+  status: 200
+  response:
+    message: success
+  query:
+    type: track_table
+    args:
+      table: user
+
+- description: Track the users table with conflicting custom root level node names
+  url: /v1/query
+  status: 500
+  response:
+    path: $.args
+    code: unexpected
+    error: "found duplicate fields in selection set: user"
+  query:
+    type: track_table
+    version: 2
+    args:
+      table: users
+      configuration:
+        custom_root_fields:
+          select_by_pk: user

server/tests-py/test_schema_duplication.py

@@ -0,0 +1,20 @@
+#!/usrbin/env python3
+
+import pytest
+from validate import check_query_f
+
+@pytest.mark.usefixtures('per_method_tests_db_state')
+class TestSchemaDuplication:
+
+    @classmethod
+    def dir(cls):
+        return "queries/schema/duplication/"
+
+    def test_create_action_followed_by_track_table(self, hge_ctx):
+        check_query_f(hge_ctx, self.dir() + "create_action_and_track_table_fail.yaml")
+
+    def test_track_table_followed_by_create_action(self, hge_ctx):
+        check_query_f(hge_ctx, self.dir() + "track_table_and_create_action_fail.yaml")
+
+    def test_track_table_with_conflicting_custom_root_node_names(self, hge_ctx):
+        check_query_f(hge_ctx, self.dir() + 'track_table_with_conflicting_custom_root_node_names_fail.yaml')