allow identical fields in custom column names configuration (fix #3137) & improve root fields validation (#3154)

* allow identical column fields in 'custom_column_names' * improve validation of custom column names * improve 'checkForFieldConflicts' & validate custom column names with non column fields * split `validateTableConfig` into two isolated validation logic * improve validation of root fields * improve validating custom root fields and duplicate root fields * move 'validateCustomRootFields' to 'buildSchemaCache'
2024-12-15 09:22:43 +03:00 · 2019-11-20 18:10:56 +05:30 · 2019-11-20 18:10:56 +05:30 · c4c5dd87ac
commit c4c5dd87ac
parent 9b8e6b42d1
13 changed files with 271 additions and 55 deletions
--- a/server/src-lib/Hasura/GraphQL/Context.hs
+++ b/server/src-lib/Hasura/GraphQL/Context.hs
@ -10,11 +10,13 @@ import           Language.Haskell.TH.Syntax    (Lift)

 import qualified Data.HashMap.Strict           as Map
 import qualified Data.HashSet                  as Set
+import qualified Data.Text                     as T
 import qualified Language.GraphQL.Draft.Syntax as G

 import           Hasura.GraphQL.Resolve.Types
 import           Hasura.GraphQL.Validate.Types
 import           Hasura.RQL.Types.Permission
+import           Hasura.Server.Utils           (duplicates)

 -- | A /GraphQL context/, aka the final output of GraphQL schema generation. Used to both validate
 -- incoming queries and respond to introspection queries.
@ -86,7 +88,27 @@ data TableCustomRootFields
  , _tcrfUpdate          :: !(Maybe G.Name)
  , _tcrfDelete          :: !(Maybe G.Name)
  } deriving (Show, Eq, Lift)
-$(deriveJSON (aesonDrop 5 snakeCase) ''TableCustomRootFields)
+$(deriveToJSON (aesonDrop 5 snakeCase) ''TableCustomRootFields)
+
+instance FromJSON TableCustomRootFields where
+  parseJSON = withObject "Object" $ \obj -> do
+    select <- obj .:? "select"
+    selectByPk <- obj .:? "select_by_pk"
+    selectAggregate <- obj .:? "select_aggregate"
+    insert <- obj .:? "insert"
+    update <- obj .:? "update"
+    delete <- obj .:? "delete"
+
+    let duplicateRootFields = duplicates $
+                              catMaybes [ select, selectByPk, selectAggregate
+                                        , insert, update, delete
+                                        ]
+    when (not $ null duplicateRootFields) $ fail $ T.unpack $
+      "the following custom root field names are duplicated: "
+      <> showNames duplicateRootFields
+
+    pure $ TableCustomRootFields select selectByPk selectAggregate
+                                 insert update delete

 emptyCustomRootFields :: TableCustomRootFields
 emptyCustomRootFields =
--- a/server/src-lib/Hasura/GraphQL/Schema.hs
+++ b/server/src-lib/Hasura/GraphQL/Schema.hs
@ -333,8 +333,8 @@ getRootFldsRole'
 getRootFldsRole' tn primCols constraints fields funcs insM
                 selM updM delM viM tableConfig =
  RootFields
-    { rootQueryFields = makeFieldMap
-        $  funcQueries
+    { rootQueryFields = makeFieldMap $
+        funcQueries
        <> funcAggQueries
        <> catMaybes
          [ getSelDet <$> selM
@ -348,10 +348,10 @@ getRootFldsRole' tn primCols constraints fields funcs insM
        ]
    }
  where
+    makeFieldMap = mapFromL (_fiName . snd)
    customRootFields = _tcCustomRootFields tableConfig
    colGNameMap = mkPGColGNameMap $ getValidCols fields

-    makeFieldMap = mapFromL (_fiName . snd)
    allCols = getCols fields
    funcQueries = maybe [] getFuncQueryFlds selM
    funcAggQueries = maybe [] getFuncAggQueryFlds selM
@ -697,27 +697,44 @@ noFilter :: AnnBoolExpPartialSQL
 noFilter = annBoolExpTrue

 mkGCtxMap
-  :: (MonadError QErr m)
+  :: forall m. (MonadError QErr m)
  => TableCache PGColumnInfo -> FunctionCache -> m GCtxMap
 mkGCtxMap tableCache functionCache = do
  typesMapL <- mapM (mkGCtxMapTable tableCache functionCache) $
               filter tableFltr $ Map.elems tableCache
-  -- since root field names are customisable, we need to check for
-  -- duplicate root field names across all tables
-  duplicateRootFlds <- (duplicates . concat) <$> forM typesMapL getRootFlds
-  unless (null duplicateRootFlds) $
-    throw400 Unexpected $ "following root fields are duplicated: "
-    <> showNames duplicateRootFlds
-  let typesMap = foldr (Map.unionWith mappend) Map.empty typesMapL
+  typesMap <- combineTypes typesMapL
  return $ flip Map.map typesMap $ \(ty, flds, insCtxMap) ->
    mkGCtx ty flds insCtxMap
  where
    tableFltr ti = not (isSystemDefined $ _tiSystemDefined ti) && isValidObjectName (_tiName ti)

-    getRootFlds roleMap = do
-      (_, RootFields query mutation, _) <- onNothing
-        (Map.lookup adminRole roleMap) $ throw500 "admin schema not found"
-      return $ Map.keys query <> Map.keys mutation
+    combineTypes
+      :: [Map.HashMap RoleName (TyAgg, RootFields, InsCtxMap)]
+      -> m (Map.HashMap RoleName (TyAgg, RootFields, InsCtxMap))
+    combineTypes maps = do
+      let listMap = foldr (Map.unionWith (++) . Map.map pure) Map.empty maps
+      flip Map.traverseWithKey listMap $ \_ typeList -> do
+        let tyAgg = mconcat $ map (^. _1) typeList
+            insCtx = mconcat $ map (^. _3) typeList
+        rootFields <- combineRootFields $ map (^. _2) typeList
+        pure (tyAgg, rootFields, insCtx)
+
+    combineRootFields :: [RootFields] -> m RootFields
+    combineRootFields rootFields = do
+      let duplicateQueryFields = duplicates $
+            concatMap (Map.keys . rootQueryFields) rootFields
+          duplicateMutationFields = duplicates $
+            concatMap (Map.keys . rootMutationFields) rootFields
+
+      when (not $ null duplicateQueryFields) $
+        throw400 Unexpected $ "following query root fields are duplicated: "
+        <> showNames duplicateQueryFields
+
+      when (not $ null duplicateMutationFields) $
+        throw400 Unexpected $ "following mutation root fields are duplicated: "
+        <> showNames duplicateMutationFields
+
+      pure $ mconcat rootFields

 -- | build GraphQL schema from postgres tables and functions
 buildGCtxMapPG
@ -784,7 +801,7 @@ data RootFields

 instance Semigroup RootFields where
  RootFields a1 b1 <> RootFields a2 b2
-    = RootFields (Map.union a1 a2) (Map.union b1 b2)
+    = RootFields (a1 <> a2) (b1 <> b2)

 instance Monoid RootFields where
  mempty = RootFields Map.empty Map.empty
--- a/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs
+++ b/server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs
@ -29,6 +29,7 @@ import qualified Database.PG.Query                  as Q

 import           Data.Aeson

+import qualified Hasura.GraphQL.Context             as GC
 import qualified Hasura.GraphQL.Schema              as GS

 import           Hasura.Db
@ -153,6 +154,9 @@ buildSchemaCacheWithOptions withSetup = do
  -- remote schemas
  forM_ remoteSchemas resolveSingleRemoteSchema

+  -- validate tables' custom root fields
+  validateTablesCustomRootFields
+
  where
    permHelper setup sqlGenCtx qt rn pDef pa = do
      qCtx <- mkAdminQCtx sqlGenCtx <$> askSchemaCache
@ -180,6 +184,16 @@ buildSchemaCacheWithOptions withSetup = do
                            , scDefaultRemoteGCtx = mergedDefGCtx
                            }

+    validateTablesCustomRootFields = do
+      sc <- askSchemaCache
+      let tables = M.elems $ scTables sc
+          defRemoteGCtx = scDefaultRemoteGCtx sc
+      forM_ tables $ \table -> do
+        let GC.TableCustomRootFields sel selByPk selAgg ins upd del =
+              _tcCustomRootFields $ _tiCustomConfig table
+            rootFldNames = catMaybes [sel, selByPk, selAgg, ins, upd, del]
+        forM_ rootFldNames $ GS.checkConflictingNode defRemoteGCtx
+
 -- | Rebuilds the schema cache. If an object with the given object id became newly inconsistent,
 -- raises an error about it specifically. Otherwise, raises a generic metadata inconsistency error.
 buildSchemaCacheFor :: (CacheBuildM m) => MetadataObjId -> m ()
--- a/server/src-lib/Hasura/RQL/DDL/Schema/Table.hs
+++ b/server/src-lib/Hasura/RQL/DDL/Schema/Table.hs
@ -98,35 +98,6 @@ trackExistingTableOrViewP1 qt = do
  when (M.member qf $ scFunctions rawSchemaCache) $
    throw400 NotSupported $ "function with name " <> qt <<> " already exists"

-validateCustomRootFlds
-  :: (MonadError QErr m)
-  => GS.GCtx
-  -> GC.TableCustomRootFields
-  -> m ()
-validateCustomRootFlds defRemoteGCtx rootFlds =
-  forM_ rootFldNames $ GS.checkConflictingNode defRemoteGCtx
-  where
-    GC.TableCustomRootFields sel selByPk selAgg ins upd del = rootFlds
-    rootFldNames = catMaybes [sel, selByPk, selAgg, ins, upd, del]
-
-validateTableConfig
-  :: (QErrM m, CacheRM m)
-  => TableInfo a -> TableConfig -> m ()
-validateTableConfig tableInfo (TableConfig rootFlds colFlds) = do
-    withPathK "custom_root_fields" $ do
-      sc <- askSchemaCache
-      let defRemoteGCtx = scDefaultRemoteGCtx sc
-      validateCustomRootFlds defRemoteGCtx rootFlds
-    withPathK "custom_column_names" $
-      forM_ (M.toList colFlds) $ \(col, customName) -> do
-        void $ askPGColInfo (_tiFieldInfoMap tableInfo) col ""
-        withPathK (getPGColTxt col) $
-          checkForFieldConflict tableInfo $ FieldName $ G.unName customName
-        when (not $ null duplicateNames) $ throw400 NotSupported $
-          "the following names are duplicated: " <> showNames duplicateNames
-  where
-    duplicateNames = duplicates $ M.elems colFlds
-
 trackExistingTableOrViewP2
  :: (CacheBuildM m) => QualifiedTable -> SystemDefined -> Bool -> TableConfig -> m EncJSON
 trackExistingTableOrViewP2 tableName systemDefined isEnum config = do
@ -182,11 +153,20 @@ instance FromJSON SetTableCustomFields where
 runSetTableCustomFieldsQV2 :: (CacheBuildM m, UserInfoM m) => SetTableCustomFields -> m EncJSON
 runSetTableCustomFieldsQV2 (SetTableCustomFields tableName rootFields columnNames) = do
  adminOnly
-  void $ askTabInfo tableName
+  fields <- _tiFieldInfoMap <$> askTabInfo tableName
  let tableConfig = TableConfig rootFields columnNames
+  withPathK "custom_column_names" $ validateWithNonColumnFields fields
  updateTableConfig tableName tableConfig
  buildSchemaCacheFor (MOTable tableName)
  return successMsg
+  where
+    validateWithNonColumnFields fields = do
+      let customNames = M.elems columnNames
+          nonColumnFields = possibleNonColumnGraphQLFields fields
+          conflictingNames = customNames `intersect` nonColumnFields
+      when (not $ null conflictingNames) $ throw400 NotSupported $
+        "the following custom column names conflict with existing non-column fields: "
+        <> showNames conflictingNames

 unTrackExistingTableOrViewP1
  :: (CacheRM m, UserInfoM m, QErrM m) => UntrackTable -> m ()
@ -419,8 +399,9 @@ buildTableCache = processTableCache <=< buildRawTableCache
              , _tiDescription = maybeDesc
              }

-        -- validate tableConfig
-        withPathK "configuration" $ validateTableConfig info config
+        -- validate custom column names with existing columns
+        withPathK "configuration" $
+          validateWithExistingColumns columnFields $ _tcCustomColumnNames config
        pure (name, info)

    -- Step 2: Process the raw table cache to replace Postgres column types with logical column
@ -435,6 +416,23 @@ buildTableCache = processTableCache <=< buildRawTableCache
      where
        enumTables = M.mapMaybe _tiEnumValues rawTables

+    validateWithExistingColumns :: FieldInfoMap PGRawColumnInfo -> CustomColumnNames -> m ()
+    validateWithExistingColumns columnFields customColumnNames = do
+      withPathK "custom_column_names" $ do
+        -- Check all keys are valid columns
+        forM_ (M.keys customColumnNames) $ \col -> void $ askPGColInfo columnFields col ""
+        let columns = getCols columnFields
+            defaultNameMap = M.fromList $ flip map columns $
+              \col -> ( prciName col
+                      , G.Name $ getPGColTxt $ prciName col
+                      )
+            customNames = M.elems $ defaultNameMap `M.union` customColumnNames
+            conflictingCustomNames = duplicates customNames
+
+        when (not $ null conflictingCustomNames) $ throw400 NotSupported $
+          "the following custom column names are conflicting: " <> showNames conflictingCustomNames
+
+

 -- | “Processes” a 'PGRawColumnInfo' into a 'PGColumnInfo' by resolving its type using a map of known
 -- enum tables.
--- a/server/src-lib/Hasura/RQL/Types/SchemaCache.hs
+++ b/server/src-lib/Hasura/RQL/Types/SchemaCache.hs
@ -55,6 +55,7 @@ module Hasura.RQL.Types.SchemaCache
       , getCols
       , getRels
       , getComputedFieldInfos
+       , possibleNonColumnGraphQLFields

       , isPGColInfo
       , RelInfo(..)
@ -144,6 +145,7 @@ import           Language.Haskell.TH.Syntax        (Lift)
 import qualified Data.HashMap.Strict               as M
 import qualified Data.HashSet                      as HS
 import qualified Data.Text                         as T
+import qualified Language.GraphQL.Draft.Syntax     as G

 reportSchemaObjs :: [SchemaObjId] -> T.Text
 reportSchemaObjs = T.intercalate ", " . map reportSchemaObj
@ -176,6 +178,18 @@ $(makePrisms ''FieldInfo)

 type FieldInfoMap columnInfo = M.HashMap FieldName (FieldInfo columnInfo)

+possibleNonColumnGraphQLFields :: FieldInfoMap PGColumnInfo -> [G.Name]
+possibleNonColumnGraphQLFields fields =
+  flip concatMap (M.toList fields) $ \case
+    (_, FIColumn _)             -> []
+    (_, FIRelationship relInfo) ->
+      let relationshipName = G.Name $ relNameToTxt $ riName relInfo
+      in case riType relInfo of
+           ObjRel -> [relationshipName]
+           ArrRel -> [relationshipName, relationshipName <> "_aggregate"]
+    (_, FIComputedField info) ->
+      pure $ G.Name $ computedFieldNameToText $ _cfiName info
+
 getCols :: FieldInfoMap columnInfo -> [columnInfo]
 getCols = mapMaybe (^? _FIColumn) . M.elems

@ -366,17 +380,25 @@ $(makeLenses ''TableInfo)

 checkForFieldConflict
  :: (MonadError QErr m)
-  => TableInfo a
+  => TableInfo PGColumnInfo
  -> FieldName
  -> m ()
-checkForFieldConflict tabInfo f =
-  case M.lookup f (_tiFieldInfoMap tabInfo) of
+checkForFieldConflict tableInfo f = do
+  case M.lookup f fieldInfoMap of
    Just _ -> throw400 AlreadyExists $ mconcat
-      [ "column/relationship " <>> f
-      , " of table " <>> _tiName tabInfo
+      [ "column/relationship/computed field " <>> f
+      , " of table " <>> tableName
      , " already exists"
      ]
    Nothing -> return ()
+  when (f `elem` customColumnFields) $
+    throw400 AlreadyExists $
+    "custom column name " <> f <<> " of table " <> tableName <<> " already exists"
+  where
+    tableName = _tiName tableInfo
+    fieldInfoMap = _tiFieldInfoMap tableInfo
+    customColumnFields =
+      map (FieldName . G.unName . pgiName) $ getCols fieldInfoMap

 type TableCache columnInfo = M.HashMap QualifiedTable (TableInfo columnInfo) -- info of all tables
 type FunctionCache = M.HashMap QualifiedFunction FunctionInfo -- info of all functions
--- a/server/tests-py/queries/v1/computed_fields/add_computed_field_errors.yaml
+++ b/server/tests-py/queries/v1/computed_fields/add_computed_field_errors.yaml
@ -25,7 +25,7 @@
        function: full_name
  response:
    path: "$.args.name"
-    error: column/relationship "first_name" of table "author" already exists
+    error: column/relationship/computed field "first_name" of table "author" already exists
    code: already-exists

 - description: Try adding computed field with invalid function
--- a/server/tests-py/queries/v1/set_table_custom_fields/column_field_swap.yaml
+++ b/server/tests-py/queries/v1/set_table_custom_fields/column_field_swap.yaml
@ -0,0 +1,71 @@
+- description: Set custom column names for article table by swaping the column names
+  url: /v1/query
+  status: 200
+  response:
+    message: success
+  query:
+    type: set_table_custom_fields
+    version: 2
+    args:
+      table: article
+      custom_root_fields: {}
+      custom_column_names:
+        title: content
+        content: title
+
+- description: Perform graphql query
+  url: /v1/graphql
+  status: 200
+  response:
+    data:
+      article:
+      - id: 1
+        title: Article 1 content
+        content: Article 1 title
+      - id: 2
+        title: Article 2 content
+        content: Article 2 title
+  query:
+    query: |
+      query {
+        article{
+          id
+          title
+          content
+        }
+      }
+
+- description: Unset the custom column names
+  url: /v1/query
+  status: 200
+  response:
+    message: success
+  query:
+    type: set_table_custom_fields
+    version: 2
+    args:
+      table: article
+      custom_root_fields: {}
+      custom_column_names: {}
+
+- description: Peform graphql query
+  url: /v1/graphql
+  status: 200
+  response:
+    data:
+      article:
+      - id: 1
+        title: Article 1 title
+        content: Article 1 content
+      - id: 2
+        title: Article 2 title
+        content: Article 2 content
+  query:
+    query: |
+      query {
+        article{
+          id
+          title
+          content
+        }
+      }
--- a/server/tests-py/queries/v1/set_table_custom_fields/conflict_with_relationship.yaml
+++ b/server/tests-py/queries/v1/set_table_custom_fields/conflict_with_relationship.yaml
@ -0,0 +1,17 @@
+description: Set custom column names conflicting with existing relationship
+url: /v1/query
+status: 400
+response:
+  path: "$.args.custom_column_names"
+  error: 'the following custom column names conflict with existing non-column fields:
+    articles_aggregate, articles'
+  code: not-supported
+query:
+  type: set_table_custom_fields
+  version: 2
+  args:
+    table: author
+    custom_root_fields: {}
+    custom_column_names:
+      name: articles
+      age: articles_aggregate
--- a/server/tests-py/queries/v1/set_table_custom_fields/relationship_conflict_with_custom_column.yaml
+++ b/server/tests-py/queries/v1/set_table_custom_fields/relationship_conflict_with_custom_column.yaml
@ -0,0 +1,16 @@
+description: Try to define a relationship with custom column name
+url: /v1/query
+status: 400
+response:
+  path: "$.args"
+  error: custom column name "AuthorId" of table "author" already exists
+  code: already-exists
+query:
+  type: create_array_relationship
+  args:
+    name: AuthorId
+    table: author
+    using:
+      foreign_key_constraint_on:
+        table: article
+        column: author_id
--- a/server/tests-py/queries/v1/set_table_custom_fields/set_and_unset.yaml
+++ b/server/tests-py/queries/v1/set_table_custom_fields/set_and_unset.yaml
@ -14,6 +14,7 @@
      custom_column_names:
        id: AuthorId
        name: AuthorName
+        age: age

 - description: Check that above query has changed the schema
  url: /v1/graphql
--- a/server/tests-py/queries/v1/set_table_custom_fields/setup.yaml
+++ b/server/tests-py/queries/v1/set_table_custom_fields/setup.yaml
@ -14,6 +14,17 @@ args:
        ('Clarke',  23),
        ('Bellamy', NULL);

+      CREATE TABLE article (
+        id SERIAL PRIMARY KEY,
+        title TEXT NOT NULL,
+        content TEXT,
+        author_id INTEGER NOT NULL REFERENCES author(id)
+      );
+
+      INSERT INTO article (title, content, author_id) VALUES
+        ('Article 1 title', 'Article 1 content', 1),
+        ('Article 2 title', 'Article 2 content', 2);
+
 - type: track_table
  version: 2
  args:
@ -21,3 +32,20 @@ args:
    configuration:
      custom_root_fields:
        select: Authors
+      custom_column_names:
+        id: AuthorId
+
+- type: track_table
+  version: 2
+  args:
+    table: article
+    configuration: {}
+
+- type: create_array_relationship
+  args:
+    name: articles
+    table: author
+    using:
+      foreign_key_constraint_on:
+        table: article
+        column: author_id
--- a/server/tests-py/queries/v1/set_table_custom_fields/teardown.yaml
+++ b/server/tests-py/queries/v1/set_table_custom_fields/teardown.yaml
@ -3,5 +3,6 @@ args:
 - type: run_sql
  args:
    sql: |
+      DROP TABLE article;
      DROP TABLE author;
    cascade: true
--- a/server/tests-py/test_v1_queries.py
+++ b/server/tests-py/test_v1_queries.py
@ -686,6 +686,15 @@ class TestSetTableCustomFields(DefaultTestQueries):
    def test_alter_column(self, hge_ctx):
        check_query_f(hge_ctx, self.dir() + '/alter_column.yaml')

+    def test_conflict_with_relationship(self, hge_ctx):
+        check_query_f(hge_ctx, self.dir() + '/conflict_with_relationship.yaml')
+
+    def test_column_field_swap(self, hge_ctx):
+        check_query_f(hge_ctx, self.dir() + "/column_field_swap.yaml")
+
+    def test_relationship_conflict_with_custom_column(self, hge_ctx):
+        check_query_f(hge_ctx, self.dir() + "/relationship_conflict_with_custom_column.yaml")
+
 class TestComputedFields(DefaultTestQueries):
    @classmethod
    def dir(cls):