refactor: dry RPC param parsing

src/PostgREST/ApiRequest.hs

@@ -182,9 +182,9 @@ data ApiRequest = ApiRequest {
 -- | Examines HTTP request and translates it into user intent.
 userApiRequest :: AppConfig -> SchemaCache -> Request -> RequestBody -> Either ApiRequestError ApiRequest
 userApiRequest conf sCache req reqBody = do
-  qPrms <- first QueryParamError $ QueryParams.parse $ rawQueryString req
   pInfo <- getPathInfo conf $ pathInfo req
   act <- getAction pInfo $ requestMethod req
+  qPrms <- first QueryParamError $ QueryParams.parse (pathIsProc pInfo && act `elem` [ActionInvoke InvGet, ActionInvoke InvHead]) $ rawQueryString req
   mediaTypes <- getMediaTypes conf (requestHeaders req) act pInfo
   negotiatedSchema <- getSchema conf (requestHeaders req) (requestMethod req)
   apiRequest conf sCache req reqBody qPrms pInfo act mediaTypes negotiatedSchema
@@ -253,7 +253,6 @@ apiRequest :: AppConfig -> SchemaCache -> Request -> RequestBody -> QueryParams.
 apiRequest conf sCache req reqBody queryparams@QueryParams{..} PathInfo{pathName, pathIsProc, pathIsRootSpec, pathIsDefSpec} action (acceptMediaType, contentMediaType) (schema, negotiatedByProfile)
   | isInvalidRange = Left $ InvalidRange (if rangeIsEmpty headerRange then LowerGTUpper else NegativeLimit)
   | shouldParsePayload && isLeft payload = either (Left . InvalidBody) witness payload
-  | not expectParams && not (L.null qsParams) = Left $ ParseRequestError "Unexpected param or filter missing operator" ("Failed to parse " <> show qsParams)
   | method `elem` ["PATCH", "DELETE"] && not (null qsRanges) && null qsOrder = Left LimitNoOrderError
   | method == "PUT" && topLevelRange /= allRange = Left PutRangeNotAllowedError
   | otherwise = do
@@ -282,8 +281,6 @@ apiRequest conf sCache req reqBody queryparams@QueryParams{..} PathInfo{pathName
       , iBinaryField = bField
       }
  where
-  expectParams = pathIsProc && method /= "POST"
-
   columns = case action of
     ActionMutate MutationCreate -> qsColumns
     ActionMutate MutationUpdate -> qsColumns

src/PostgREST/ApiRequest/QueryParams.hs

@@ -114,39 +114,45 @@ data QueryParams =
 --
 -- The canonical representation of the query string has parameters sorted alphabetically:
 --
--- >>> qsCanonical <$> parse "a=1&c=3&b=2&d"
+-- >>> qsCanonical <$> parse True "a=1&c=3&b=2&d"
 -- Right "a=1&b=2&c=3&d="
 --
 -- 'select' is a reserved parameter that selects the fields to be returned:
 --
--- >>> qsSelect <$> parse "select=name,location"
+-- >>> qsSelect <$> parse False "select=name,location"
 -- Right [Node {rootLabel = SelectField {selField = ("name",[]), selCast = Nothing, selAlias = Nothing}, subForest = []},Node {rootLabel = SelectField {selField = ("location",[]), selCast = Nothing, selAlias = Nothing}, subForest = []}]
 --
 -- Filters are parameters whose value contains an operator, separated by a '.' from its value:
 --
--- >>> qsFilters <$> parse "a.b=eq.0"
+-- >>> qsFilters <$> parse False "a.b=eq.0"
 -- Right [(["a"],Filter {field = ("b",[]), opExpr = OpExpr False (Op OpEqual "0")})]
 --
 -- If the operator specified in a filter does not exist, parsing the query string fails:
 --
--- >>> qsFilters <$> parse "a.b=noop.0"
+-- >>> qsFilters <$> parse False "a.b=noop.0"
--- Left (QPError "\"failed to parse filter (noop.0)\" (line 1, column 6)" "unknown single value operator noop")
+-- Left (QPError "\"failed to parse filter (noop.0)\" (line 1, column 6)" "expecting operator (eq, gt, ...) unknown single value operator noop")
-parse :: ByteString -> Either QPError QueryParams
+parse :: Bool -> ByteString -> Either QPError QueryParams
-parse qs =
+parse isRpcGet qs = do
-  QueryParams
+  rOrd                      <- pRequestOrder `traverse` order
-    canonical
+  rLogic                    <- pRequestLogicTree `traverse` logic
-    params
+  rCols                     <- pRequestColumns columns
-    ranges
+  rSel                      <- pRequestSelect select
-    <$> pRequestOrder `traverse` order
+  (rFlts, params)           <- L.partition hasOp <$> pRequestFilter isRpcGet `traverse` filters
-    <*> pRequestLogicTree `traverse` logic
+  (rFltsRoot, rFltsNotRoot) <- pure $ L.partition hasRootFilter rFlts
-    <*> pRequestColumns columns
+  rOnConflict               <- pRequestOnConflict `traverse` onConflict
-    <*> pRequestSelect select
+
-    <*> pRequestFilter `traverse` filters
+  let rFltsFields           = S.fromList (fst <$> filters)
-    <*> (fmap snd <$> (pRequestFilter `traverse` filtersRoot))
+      params'               = mapMaybe (\case {(_, Filter (fld, _) (NoOpExpr v)) -> Just (fld,v); _ -> Nothing}) params
-    <*> pRequestFilter `traverse` filtersNotRoot
+      rFltsRoot'            = snd <$> rFltsRoot
-    <*> pure (S.fromList (fst <$> filters))
+
-    <*> pRequestOnConflict `traverse` onConflict
+  return $ QueryParams canonical params' ranges rOrd rLogic rCols rSel rFlts rFltsRoot' rFltsNotRoot rFltsFields rOnConflict
   where
+    hasRootFilter, hasOp :: (EmbedPath, Filter) -> Bool
+    hasRootFilter ([], _) = True
+    hasRootFilter _       = False
+    hasOp (_, Filter (_, _) (NoOpExpr _)) = False
+    hasOp _                               = True
+
     logic = filter (endingIn ["and", "or"] . fst) nonemptyParams
     select = fromMaybe "*" $ lookupParam "select"
     onConflict = lookupParam "on_conflict"
@@ -173,32 +179,11 @@ parse qs =
     endingIn xx key = lastWord `elem` xx
       where lastWord = L.last $ T.split (== '.') key
 
-    (filters, params) = L.partition isParam filtersAndParams
+    filters = filter (isFilter . fst) nonemptyParams
-    isParam (k, v) = isEmbedPath k || hasOperator v || hasFtsOperator v
+    isFilter k = not (endingIn reservedEmbeddable k) && notElem k reserved
-
-    filtersAndParams = filter (isFilterOrParam . fst)  nonemptyParams
-    isFilterOrParam k = not (endingIn reservedEmbeddable k) && notElem k reserved
     reserved = ["select", "columns", "on_conflict"]
     reservedEmbeddable = ["order", "limit", "offset", "and", "or"]
 
-    (filtersNotRoot, filtersRoot) = L.partition isNotRoot filters
-    isNotRoot = flip T.isInfixOf "." . fst
-
-    -- TODO: These checks are redundant to the parsers, should use parsers to differentiate params
-    hasOperator val =
-      case T.splitOn "." val of
-        "not" : _ : _ -> True
-        "is" : _      -> True
-        "in" : _      -> True
-        x : _         -> isJust (operator x) || isJust (ftsOperator x)
-        _             -> False
-
-    hasFtsOperator val =
-      case T.splitOn "(" val of
-        x : _ : _ -> isJust $ ftsOperator x
-        _         -> False
-
-    isEmbedPath = T.isInfixOf "."
     replaceLast x s = T.intercalate "." $ L.init (T.split (=='.') s) <> [x]
 
     ranges :: HM.HashMap Text (Range Integer)
@@ -237,18 +222,8 @@ operator = \case
   "imatch" -> Just OpIMatch
   _        -> Nothing
 
-ftsOperator :: Text -> Maybe FtsOperator
-ftsOperator = \case
-  "fts"   -> Just FilterFts
-  "plfts" -> Just FilterFtsPlain
-  "phfts" -> Just FilterFtsPhrase
-  "wfts"  -> Just FilterFtsWebsearch
-  _       -> Nothing
-
-
 -- PARSERS
 
-
 pRequestSelect :: Text -> Either QPError [Tree SelectItem]
 pRequestSelect selStr =
   mapError $ P.parse pFieldForest ("failed to parse select parameter (" <> toS selStr <> ")") (toS selStr)
@@ -257,11 +232,25 @@ pRequestOnConflict :: Text -> Either QPError [FieldName]
 pRequestOnConflict oncStr =
   mapError $ P.parse pColumns ("failed to parse on_conflict parameter (" <> toS oncStr <> ")") (toS oncStr)
 
-pRequestFilter :: (Text, Text) -> Either QPError (EmbedPath, Filter)
+-- |
-pRequestFilter (k, v) = mapError $ (,) <$> path <*> (Filter <$> fld <*> oper)
+-- Parse `id=eq.1`(id, eq.1) into (EmbedPath, Filter)
+--
+-- >>> pRequestFilter False ("id", "eq.1")
+-- Right ([],Filter {field = ("id",[]), opExpr = OpExpr False (Op OpEqual "1")})
+--
+-- >>> pRequestFilter False ("id", "val")
+-- Left (QPError "\"failed to parse filter (val)\" (line 1, column 4)" "unexpected end of input expecting operator (eq, gt, ...)")
+--
+-- >>> pRequestFilter True ("id", "val")
+-- Right ([],Filter {field = ("id",[]), opExpr = NoOpExpr "val"})
+pRequestFilter :: Bool -> (Text, Text) -> Either QPError (EmbedPath, Filter)
+pRequestFilter isRpcGet (k, v) = mapError $ (,) <$> path <*> (Filter <$> fld <*> oper)
   where
     treePath = P.parse pTreePath ("failed to parse tree path (" ++ toS k ++ ")") $ toS k
-    oper = P.parse (pOpExpr pSingleVal) ("failed to parse filter (" ++ toS v ++ ")") $ toS v
+    oper = P.parse parseFlt ("failed to parse filter (" ++ toS v ++ ")") $ toS v
+    parseFlt = if isRpcGet
+      then pOpExpr pSingleVal <|> pure (NoOpExpr v)
+      else pOpExpr pSingleVal
     path = fst <$> treePath
     fld = snd <$> treePath
 
@@ -592,12 +581,15 @@ pEmbedParams = do
 --
 -- >>> P.parse (pOpExpr pSingleVal) "" "fts().value"
 -- Left (line 1, column 7):
+-- expecting operator (eq, gt, ...)
 -- unknown single value operator fts()
 pOpExpr :: Parser SingleVal -> Parser OpExpr
-pOpExpr pSVal = try ( string "not" *> pDelimiter *> (OpExpr True <$> pOperation)) <|> OpExpr False <$> pOperation
+pOpExpr pSVal = do
+  boolExpr <- try (string "not" *> pDelimiter $> True) <|> pure False
+  OpExpr boolExpr <$> pOperation
   where
     pOperation :: Parser Operation
-    pOperation = pIn <|> pIs <|> try pFts <|> pOp <?> "operator (eq, gt, ...)"
+    pOperation = pIn <|> pIs <|> try pFts <|> try pOp <?> "operator (eq, gt, ...)"
 
     pIn = In <$> (try (string "in" *> pDelimiter) *> pListVal)
     pIs = Is <$> (try (string "is" *> pDelimiter) *> pTriVal)
@@ -614,8 +606,11 @@ pOpExpr pSVal = try ( string "not" *> pDelimiter *> (OpExpr True <$> pOperation)
           <?> "null or trilean value (unknown, true, false)"
 
     pFts = do
-      opStr <- try (P.many (noneOf ".("))
+      op <-  try (string "fts"   $> FilterFts)
-      op <- parseMaybe ("unknown fts operator " <> opStr) . ftsOperator $ toS opStr
+         <|> try (string "plfts" $> FilterFtsPlain)
+         <|> try (string "phfts" $> FilterFtsPhrase)
+         <|> try (string "wfts"  $> FilterFtsWebsearch)
+
       lang <- optionMaybe $ try (between (char '(') (char ')') pIdentifier)
       pDelimiter >> Fts op (toS <$> lang) <$> pSVal
 

src/PostgREST/ApiRequest/Types.hs

@@ -76,7 +76,6 @@ data ApiRequestError
   | NoRelBetween Text Text (Maybe Text) Text RelationshipsMap
   | NoRpc Text Text [Text] Bool MediaType Bool [QualifiedIdentifier] [ProcDescription]
   | NotEmbedded Text
-  | ParseRequestError Text Text
   | PutRangeNotAllowedError
   | QueryParamError QPError
   | RelatedOrderNotToOne Text Text
@@ -183,8 +182,9 @@ data Filter
   | FilterNullEmbed Bool FieldName
   deriving (Eq)
 
-data OpExpr =
+data OpExpr
-  OpExpr Bool Operation
+  = OpExpr Bool Operation
+  | NoOpExpr Text
   deriving (Eq)
 
 data Operation

src/PostgREST/Error.hs

@@ -72,7 +72,6 @@ instance PgrstError ApiRequestError where
   status NoRelBetween{}          = HTTP.status400
   status NoRpc{}                 = HTTP.status404
   status NotEmbedded{}           = HTTP.status400
-  status ParseRequestError{}     = HTTP.status400
   status PutRangeNotAllowedError = HTTP.status400
   status QueryParamError{}       = HTTP.status400
   status RelatedOrderNotToOne{}  = HTTP.status400
@@ -109,11 +108,6 @@ instance JSON.ToJSON ApiRequestError where
                    LowerGTUpper            -> "The lower boundary must be lower than or equal to the upper boundary in the Range header."
                    OutOfBounds lower total -> "An offset of " <> lower <> " was requested, but there are only " <> total <> " rows."),
     "hint"    .= JSON.Null]
-  toJSON (ParseRequestError message details) = JSON.object [
-    "code"    .= ApiRequestErrorCode04,
-    "message" .= message,
-    "details" .= details,
-    "hint"    .= JSON.Null]
   toJSON InvalidFilters = JSON.object [
     "code"    .= ApiRequestErrorCode05,
     "message" .= ("Filters must include all and only primary key columns with 'eq' operators" :: Text),
@@ -586,7 +580,7 @@ data ErrorCode
   | ApiRequestErrorCode01
   | ApiRequestErrorCode02
   | ApiRequestErrorCode03
-  | ApiRequestErrorCode04
+  | ApiRequestErrorCode04 -- no longer used (used to be mapped to ParseRequestError)
   | ApiRequestErrorCode05
   | ApiRequestErrorCode06
   | ApiRequestErrorCode07

src/PostgREST/Plan.hs

@@ -33,7 +33,6 @@ import Data.Tree               (Tree (..))
 
 import PostgREST.ApiRequest               (Action (..),
                                            ApiRequest (..),
-                                           InvokeMethod (..),
                                            Mutation (..),
                                            Payload (..))
 import PostgREST.Config                   (AppConfig (..))
@@ -291,11 +290,9 @@ addFilters ApiRequest{..} rReq =
     QueryParams.QueryParams{..} = iQueryParams
     flts =
       case iAction of
-        ActionInvoke InvGet  -> qsFilters
+        ActionInvoke _ -> qsFilters
-        ActionInvoke InvHead -> qsFilters
+        ActionRead _   -> qsFilters
-        ActionInvoke _       -> qsFilters
+        _              -> qsFiltersNotRoot
-        ActionRead _         -> qsFilters
-        _                    -> qsFiltersNotRoot
 
     addFilterToNode :: (EmbedPath, Filter) -> Either ApiRequestError ReadPlanTree ->  Either ApiRequestError ReadPlanTree
     addFilterToNode =

src/PostgREST/Query/SqlFragment.hs

@@ -278,6 +278,7 @@ pgFmtOrderTerm qi ot =
 
 pgFmtFilter :: QualifiedIdentifier -> Filter -> SQL.Snippet
 pgFmtFilter _ (FilterNullEmbed hasNot fld) = SQL.sql (pgFmtIdent fld) <> " IS " <> (if hasNot then "NOT" else mempty) <> " NULL"
+pgFmtFilter _ (Filter _ (NoOpExpr _)) = mempty -- TODO unreachable because NoOpExpr is filtered on QueryParams
 pgFmtFilter table (Filter fld (OpExpr hasNot oper)) = notOp <> " " <> case oper of
    Op op val  -> pgFmtFieldOp op <> " " <> case op of
      OpLike  -> unknownLiteral (T.map star val)

test/spec/Feature/Query/QuerySpec.hs

@@ -1002,7 +1002,8 @@ spec actualPgVersion = do
         { matchHeaders = [matchContentTypeJson] }
 
     it "fails if an operator is not given" $
-      get "/ghostBusters?id=0" `shouldRespondWith` [json| {"details":"Failed to parse [(\"id\",\"0\")]","message":"Unexpected param or filter missing operator","code":"PGRST104","hint":null} |]
+      get "/ghostBusters?id=0" `shouldRespondWith`
+        [json| {"code":"PGRST100","details":"unexpected end of input expecting operator (eq, gt, ...)","hint":null,"message":"\"failed to parse filter (0)\" (line 1, column 2)"} |]
         { matchStatus  = 400
         , matchHeaders = [matchContentTypeJson]
         }