From 132ecd8d746c4dd3dea5c07b36d0af9494dfc84e Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Thu, 25 Nov 2021 19:57:26 -0500 Subject: [PATCH] Correct count query when doing inner joins --- src/PostgREST/Query/QueryBuilder.hs | 24 ++++- test/Feature/EmbedInnerJoinSpec.hs | 139 ++++++++++++++++++++++++---- 2 files changed, 144 insertions(+), 19 deletions(-) diff --git a/src/PostgREST/Query/QueryBuilder.hs b/src/PostgREST/Query/QueryBuilder.hs index f25797aeb..133e149c7 100644 --- a/src/PostgREST/Query/QueryBuilder.hs +++ b/src/PostgREST/Query/QueryBuilder.hs @@ -179,10 +179,28 @@ requestToCallProcQuery (FunctionCall qi params args returnsScalar multipleCall r -- It only takes WHERE into account and doesn't include LIMIT/OFFSET because it would reduce the COUNT. -- SELECT 1 is done instead of SELECT * to prevent doing expensive operations(like functions based on the columns) -- inside the FROM target. +-- If the request contains INNER JOINs, then the COUNT of the root node will change. +-- For this case, we use a WHERE EXISTS instead of an INNER JOIN on the count query. +-- See https://github.com/PostgREST/postgrest/issues/2009#issuecomment-977473031 +-- Only for the nodes that have an INNER JOIN linked to the root level. readRequestToCountQuery :: ReadRequest -> SQL.Snippet -readRequestToCountQuery (Node (Select{from=qi, where_=logicForest}, _) _) = - "SELECT 1 " <> "FROM " <> SQL.sql (fromQi qi) <> " " <> - if null logicForest then mempty else "WHERE " <> intercalateSnippet " AND " (map (pgFmtLogicTree qi) logicForest) +readRequestToCountQuery (Node (Select{from=qi, implicitJoins=implJoins, where_=logicForest, joinConditions=joinConditions_}, _) forest) = + "SELECT 1 FROM " <> SQL.sql (BS.intercalate ", " (fromQi qi:(fromQi <$> implJoins))) <> + (if null logicForest && null joinConditions_ && null subQueries + then mempty + else " WHERE " ) <> + intercalateSnippet " AND " ( + map (pgFmtLogicTree qi) logicForest ++ + map pgFmtJoinCondition joinConditions_ ++ + subQueries + ) + where + subQueries = foldr existsSubquery [] forest + existsSubquery :: ReadRequest -> [SQL.Snippet] -> [SQL.Snippet] + existsSubquery readReq@(Node (_, (_, _, _, _, joinType, _)) _) rest = + if joinType == Just JTInner + then ("EXISTS (" <> readRequestToCountQuery readReq <> " )"):rest + else mempty limitedQuery :: SQL.Snippet -> Maybe Integer -> SQL.Snippet limitedQuery query maxRows = query <> SQL.sql (maybe mempty (\x -> " LIMIT " <> BS.pack (show x)) maxRows) diff --git a/test/Feature/EmbedInnerJoinSpec.hs b/test/Feature/EmbedInnerJoinSpec.hs index 47c5c7a9e..25cba0e18 100644 --- a/test/Feature/EmbedInnerJoinSpec.hs +++ b/test/Feature/EmbedInnerJoinSpec.hs @@ -1,6 +1,7 @@ module Feature.EmbedInnerJoinSpec where -import Network.Wai (Application) +import Network.HTTP.Types +import Network.Wai (Application) import Test.Hspec import Test.Hspec.Wai @@ -25,6 +26,12 @@ spec = {"id":3,"clients":{"id":2}}, {"id":4,"clients":{"id":2}}, {"id":5,"clients":null}]|] { matchHeaders = [matchContentTypeJson] } + request methodHead "/projects?select=id,clients!inner(id)" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-3/4" ] + } it "filters source tables when the embedded table is filtered" $ do get "/projects?select=id,clients!inner(id)&clients.id=eq.1" `shouldRespondWith` @@ -40,6 +47,12 @@ spec = get "/projects?select=id,clients!inner(id)&clients.id=eq.0" `shouldRespondWith` [json|[]|] { matchHeaders = [matchContentTypeJson] } + request methodHead "/projects?select=id,clients!inner(id)&clients.id=eq.1" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-1/2" ] + } it "filters source tables when a two levels below embedded table is filtered" $ do get "/tasks?select=id,projects!inner(id,clients!inner(id))&projects.clients.id=eq.1" `shouldRespondWith` @@ -56,8 +69,14 @@ spec = {"id":7,"projects":{"id":4,"clients":{"id":2}}}, {"id":8,"projects":{"id":4,"clients":{"id":2}}}]|] { matchHeaders = [matchContentTypeJson] } + request methodHead "/tasks?select=id,projects!inner(id,clients!inner(id))&projects.clients.id=eq.1" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-3/4" ] + } - it "only affects the source table rows if his direct embedding is an inner join" $ + it "only affects the source table rows if his direct embedding is an inner join" $ do get "/tasks?select=id,projects(id,clients!inner(id))&projects.clients.id=eq.2" `shouldRespondWith` [json|[ {"id":1,"projects":null}, @@ -69,11 +88,23 @@ spec = {"id":7,"projects":{"id":4,"clients":{"id":2}}}, {"id":8,"projects":{"id":4,"clients":{"id":2}}}]|] { matchHeaders = [matchContentTypeJson] } + request methodHead "/tasks?select=id,projects(id,clients!inner(id))&projects.clients.id=eq.2" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-7/8" ] + } - it "works with views" $ + it "works with views" $ do get "/books?select=title,authors!inner(name)&authors.name=eq.George%20Orwell" `shouldRespondWith` [json| [{"title":"1984","authors":{"name":"George Orwell"}}] |] { matchHeaders = [matchContentTypeJson] } + request methodHead "/books?select=title,authors!inner(name)&authors.name=eq.George%20Orwell" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-0/1" ] + } context "one-to-many relationships" $ do it "ignores empty array embeddings while the default left join doesn't" $ do @@ -89,6 +120,12 @@ spec = {"id":3,"child_entities":[]}, {"id":4,"child_entities":[]}] |] { matchHeaders = [matchContentTypeJson] } + request methodHead "/entities?select=id,child_entities!inner(id)" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-1/2" ] + } it "filters source tables when the embedded table is filtered" $ do get "/entities?select=id,child_entities!inner(id)&child_entities.id=eq.1" `shouldRespondWith` @@ -100,6 +137,12 @@ spec = get "/entities?select=id,child_entities!inner(id)&child_entities.id=eq.0" `shouldRespondWith` [json|[]|] { matchHeaders = [matchContentTypeJson] } + request methodHead "/entities?select=id,child_entities!inner(id)&child_entities.id=eq.1" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-0/1" ] + } it "filters source tables when a two levels below embedded table is filtered" $ do get "/entities?select=id,child_entities!inner(id,grandchild_entities!inner(id))&child_entities.grandchild_entities.id=in.(1,5)" @@ -122,8 +165,14 @@ spec = } ]|] { matchHeaders = [matchContentTypeJson] } + request methodHead "/entities?select=id,child_entities!inner(id,grandchild_entities!inner(id))&child_entities.grandchild_entities.id=in.(1,5)" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-0/1" ] + } - it "only affects the source table rows if his direct embedding is an inner join" $ + it "only affects the source table rows if his direct embedding is an inner join" $ do get "/entities?select=id,child_entities!inner(id,grandchild_entities(id))&child_entities.grandchild_entities.id=eq.2" `shouldRespondWith` [json|[ { @@ -142,11 +191,23 @@ spec = } ]|] { matchHeaders = [matchContentTypeJson] } + request methodHead "/entities?select=id,child_entities!inner(id,grandchild_entities(id))&child_entities.grandchild_entities.id=eq.2" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-1/2" ] + } - it "works with views" $ + it "works with views" $ do get "/authors?select=*,books!inner(*)&books.title=eq.1984" `shouldRespondWith` [json| [{"id":1,"name":"George Orwell","books":[{"id":1,"title":"1984","publication_year":1949,"author_id":1}]}] |] { matchHeaders = [matchContentTypeJson] } + request methodHead "/authors?select=*,books!inner(*)&books.title=eq.1984" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-0/1" ] + } context "many-to-many relationships" $ do it "ignores empty array embeddings while the default left join doesn't" $ do @@ -161,6 +222,12 @@ spec = {"id":2,"suppliers":[{"id":1}, {"id":3}]}, {"id":3,"suppliers":[]}] |] { matchHeaders = [matchContentTypeJson] } + request methodHead "/products?select=id,suppliers!inner(id)" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-1/2" ] + } it "filters source tables when the embedded table is filtered" $ do get "/products?select=id,suppliers!inner(id)&suppliers.id=eq.2" `shouldRespondWith` @@ -172,6 +239,12 @@ spec = get "/products?select=id,suppliers!inner(id)&suppliers.id=eq.0" `shouldRespondWith` [json| [] |] { matchHeaders = [matchContentTypeJson] } + request methodHead "/products?select=id,suppliers!inner(id)&suppliers.id=eq.2" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-0/1" ] + } it "filters source tables when a two levels below embedded table is filtered" $ do get "/products?select=id,suppliers!inner(id,trade_unions!inner(id))&suppliers.trade_unions.id=eq.3" @@ -182,13 +255,25 @@ spec = `shouldRespondWith` [json|[{"id":1,"suppliers":[{"id":2,"trade_unions":[{"id":4}]}]}] |] { matchHeaders = [matchContentTypeJson] } + request methodHead "/products?select=id,suppliers!inner(id,trade_unions!inner(id))&suppliers.trade_unions.id=eq.3" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-0/1" ] + } - it "only affects the source table rows if his direct embedding is an inner join" $ + it "only affects the source table rows if his direct embedding is an inner join" $ do get "/products?select=id,suppliers!inner(id,trade_unions(id))&suppliers.trade_unions.id=eq.3" `shouldRespondWith` [json|[ {"id":1,"suppliers":[{"id":1,"trade_unions":[]}, {"id":2,"trade_unions":[{"id":3}]}]}, {"id":2,"suppliers":[{"id":1,"trade_unions":[]}, {"id":3,"trade_unions":[]}]}]|] { matchHeaders = [matchContentTypeJson] } + request methodHead "/products?select=id,suppliers!inner(id,trade_unions(id))&suppliers.trade_unions.id=eq.3" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-1/2" ] + } it "works with views" $ do get "/actors?select=*,films!inner(*)&films.title=eq.douze%20commandements" `shouldRespondWith` @@ -197,8 +282,14 @@ spec = get "/films?select=*,actors!inner(*)&actors.name=eq.john" `shouldRespondWith` [json| [{"id":12,"title":"douze commandements","actors":[{"id":1,"name":"john"}]}] |] { matchHeaders = [matchContentTypeJson] } + request methodHead "/actors?select=*,films!inner(*)&films.title=eq.douze%20commandements" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-0/1" ] + } - it "works with m2o and m2m relationships combined" $ + it "works with m2o and m2m relationships combined" $ do get "/projects?select=name,clients!inner(name),users!inner(name)" `shouldRespondWith` [json| [ {"name":"Windows 7","clients":{"name":"Microsoft"},"users":[{"name":"Angela Martin"}, {"name":"Dwight Schrute"}]}, @@ -206,11 +297,23 @@ spec = {"name":"IOS","clients":{"name":"Apple"},"users":[{"name":"Michael Scott"}, {"name":"Dwight Schrute"}]}, {"name":"OSX","clients":{"name":"Apple"},"users":[{"name":"Michael Scott"}]}]|] { matchHeaders = [matchContentTypeJson] } + request methodHead "/projects?select=name,clients!inner(name),users!inner(name)" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-3/4" ] + } - it "works with rpc" $ + it "works with rpc" $ do get "/rpc/getallprojects?select=id,clients!inner(id)&clients.id=eq.1" `shouldRespondWith` [json| [{"id":1,"clients":{"id":1}}, {"id":2,"clients":{"id":1}}] |] { matchHeaders = [matchContentTypeJson] } + request methodHead "/rpc/getallprojects?select=id,clients!inner(id)&clients.id=eq.1" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-1/2" ] + } it "works when using hints" $ do get "/projects?select=id,clients!client!inner(id)&clients.id=eq.2" `shouldRespondWith` @@ -219,6 +322,12 @@ spec = get "/projects?select=id,client!inner(id)&client.id=eq.2" `shouldRespondWith` [json| [{"id":3,"client":{"id":2}}, {"id":4,"client":{"id":2}}] |] { matchHeaders = [matchContentTypeJson] } + request methodHead "/projects?select=id,clients!client!inner(id)&clients.id=eq.2" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-1/2" ] + } it "works with many one-to-many relationships" $ do -- https://github.com/PostgREST/postgrest/issues/1977 @@ -239,11 +348,9 @@ spec = {"id":2,"name":"Target","clientinfo":[{"other":"456 South 3rd St"}],"contact":[{"name":"Tabby Targo"}]} ]|] { matchHeaders = [matchContentTypeJson] } - - it "can use default left join behavior explicitly" $ - get "/projects?select=id,clients!left(id)" `shouldRespondWith` - [json|[ - {"id":1,"clients":{"id":1}}, {"id":2,"clients":{"id":1}}, - {"id":3,"clients":{"id":2}}, {"id":4,"clients":{"id":2}}, - {"id":5,"clients":null}]|] - { matchHeaders = [matchContentTypeJson] } + request methodHead "/client?select=id,name,contact!inner(name),clientinfo!inner(other)" [("Prefer", "count=exact")] mempty + `shouldRespondWith` "" + { matchStatus = 200 + , matchHeaders = [ matchContentTypeJson + , "Content-Range" <:> "0-2/3" ] + }