server: mark remote schema as inconsistent on conflicting types

PR-URL: https://github.com/hasura/graphql-engine-mono/pull/11078 GitOrigin-RevId: 32a1d41ee7d22c6137ee3bfed5844d6a1ebc61d5
2024-12-15 01:12:56 +03:00 · 2024-12-03 18:03:27 +05:30 · 2024-12-03 18:03:27 +05:30 · 4e5525f261
commit 4e5525f261
parent 3c69cebb1f
4 changed files with 173 additions and 16 deletions
--- a/server/lib/api-tests/api-tests.cabal
+++ b/server/lib/api-tests/api-tests.cabal
@ -232,6 +232,7 @@ library
    Test.Regression.ObjectRelationshipsLimit7936Spec
    Test.Regression.OptimizePermissionFiltersSpec
    Test.Regression.RemoteRelationshipStringifyNum8387Spec
+    Test.Regression.RemoteSchemaConflictingTypesSpec
    Test.Regression.RootExistElementAliasing9457Spec
    Test.Regression.SqlServerIdentifierQuotingSpec
    Test.Regression.SqlServerRemoteRelationshipRhs8712Spec
--- a/server/lib/api-tests/src/Test/Regression/RemoteSchemaConflictingTypesSpec.hs
+++ b/server/lib/api-tests/src/Test/Regression/RemoteSchemaConflictingTypesSpec.hs
@ -0,0 +1,134 @@
+{-# LANGUAGE QuasiQuotes #-}
+
+-- | Test remote schema marked as inconsistent when conflicting types are present
+module Test.Regression.RemoteSchemaConflictingTypesSpec where
+
+import Data.Aeson qualified as J
+import Data.Aeson.KeyMap qualified as J
+import Data.List.NonEmpty qualified as NE
+import Data.Morpheus.Document (gqlDocument)
+import Harness.Backend.Postgres qualified as Postgres
+import Harness.GraphqlEngine qualified as GraphqlEngine
+import Harness.Quoter.Yaml (yaml)
+import Harness.RemoteServer qualified as RemoteServer
+import Harness.Test.Fixture (Fixture (..))
+import Harness.Test.Fixture qualified as Fixture
+import Harness.TestEnvironment (GlobalTestEnvironment, Server, TestEnvironment (..), stopServer)
+import Harness.Yaml (shouldReturnYamlF)
+import Hasura.Prelude
+import Test.Hspec (SpecWith, describe, it)
+
+--------------------------------------------------------------------------
+-- Preamble
+
+spec :: SpecWith GlobalTestEnvironment
+spec = Fixture.runWithLocalTestEnvironment (NE.fromList [context]) tests
+  where
+    context =
+      (Fixture.fixture $ Fixture.RemoteGraphQLServer)
+        { Fixture.mkLocalTestEnvironment = \_testEnvironment ->
+            RemoteServer.run
+              $ RemoteServer.generateQueryInterpreter
+              $ Query {user = getUserResolver},
+          setupTeardown = \(testEnvironment, server) ->
+            [ Fixture.SetupAction
+                { Fixture.setupAction = GraphqlEngine.clearMetadata testEnvironment,
+                  Fixture.teardownAction = \_ -> do
+                    GraphqlEngine.clearMetadata testEnvironment
+                    stopServer server
+                },
+              Fixture.SetupAction
+                { Fixture.setupAction = postgresSetup testEnvironment,
+                  Fixture.teardownAction = \_ -> postgresTeardown testEnvironment
+                }
+            ]
+        }
+
+--------------------------------------------------------------------------
+-- Tests
+tests :: SpecWith (TestEnvironment, Server)
+tests = describe "Remote schema conflicting types" $ do
+  it "should mark remote schema as inconsistent when conflicting types are present" $ \(testEnvironment, remoteServer) -> do
+    let remoteSchemaEndpoint = RemoteServer.graphqlEndpoint remoteServer
+        expectedResponse =
+          [yaml|
+            "Inconsistent object: Found conflicting definitions for GraphQL typeFound conflicting
+                   definitions for GraphQL type 'User'.  The definition at user differs from the definitions
+                   at [delete_User_by_pk, update_User_by_pk, insert_User_one, insert_User.returning,
+                   User_aggregate.nodes, User_by_pk, User].\nFormer has definition:\ntype User { id:
+                   Int!\n  name: String!\n}\nLatter has definition:\n\"columns and relationships of
+                   \\\"User\\\"\" \ntype User { first_name: String\n  id: Int!\n  last_name: String\n}"
+          |]
+        addRemoteSchema =
+          [yaml|
+          type: add_remote_schema
+          args:
+            name: remote
+            definition:
+              url: *remoteSchemaEndpoint
+          |]
+        getError (J.Object o) = onNothing (J.lookup "error" o) (fail "Should return error field")
+        getError _ = fail "Should return Object"
+    shouldReturnYamlF
+      testEnvironment
+      getError
+      (GraphqlEngine.postMetadataWithStatus 400 testEnvironment addRemoteSchema)
+      expectedResponse
+
+--------------------------------------------------------------------------
+--- Postgres
+
+postgresSetup :: TestEnvironment -> IO ()
+postgresSetup testEnvironment = do
+  Postgres.createDatabase testEnvironment
+  let sourceName :: Text = "db"
+      sourceConfig = Postgres.defaultSourceConfiguration testEnvironment
+  GraphqlEngine.postMetadata_
+    testEnvironment
+    [yaml|
+type: pg_add_source
+args:
+  name: *sourceName
+  configuration: *sourceConfig
+|]
+  -- setup tables only
+  GraphqlEngine.postV2Query_
+    testEnvironment
+    [yaml|
+type: pg_run_sql
+args:
+  source: *sourceName
+  sql: |
+    CREATE TABLE "User" (
+      id serial PRIMARY KEY,
+      first_name text,
+      last_name text
+    );
+|]
+  GraphqlEngine.postMetadata_
+    testEnvironment
+    [yaml|
+type: pg_track_table
+args:
+  source: *sourceName
+  table: User
+|]
+
+postgresTeardown :: TestEnvironment -> IO ()
+postgresTeardown testEnvironment = Postgres.dropDatabase testEnvironment
+
+--------------------------------------------------------------------------
+--- Remote schema
+[gqlDocument|
+type Query {
+  user: User!
+}
+
+type User {
+  id: Int!
+  name: String!
+}
+|]
+
+getUserResolver :: (Monad m) => m (User m)
+getUserResolver = pure $ User (pure 1) (pure "Alice")
--- a/server/lib/hasura-prelude/src/Hasura/Prelude.hs
+++ b/server/lib/hasura-prelude/src/Hasura/Prelude.hs
@ -28,6 +28,7 @@ module Hasura.Prelude

    -- * Either
    onLeft,
+    onLeft_,
    onLeftM,
    mapLeft,
    liftEitherM,
@ -251,6 +252,10 @@ hoistMaybe = MaybeT . pure
 onLeft :: (Applicative m) => Either e a -> (e -> m a) -> m a
 onLeft e f = either f pure e

+-- | Similar to 'onLeft', but ignores the 'Right' value.
+onLeft_ :: (Applicative m) => Either e a -> (e -> m ()) -> m ()
+onLeft_ e f = either f (const $ pure ()) e
+
 -- | Similar to 'onLeft', but accepts a monadic action on its LHS.
 onLeftM :: (Monad m) => m (Either e a) -> (e -> m a) -> m a
 onLeftM e f = e >>= (`onLeft` f)
--- a/server/src-lib/Hasura/GraphQL/Schema.hs
+++ b/server/src-lib/Hasura/GraphQL/Schema.hs
@ -328,16 +328,6 @@ buildRoleContext sampledFeatureFlags options sources remotes actions customTypes
      fmap mconcat $ for (toList sources) \sourceInfo ->
        AB.dispatchAnyBackend @BackendSchema sourceInfo $ buildSource schemaContext schemaOptions

-    -- build all remote schemas
-    -- we only keep the ones that don't result in a name conflict
-    (remoteSchemaFields, !remoteSchemaErrors) <-
-      runRemoteSchema schemaContext (soRemoteNullForwardingPolicy schemaOptions)
-        $ buildAndValidateRemoteSchemas remotes sourcesQueryFields sourcesMutationBackendFields role remoteSchemaPermsCtx
-    let remotesQueryFields = concatMap (\(n, rf) -> (n,) <$> piQuery rf) remoteSchemaFields
-        remotesMutationFields = concat $ mapMaybe (\(n, rf) -> fmap (n,) <$> piMutation rf) remoteSchemaFields
-        remotesSubscriptionFields = concat $ mapMaybe (\(n, rf) -> fmap (n,) <$> piSubscription rf) remoteSchemaFields
-        apolloQueryFields = apolloRootFields apolloFederationStatus apolloFedTableParsers
-
    -- build all actions
    -- we use the source context due to how async query relationships are implemented
    (actionsQueryFields, actionsMutationFields, actionsSubscriptionFields) <-
@ -349,6 +339,16 @@ buildRoleContext sampledFeatureFlags options sources remotes actions customTypes
          subscriptionFields <- buildActionSubscriptionFields customTypes action
          pure (queryFields, mutationFields, subscriptionFields)

+    -- build all remote schemas
+    -- we only keep the ones that don't result in a name conflict
+    (remoteSchemaFields, !remoteSchemaErrors) <-
+      runRemoteSchema schemaContext (soRemoteNullForwardingPolicy schemaOptions)
+        $ buildAndValidateRemoteSchemas remotes sourcesQueryFields sourcesMutationBackendFields actionsQueryFields actionsMutationFields role remoteSchemaPermsCtx
+    let remotesQueryFields = concatMap (\(n, rf) -> (n,) <$> piQuery rf) remoteSchemaFields
+        remotesMutationFields = concat $ mapMaybe (\(n, rf) -> fmap (n,) <$> piMutation rf) remoteSchemaFields
+        remotesSubscriptionFields = concat $ mapMaybe (\(n, rf) -> fmap (n,) <$> piSubscription rf) remoteSchemaFields
+        apolloQueryFields = apolloRootFields apolloFederationStatus apolloFedTableParsers
+
    mutationParserFrontend <-
      buildMutationParser sourcesMutationFrontendFields remotesMutationFields actionsMutationFields
    mutationParserBackend <-
@ -631,7 +631,7 @@ unauthenticatedContext options sources allRemotes expFeatures schemaSampledFeatu
        -- Permissions are disabled, unauthenticated users have access to remote schemas.
        (remoteFields, remoteSchemaErrors) <-
          runRemoteSchema fakeSchemaContext (soRemoteNullForwardingPolicy schemaOptions)
-            $ buildAndValidateRemoteSchemas allRemotes [] [] fakeRole remoteSchemaPermsCtx
+            $ buildAndValidateRemoteSchemas allRemotes [] [] [] [] fakeRole remoteSchemaPermsCtx
        pure
          ( (\(n, rf) -> fmap (fmap (RFRemote n)) rf) <$> concatMap (\(n, q) -> (n,) <$> piQuery q) remoteFields,
            (\(n, rf) -> fmap (fmap (RFRemote n)) rf) <$> concat (mapMaybe (\(n, q) -> fmap (n,) <$> piMutation q) remoteFields),
@ -664,8 +664,10 @@ buildAndValidateRemoteSchemas ::
    MonadIO m
  ) =>
  HashMap RemoteSchemaName (RemoteSchemaCtx, MetadataObject) ->
-  [FieldParser P.Parse (NamespacedField (QueryRootField UnpreparedValue))] ->
-  [FieldParser P.Parse (NamespacedField (MutationRootField UnpreparedValue))] ->
+  [FieldParser P.Parse (NamespacedField (QueryRootField UnpreparedValue))] -> -- Sources query fields
+  [FieldParser P.Parse (NamespacedField (MutationRootField UnpreparedValue))] -> -- Sources mutation fields
+  [FieldParser P.Parse (QueryRootField UnpreparedValue)] -> -- Action query fields
+  [FieldParser P.Parse (MutationRootField UnpreparedValue)] -> -- Action mutation fields
  RoleName ->
  Options.RemoteSchemaPermissions ->
  SchemaT
@ -676,7 +678,7 @@ buildAndValidateRemoteSchemas ::
    )
    (MemoizeT m)
    ([(RemoteSchemaName, RemoteSchemaParser P.Parse)], HashSet InconsistentMetadata)
-buildAndValidateRemoteSchemas remotes sourcesQueryFields sourcesMutationFields role remoteSchemaPermsCtx =
+buildAndValidateRemoteSchemas remotes sourcesQueryFields sourcesMutationFields actionQueryFields actionMutationFields role remoteSchemaPermsCtx =
  runWriterT $ foldlM step [] (HashMap.elems remotes)
  where
    getFieldName = P.getName . P.fDefinition
@ -711,8 +713,23 @@ buildAndValidateRemoteSchemas remotes sourcesQueryFields sourcesMutationFields r
              --   - between this remote and the sources:
              for_ (duplicates $ newSchemaMutationFieldNames <> sourcesMutationFieldNames)
                $ \name -> reportInconsistency $ "Field cannot be overwritten by remote field " <> squote name
-          -- No need to check for conflicts between subscription fields, since
-          -- remote subscriptions aren't supported yet.
+            -- No need to check for conflicts between subscription fields, since
+            -- remote subscriptions aren't supported yet.
+
+            -- check for conflicting types of remote schemas with existing sources and actions schema
+            let collectedTypes =
+                  P.collectTypeDefinitions @MetadataObjId
+                    [ -- sources and actions schema
+                      P.TypeDefinitionsWrapper $ map P.fDefinition sourcesQueryFields,
+                      P.TypeDefinitionsWrapper $ map P.fDefinition sourcesMutationFields,
+                      P.TypeDefinitionsWrapper $ map P.fDefinition actionQueryFields,
+                      P.TypeDefinitionsWrapper $ map P.fDefinition actionMutationFields,
+                      -- remote schema
+                      P.TypeDefinitionsWrapper $ map P.fDefinition $ piQuery remoteSchemaParser,
+                      P.TypeDefinitionsWrapper $ map P.fDefinition <$> piMutation remoteSchemaParser
+                    ]
+            onLeft_ collectedTypes $ \conflictingTypes ->
+              reportInconsistency $ "Found conflicting definitions for GraphQL type" <> fromErrorMessage (toErrorValue conflictingTypes)

          -- Only add this new remote to the list if there was no error
          pure