From d5610242542eb10c8d3ba825f2bd34fb23fb4faf Mon Sep 17 00:00:00 2001 From: Rakesh Emmadi <12475069+rakeshkky@users.noreply.github.com> Date: Mon, 11 Apr 2022 13:34:12 +0530 Subject: [PATCH] server/postgres/citus: fix inserting values into columns with case sensitive enum types PR-URL: https://github.com/hasura/graphql-engine-mono/pull/4196 GitOrigin-RevId: bbb9e565bc7143080eb1c50ade86b0e47b751387 --- CHANGELOG.md | 1 + server/graphql-engine.cabal | 1 + .../Backends/Postgres/DDL/ComputedField.hs | 3 +- .../Hasura/Backends/Postgres/SQL/Types.hs | 91 +++++--- .../Hasura/Backends/Postgres/SQL/Value.hs | 5 +- server/src-rsr/citus_table_metadata.sql | 2 +- server/src-rsr/pg_table_metadata.sql | 2 +- .../tests-hspec/Test/InsertEnumColumnSpec.hs | 217 ++++++++++++++++++ 8 files changed, 281 insertions(+), 41 deletions(-) create mode 100644 server/tests-hspec/Test/InsertEnumColumnSpec.hs diff --git a/CHANGELOG.md b/CHANGELOG.md index f56851bafa680..5d54c0eb569eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Bug fixes and improvements +- server: fix inserting values into columns with case sensitive enum types for Postgres/Citus backends (fix #4014) - server: remote relationships can be defined _on_ and _to_ SQLServer tables - server: fix JSON key encoding issue for remote schemas (fixes #7543 and #8200) - server: fix MSSQL insert mutation when relationships are used in check permissions (fix #8225) diff --git a/server/graphql-engine.cabal b/server/graphql-engine.cabal index 69d1433fd033d..9b00ffab76fc8 100644 --- a/server/graphql-engine.cabal +++ b/server/graphql-engine.cabal @@ -1103,6 +1103,7 @@ test-suite tests-hspec Test.WhereSpec Test.RunSQLSpec Test.InsertCheckPermissionSpec + Test.InsertEnumColumnSpec test-suite tests-gdw-api import: common-all, common-exe diff --git a/server/src-lib/Hasura/Backends/Postgres/DDL/ComputedField.hs b/server/src-lib/Hasura/Backends/Postgres/DDL/ComputedField.hs index 301df78183235..27bbfcb55e8f8 100644 --- a/server/src-lib/Hasura/Backends/Postgres/DDL/ComputedField.hs +++ b/server/src-lib/Hasura/Backends/Postgres/DDL/ComputedField.hs @@ -20,7 +20,6 @@ import Hasura.RQL.Types.Common (Comment (..)) import Hasura.RQL.Types.ComputedField import Hasura.RQL.Types.Function import Hasura.SQL.Backend -import Hasura.SQL.Types import Hasura.Server.Utils import Language.GraphQL.Draft.Syntax qualified as G @@ -61,7 +60,7 @@ showError qf = \case CFVEInvalidSessionArgument (ISANotJSON functionArg) -> showFunctionSessionArgument functionArg <> " is not of type JSON" CFVENotBaseReturnType scalarType -> - "the function " <> qf <<> " returning type " <> toSQLTxt scalarType + "the function " <> qf <<> " returning type " <> pgScalarTypeToText scalarType <> " is not a BASE type" CFVEReturnTableNotFound table -> "the function " <> qf <<> " returning set of table " <> table diff --git a/server/src-lib/Hasura/Backends/Postgres/SQL/Types.hs b/server/src-lib/Hasura/Backends/Postgres/SQL/Types.hs index 47ed134ec364b..846fa5365c90e 100644 --- a/server/src-lib/Hasura/Backends/Postgres/SQL/Types.hs +++ b/server/src-lib/Hasura/Backends/Postgres/SQL/Types.hs @@ -37,6 +37,7 @@ module Hasura.Backends.Postgres.SQL.Types qualifiedObjectToName, PGScalarType (..), textToPGScalarType, + pgScalarTypeToText, PGTypeKind (..), QualifiedPGType (..), isBaseType, @@ -317,6 +318,7 @@ data PGScalarType | PGLtxtquery | PGUnknown !Text | PGCompositeScalar !Text + | PGEnumScalar !Text deriving (Show, Eq, Ord, Generic, Data) instance NFData PGScalarType @@ -325,46 +327,55 @@ instance Hashable PGScalarType instance Cacheable PGScalarType +pgScalarTypeToText :: PGScalarType -> Text +pgScalarTypeToText = \case + PGSmallInt -> "smallint" + PGInteger -> "integer" + PGBigInt -> "bigint" + PGSerial -> "serial" + PGBigSerial -> "bigserial" + PGFloat -> "real" + PGDouble -> "float8" + PGNumeric -> "numeric" + PGMoney -> "money" + PGBoolean -> "boolean" + PGChar -> "bpchar" + PGVarchar -> "varchar" + PGText -> "text" + PGCitext -> "citext" + PGDate -> "date" + PGTimeStamp -> "timestamp" + PGTimeStampTZ -> "timestamptz" + PGTimeTZ -> "timetz" + PGJSON -> "json" + PGJSONB -> "jsonb" + PGGeometry -> "geometry" + PGGeography -> "geography" + PGRaster -> "raster" + PGUUID -> "uuid" + PGLtree -> "ltree" + PGLquery -> "lquery" + PGLtxtquery -> "ltxtquery" + PGUnknown t -> t + PGCompositeScalar t -> t + PGEnumScalar t -> t + instance ToSQL PGScalarType where - toSQL = \case - PGSmallInt -> "smallint" - PGInteger -> "integer" - PGBigInt -> "bigint" - PGSerial -> "serial" - PGBigSerial -> "bigserial" - PGFloat -> "real" - PGDouble -> "float8" - PGNumeric -> "numeric" - PGMoney -> "money" - PGBoolean -> "boolean" - PGChar -> "bpchar" - PGVarchar -> "varchar" - PGText -> "text" - PGCitext -> "citext" - PGDate -> "date" - PGTimeStamp -> "timestamp" - PGTimeStampTZ -> "timestamptz" - PGTimeTZ -> "timetz" - PGJSON -> "json" - PGJSONB -> "jsonb" - PGGeometry -> "geometry" - PGGeography -> "geography" - PGRaster -> "raster" - PGUUID -> "uuid" - PGLtree -> "ltree" - PGLquery -> "lquery" - PGLtxtquery -> "ltxtquery" - PGUnknown t -> TB.text t - PGCompositeScalar t -> TB.text t + toSQL = + TB.text . \case + -- Format enum type names as identifiers to preserve case sensitivity + -- https://github.com/hasura/graphql-engine/issues/4014 + PGEnumScalar t -> pgFmtIdentifier t + scalarType -> pgScalarTypeToText scalarType instance ToJSON PGScalarType where - toJSON = String . toSQLTxt + toJSON = String . pgScalarTypeToText instance ToJSONKey PGScalarType where - toJSONKey = toJSONKeyText toSQLTxt + toJSONKey = toJSONKeyText pgScalarTypeToText instance ToTxt PGScalarType where - toTxt = toSQLTxt + toTxt = pgScalarTypeToText textToPGScalarType :: Text -> PGScalarType textToPGScalarType t = fromMaybe (PGUnknown t) (lookup t pgScalarTranslations) @@ -418,7 +429,15 @@ pgScalarTranslations = instance FromJSON PGScalarType where parseJSON (String t) = return $ textToPGScalarType t - parseJSON _ = fail "Expecting a string for PGScalarType" + parseJSON (Object o) = do + typeType <- o .: "type" + typeName <- o .: "name" + pure $ + case typeType of + PGKindEnum -> PGEnumScalar typeName + PGKindComposite -> PGCompositeScalar typeName + _ -> textToPGScalarType typeName + parseJSON _ = fail "Expecting a string or object for PGScalarType" isNumType :: PGScalarType -> Bool isNumType PGInteger = True @@ -528,7 +547,7 @@ isBaseType (QualifiedPGType _ n ty) = typeToTable :: QualifiedPGType -> QualifiedTable typeToTable (QualifiedPGType sch n _) = - QualifiedObject sch $ TableName $ toSQLTxt n + QualifiedObject sch $ TableName $ pgScalarTypeToText n mkFunctionArgScalarType :: QualifiedPGType -> PGScalarType mkFunctionArgScalarType (QualifiedPGType _schema name type') = @@ -582,7 +601,7 @@ mkScalarTypeName (PGCompositeScalar compositeScalarType) = <> "valid GraphQL identifier" ) mkScalarTypeName scalarType = - G.mkName (toSQLTxt scalarType) + G.mkName (pgScalarTypeToText scalarType) `onNothing` throw400 ValidationFailed ( "cannot use SQL type " <> scalarType <<> " in the GraphQL schema because its name is not a " diff --git a/server/src-lib/Hasura/Backends/Postgres/SQL/Value.hs b/server/src-lib/Hasura/Backends/Postgres/SQL/Value.hs index ca1336a89a445..b84f988bbe2de 100644 --- a/server/src-lib/Hasura/Backends/Postgres/SQL/Value.hs +++ b/server/src-lib/Hasura/Backends/Postgres/SQL/Value.hs @@ -210,6 +210,8 @@ parsePGValue ty val = case (ty, val) of fail $ "A string is expected for type: " ++ T.unpack tyName PGCompositeScalar tyName -> fail $ "A string is expected for type: " ++ T.unpack tyName + PGEnumScalar tyName -> + fail $ "A string is expected for type: " ++ T.unpack tyName txtEncodedVal :: PGScalarValue -> TxtEncodedVal txtEncodedVal = \case @@ -284,8 +286,9 @@ pgTypeOid = \case PGLtree -> PTI.text PGLquery -> PTI.text PGLtxtquery -> PTI.text - (PGUnknown _) -> PTI.auto + PGUnknown _ -> PTI.auto PGCompositeScalar _ -> PTI.auto + PGEnumScalar _ -> PTI.auto binEncoder :: PGScalarValue -> Q.PrepArg binEncoder = \case diff --git a/server/src-rsr/citus_table_metadata.sql b/server/src-rsr/citus_table_metadata.sql index 04ba2124c5086..fad4e14e35f08 100644 --- a/server/src-rsr/citus_table_metadata.sql +++ b/server/src-rsr/citus_table_metadata.sql @@ -61,7 +61,7 @@ LEFT JOIN LATERAL ( SELECT jsonb_agg(jsonb_build_object( 'name', "column".attname, 'position', "column".attnum, - 'type', coalesce(base_type.typname, "type".typname), + 'type', json_build_object('name', coalesce(base_type.typname, "type".typname), 'type', "type".typtype), 'is_nullable', NOT "column".attnotnull, 'description', pg_catalog.col_description("table".oid, "column".attnum), 'mutability', jsonb_build_object( diff --git a/server/src-rsr/pg_table_metadata.sql b/server/src-rsr/pg_table_metadata.sql index 7f58f7153aa1a..a169a55103ee2 100644 --- a/server/src-rsr/pg_table_metadata.sql +++ b/server/src-rsr/pg_table_metadata.sql @@ -53,7 +53,7 @@ LEFT JOIN LATERAL ( SELECT jsonb_agg(jsonb_build_object( 'name', "column".attname, 'position', "column".attnum, - 'type', coalesce(base_type.typname, "type".typname), + 'type', json_build_object('name', coalesce(base_type.typname, "type".typname), 'type', "type".typtype), 'is_nullable', NOT "column".attnotnull, 'description', pg_catalog.col_description("table".oid, "column".attnum), 'mutability', jsonb_build_object( diff --git a/server/tests-hspec/Test/InsertEnumColumnSpec.hs b/server/tests-hspec/Test/InsertEnumColumnSpec.hs new file mode 100644 index 0000000000000..270bbbd7fc56b --- /dev/null +++ b/server/tests-hspec/Test/InsertEnumColumnSpec.hs @@ -0,0 +1,217 @@ +{-# LANGUAGE QuasiQuotes #-} + +-- | Testing inserting values into column with enum type +-- +-- Test fix for https://github.com/hasura/graphql-engine/issues/4014 +module Test.InsertEnumColumnSpec (spec) where + +import Harness.Backend.Citus as Citus +import Harness.Backend.Postgres as Postgres +import Harness.GraphqlEngine qualified as GraphqlEngine +import Harness.Quoter.Graphql +import Harness.Quoter.Sql +import Harness.Quoter.Yaml +import Harness.State (State) +import Harness.Test.Context qualified as Context +import Test.Hspec +import Prelude + +-------------------------------------------------------------------------------- +-- Preamble + +spec :: SpecWith State +spec = + Context.run + [ Context.Context + { name = Context.Backend Context.Postgres, + mkLocalState = Context.noLocalState, + setup = pgSetup, + teardown = pgTeardown, + customOptions = Nothing + }, + Context.Context + { name = Context.Backend Context.Citus, + mkLocalState = Context.noLocalState, + setup = citusSetup, + teardown = citusTeardown, + customOptions = Nothing + } + ] + tests + +-------------------------------------------------------------------------------- +-- Common SQL + +commonSetupSQL :: String +commonSetupSQL = + [sql| +CREATE TYPE "UserRole" AS ENUM ('Admin', 'Editor', 'Moderator'); + +CREATE TABLE "User" ( + "Id" SERIAL PRIMARY KEY, + "Name" TEXT NOT NULL, + "Role" "UserRole" NOT NULL +); +|] + +commonTeardownSQL :: String +commonTeardownSQL = + [sql| +DROP TABLE "User"; +DROP TYPE "UserRole"; +|] + +-------------------------------------------------------------------------------- + +-- * Postgres backend + +pgSetup :: (State, ()) -> IO () +pgSetup (state, ()) = do + -- Clear and reconfigure the metadata + GraphqlEngine.setSource state Postgres.defaultSourceMetadata + + -- Setup schema + Postgres.run_ commonSetupSQL + + -- Track tables + GraphqlEngine.postMetadata_ + state + [yaml| +type: pg_track_table +args: + source: postgres + table: + schema: hasura + name: User +|] + +pgTeardown :: (State, ()) -> IO () +pgTeardown (state, ()) = do + -- Untrack tables + GraphqlEngine.postMetadata_ + state + [yaml| +type: pg_untrack_table +args: + source: postgres + table: + schema: hasura + name: User +|] + + -- Teardown schema + Postgres.run_ commonTeardownSQL + + -- Clear metadata + GraphqlEngine.clearMetadata state + +-------------------------------------------------------------------------------- + +-- * Citus backend + +citusSetup :: (State, ()) -> IO () +citusSetup (state, ()) = do + -- Clear and reconfigure the metadata + GraphqlEngine.setSource state Citus.defaultSourceMetadata + + -- Setup schema + Citus.run_ commonSetupSQL + + -- Track tables + GraphqlEngine.postMetadata_ + state + [yaml| +type: citus_track_table +args: + source: citus + table: + schema: hasura + name: User +|] + +citusTeardown :: (State, ()) -> IO () +citusTeardown (state, ()) = do + -- Untrack tables + GraphqlEngine.postMetadata_ + state + [yaml| +type: citus_untrack_table +args: + source: citus + table: + schema: hasura + name: User +|] + + -- Teardown schema + Citus.run_ commonTeardownSQL + + -- Clear metadata + GraphqlEngine.clearMetadata state + +-------------------------------------------------------------------------------- + +-- * Tests + +tests :: Context.Options -> SpecWith State +tests opts = do + it "Insert into enum column with valid enum values" $ \state -> + shouldReturnYaml + opts + ( GraphqlEngine.postGraphql + state + [graphql| +mutation { + insert_hasura_User( + objects: [{Name: "Bob", Role: "Admin"}, {Name: "Rob", Role: "Moderator"}] + ) { + affected_rows + returning{ + Id + Name + Role + } + } +} +|] + ) + [yaml| +data: + insert_hasura_User: + returning: + - Role: Admin + Name: Bob + Id: 1 + - Role: Moderator + Name: Rob + Id: 2 + affected_rows: 2 +|] + + it "Insert into enum column with invalid enum values" $ \state -> + shouldReturnYaml + opts + ( GraphqlEngine.postGraphql + state + [graphql| +mutation { + insert_hasura_User( + objects: [{Name: "Bob1", Role: "Admin"}, {Name: "Rob", Role: "InvalidRole"}] + ) { + affected_rows + returning{ + Id + Name + Role + } + } +} +|] + ) + [yaml| +errors: +- extensions: + path: $.selectionSet.insert_hasura_User.args.objects + code: data-exception + message: 'invalid input value for enum "UserRole": "InvalidRole"' +|]