Fix #1141, app.settings resetting on pool timeout (#1169)

* Add test for ensuring app.settings don't reset

CHANGELOG.md

@@ -21,6 +21,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 - #1152, Fix RPC failing when having arguments with reserved or uppercase keywords - @mdr1384
 - #905, Fix intermittent empty replies - @steve-chavez
 - #1139, Fix JWTIssuedAtFuture failure for valid iat claim - @steve-chavez
+- #1141, Fix app.settings resetting on pool timeout - @steve-chavez
 
 ### Changed
 

main/Main.hs

@@ -7,8 +7,7 @@ import           PostgREST.App            (postgrest)
 import           PostgREST.Config         (AppConfig (..),
                                            minimumPgVersion,
                                            prettyVersion, readOptions)
-import           PostgREST.DbStructure    (getDbStructure, getPgVersion,
+import           PostgREST.DbStructure    (getDbStructure, getPgVersion)
-                                           fillSessionWithSettings)
 import           PostgREST.Error          (encodeError)
 import           PostgREST.OpenAPI        (isMalformedProxyUri)
 import           PostgREST.Types          (DbStructure, Schema, PgVersion(..))
@@ -63,11 +62,10 @@ connectionWorker
   :: ThreadId -- ^ This thread is killed if pg version is unsupported
   -> P.Pool   -- ^ The PostgreSQL connection pool
   -> Schema   -- ^ Schema PostgREST is serving up
-  -> [(Text, Text)] -- ^ Settings or Environment passed in through the config
   -> IORef (Maybe DbStructure) -- ^ mutable reference to 'DbStructure'
   -> IORef Bool                -- ^ Used as a binary Semaphore
   -> IO ()
-connectionWorker mainTid pool schema settings refDbStructure refIsWorkerOn = do
+connectionWorker mainTid pool schema refDbStructure refIsWorkerOn = do
   isWorkerOn <- readIORef refIsWorkerOn
   unless isWorkerOn $ do
     atomicWriteIORef refIsWorkerOn True
@@ -85,7 +83,6 @@ connectionWorker mainTid pool schema settings refDbStructure refIsWorkerOn = do
               ("Cannot run in this PostgreSQL version, PostgREST needs at least "
               <> pgvName minimumPgVersion)
             killThread mainTid
-          fillSessionWithSettings settings
           dbStructure <- getDbStructure schema actualPgVersion
           liftIO $ atomicWriteIORef refDbStructure $ Just dbStructure
         case result of
@@ -184,7 +181,6 @@ main = do
     mainTid
     pool
     (configSchema conf)
-    (configSettings conf)
     refDbStructure
     refIsWorkerOn
   --
@@ -208,7 +204,6 @@ main = do
                 mainTid
                 pool
                 (configSchema conf)
-                (configSettings conf)
                 refDbStructure
                 refIsWorkerOn
       ) Nothing
@@ -229,7 +224,6 @@ main = do
          mainTid
          pool
          (configSchema conf)
-         (configSettings conf)
          refDbStructure
          refIsWorkerOn)
 

src/PostgREST/Config.hs

@@ -141,7 +141,7 @@ readOptions = do
           <*> (join . fmap coerceInt <$> C.key "max-rows")
           <*> (mfilter (/= "") <$> C.key "pre-request")
           <*> pure False
-          <*> (fmap parsedPairToTextPair <$> C.subassocs "app.settings")
+          <*> (fmap (fmap coerceText) <$> C.subassocs "app.settings")
           <*> (maybe (Right [JSPKey "role"]) parseRoleClaimKey <$> C.key "role-claim-key")
 
   case mAppConf of
@@ -152,13 +152,6 @@ readOptions = do
       return appConf
 
   where
-    parsedPairToTextPair :: (Name, Value) -> (Text, Text)
-    parsedPairToTextPair (k, v) = (k, newValue)
-      where
-        newValue = case v of
-          String textVal -> textVal
-          _ -> show v
-
     parseJwtAudience :: Name -> C.ConfigParserM (Maybe StringOrURI)
     parseJwtAudience k =
       C.key k >>= \case
@@ -168,6 +161,10 @@ readOptions = do
           (Just "") -> pure Nothing
           aud' -> pure aud'
 
+    coerceText :: Value -> Text
+    coerceText (String s) = s
+    coerceText v = show v
+
     coerceInt :: (Read i, Integral i) => Value -> Maybe i
     coerceInt (Number x) = rightToMaybe $ floatingOrInteger x
     coerceInt (String x) = readMaybe $ toS x

src/PostgREST/DbStructure.hs

@@ -10,7 +10,6 @@ module PostgREST.DbStructure (
 , accessibleProcs
 , schemaDescription
 , getPgVersion
-, fillSessionWithSettings
 ) where
 
 import qualified Hasql.Decoders                as HD
@@ -33,9 +32,6 @@ import           GHC.Exts                      (groupWith)
 import           Protolude
 import           Unsafe (unsafeHead)
 
-import           Data.Functor.Contravariant    (contramap)
-import           Contravariant.Extras          (contrazip2)
-
 getDbStructure :: Schema -> PgVersion -> H.Session DbStructure
 getDbStructure schema pgVer = do
   tabs      <- H.query () allTables
@@ -764,17 +760,3 @@ getPgVersion = H.query () $ H.statement sql HE.unit versionRow False
   where
     sql = "SELECT current_setting('server_version_num')::integer, current_setting('server_version')"
     versionRow = HD.singleRow $ PgVersion <$> HD.value HD.int4 <*> HD.value HD.text
-
-fillSessionWithSettings :: [(Text, Text)] -> H.Session ()
-fillSessionWithSettings settings =
-    -- Send all of the config settings to the set_config function, using pgsql's `unnest` to transform arrays of values
-    H.query settings $ H.statement "SELECT set_config(k, v, false) FROM unnest($1, $2) AS f1(k, v)" encoder HD.unit False
-
-  where
-    -- Take a list of (key, value) pairs and encode each as an array to later bind to the query
-    -- see Insert Many section at https://hackage.haskell.org/package/hasql-1.1.1/docs/Hasql-Encoders.html
-    encoder = contramap L.unzip $ contrazip2 (vector HE.text) (vector HE.text)
-      where
-        vector value =
-          HE.value $ HE.array $ HE.arrayDimension foldl' $ HE.arrayValue value
-

src/PostgREST/Middleware.hs

@@ -19,7 +19,7 @@ import           PostgREST.ApiRequest          (ApiRequest(..))
 import           PostgREST.Auth                (JWTAttempt(..))
 import           PostgREST.Config              (AppConfig (..), corsPolicy)
 import           PostgREST.Error               (simpleError)
-import           PostgREST.QueryBuilder        (pgFmtLit, unquoted, pgFmtEnvVar)
+import           PostgREST.QueryBuilder        (pgFmtLit, unquoted, pgFmtSetLocal)
 
 import           Protolude                     hiding (concat, null)
 
@@ -32,13 +32,14 @@ runWithClaims conf eClaims app req =
     JWTInvalid e -> return $ unauthed $ show e
     JWTMissingSecret -> return $ simpleError status500 [] "Server lacks JWT secret"
     JWTClaims claims -> do
-      H.sql $ toS.mconcat $ setSchemaSql ++ setRoleSql ++ claimsSql ++ headersSql ++ cookiesSql
+      H.sql $ toS.mconcat $ setSchemaSql ++ setRoleSql ++ claimsSql ++ headersSql ++ cookiesSql ++ appSettingsSql
       mapM_ H.sql customReqCheck
       app req
       where
-        headersSql = map (pgFmtEnvVar "request.header.") $ iHeaders req
+        headersSql = pgFmtSetLocal "request.header." <$> iHeaders req
-        cookiesSql = map (pgFmtEnvVar "request.cookie.") $ iCookies req
+        cookiesSql = pgFmtSetLocal "request.cookie." <$> iCookies req
-        claimsSql = map (pgFmtEnvVar "request.jwt.claim.") [(c,unquoted v) | (c,v) <- M.toList claimsWithRole]
+        claimsSql = pgFmtSetLocal "request.jwt.claim." <$> [(c,unquoted v) | (c,v) <- M.toList claimsWithRole]
+        appSettingsSql = pgFmtSetLocal mempty <$> configSettings conf
         setRoleSql = maybeToList $
           (\r -> "set local role " <> r <> ";") . toS . pgFmtLit . unquoted <$> M.lookup "role" claimsWithRole
         setSchemaSql = ["set schema " <> pgFmtLit (configSchema conf) <> ";"] :: [Text]

src/PostgREST/QueryBuilder.hs

@@ -23,7 +23,7 @@ module PostgREST.QueryBuilder (
   , requestToCountQuery
   , unquoted
   , ResultsWithCount
-  , pgFmtEnvVar
+  , pgFmtSetLocal
   ) where
 
 import qualified Hasql.Query             as H
@@ -469,8 +469,8 @@ pgFmtAs fName jp Nothing = case jOp <$> lastMay jp of
   Nothing -> ""
 pgFmtAs _ _ (Just alias) = " AS " <> pgFmtIdent alias
 
-pgFmtEnvVar :: Text -> (Text, Text) -> SqlFragment
+pgFmtSetLocal :: Text -> (Text, Text) -> SqlFragment
-pgFmtEnvVar prefix (k, v) =
+pgFmtSetLocal prefix (k, v) =
   "set local " <> pgFmtIdent (prefix <> k) <> " = " <> pgFmtLit v <> ";"
 
 trimNullChars :: Text -> Text

test/Main.hs

@@ -6,8 +6,8 @@ import SpecHelper
 import qualified Hasql.Pool as P
 
 import PostgREST.App (postgrest)
-import PostgREST.Config (pgVersion95, pgVersion96, configSettings)
+import PostgREST.Config (pgVersion95, pgVersion96)
-import PostgREST.DbStructure (getDbStructure, getPgVersion, fillSessionWithSettings)
+import PostgREST.DbStructure (getDbStructure, getPgVersion)
 import PostgREST.Types (DbStructure(..))
 import Control.AutoUpdate (defaultUpdateSettings, mkAutoUpdate, updateAction)
 import Data.Function (id)
@@ -66,7 +66,7 @@ main = do
       nonexistentSchemaApp = return $ postgrest (testNonexistentSchemaCfg testDbConn)   refDbStructure pool getTime $ pure ()
 
   let reset :: IO ()
-      reset = P.use pool (fillSessionWithSettings (configSettings $ testCfg testDbConn)) >> resetDb testDbConn
+      reset = resetDb testDbConn
 
       actualPgVersion = pgVersion dbStructure
       extraSpecs =

test/io-tests.sh

@@ -143,6 +143,26 @@ ensureIatClaimWorks(){
   pgrStop
 }
 
+# ensure app settings don't reset on pool timeout of 10 seconds, see https://github.com/PostgREST/postgrest/issues/1141
+ensureAppSettings(){
+  pgrStart "./configs/app-settings.config"
+  while pgrStarted && test "$( rootStatus )" -ne 200
+  do
+    # wait for the server to start
+    sleep 0.1 \
+    || sleep 1 # fallback: subsecond sleep is not standard and may fail
+  done
+  sleep 11
+  response=$(curl -s "http://localhost:$pgrPort/rpc/get_guc_value?name=app.settings.external_api_secret")
+  if test "$response" = "\"0123456789abcdef\""
+  then
+    ok "GET /rpc/get_guc_value response is $response"
+  else
+    ko "GET /rpc/get_guc_value response was $response"
+  fi
+  pgrStop
+}
+
 # PRE: curl must be available
 test -n "$(command -v curl)" || bailOut 'curl is not available'
 
@@ -181,6 +201,7 @@ invalidRoleClaimKey ''
 invalidRoleClaimKey 1234
 
 ensureIatClaimWorks
+ensureAppSettings
 
 cleanUp
 

test/io-tests/configs/app-settings.config

@@ -0,0 +1,8 @@
+db-uri = "postgres:///postgrest_test"
+db-schema = "test"
+db-anon-role = "postgrest_test_anonymous"
+db-pool = 1
+server-host = "127.0.0.1"
+server-port = 49421
+
+app.settings.external_api_secret = "0123456789abcdef"