From 21b6c3a88de1077a996b2fedca1f59416a5d62cb Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Mon, 12 Feb 2024 16:38:10 +0100 Subject: [PATCH] fix: Avoid casting to table type when select= and media type handler are used Previously using a generic mimetype handler failed when any kind of select= was given, because we tried to cast the select-result to the original table type. With this change, this cast is only applied when select=* is given implicitly or explicitly. This is the only case where this makes sense, because this guarantees that correct columns are selected in the correct order for this cast to succeed. Resolves #3160 --- CHANGELOG.md | 1 + src/PostgREST/Plan.hs | 28 +++++++----- src/PostgREST/Query/SqlFragment.hs | 9 ++-- src/PostgREST/Query/Statements.hs | 6 +-- test/spec/Feature/Query/CustomMediaSpec.hs | 53 ++++++++++++++++++++++ test/spec/fixtures/schema.sql | 13 ++++++ update-ghc | 2 +- 7 files changed, 92 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1637d3fbe4..14285b36061 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #3149, Misleading "Starting PostgREST.." logs on schema cache reloading - @steve-chavez - #2815, Build static executable with GSSAPI support - @wolfgangwalther - #3205, Fix wrong subquery error returning a status of 400 Bad Request - @steve-chavez + - #3160, Fix using select= query parameter for custom media type handlers - @wolfgangwalther ### Deprecated diff --git a/src/PostgREST/Plan.hs b/src/PostgREST/Plan.hs index b948c88b484..483426c81b9 100644 --- a/src/PostgREST/Plan.hs +++ b/src/PostgREST/Plan.hs @@ -97,7 +97,7 @@ data WrappedReadPlan = WrappedReadPlan { , wrTxMode :: SQL.Mode , wrHandler :: MediaHandler , wrMedia :: MediaType -, wrIdent :: QualifiedIdentifier +, wrIdent :: Maybe QualifiedIdentifier } data MutateReadPlan = MutateReadPlan { @@ -106,7 +106,7 @@ data MutateReadPlan = MutateReadPlan { , mrTxMode :: SQL.Mode , mrHandler :: MediaHandler , mrMedia :: MediaType -, mrIdent :: QualifiedIdentifier +, mrIdent :: Maybe QualifiedIdentifier } data CallReadPlan = CallReadPlan { @@ -116,7 +116,7 @@ data CallReadPlan = CallReadPlan { , crProc :: Routine , crHandler :: MediaHandler , crMedia :: MediaType -, crIdent :: QualifiedIdentifier +, crIdent :: Maybe QualifiedIdentifier } data InspectPlan = InspectPlan { @@ -126,18 +126,18 @@ data InspectPlan = InspectPlan { wrappedReadPlan :: QualifiedIdentifier -> AppConfig -> SchemaCache -> ApiRequest -> Either Error WrappedReadPlan wrappedReadPlan identifier conf sCache apiRequest@ApiRequest{iPreferences=Preferences{..},..} = do - rPlan <- readPlan identifier conf sCache apiRequest - (hdler, mediaType) <- mapLeft ApiRequestError $ negotiateContent conf apiRequest identifier iAcceptMediaType (dbMediaHandlers sCache) + rPlan@(Node ReadPlan{select} forest) <- readPlan identifier conf sCache apiRequest + (handler, mediaType) <- mapLeft ApiRequestError $ negotiateContent conf apiRequest identifier iAcceptMediaType (dbMediaHandlers sCache) if not (null invalidPrefs) && preferHandling == Just Strict then Left $ ApiRequestError $ InvalidPreferences invalidPrefs else Right () - return $ WrappedReadPlan rPlan SQL.Read hdler mediaType identifier + return $ WrappedReadPlan rPlan SQL.Read handler mediaType (if isStarOnly select && null forest then Just identifier else Nothing) mutateReadPlan :: Mutation -> ApiRequest -> QualifiedIdentifier -> AppConfig -> SchemaCache -> Either Error MutateReadPlan mutateReadPlan mutation apiRequest@ApiRequest{iPreferences=Preferences{..},..} identifier conf sCache = do - rPlan <- readPlan identifier conf sCache apiRequest + rPlan@(Node ReadPlan{select} forest) <- readPlan identifier conf sCache apiRequest mPlan <- mutatePlan mutation identifier apiRequest sCache rPlan if not (null invalidPrefs) && preferHandling == Just Strict then Left $ ApiRequestError $ InvalidPreferences invalidPrefs else Right () - (hdler, mediaType) <- mapLeft ApiRequestError $ negotiateContent conf apiRequest identifier iAcceptMediaType (dbMediaHandlers sCache) - return $ MutateReadPlan rPlan mPlan SQL.Write hdler mediaType identifier + (handler, mediaType) <- mapLeft ApiRequestError $ negotiateContent conf apiRequest identifier iAcceptMediaType (dbMediaHandlers sCache) + return $ MutateReadPlan rPlan mPlan SQL.Write handler mediaType (if isStarOnly select && null forest then Just identifier else Nothing) callReadPlan :: QualifiedIdentifier -> AppConfig -> SchemaCache -> ApiRequest -> InvokeMethod -> Either Error CallReadPlan callReadPlan identifier conf sCache apiRequest@ApiRequest{iPreferences=Preferences{..},..} invMethod = do @@ -148,7 +148,7 @@ callReadPlan identifier conf sCache apiRequest@ApiRequest{iPreferences=Preferenc proc@Function{..} <- mapLeft ApiRequestError $ findProc identifier paramKeys (preferParameters == Just SingleObject) (dbRoutines sCache) iContentMediaType (invMethod == InvPost) let relIdentifier = QualifiedIdentifier pdSchema (fromMaybe pdName $ Routine.funcTableName proc) -- done so a set returning function can embed other relations - rPlan <- readPlan relIdentifier conf sCache apiRequest + rPlan@(Node ReadPlan{select} forest) <- readPlan relIdentifier conf sCache apiRequest let args = case (invMethod, iContentMediaType) of (InvGet, _) -> jsonRpcParams proc qsParams' (InvHead, _) -> jsonRpcParams proc qsParams' @@ -161,12 +161,16 @@ callReadPlan identifier conf sCache apiRequest@ApiRequest{iPreferences=Preferenc (InvPost, Routine.Immutable) -> SQL.Read (InvPost, Routine.Volatile) -> SQL.Write cPlan = callPlan proc apiRequest paramKeys args rPlan - (hdler, mediaType) <- mapLeft ApiRequestError $ negotiateContent conf apiRequest relIdentifier iAcceptMediaType (dbMediaHandlers sCache) + (handler, mediaType) <- mapLeft ApiRequestError $ negotiateContent conf apiRequest relIdentifier iAcceptMediaType (dbMediaHandlers sCache) if not (null invalidPrefs) && preferHandling == Just Strict then Left $ ApiRequestError $ InvalidPreferences invalidPrefs else Right () - return $ CallReadPlan rPlan cPlan txMode proc hdler mediaType relIdentifier + return $ CallReadPlan rPlan cPlan txMode proc handler mediaType (if isStarOnly select && null forest then Just relIdentifier else Nothing) where qsParams' = QueryParams.qsParams iQueryParams +isStarOnly :: [CoercibleSelectField] -> Bool +isStarOnly [CoercibleSelectField{csField=CoercibleField{cfName}}] = cfName == "*" +isStarOnly _ = False + inspectPlan :: ApiRequest -> Either Error InspectPlan inspectPlan apiRequest = do let producedMTs = [MTOpenAPI, MTApplicationJSON, MTAny] diff --git a/src/PostgREST/Query/SqlFragment.hs b/src/PostgREST/Query/SqlFragment.hs index 03f6177bd26..4556888102d 100644 --- a/src/PostgREST/Query/SqlFragment.hs +++ b/src/PostgREST/Query/SqlFragment.hs @@ -221,10 +221,11 @@ asJsonF rout strip asGeoJsonF :: SQL.Snippet asGeoJsonF = "json_build_object('type', 'FeatureCollection', 'features', coalesce(json_agg(ST_AsGeoJSON(_postgrest_t)::json), '[]'))" -customFuncF :: Maybe Routine -> QualifiedIdentifier -> QualifiedIdentifier -> SQL.Snippet -customFuncF rout funcQi target +customFuncF :: Maybe Routine -> QualifiedIdentifier -> Maybe QualifiedIdentifier -> SQL.Snippet +customFuncF rout funcQi _ | (funcReturnsScalar <$> rout) == Just True = fromQi funcQi <> "(_postgrest_t.pgrst_scalar)" - | otherwise = fromQi funcQi <> "(_postgrest_t::" <> fromQi target <> ")" +customFuncF _ funcQi (Just target) = fromQi funcQi <> "(_postgrest_t::" <> fromQi target <> ")" +customFuncF _ funcQi Nothing = fromQi funcQi <> "(_postgrest_t)" locationF :: [Text] -> SQL.Snippet locationF pKeys = [qc|( @@ -559,7 +560,7 @@ setConfigWithConstantNameJSON prefix keyVals = [setConfigWithConstantName (prefi arrayByteStringToText :: [(ByteString, ByteString)] -> [(Text,Text)] arrayByteStringToText keyVal = (T.decodeUtf8 *** T.decodeUtf8) <$> keyVal -handlerF :: Maybe Routine -> QualifiedIdentifier -> MediaHandler -> SQL.Snippet +handlerF :: Maybe Routine -> Maybe QualifiedIdentifier -> MediaHandler -> SQL.Snippet handlerF rout target = \case BuiltinAggArrayJsonStrip -> asJsonF rout True BuiltinAggSingleJson strip -> asJsonSingleF rout strip diff --git a/src/PostgREST/Query/Statements.hs b/src/PostgREST/Query/Statements.hs index 77947d04cd1..68c466e7d3a 100644 --- a/src/PostgREST/Query/Statements.hs +++ b/src/PostgREST/Query/Statements.hs @@ -56,7 +56,7 @@ data ResultSet | RSPlan BS.ByteString -- ^ the plan of the query -prepareWrite :: QualifiedIdentifier -> SQL.Snippet -> SQL.Snippet -> Bool -> Bool -> MediaType -> MediaHandler -> +prepareWrite :: Maybe QualifiedIdentifier -> SQL.Snippet -> SQL.Snippet -> Bool -> Bool -> MediaType -> MediaHandler -> Maybe PreferRepresentation -> Maybe PreferResolution -> [Text] -> Bool -> SQL.Statement () ResultSet prepareWrite qi selectQuery mutateQuery isInsert isPut mt handler rep resolution pKeys = SQL.dynamicallyParameterized (mtSnippet mt snippet) decodeIt @@ -94,7 +94,7 @@ prepareWrite qi selectQuery mutateQuery isInsert isPut mt handler rep resolution MTVndPlan{} -> planRow _ -> fromMaybe (RSStandard Nothing 0 mempty mempty Nothing Nothing Nothing) <$> HD.rowMaybe (standardRow False) -prepareRead :: QualifiedIdentifier -> SQL.Snippet -> SQL.Snippet -> Bool -> MediaType -> MediaHandler -> Bool -> SQL.Statement () ResultSet +prepareRead :: Maybe QualifiedIdentifier -> SQL.Snippet -> SQL.Snippet -> Bool -> MediaType -> MediaHandler -> Bool -> SQL.Statement () ResultSet prepareRead qi selectQuery countQuery countTotal mt handler = SQL.dynamicallyParameterized (mtSnippet mt snippet) decodeIt where @@ -117,7 +117,7 @@ prepareRead qi selectQuery countQuery countTotal mt handler = MTVndPlan{} -> planRow _ -> HD.singleRow $ standardRow True -prepareCall :: QualifiedIdentifier -> Routine -> SQL.Snippet -> SQL.Snippet -> SQL.Snippet -> Bool -> +prepareCall :: Maybe QualifiedIdentifier -> Routine -> SQL.Snippet -> SQL.Snippet -> SQL.Snippet -> Bool -> MediaType -> MediaHandler -> Bool -> SQL.Statement () ResultSet prepareCall qi rout callProcQuery selectQuery countQuery countTotal mt handler = diff --git a/test/spec/Feature/Query/CustomMediaSpec.hs b/test/spec/Feature/Query/CustomMediaSpec.hs index c5c1786040f..be24840b88e 100644 --- a/test/spec/Feature/Query/CustomMediaSpec.hs +++ b/test/spec/Feature/Query/CustomMediaSpec.hs @@ -230,6 +230,59 @@ spec = describe "custom media types" $ do simpleHeaders r `shouldContain` [("Content-Type", "text/csv; charset=utf-8")] simpleHeaders r `shouldContain` [("Content-Disposition", "attachment; filename=\"lines.csv\"")] + -- https://github.com/PostgREST/postgrest/issues/3160 + context "using select query parameter" $ do + it "without select" $ do + request methodGet "/projects?id=in.(1,2)" (acceptHdrs "pg/outfunc") "" + `shouldRespondWith` + [str|(1,"Windows 7",1) + |(2,"Windows 10",1) + |] + { matchStatus = 200 + , matchHeaders = ["Content-Type" <:> "pg/outfunc"] + } + + it "with fewer columns selected" $ do + request methodGet "/projects?id=in.(1,2)&select=id,name" (acceptHdrs "pg/outfunc") "" + `shouldRespondWith` + [str|(1,"Windows 7") + |(2,"Windows 10") + |] + { matchStatus = 200 + , matchHeaders = ["Content-Type" <:> "pg/outfunc"] + } + + it "with columns in different order" $ do + request methodGet "/projects?id=in.(1,2)&select=name,id,client_id" (acceptHdrs "pg/outfunc") "" + `shouldRespondWith` + [str|("Windows 7",1,1) + |("Windows 10",2,1) + |] + { matchStatus = 200 + , matchHeaders = ["Content-Type" <:> "pg/outfunc"] + } + + it "with computed columns" $ do + request methodGet "/items?id=in.(1,2)&select=id,always_true" (acceptHdrs "pg/outfunc") "" + `shouldRespondWith` + [str|(1,t) + |(2,t) + |] + { matchStatus = 200 + , matchHeaders = ["Content-Type" <:> "pg/outfunc"] + } + + -- TODO: Embeddings should not return JSON. Arrays of record would be much better. + it "with embedding" $ do + request methodGet "/projects?id=in.(1,2)&select=*,clients(id)" (acceptHdrs "pg/outfunc") "" + `shouldRespondWith` + [str|(1,"Windows 7",1,"{""id"": 1}") + |(2,"Windows 10",1,"{""id"": 1}") + |] + { matchStatus = 200 + , matchHeaders = ["Content-Type" <:> "pg/outfunc"] + } + context "any media type" $ do context "on functions" $ do it "returns application/json for */* if not explicitly set" $ do diff --git a/test/spec/fixtures/schema.sql b/test/spec/fixtures/schema.sql index d086c614291..afb4ed27fb9 100644 --- a/test/spec/fixtures/schema.sql +++ b/test/spec/fixtures/schema.sql @@ -3755,3 +3755,16 @@ create aggregate test.some_agg (some_numbers) ( create view bad_subquery as select * from projects where id = (select id from projects); + +-- custom generic mimetype +create domain "pg/outfunc" as text; +create function test.outfunc_trans (state text, next anyelement) +returns "pg/outfunc" as $$ + select (state || next::text || E'\n')::"pg/outfunc"; +$$ language sql; + +create aggregate test.outfunc_agg (anyelement) ( + initcond = '' +, stype = "pg/outfunc" +, sfunc = outfunc_trans +); diff --git a/update-ghc b/update-ghc index e2b67233f76..ee6d6727cd9 160000 --- a/update-ghc +++ b/update-ghc @@ -1 +1 @@ -Subproject commit e2b67233f76e45269e3da41f59caaf7ab8fd75b0 +Subproject commit ee6d6727cd98342fd48e56bf2505cddf27462a79