From d1cf004cf3da1a20d1c645a0e351c55822663084 Mon Sep 17 00:00:00 2001 From: rakeshkky <12475069+rakeshkky@users.noreply.github.com> Date: Mon, 29 Jul 2019 19:59:28 +0530 Subject: [PATCH 1/3] don't use sub-query for permission limit if aggregations are not present --- .../src-lib/Hasura/RQL/DML/Select/Internal.hs | 90 ++++++++++----- server/stack.yaml.lock | 103 ++++++++++++++++++ 2 files changed, 168 insertions(+), 25 deletions(-) create mode 100644 server/stack.yaml.lock diff --git a/server/src-lib/Hasura/RQL/DML/Select/Internal.hs b/server/src-lib/Hasura/RQL/DML/Select/Internal.hs index ad2242cc5be23..ab29dd06ca5e6 100644 --- a/server/src-lib/Hasura/RQL/DML/Select/Internal.hs +++ b/server/src-lib/Hasura/RQL/DML/Select/Internal.hs @@ -78,10 +78,15 @@ asSingleRowExtr col = , S.SEUnsafe "0" ] -withJsonAggExtr :: Maybe Int -> Maybe S.OrderByExp -> S.Alias -> S.SQLExp -withJsonAggExtr permLimitM ordBy alias = - maybe (mkSimpleJsonAgg rowIdenExp ordBy) withPermLimit permLimitM +withJsonAggExtr + :: Bool -> Maybe Int -> Maybe S.OrderByExp -> S.Alias -> S.SQLExp +withJsonAggExtr hasAggSel permLimitM ordBy alias = + -- if select has aggregations and permission limit is defined + -- then use subquery to apply permission limit + if hasAggSel then maybe simpleJsonAgg withPermLimit permLimitM + else simpleJsonAgg where + simpleJsonAgg = mkSimpleJsonAgg rowIdenExp ordBy rowIdenExp = S.SEIden $ S.getAlias alias subSelAls = Iden "sub_query" unnestTable = Iden "unnest_table" @@ -130,10 +135,12 @@ withJsonAggExtr permLimitM ordBy alias = asJsonAggExtr - :: Bool -> S.Alias -> Maybe Int -> Maybe S.OrderByExp -> S.Extractor -asJsonAggExtr singleObj als permLimit ordByExpM = + :: Bool -> S.Alias -> Bool -> Maybe Int -> Maybe S.OrderByExp -> S.Extractor +asJsonAggExtr singleObj als hasAggSel permLimit ordByExpM = flip S.Extractor (Just als) $ - bool (withJsonAggExtr permLimit ordByExpM als) (asSingleRowExtr als) singleObj + bool (withJsonAggExtr hasAggSel permLimit ordByExpM als) + (asSingleRowExtr als) + singleObj -- array relationships are not grouped, so have to be prefixed by -- parent's alias @@ -316,7 +323,8 @@ processAnnOrderByCol pfx parAls arrRelCtx strfyNum = \case tabPerm = TablePerm relFltr Nothing (extr, arrFlds) = mkAggObExtrAndFlds annAggOb selFld = TAFAgg arrFlds - bn = mkBaseNode arrPfx fldName selFld tabFrom tabPerm noTableArgs strfyNum + bn = mkBaseNode False arrPfx fldName selFld tabFrom + tabPerm noTableArgs strfyNum aggNode = ArrNode [extr] colMapping $ mergeBaseNodes bn $ mkEmptyBaseNode arrPfx tabFrom obAls = arrPfx <> Iden "." <> toIden fldName @@ -367,16 +375,22 @@ aggSelToArrNode pfx als aggSel = mergedBN = foldr mergeBaseNodes emptyBN allBNs mkAggBaseNode (fn, selFld) = - mkBaseNode pfx fn selFld tabFrm tabPerm tabArgs strfyNum + mkBaseNode hasAggSel pfx fn selFld tabFrm tabPerm tabArgs strfyNum selFldToExtr (FieldName t, fld) = (:) (S.SELit t) $ pure $ case fld of TAFAgg flds -> aggFldToExp flds - TAFNodes _ -> withJsonAggExtr permLimit ordBy $ S.Alias $ Iden t + TAFNodes _ -> + withJsonAggExtr hasAggSel permLimit ordBy $ S.Alias $ Iden t TAFExp e -> -- bool_or to force aggregation S.SEFnApp "coalesce" [ S.SELit e , S.SEUnsafe "bool_or('true')::text"] Nothing + hasAggSel = any (isTabAggFld . snd) aggFlds + + isTabAggFld (TAFAgg _) = True + isTabAggFld _ = False + mkArrNodePfx :: Iden -> FieldName @@ -458,14 +472,36 @@ mkOrdByItems pfx fldAls orderByM strfyNum arrRelCtx = getOrdByAggNode _ = Nothing mkBaseNode - :: Iden -> FieldName -> TableAggFld -> TableFrom - -> TablePerm -> TableArgs -> Bool -> BaseNode -mkBaseNode pfx fldAls annSelFlds tableFrom tablePerm tableArgs strfyNum = - BaseNode pfx distExprM fromItem finalWhere ordByExpM limitM offsetM + :: Bool + -> Iden + -> FieldName + -> TableAggFld + -> TableFrom + -> TablePerm + -> TableArgs + -> Bool + -> BaseNode +mkBaseNode hasAggSel pfx fldAls annSelFlds tableFrom + tablePerm tableArgs strfyNum = + BaseNode pfx distExprM fromItem finalWhere ordByExpM finalLimit offsetM allExtrs allObjsWithOb allArrsWithOb where - fltr = _tpFilter tablePerm - TableArgs whereM orderByM limitM offsetM distM = tableArgs + TablePerm permFilter permLimit = tablePerm + TableArgs whereM orderByM inpLimitM offsetM distM = tableArgs + + -- if selection has aggregations then only use input limit + -- else compare input and permission limits + finalLimit = + if hasAggSel then inpLimitM + else withPermLimit + + withPermLimit = + case (inpLimitM, permLimit) of + (inpLim, Nothing) -> inpLim + (Nothing, permLim) -> permLim + (Just i, Just p) -> Just $ if i < p then i else p + + aggOrdByRelNames = fetchOrdByAggRels orderByM (allExtrs, allObjsWithOb, allArrsWithOb, ordByExpM) = @@ -478,8 +514,9 @@ mkBaseNode pfx fldAls annSelFlds tableFrom tablePerm tableArgs strfyNum = objNodes = HM.fromListWith mergeObjNodes $ map mkObjItem (mapMaybe getAnnObj flds) -- all array items (array relationships + aggregates) + hasArrAggSel = any (isArrFldAgg . snd) arrFlds arrNodes = HM.fromListWith mergeArrNodes $ - map (mkArrItem arrRelCtx) arrFlds + map (mkArrItem hasArrAggSel arrRelCtx) arrFlds (obExtrs, ordByObjs, ordByArrs, obeM) = mkOrdByItems' arrRelCtx @@ -523,8 +560,8 @@ mkBaseNode pfx fldAls annSelFlds tableFrom tablePerm tableArgs strfyNum = in Just (S.Alias colAls, qualCol) mkColExp _ = Nothing - finalWhere = - toSQLBoolExp tableQual $ maybe fltr (andAnnBoolExps fltr) whereM + finalWhere = toSQLBoolExp tableQual $ + maybe permFilter (andAnnBoolExps permFilter) whereM fromItem = tableFromToFromItem tableFrom tableQual = tableFromToQual tableFrom @@ -544,10 +581,10 @@ mkBaseNode pfx fldAls annSelFlds tableFrom tablePerm tableArgs strfyNum = in (relName, objNode) -- process an array/array-aggregate item - mkArrItem arrRelCtx (fld, arrSel) = + mkArrItem hasArrAggSel arrRelCtx (fld, arrSel) = let (arrAls, arrPfx) = mkArrNodePfx pfx fldAls arrRelCtx $ ANIField (fld, arrSel) - arrNode = mkArrNode arrPfx (fld, arrSel) + arrNode = mkArrNode hasArrAggSel arrPfx (fld, arrSel) in (arrAls, arrNode) getAnnObj (f, annFld) = case annFld of @@ -558,9 +595,12 @@ mkBaseNode pfx fldAls annSelFlds tableFrom tablePerm tableArgs strfyNum = FArr ar -> Just (f, ar) _ -> Nothing + isArrFldAgg (ASAgg _) = True + isArrFldAgg (ASSimple _) = False + annSelToBaseNode :: Iden -> FieldName -> AnnSimpleSel -> BaseNode annSelToBaseNode pfx fldAls annSel = - mkBaseNode pfx fldAls (TAFNodes selFlds) tabFrm tabPerm tabArgs strfyNum + mkBaseNode False pfx fldAls (TAFNodes selFlds) tabFrm tabPerm tabArgs strfyNum where AnnSelG selFlds tabFrm tabPerm tabArgs strfyNum = annSel @@ -568,12 +608,12 @@ mkObjNode :: Iden -> (FieldName, ObjSel) -> ObjNode mkObjNode pfx (fldName, AnnRelG _ rMapn rAnnSel) = ObjNode rMapn $ annSelToBaseNode pfx fldName rAnnSel -mkArrNode :: Iden -> (FieldName, ArrSel) -> ArrNode -mkArrNode pfx (fldName, annArrSel) = case annArrSel of +mkArrNode :: Bool -> Iden -> (FieldName, ArrSel) -> ArrNode +mkArrNode hasAggSel pfx (fldName, annArrSel) = case annArrSel of ASSimple annArrRel -> let bn = annSelToBaseNode pfx fldName $ aarAnnSel annArrRel permLimit = getPermLimit $ aarAnnSel annArrRel - extr = asJsonAggExtr False (S.toAlias fldName) permLimit $ + extr = asJsonAggExtr False (S.toAlias fldName) hasAggSel permLimit $ _bnOrderBy bn in ArrNode [extr] (aarMapping annArrRel) bn @@ -650,7 +690,7 @@ mkSQLSelect isSingleObject annSel = prefixNumToAliases $ arrNodeToSelect baseNode extrs $ S.BELit True where permLimit = getPermLimit annSel - extrs = pure $ asJsonAggExtr isSingleObject rootFldAls permLimit + extrs = pure $ asJsonAggExtr isSingleObject rootFldAls False permLimit $ _bnOrderBy baseNode baseNode = annSelToBaseNode (toIden rootFldName) rootFldName annSel rootFldName = FieldName "root" diff --git a/server/stack.yaml.lock b/server/stack.yaml.lock new file mode 100644 index 0000000000000..5367dd13598e0 --- /dev/null +++ b/server/stack.yaml.lock @@ -0,0 +1,103 @@ +# This file was autogenerated by Stack. +# You should not edit this file by hand. +# For more information, please see the documentation at: +# https://docs.haskellstack.org/en/stable/lock_files + +packages: +- completed: + cabal-file: + size: 2829 + sha256: 0ef7cf19d08caa11c690a732f14e4b7159c46cdda1d9e66dceb6d4bedd9fd9c1 + name: pg-client + version: 0.1.0 + git: https://github.com/hasura/pg-client-hs.git + pantry-tree: + size: 1051 + sha256: fbab919039158f3714c3f0ac3d605d3fb9c85061517b4b9fc67147403b0b9975 + commit: 16f27a134f12c4f24fee19d57f0ca179bdb4135b + original: + git: https://github.com/hasura/pg-client-hs.git + commit: 16f27a134f12c4f24fee19d57f0ca179bdb4135b +- completed: + cabal-file: + size: 3295 + sha256: 5c53f2e67996f9e3f85002f95a929a04b85a44e00ffbf1b7a8c034c5098aa630 + name: graphql-parser + version: 0.1.0.0 + git: https://github.com/hasura/graphql-parser-hs.git + pantry-tree: + size: 1771 + sha256: 6e506a5dbf965325c515efc523e63bbc15640c994a5dbe21448d328c7ba619a1 + commit: 1ccdbb4c4d743b679f3141992df39feaee971640 + original: + git: https://github.com/hasura/graphql-parser-hs.git + commit: 1ccdbb4c4d743b679f3141992df39feaee971640 +- completed: + cabal-file: + size: 1253 + sha256: d0356b65cbe17a2fecf9334aa133de238161ef9c22af015d67ab622e08eb8ed1 + name: ci-info + version: 0.1.0.0 + git: https://github.com/hasura/ci-info-hs.git + pantry-tree: + size: 512 + sha256: 78d311887af8c825aa21563635fbe31503febcc490e0e5d81919835d83ef9046 + commit: ad6df731584dc89b72a6e131687d37ef01714fe8 + original: + git: https://github.com/hasura/ci-info-hs.git + commit: ad6df731584dc89b72a6e131687d37ef01714fe8 +- completed: + hackage: ginger-0.8.4.0@sha256:21c3051af3c90af39c40a50400c9a1a0fcccb544528e37cde30bdd30048437d8,3151 + pantry-tree: + size: 1375 + sha256: f8a7cb091ea4d8011bd530f83a22941a009d97ee7ccd4c93d0528aa72f3636ea + original: + hackage: ginger-0.8.4.0 +- completed: + hackage: select-0.4.0.1@sha256:d409315752a069693bdd4169fa9a8ea7777d814da77cd8604f367cf0741de295,2492 + pantry-tree: + size: 1256 + sha256: b6ae36ccba2a7cdd1ad130575931364002682e532d7043da62771e58294ddb7a + original: + hackage: select-0.4.0.1 +- completed: + hackage: primitive-extras-0.7.1@sha256:23905c57089418b1a2d324cfee3e81bbd5a344a0fa56a827867b2dce275fdb5e,2945 + pantry-tree: + size: 1181 + sha256: f788dae21ca4f0d0377d97a5ba7fd008d4b0af8991a745f94c29f2caa11dc6bd + original: + hackage: primitive-extras-0.7.1 +- completed: + hackage: stm-hamt-1.2.0.2@sha256:18126db7bf2d9c967a6020c677b3005dd957a4c39d69aeaea3c29c90de8f6124,3972 + pantry-tree: + size: 1009 + sha256: ef426797655d6b4b9238b1200c4129d44e91f996f26b92a035b1333b8b8a6f62 + original: + hackage: stm-hamt-1.2.0.2 +- completed: + hackage: stm-containers-1.1.0.4@sha256:f83a683357b6e3b1dda3e70d2077a37224ed534df1f74c4e11f3f6daa7945c5b,3248 + pantry-tree: + size: 761 + sha256: 059c5a2d657d392aca0a887648f57380d6321734dc8879c056a44d4414308ac6 + original: + hackage: stm-containers-1.1.0.4 +- completed: + hackage: reroute-0.5.0.0@sha256:3360747cdc700c9808a38bff48b75926efa443d4af282396082329a218a8d9d3,2446 + pantry-tree: + size: 660 + sha256: 52afcff0a5dba2fb746be4fa8cfa56cf774272872b61130537fe0e7ad463c0cd + original: + hackage: reroute-0.5.0.0 +- completed: + hackage: Spock-core-0.13.0.0@sha256:06e007f23c47bdda52d2927da54160d73f1b6f51a977f3ca9087275698db8f0a,3400 + pantry-tree: + size: 1113 + sha256: 86140298020f68bb09d07b26a6a6f1666fc3a02715d7986b09150727247a1a84 + original: + hackage: Spock-core-0.13.0.0 +snapshots: +- completed: + size: 498167 + url: https://raw.githubusercontent.com/commercialhaskell/stackage-snapshots/master/lts/13/20.yaml + sha256: cda928d57b257a5f17bcad796843c9daa674fef47d600dbea3aa7b0e49d64a11 + original: lts-13.20 From 03e08404b6942053b538c35c5b7e4913b3279c68 Mon Sep 17 00:00:00 2001 From: rakeshkky <12475069+rakeshkky@users.noreply.github.com> Date: Tue, 30 Jul 2019 16:02:42 +0530 Subject: [PATCH 2/3] use subquery for for array relation select if node underlying is being merged with aggregate select one --- .../src-lib/Hasura/RQL/DML/Select/Internal.hs | 92 ++++++++++--------- server/src-lib/Hasura/RQL/DML/Select/Types.hs | 7 ++ 2 files changed, 56 insertions(+), 43 deletions(-) diff --git a/server/src-lib/Hasura/RQL/DML/Select/Internal.hs b/server/src-lib/Hasura/RQL/DML/Select/Internal.hs index ab29dd06ca5e6..2e2f9debe6035 100644 --- a/server/src-lib/Hasura/RQL/DML/Select/Internal.hs +++ b/server/src-lib/Hasura/RQL/DML/Select/Internal.hs @@ -80,10 +80,9 @@ asSingleRowExtr col = withJsonAggExtr :: Bool -> Maybe Int -> Maybe S.OrderByExp -> S.Alias -> S.SQLExp -withJsonAggExtr hasAggSel permLimitM ordBy alias = - -- if select has aggregations and permission limit is defined - -- then use subquery to apply permission limit - if hasAggSel then maybe simpleJsonAgg withPermLimit permLimitM +withJsonAggExtr subQueryReq permLimitM ordBy alias = + -- if select has aggregations then use subquery to apply permission limit + if subQueryReq then maybe simpleJsonAgg withPermLimit permLimitM else simpleJsonAgg where simpleJsonAgg = mkSimpleJsonAgg rowIdenExp ordBy @@ -133,12 +132,11 @@ withJsonAggExtr hasAggSel permLimitM ordBy alias = , iden ) - asJsonAggExtr :: Bool -> S.Alias -> Bool -> Maybe Int -> Maybe S.OrderByExp -> S.Extractor -asJsonAggExtr singleObj als hasAggSel permLimit ordByExpM = +asJsonAggExtr singleObj als subQueryReq permLimit ordByExpM = flip S.Extractor (Just als) $ - bool (withJsonAggExtr hasAggSel permLimit ordByExpM als) + bool (withJsonAggExtr subQueryReq permLimit ordByExpM als) (asSingleRowExtr als) singleObj @@ -201,8 +199,8 @@ buildJsonObject pfx parAls arrRelCtx strfyNum flds = let qual = mkObjRelTableAls pfx $ aarName objSel in S.mkQIdenExp qual fldAls FArr arrSel -> - let arrPfx = snd $ mkArrNodePfx pfx parAls arrRelCtx $ - ANIField (fldAls, arrSel) + let arrPfx = _aniPrefix $ mkArrNodeInfo pfx parAls arrRelCtx $ + ANIField (fldAls, arrSel) in S.mkQIdenExp arrPfx fldAls toSQLCol :: PGColInfo -> Maybe ColOp -> S.SQLExp @@ -315,8 +313,8 @@ processAnnOrderByCol pfx parAls arrRelCtx strfyNum = \case , OBNObjNode rn relNode ) AOCAgg (RelInfo rn _ colMapping relTab _ ) relFltr annAggOb -> - let (arrAls, arrPfx) = - mkArrNodePfx pfx parAls arrRelCtx $ ANIAggOrdBy rn + let ArrNodeInfo arrAls arrPfx _ = + mkArrNodeInfo pfx parAls arrRelCtx $ ANIAggOrdBy rn fldName = mkAggObFld annAggOb qOrdBy = S.mkQIdenExp arrPfx $ toIden fldName tabFrom = TableFrom relTab Nothing @@ -375,50 +373,61 @@ aggSelToArrNode pfx als aggSel = mergedBN = foldr mergeBaseNodes emptyBN allBNs mkAggBaseNode (fn, selFld) = - mkBaseNode hasAggSel pfx fn selFld tabFrm tabPerm tabArgs strfyNum + mkBaseNode subQueryReq pfx fn selFld tabFrm tabPerm tabArgs strfyNum selFldToExtr (FieldName t, fld) = (:) (S.SELit t) $ pure $ case fld of TAFAgg flds -> aggFldToExp flds TAFNodes _ -> - withJsonAggExtr hasAggSel permLimit ordBy $ S.Alias $ Iden t + withJsonAggExtr subQueryReq permLimit ordBy $ S.Alias $ Iden t TAFExp e -> -- bool_or to force aggregation S.SEFnApp "coalesce" [ S.SELit e , S.SEUnsafe "bool_or('true')::text"] Nothing - hasAggSel = any (isTabAggFld . snd) aggFlds + subQueryReq = hasAggFld aggFlds +hasAggFld :: Foldable t => t (a, TableAggFldG v) -> Bool +hasAggFld = any (isTabAggFld . snd) + where isTabAggFld (TAFAgg _) = True isTabAggFld _ = False -mkArrNodePfx +mkArrNodeInfo :: Iden -> FieldName -> ArrRelCtx -> ArrNodeItem - -> (S.Alias, Iden) -mkArrNodePfx pfx parAls (ArrRelCtx arrFlds obRels) = \case + -> ArrNodeInfo +mkArrNodeInfo pfx parAls (ArrRelCtx arrFlds obRels) = \case ANIField aggFld@(fld, annArrSel) -> let (rn, tabArgs) = fetchRNAndTArgs annArrSel similarFlds = getSimilarAggFlds rn tabArgs $ delete aggFld + similarFldNames = map fst similarFlds similarOrdByFound = rn `elem` obRels && tabArgs == noTableArgs extraOrdByFlds = bool [] [ordByFldName] similarOrdByFound - sortedFlds = sort $ fld : (similarFlds <> extraOrdByFlds) - in ( S.Alias $ mkUniqArrRelAls parAls sortedFlds - , mkArrRelTableAls pfx parAls sortedFlds - ) + sortedFlds = sort $ fld : (similarFldNames <> extraOrdByFlds) + alias = S.Alias $ mkUniqArrRelAls parAls sortedFlds + prefix = mkArrRelTableAls pfx parAls sortedFlds + in ArrNodeInfo alias prefix $ + subQueryRequired similarFlds similarOrdByFound ANIAggOrdBy rn -> - let similarFlds = getSimilarAggFlds rn noTableArgs id + let similarFlds = map fst $ getSimilarAggFlds rn noTableArgs id sortedFlds = sort $ ordByFldName:similarFlds - in ( S.Alias $ mkUniqArrRelAls parAls sortedFlds - , mkArrRelTableAls pfx parAls sortedFlds - ) + alias = S.Alias $ mkUniqArrRelAls parAls sortedFlds + prefix = mkArrRelTableAls pfx parAls sortedFlds + in ArrNodeInfo alias prefix False where - getSimilarAggFlds rn tabArgs f = map fst $ + getSimilarAggFlds rn tabArgs f = flip filter (f arrFlds) $ \(_, annArrSel) -> let (lrn, lTabArgs) = fetchRNAndTArgs annArrSel in (lrn == rn) && (lTabArgs == tabArgs) + subQueryRequired similarFlds hasSimOrdBy = + hasSimOrdBy || any hasAgg similarFlds + + hasAgg (_, ASSimple _) = False + hasAgg (_, ASAgg (AnnRelG _ _ annSel)) = hasAggFld $ _asnFields annSel + fetchRNAndTArgs (ASSimple (AnnRelG rn _ annSel)) = (rn, _asnArgs annSel) fetchRNAndTArgs (ASAgg (AnnRelG rn _ annSel)) = @@ -481,7 +490,7 @@ mkBaseNode -> TableArgs -> Bool -> BaseNode -mkBaseNode hasAggSel pfx fldAls annSelFlds tableFrom +mkBaseNode subQueryReq pfx fldAls annSelFlds tableFrom tablePerm tableArgs strfyNum = BaseNode pfx distExprM fromItem finalWhere ordByExpM finalLimit offsetM allExtrs allObjsWithOb allArrsWithOb @@ -489,17 +498,18 @@ mkBaseNode hasAggSel pfx fldAls annSelFlds tableFrom TablePerm permFilter permLimit = tablePerm TableArgs whereM orderByM inpLimitM offsetM distM = tableArgs - -- if selection has aggregations then only use input limit + -- if sub query is used, then only use input limit + -- because permission limit is being applied in subquery -- else compare input and permission limits finalLimit = - if hasAggSel then inpLimitM + if subQueryReq then inpLimitM else withPermLimit withPermLimit = case (inpLimitM, permLimit) of - (inpLim, Nothing) -> inpLim - (Nothing, permLim) -> permLim - (Just i, Just p) -> Just $ if i < p then i else p + (inpLim, Nothing) -> inpLim + (Nothing, permLim) -> permLim + (Just inp, Just perm) -> Just $ if inp < perm then inp else perm aggOrdByRelNames = fetchOrdByAggRels orderByM @@ -514,9 +524,8 @@ mkBaseNode hasAggSel pfx fldAls annSelFlds tableFrom objNodes = HM.fromListWith mergeObjNodes $ map mkObjItem (mapMaybe getAnnObj flds) -- all array items (array relationships + aggregates) - hasArrAggSel = any (isArrFldAgg . snd) arrFlds arrNodes = HM.fromListWith mergeArrNodes $ - map (mkArrItem hasArrAggSel arrRelCtx) arrFlds + map (mkArrItem arrRelCtx) arrFlds (obExtrs, ordByObjs, ordByArrs, obeM) = mkOrdByItems' arrRelCtx @@ -581,10 +590,10 @@ mkBaseNode hasAggSel pfx fldAls annSelFlds tableFrom in (relName, objNode) -- process an array/array-aggregate item - mkArrItem hasArrAggSel arrRelCtx (fld, arrSel) = - let (arrAls, arrPfx) = mkArrNodePfx pfx fldAls arrRelCtx $ - ANIField (fld, arrSel) - arrNode = mkArrNode hasArrAggSel arrPfx (fld, arrSel) + mkArrItem arrRelCtx (fld, arrSel) = + let ArrNodeInfo arrAls arrPfx subQReq = + mkArrNodeInfo pfx fldAls arrRelCtx $ ANIField (fld, arrSel) + arrNode = mkArrNode subQReq arrPfx (fld, arrSel) in (arrAls, arrNode) getAnnObj (f, annFld) = case annFld of @@ -595,9 +604,6 @@ mkBaseNode hasAggSel pfx fldAls annSelFlds tableFrom FArr ar -> Just (f, ar) _ -> Nothing - isArrFldAgg (ASAgg _) = True - isArrFldAgg (ASSimple _) = False - annSelToBaseNode :: Iden -> FieldName -> AnnSimpleSel -> BaseNode annSelToBaseNode pfx fldAls annSel = mkBaseNode False pfx fldAls (TAFNodes selFlds) tabFrm tabPerm tabArgs strfyNum @@ -609,11 +615,11 @@ mkObjNode pfx (fldName, AnnRelG _ rMapn rAnnSel) = ObjNode rMapn $ annSelToBaseNode pfx fldName rAnnSel mkArrNode :: Bool -> Iden -> (FieldName, ArrSel) -> ArrNode -mkArrNode hasAggSel pfx (fldName, annArrSel) = case annArrSel of +mkArrNode subQueryReq pfx (fldName, annArrSel) = case annArrSel of ASSimple annArrRel -> let bn = annSelToBaseNode pfx fldName $ aarAnnSel annArrRel permLimit = getPermLimit $ aarAnnSel annArrRel - extr = asJsonAggExtr False (S.toAlias fldName) hasAggSel permLimit $ + extr = asJsonAggExtr False (S.toAlias fldName) subQueryReq permLimit $ _bnOrderBy bn in ArrNode [extr] (aarMapping annArrRel) bn diff --git a/server/src-lib/Hasura/RQL/DML/Select/Types.hs b/server/src-lib/Hasura/RQL/DML/Select/Types.hs index 90530789a4e3f..ca735502c6ba5 100644 --- a/server/src-lib/Hasura/RQL/DML/Select/Types.hs +++ b/server/src-lib/Hasura/RQL/DML/Select/Types.hs @@ -389,3 +389,10 @@ mergeArrNodes lNode rNode = where ArrNode lExtrs colMapping lBN = lNode ArrNode rExtrs _ rBN = rNode + +data ArrNodeInfo + = ArrNodeInfo + { _aniAlias :: !S.Alias + , _aniPrefix :: !Iden + , _aniSubQueryRequired :: !Bool + } deriving (Show, Eq) From a577f3a179102f0098e24dcdc15730d310a2d90d Mon Sep 17 00:00:00 2001 From: rakeshkky <12475069+rakeshkky@users.noreply.github.com> Date: Tue, 30 Jul 2019 17:13:11 +0530 Subject: [PATCH 3/3] add tests --- .../src-lib/Hasura/RQL/DML/Select/Internal.hs | 12 +- .../agg_perm/author_post_agg_order_by.yaml | 27 ++++ .../queries/graphql_query/agg_perm/setup.yaml | 143 ++++++++++++------ .../graphql_query/agg_perm/teardown.yaml | 1 + server/tests-py/test_graphql_queries.py | 3 + 5 files changed, 136 insertions(+), 50 deletions(-) create mode 100644 server/tests-py/queries/graphql_query/agg_perm/author_post_agg_order_by.yaml diff --git a/server/src-lib/Hasura/RQL/DML/Select/Internal.hs b/server/src-lib/Hasura/RQL/DML/Select/Internal.hs index 2e2f9debe6035..8b523939c9962 100644 --- a/server/src-lib/Hasura/RQL/DML/Select/Internal.hs +++ b/server/src-lib/Hasura/RQL/DML/Select/Internal.hs @@ -604,20 +604,20 @@ mkBaseNode subQueryReq pfx fldAls annSelFlds tableFrom FArr ar -> Just (f, ar) _ -> Nothing -annSelToBaseNode :: Iden -> FieldName -> AnnSimpleSel -> BaseNode -annSelToBaseNode pfx fldAls annSel = - mkBaseNode False pfx fldAls (TAFNodes selFlds) tabFrm tabPerm tabArgs strfyNum +annSelToBaseNode :: Bool -> Iden -> FieldName -> AnnSimpleSel -> BaseNode +annSelToBaseNode subQueryReq pfx fldAls annSel = + mkBaseNode subQueryReq pfx fldAls (TAFNodes selFlds) tabFrm tabPerm tabArgs strfyNum where AnnSelG selFlds tabFrm tabPerm tabArgs strfyNum = annSel mkObjNode :: Iden -> (FieldName, ObjSel) -> ObjNode mkObjNode pfx (fldName, AnnRelG _ rMapn rAnnSel) = - ObjNode rMapn $ annSelToBaseNode pfx fldName rAnnSel + ObjNode rMapn $ annSelToBaseNode False pfx fldName rAnnSel mkArrNode :: Bool -> Iden -> (FieldName, ArrSel) -> ArrNode mkArrNode subQueryReq pfx (fldName, annArrSel) = case annArrSel of ASSimple annArrRel -> - let bn = annSelToBaseNode pfx fldName $ aarAnnSel annArrRel + let bn = annSelToBaseNode subQueryReq pfx fldName $ aarAnnSel annArrRel permLimit = getPermLimit $ aarAnnSel annArrRel extr = asJsonAggExtr False (S.toAlias fldName) subQueryReq permLimit $ _bnOrderBy bn @@ -698,7 +698,7 @@ mkSQLSelect isSingleObject annSel = permLimit = getPermLimit annSel extrs = pure $ asJsonAggExtr isSingleObject rootFldAls False permLimit $ _bnOrderBy baseNode - baseNode = annSelToBaseNode (toIden rootFldName) rootFldName annSel + baseNode = annSelToBaseNode False (toIden rootFldName) rootFldName annSel rootFldName = FieldName "root" rootFldAls = S.Alias $ toIden rootFldName diff --git a/server/tests-py/queries/graphql_query/agg_perm/author_post_agg_order_by.yaml b/server/tests-py/queries/graphql_query/agg_perm/author_post_agg_order_by.yaml new file mode 100644 index 0000000000000..3628807dbf3db --- /dev/null +++ b/server/tests-py/queries/graphql_query/agg_perm/author_post_agg_order_by.yaml @@ -0,0 +1,27 @@ +description: Query author along with posts with order by posts count +url: /v1/graphql +status: 200 +headers: + X-Hasura-Role: user +response: + data: + author: + - id: 2 + name: Author 2 + posts: + - id: 4 + title: Post 4 + content: Post 4 Content +query: + query: | + query { + author(order_by: {posts_aggregate: {count: asc}}){ + id + name + posts{ + id + title + content + } + } + } diff --git a/server/tests-py/queries/graphql_query/agg_perm/setup.yaml b/server/tests-py/queries/graphql_query/agg_perm/setup.yaml index efd830b764acd..a0b7c2170d6d9 100644 --- a/server/tests-py/queries/graphql_query/agg_perm/setup.yaml +++ b/server/tests-py/queries/graphql_query/agg_perm/setup.yaml @@ -1,23 +1,15 @@ type: bulk args: -#Author table - type: run_sql args: sql: | + -- author table create table author( id serial primary key, name text unique ); -- type: track_table - args: - schema: public - name: author - -#Article table -- type: run_sql - args: - sql: | + -- article table CREATE TABLE article ( id SERIAL PRIMARY KEY, title TEXT, @@ -25,13 +17,89 @@ args: author_id INTEGER REFERENCES author(id), is_published BOOLEAN, published_on TIMESTAMP NOT NULL DEFAULT NOW() - ) + ); + -- post table + CREATE TABLE post ( + id SERIAL PRIMARY KEY, + title TEXT, + content TEXT, + author_id INTEGER REFERENCES author(id) + ); + + -- insert data + INSERT INTO author (name) + VALUES + ('Author 1'), + ('Author 2') + ; + + INSERT INTO article (title,content,author_id,is_published) + VALUES + ( + 'Article 1', + 'Sample article content 1', + 1, + false + ), + ( + 'Article 2', + 'Sample article content 2', + 1, + true + ), + ( + 'Article 3', + 'Sample article content 3', + 2, + true + ) + ; + + INSERT INTO post (title, content, author_id) + VALUES + ( + 'Post 1', + 'Post 1 Content', + 1 + ), + ( + 'Post 2', + 'Post 2 Content', + 1 + ), + ( + 'Post 3', + 'Post 3 Content', + 1 + ), + ( + 'Post 4', + 'Post 4 Content', + 2 + ), + ( + 'Post 5', + 'Post 5 Content', + 2 + ) + ; + +- type: track_table + args: + schema: public + name: author + - type: track_table args: schema: public name: article -#Object relationship +- type: track_table + args: + schema: public + name: post + +#Object relationship article <-> author - type: create_object_relationship args: table: article @@ -39,7 +107,7 @@ args: using: foreign_key_constraint_on: author_id -#Array relationship +#Array relationship author <-> article - type: create_array_relationship args: table: author @@ -49,6 +117,16 @@ args: table: article column: author_id +#Array relationship author <-> post +- type: create_array_relationship + args: + table: author + name: posts + using: + foreign_key_constraint_on: + table: post + column: author_id + #Select pemissions on User - type: create_select_permission args: @@ -70,35 +148,12 @@ args: allow_aggregations: false limit: 1 -#Insert values -- type: run_sql - args: - sql: | - insert into author (name) - values - ('Author 1'), - ('Author 2') - -- type: run_sql +- type: create_select_permission args: - sql: | - insert into article (title,content,author_id,is_published) - values - ( - 'Article 1', - 'Sample article content 1', - 1, - false - ), - ( - 'Article 2', - 'Sample article content 2', - 1, - true - ), - ( - 'Article 3', - 'Sample article content 3', - 2, - true - ) + table: post + role: user + permission: + columns: '*' + filter: {} + allow_aggregations: true + limit: 1 diff --git a/server/tests-py/queries/graphql_query/agg_perm/teardown.yaml b/server/tests-py/queries/graphql_query/agg_perm/teardown.yaml index 278daa3ea5e3f..823e94bfc5a61 100644 --- a/server/tests-py/queries/graphql_query/agg_perm/teardown.yaml +++ b/server/tests-py/queries/graphql_query/agg_perm/teardown.yaml @@ -12,5 +12,6 @@ args: args: sql: | DROP TABLE article; + DROP TABLE post; DROP TABLE author; cascade: true diff --git a/server/tests-py/test_graphql_queries.py b/server/tests-py/test_graphql_queries.py index 56d812162aa6a..bbaa5f50ba1cb 100644 --- a/server/tests-py/test_graphql_queries.py +++ b/server/tests-py/test_graphql_queries.py @@ -90,6 +90,9 @@ def test_article_agg_fail(self, hge_ctx, transport): def test_author_articles_agg_fail(self, hge_ctx, transport): check_query_f(hge_ctx, self.dir() + '/author_articles_agg_fail.yaml', transport) + def test_author_post_agg_order_by(self, hge_ctx, transport): + check_query_f(hge_ctx, self.dir() + '/author_post_agg_order_by.yaml', transport) + @classmethod def dir(cls): return 'queries/graphql_query/agg_perm'