From 223ff9f0c3df740571744c39a53ad1b27f4efd28 Mon Sep 17 00:00:00 2001 From: Vamshi Surabhi Date: Thu, 11 Oct 2018 17:29:23 +0530 Subject: [PATCH 1/3] work around postgres's default limit of 63 chars for identifiers, closes #688 --- server/graphql-engine.cabal | 1 + server/src-lib/Hasura/RQL/DML/Select.hs | 43 +++++- server/src-lib/Hasura/SQL/DML.hs | 53 +++---- server/src-lib/Hasura/SQL/Rewrite.hs | 177 ++++++++++++++++++++++++ 4 files changed, 247 insertions(+), 27 deletions(-) create mode 100644 server/src-lib/Hasura/SQL/Rewrite.hs diff --git a/server/graphql-engine.cabal b/server/graphql-engine.cabal index d5a792eb3c647..93c6f31b3c1d1 100644 --- a/server/graphql-engine.cabal +++ b/server/graphql-engine.cabal @@ -206,6 +206,7 @@ library , Hasura.SQL.Value , Hasura.SQL.GeoJSON , Hasura.SQL.Time + , Hasura.SQL.Rewrite , Hasura.Prelude , Hasura.Logging , Ops diff --git a/server/src-lib/Hasura/RQL/DML/Select.hs b/server/src-lib/Hasura/RQL/DML/Select.hs index 7a909d487e296..9c9b326910082 100644 --- a/server/src-lib/Hasura/RQL/DML/Select.hs +++ b/server/src-lib/Hasura/RQL/DML/Select.hs @@ -22,6 +22,7 @@ import Hasura.Prelude import Hasura.RQL.DML.Internal import Hasura.RQL.GBoolExp import Hasura.RQL.Types +import Hasura.SQL.Rewrite (prefixNumToAliases) import Hasura.SQL.Types import qualified Database.PG.Query as Q @@ -134,10 +135,23 @@ mkBaseTableAls :: Iden -> Iden mkBaseTableAls pfx = pfx <> Iden ".base" -mkSelExtr +-- posttgres ignores anything beyond 63 chars for an iden +-- in this case, we'll need to use json_build_object function +-- json_build_object is slower than row_to_json hence it is only +-- used when needed +buildJsonObject :: Iden -> FieldName -> [(FieldName, AnnFld)] -> (S.Alias, S.SQLExp) -mkSelExtr pfx parAls flds = +buildJsonObject pfx parAls flds = + if any ( (> 63) . T.length . getFieldNameTxt . fst ) flds + then withJsonBuildObj pfx parAls flds + else withRowToJSON pfx parAls flds + +-- uses row_to_json to build a json object +withRowToJSON + :: Iden -> FieldName + -> [(FieldName, AnnFld)] -> (S.Alias, S.SQLExp) +withRowToJSON pfx parAls flds = (S.toAlias parAls, jsonRow) where withAls fldName sqlExp = @@ -153,6 +167,28 @@ mkSelExtr pfx parAls flds = ArrRel -> mkArrRelTableAls pfx parAls fldAls in S.mkQIdenExp qual fldAls +-- uses json_build_object to build a json object +withJsonBuildObj + :: Iden -> FieldName + -> [(FieldName, AnnFld)] -> (S.Alias, S.SQLExp) +withJsonBuildObj pfx parAls flds = + (S.toAlias parAls, jsonRow) + where + withAls fldName sqlExp = + [S.SELit $ getFieldNameTxt fldName, sqlExp] + + jsonRow = S.SEFnApp "json_build_object" (concatMap toFldExtr flds) Nothing + + toFldExtr (fldAls, fld) = withAls fldAls $ case fld of + FCol col -> toJSONableExp (pgiType col) $ + S.mkQIdenExp (mkBaseTableAls pfx) $ pgiName col + FExp e -> S.SELit e + FRel annRel -> + let qual = case arType annRel of + ObjRel -> mkObjRelTableAls pfx $ arName annRel + ArrRel -> mkArrRelTableAls pfx parAls fldAls + in S.mkQIdenExp qual fldAls + processAnnOrderByItem :: Iden -> AnnOrderByItem @@ -220,7 +256,7 @@ mkBaseNode pfx fldAls (AnnSel flds tn fromItemM fltr permLimitM tableArgs) = fromItem = fromMaybe (S.FISimple tn Nothing) fromItemM -- the selection extractor - selExtr = mkSelExtr pfx fldAls flds + selExtr = buildJsonObject pfx fldAls flds -- all the relationships (allObjs, allArrs) = @@ -611,6 +647,7 @@ getSelectDeps (AnnSel flds tn _ _ _ tableArgs) = mkSQLSelect :: Bool -> AnnSel -> S.Select mkSQLSelect isSingleObject annSel = + prefixNumToAliases $ if isSingleObject then asSingleRow rootAls rootSelAsSubQuery else withJsonAgg Nothing rootAls rootSelAsSubQuery diff --git a/server/src-lib/Hasura/SQL/DML.hs b/server/src-lib/Hasura/SQL/DML.hs index d162abb340e46..22abf8590b00c 100644 --- a/server/src-lib/Hasura/SQL/DML.hs +++ b/server/src-lib/Hasura/SQL/DML.hs @@ -337,9 +337,10 @@ instance ToSQL Extractor where toSQL (Extractor ce mal) = toSQL ce <-> toSQL mal -data DistinctExpr = DistinctSimple - | DistinctOn ![SQLExp] - deriving (Show, Eq) +data DistinctExpr + = DistinctSimple + | DistinctOn ![SQLExp] + deriving (Show, Eq) instance ToSQL DistinctExpr where toSQL DistinctSimple = BB.string7 "DISTINCT" @@ -377,11 +378,12 @@ instance ToSQL Lateral where toSQL (Lateral False) = mempty data JoinExpr - = JoinExpr { tjeLeft :: !FromItem - , tjeType :: !JoinType - , tjeRight :: !FromItem - , tjeJC :: !JoinCond - } deriving (Show, Eq) + = JoinExpr + { tjeLeft :: !FromItem + , tjeType :: !JoinType + , tjeRight :: !FromItem + , tjeJC :: !JoinCond + } deriving (Show, Eq) instance ToSQL JoinExpr where toSQL je = @@ -390,11 +392,12 @@ instance ToSQL JoinExpr where <-> toSQL (tjeRight je) <-> toSQL (tjeJC je) -data JoinType = Inner - | LeftOuter - | RightOuter - | FullOuter - deriving (Eq, Show) +data JoinType + = Inner + | LeftOuter + | RightOuter + | FullOuter + deriving (Eq, Show) instance ToSQL JoinType where toSQL Inner = BB.string7 "INNER JOIN" @@ -402,9 +405,10 @@ instance ToSQL JoinType where toSQL RightOuter = BB.string7 "RIGHT OUTER JOIN" toSQL FullOuter = BB.string7 "FULL OUTER JOIN" -data JoinCond = JoinOn !BoolExp - | JoinUsing ![PGCol] - deriving (Show, Eq) +data JoinCond + = JoinOn !BoolExp + | JoinUsing ![PGCol] + deriving (Show, Eq) instance ToSQL JoinCond where toSQL (JoinOn be) = @@ -412,14 +416,15 @@ instance ToSQL JoinCond where toSQL (JoinUsing cols) = BB.string7 "USING" <-> paren ("," <+> cols) -data BoolExp = BELit !Bool - | BEBin !BinOp !BoolExp !BoolExp - | BENot !BoolExp - | BECompare !CompareOp !SQLExp !SQLExp - | BENull !SQLExp - | BENotNull !SQLExp - | BEExists !Select - deriving (Show, Eq) +data BoolExp + = BELit !Bool + | BEBin !BinOp !BoolExp !BoolExp + | BENot !BoolExp + | BECompare !CompareOp !SQLExp !SQLExp + | BENull !SQLExp + | BENotNull !SQLExp + | BEExists !Select + deriving (Show, Eq) -- removes extraneous 'AND true's simplifyBoolExp :: BoolExp -> BoolExp diff --git a/server/src-lib/Hasura/SQL/Rewrite.hs b/server/src-lib/Hasura/SQL/Rewrite.hs new file mode 100644 index 0000000000000..3cec584af6623 --- /dev/null +++ b/server/src-lib/Hasura/SQL/Rewrite.hs @@ -0,0 +1,177 @@ +{-# LANGUAGE LambdaCase #-} +{-# LANGUAGE MultiWayIf #-} +{-# LANGUAGE OverloadedStrings #-} + +module Hasura.SQL.Rewrite + ( prefixNumToAliases + ) where + +import qualified Data.HashMap.Strict as Map +import qualified Data.Text as T +import Hasura.Prelude +import qualified Hasura.SQL.DML as S +import Hasura.SQL.Types (Iden (..)) + + +-- add an int as a prefix to all aliases. +-- This is needed in cases identifiers exceed 63 chars +-- as postgres only considers first 63 chars of +-- an identifier +prefixNumToAliases :: S.Select -> S.Select +prefixNumToAliases s = + uSelect s `evalState` UniqSt 0 Map.empty + +type Rewrite a = State a + +data UniqSt + = UniqSt + { _uqVar :: !Int + , _uqIdens :: !(Map.HashMap Iden Int) + } deriving (Show, Eq) + +type Uniq = Rewrite UniqSt + +withNumPfx :: Iden -> Int -> Iden +withNumPfx iden i = + Iden pfx <> iden + where + pfx = T.pack $ "_" <> show i <> "_" + +addAlias :: S.Alias -> Uniq S.Alias +addAlias (S.Alias iden) = do + UniqSt var idens <- get + put $ UniqSt (var + 1) $ Map.insert iden var idens + return $ S.Alias $ withNumPfx iden var + +getIden :: Iden -> Uniq Iden +getIden iden = do + UniqSt _ idens <- get + let varNumM = Map.lookup iden idens + return $ maybe iden (withNumPfx iden) varNumM + +restoringIdens :: Uniq a -> Uniq a +restoringIdens action = do + UniqSt _ idens <- get + res <- action + -- restore the idens to before the action + modify' $ \s -> s { _uqIdens = idens } + return res + +uSelect :: S.Select -> Uniq S.Select +uSelect sel = do + -- this has to be the first thing to process + newFromM <- mapM uFromExp fromM + + newWhereM <- forM whereM $ + \(S.WhereFrag be) -> S.WhereFrag <$> uBoolExp be + newGrpM <- forM grpM $ + \(S.GroupByExp l) -> S.GroupByExp <$> mapM uSqlExp l + newHavnM <- forM havnM $ + \(S.HavingExp be) -> S.HavingExp <$> uBoolExp be + newOrdM <- mapM uOrderBy ordByM + newDistM <- mapM uDistinct distM + newExtrs <- mapM uExtractor extrs + return $ S.Select newDistM newExtrs newFromM newWhereM newGrpM + newHavnM newOrdM limitM offM + where + S.Select distM extrs fromM whereM grpM havnM ordByM limitM offM = sel + uDistinct = \case + S.DistinctSimple -> return S.DistinctSimple + S.DistinctOn l -> S.DistinctOn <$> mapM uSqlExp l + uExtractor (S.Extractor e alM) = + S.Extractor <$> uSqlExp e <*> return alM + +uFromExp :: S.FromExp -> Uniq S.FromExp +uFromExp (S.FromExp fromItems) = + S.FromExp <$> mapM uFromItem fromItems + +uFromItem :: S.FromItem -> Uniq S.FromItem +uFromItem fromItem = case fromItem of + S.FISimple t alM -> + S.FISimple t <$> mapM addAlias alM + S.FIIden iden -> + S.FIIden <$> return iden + S.FISelect isLateral sel al -> do + -- we are kind of ignoring if we have to reset + -- idens to empty based on correlation + -- unless isLateral $ modify' $ \s -> s { _uqIdens = Map.empty} + newSel <- restoringIdens $ uSelect sel + newAls <- addAlias al + return $ S.FISelect isLateral newSel newAls + S.FIJoin joinExp -> + S.FIJoin <$> uJoinExp joinExp + +uJoinExp :: S.JoinExpr -> Uniq S.JoinExpr +uJoinExp (S.JoinExpr left ty right joinCond) = do + leftN <- uFromItem left + rightN <- uFromItem right + S.JoinExpr leftN ty rightN <$> uJoinCond joinCond + +uJoinCond :: S.JoinCond -> Uniq S.JoinCond +uJoinCond joinCond = case joinCond of + S.JoinOn be -> S.JoinOn <$> uBoolExp be + S.JoinUsing cols -> return $ S.JoinUsing cols + +uBoolExp :: S.BoolExp -> Uniq S.BoolExp +uBoolExp = restoringIdens . \case + S.BELit b -> return $ S.BELit b + S.BEBin op left right -> + S.BEBin <$> return op <*> uBoolExp left <*> uBoolExp right + S.BENot b -> S.BENot <$> uBoolExp b + S.BECompare op left right -> + S.BECompare <$> return op <*> uSqlExp left <*> uSqlExp right + S.BENull e -> S.BENull <$> uSqlExp e + S.BENotNull e -> S.BENotNull <$> uSqlExp e + S.BEExists sel -> S.BEExists <$> uSelect sel + +uOrderBy :: S.OrderByExp -> Uniq S.OrderByExp +uOrderBy (S.OrderByExp ordByItems) = + S.OrderByExp <$> mapM uOrderByItem ordByItems + where + uOrderByItem (S.OrderByItem e ordTyM nullsOrdM) = + S.OrderByItem + <$> uSqlExp e + <*> return ordTyM + <*> return nullsOrdM + +uSqlExp :: S.SQLExp -> Uniq S.SQLExp +uSqlExp = restoringIdens . \case + S.SEPrep i -> return $ S.SEPrep i + S.SELit t -> return $ S.SELit t + S.SEUnsafe t -> return $ S.SEUnsafe t + S.SESelect s -> S.SESelect <$> uSelect s + S.SEStar -> return S.SEStar + -- this is for row expressions + -- todo: check if this is always okay + S.SEIden iden -> S.SEIden <$> getIden iden + S.SEQIden (S.QIden qual iden) -> do + newQual <- uQual qual + return $ S.SEQIden $ S.QIden newQual iden + S.SEFnApp fn args ordByM -> + S.SEFnApp + <$> return fn + <*> mapM uSqlExp args + <*> mapM uOrderBy ordByM + S.SEOpApp op args -> + S.SEOpApp op + <$> mapM uSqlExp args + S.SETyAnn e ty -> + S.SETyAnn + <$> uSqlExp e + <*> return ty + S.SECond be onTrue onFalse -> + S.SECond + <$> uBoolExp be + <*> uSqlExp onTrue + <*> uSqlExp onFalse + S.SEBool be -> + S.SEBool <$> uBoolExp be + S.SEExcluded t -> + S.SEExcluded <$> return t + S.SEArray l -> + S.SEArray <$> mapM uSqlExp l + where + uQual = \case + S.QualIden iden -> S.QualIden <$> getIden iden + S.QualTable t -> return $ S.QualTable t + S.QualVar t -> return $ S.QualVar t From efa4abecfac1e7fdd114f159617a9ee86183b93b Mon Sep 17 00:00:00 2001 From: Vamshi Surabhi Date: Thu, 11 Oct 2018 18:36:03 +0530 Subject: [PATCH 2/3] add new row expn constructor for easier rewrite rules --- server/src-lib/Hasura/RQL/DML/Select.hs | 11 ++++++----- server/src-lib/Hasura/SQL/DML.hs | 6 +++++- server/src-lib/Hasura/SQL/Rewrite.hs | 3 ++- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/server/src-lib/Hasura/RQL/DML/Select.hs b/server/src-lib/Hasura/RQL/DML/Select.hs index 9c9b326910082..dec13f1d3996a 100644 --- a/server/src-lib/Hasura/RQL/DML/Select.hs +++ b/server/src-lib/Hasura/RQL/DML/Select.hs @@ -649,13 +649,14 @@ mkSQLSelect :: Bool -> AnnSel -> S.Select mkSQLSelect isSingleObject annSel = prefixNumToAliases $ if isSingleObject - then asSingleRow rootAls rootSelAsSubQuery - else withJsonAgg Nothing rootAls rootSelAsSubQuery + then asSingleRow rootFldAls rootSelAsSubQuery + else withJsonAgg Nothing rootFldAls rootSelAsSubQuery where - rootSelAsSubQuery = S.mkSelFromItem rootSel rootAls - rootAls = S.Alias $ Iden "root" + rootSelAsSubQuery = S.mkSelFromItem rootSel $ S.Alias $ Iden "root_alias" rootSel = baseNodeToSel (S.BELit True) rootNode - rootNode = mkBaseNode (Iden "root") (FieldName "root") annSel + rootFldName = FieldName "root" + rootFldAls = S.Alias $ toIden rootFldName + rootNode = mkBaseNode (Iden "root") rootFldName annSel asSingleRow :: S.Alias -> S.FromItem -> S.Select asSingleRow col fromItem = diff --git a/server/src-lib/Hasura/SQL/DML.hs b/server/src-lib/Hasura/SQL/DML.hs index 22abf8590b00c..ad5f758b77c9f 100644 --- a/server/src-lib/Hasura/SQL/DML.hs +++ b/server/src-lib/Hasura/SQL/DML.hs @@ -132,7 +132,7 @@ mkRowExp extrs = let -- SELECT r FROM (SELECT col1, col2, .. ) AS r outerSel = mkSelect - { selExtr = [mkExtr innerSelName] + { selExtr = [Extractor (SERowIden $ toIden innerSelName) Nothing] , selFrom = Just $ FromExp [mkSelFromExp False innerSel innerSelName] } @@ -245,6 +245,8 @@ data SQLExp | SESelect !Select | SEStar | SEIden !Iden + -- iden and row identifier are distinguished for easier rewrite rules + | SERowIden !Iden | SEQIden !QIden | SEFnApp !T.Text ![SQLExp] !(Maybe OrderByExp) | SEOpApp !SQLOp ![SQLExp] @@ -281,6 +283,8 @@ instance ToSQL SQLExp where BB.char7 '*' toSQL (SEIden iden) = toSQL iden + toSQL (SERowIden iden) = + toSQL iden toSQL (SEQIden qIden) = toSQL qIden -- https://www.postgresql.org/docs/10/static/sql-expressions.html#SYNTAX-AGGREGATES diff --git a/server/src-lib/Hasura/SQL/Rewrite.hs b/server/src-lib/Hasura/SQL/Rewrite.hs index 3cec584af6623..51d7de8b83e34 100644 --- a/server/src-lib/Hasura/SQL/Rewrite.hs +++ b/server/src-lib/Hasura/SQL/Rewrite.hs @@ -143,7 +143,8 @@ uSqlExp = restoringIdens . \case S.SEStar -> return S.SEStar -- this is for row expressions -- todo: check if this is always okay - S.SEIden iden -> S.SEIden <$> getIden iden + S.SEIden iden -> return $ S.SEIden iden + S.SERowIden iden -> S.SERowIden <$> getIden iden S.SEQIden (S.QIden qual iden) -> do newQual <- uQual qual return $ S.SEQIden $ S.QIden newQual iden From f6d68b6c5e53e7829d69fd165b5f8681efdcc491 Mon Sep 17 00:00:00 2001 From: Vamshi Surabhi Date: Fri, 12 Oct 2018 12:52:16 +0530 Subject: [PATCH 3/3] add test case for deeply nested query to generate aliases > 63 chars --- .../basic/nested_select_query_deep.yaml | 39 +++++++++++++++++++ server/tests-py/test_graphql_queries.py | 7 +++- 2 files changed, 44 insertions(+), 2 deletions(-) create mode 100644 server/tests-py/queries/graphql_query/basic/nested_select_query_deep.yaml diff --git a/server/tests-py/queries/graphql_query/basic/nested_select_query_deep.yaml b/server/tests-py/queries/graphql_query/basic/nested_select_query_deep.yaml new file mode 100644 index 0000000000000..73d1728059cab --- /dev/null +++ b/server/tests-py/queries/graphql_query/basic/nested_select_query_deep.yaml @@ -0,0 +1,39 @@ +description: Nested select on article +url: /v1alpha1/graphql +status: 200 +response: + data: + article: + - author: + articles: + - author: + articles: + - author: {id: 1} + id: 1 + id: 1 + id: 1 + id: 1 + id: 1 + +query: + query: | + query { + article(where: {id: {_eq: 1}}) { + id + author { + id + articles(where: {id: {_eq: 1}}) { + id + author { + id + articles(where: {id: {_eq: 1}}) { + id + author { + id + } + } + } + } + } + } + } diff --git a/server/tests-py/test_graphql_queries.py b/server/tests-py/test_graphql_queries.py index 30e1454663d70..c2e007ee1f976 100644 --- a/server/tests-py/test_graphql_queries.py +++ b/server/tests-py/test_graphql_queries.py @@ -16,6 +16,9 @@ def test_select_query_where(self, hge_ctx): def test_nested_select_query_article_author(self, hge_ctx): check_query_f(hge_ctx, self.dir + '/nested_select_query_article_author.yaml') + def test_nested_select_query_deep(self, hge_ctx): + check_query_f(hge_ctx, self.dir + '/nested_select_query_deep.yaml') + def test_nested_select_query_where(self, hge_ctx): check_query_f(hge_ctx, self.dir + '/nested_select_where_query_author_article.yaml') @@ -23,7 +26,7 @@ def test_select_query_user(self, hge_ctx): check_query_f(hge_ctx, "queries/graphql_query/basic/select_query_user.yaml") @pytest.fixture(autouse=True) - def transact(self, request, hge_ctx): + def transact(self, request, hge_ctx): self.dir = 'queries/graphql_query/basic' st_code, resp = hge_ctx.v1q_f(self.dir + '/setup.yaml') assert st_code == 200, resp @@ -46,7 +49,7 @@ def test_err_neg_limit_error(self, hge_ctx): check_query_f(hge_ctx, self.dir + '/select_query_article_neg_limit_error.yaml') @pytest.fixture(autouse=True) - def transact(self, request, hge_ctx): + def transact(self, request, hge_ctx): self.dir = 'queries/graphql_query/limits' st_code, resp = hge_ctx.v1q_f(self.dir + '/setup.yaml') assert st_code == 200, resp