From 59abecaf5b7339317b7743629ccf5dea4af87f7f Mon Sep 17 00:00:00 2001 From: SteveBash Date: Sun, 16 Apr 2017 00:34:57 -0500 Subject: [PATCH] Add missing tests for NOT IN accepting empty values and refactor Go back to operators map since the previously defined Operator constructors added innecesary complexity also rename Path to EmbedPath --- src/PostgREST/DbRequestBuilder.hs | 14 ++--- src/PostgREST/OpenAPI.hs | 7 +-- src/PostgREST/Parsers.hs | 26 +++++----- src/PostgREST/QueryBuilder.hs | 48 +++++++++-------- src/PostgREST/Types.hs | 86 +++++++++---------------------- test/Feature/QuerySpec.hs | 15 ++++-- test/fixtures/data.sql | 3 ++ 7 files changed, 86 insertions(+), 113 deletions(-) diff --git a/src/PostgREST/DbRequestBuilder.hs b/src/PostgREST/DbRequestBuilder.hs index 61e701fad..18859b1c4 100644 --- a/src/PostgREST/DbRequestBuilder.hs +++ b/src/PostgREST/DbRequestBuilder.hs @@ -180,7 +180,7 @@ addFiltersOrdersRanges apiRequest = foldr1 (liftA2 (.)) [ of type (ReadRequest->ReadRequest) that are in (Either ParseError a) context -} where - filters :: Either ApiRequestError [(Path, Filter)] + filters :: Either ApiRequestError [(EmbedPath, Filter)] filters = mapM pRequestFilter flts where action = iAction apiRequest @@ -188,30 +188,30 @@ addFiltersOrdersRanges apiRequest = foldr1 (liftA2 (.)) [ | action == ActionRead = iFilters apiRequest | action == ActionInvoke = iFilters apiRequest | otherwise = filter (( "." `isInfixOf` ) . fst) $ iFilters apiRequest -- there can be no filters on the root table whre we are doing insert/update - orders :: Either ApiRequestError [(Path, [OrderTerm])] + orders :: Either ApiRequestError [(EmbedPath, [OrderTerm])] orders = mapM pRequestOrder $ iOrder apiRequest - ranges :: Either ApiRequestError [(Path, NonnegRange)] + ranges :: Either ApiRequestError [(EmbedPath, NonnegRange)] ranges = mapM pRequestRange $ M.toList $ iRange apiRequest addFilterToNode :: Filter -> ReadRequest -> ReadRequest addFilterToNode flt (Node (q@Select {flt_=flts}, i) f) = Node (q {flt_=flt:flts}, i) f -addFilter :: (Path, Filter) -> ReadRequest -> ReadRequest +addFilter :: (EmbedPath, Filter) -> ReadRequest -> ReadRequest addFilter = addProperty addFilterToNode addOrderToNode :: [OrderTerm] -> ReadRequest -> ReadRequest addOrderToNode o (Node (q,i) f) = Node (q{order=Just o}, i) f -addOrder :: (Path, [OrderTerm]) -> ReadRequest -> ReadRequest +addOrder :: (EmbedPath, [OrderTerm]) -> ReadRequest -> ReadRequest addOrder = addProperty addOrderToNode addRangeToNode :: NonnegRange -> ReadRequest -> ReadRequest addRangeToNode r (Node (q,i) f) = Node (q{range_=r}, i) f -addRange :: (Path, NonnegRange) -> ReadRequest -> ReadRequest +addRange :: (EmbedPath, NonnegRange) -> ReadRequest -> ReadRequest addRange = addProperty addRangeToNode -addProperty :: (a -> ReadRequest -> ReadRequest) -> (Path, a) -> ReadRequest -> ReadRequest +addProperty :: (a -> ReadRequest -> ReadRequest) -> (EmbedPath, a) -> ReadRequest -> ReadRequest addProperty f ([], a) n = f a n addProperty f (path, a) (Node rn forest) = case targetNode of diff --git a/src/PostgREST/OpenAPI.hs b/src/PostgREST/OpenAPI.hs index ac1df669d..f75acefb7 100644 --- a/src/PostgREST/OpenAPI.hs +++ b/src/PostgREST/OpenAPI.hs @@ -8,11 +8,12 @@ module PostgREST.OpenAPI ( import Control.Lens import Data.Aeson (decode, encode) +import qualified Data.HashMap.Strict as M import Data.HashMap.Strict.InsOrd (InsOrdHashMap, fromList) import Data.Maybe (fromJust) +import qualified Data.Set as Set import Data.String (IsString (..)) import Data.Text (unpack, pack, concat, intercalate, init, tail, toLower) -import qualified Data.Set as Set import Network.URI (parseURI, isAbsoluteURI, URI (..), URIAuth (..)) @@ -23,7 +24,7 @@ import Data.Swagger import PostgREST.ApiRequest (ContentType(..)) import PostgREST.Config (prettyVersion) import PostgREST.Types (Table(..), Column(..), PgArg(..), - Proxy(..), ProcDescription(..), toMime, Operator(..)) + Proxy(..), ProcDescription(..), toMime, operators) makeMimeList :: [ContentType] -> MimeList makeMimeList cs = MimeList $ map (fromString . toS . toMime) cs @@ -72,7 +73,7 @@ makeOperatorPattern = intercalate "|" [ concat ["^", x, y, "[.]"] | x <- ["not[.]", ""], - y <- map show [Equals ..] ] + y <- M.keys operators ] makeRowFilter :: Column -> Param makeRowFilter c = diff --git a/src/PostgREST/Parsers.hs b/src/PostgREST/Parsers.hs index bef28972b..ebfc7f2fd 100644 --- a/src/PostgREST/Parsers.hs +++ b/src/PostgREST/Parsers.hs @@ -1,23 +1,23 @@ module PostgREST.Parsers where import Protolude hiding (try, intercalate) -import Control.Monad ((>>)) -import Data.Foldable (foldl1) +import Control.Monad ((>>)) +import Data.Foldable (foldl1) +import qualified Data.HashMap.Strict as M import Data.Text (intercalate, replace, strip) import Data.List (init, last) import Data.Tree import Data.Either.Combinators (mapLeft) +import PostgREST.RangeQuery (NonnegRange,allRange) import PostgREST.Types import Text.ParserCombinators.Parsec hiding (many, (<|>)) -import Text.Read (read) -import PostgREST.RangeQuery (NonnegRange,allRange) import Text.Parsec.Error pRequestSelect :: Text -> Text -> Either ApiRequestError ReadRequest pRequestSelect rootName selStr = mapError $ parse (pReadRequest rootName) ("failed to parse select parameter (" <> toS selStr <> ")") (toS selStr) -pRequestFilter :: (Text, Text) -> Either ApiRequestError (Path, Filter) +pRequestFilter :: (Text, Text) -> Either ApiRequestError (EmbedPath, Filter) pRequestFilter (k, v) = mapError $ (,) <$> path <*> (Filter <$> fld <*> oper) where treePath = parse pTreePath ("failed to parser tree path (" ++ toS k ++ ")") $ toS k @@ -25,14 +25,14 @@ pRequestFilter (k, v) = mapError $ (,) <$> path <*> (Filter <$> fld <*> oper) path = fst <$> treePath fld = snd <$> treePath -pRequestOrder :: (Text, Text) -> Either ApiRequestError (Path, [OrderTerm]) +pRequestOrder :: (Text, Text) -> Either ApiRequestError (EmbedPath, [OrderTerm]) pRequestOrder (k, v) = mapError $ (,) <$> path <*> ord' where treePath = parse pTreePath ("failed to parser tree path (" ++ toS k ++ ")") $ toS k path = fst <$> treePath ord' = parse pOrder ("failed to parse order (" ++ toS v ++ ")") $ toS v -pRequestRange :: (ByteString, NonnegRange) -> Either ApiRequestError (Path, NonnegRange) +pRequestRange :: (ByteString, NonnegRange) -> Either ApiRequestError (EmbedPath, NonnegRange) pRequestRange (k, v) = mapError $ (,) <$> path <*> pure v where treePath = parse pTreePath ("failed to parser tree path (" ++ toS k ++ ")") $ toS k @@ -59,7 +59,7 @@ pReadRequest rootNodeName = do newForest = foldr treeEntry (Node (Select [] [fn] [] Nothing allRange, (fn, Nothing, alias)) []) fldForest:rForest -pTreePath :: Parser (Path,Field) +pTreePath :: Parser (EmbedPath, Field) pTreePath = do p <- pFieldName `sepBy1` pDelimiter jp <- optionMaybe pJsonPath @@ -124,17 +124,17 @@ pOperation = try ( string "not" *> pDelimiter *> (Operation True <$> pExpr)) <|> where pExpr :: Parser (Operator, Operand) pExpr = - ((,) <$> (read <$> foldl1 (<|>) (try . string . show <$> notInOps)) <*> (pDelimiter *> pVText)) - <|> try (string (show In) *> pDelimiter *> ((,) <$> pure In <*> pVTextL)) - <|> try (string (show NotIn) *> pDelimiter *> ((,) <$> pure NotIn <*> pVTextL)) + ((,) <$> (toS <$> foldl1 (<|>) (try . ((<* pDelimiter) . string) . toS <$> M.keys notInOps)) <*> pVText) + <|> ((,) <$> (toS <$> foldl1 (<|>) (try . ((<* pDelimiter) . string) . toS <$> M.keys inOps)) <*> pVTextL) "operator (eq, gt, ...)" - notInOps = [Equals .. Contained] + inOps = M.filterWithKey (const . flip elem ["in", "notin"]) operators + notInOps = M.difference operators inOps pVText :: Parser Operand pVText = VText . toS <$> many anyChar pVTextL :: Parser Operand -pVTextL = VTextL <$> pLValue `sepBy1` char ',' +pVTextL = VTextL <$> lexeme pLValue `sepBy1` char ',' where pLValue :: Parser Text pLValue = toS <$> (try (char '"' *> many (noneOf "\"") <* char '"' <* notFollowedBy (noneOf ",") ) <|> many (noneOf ",")) diff --git a/src/PostgREST/QueryBuilder.hs b/src/PostgREST/QueryBuilder.hs index 92772356e..5c790dd4d 100644 --- a/src/PostgREST/QueryBuilder.hs +++ b/src/PostgREST/QueryBuilder.hs @@ -379,7 +379,7 @@ getJoinConditions (Relation t cols ft fcs typ lt lc1 lc2) = ftN = tableName ft ltN = fromMaybe "" (tableName <$> lt) toFilter :: Text -> Text -> Column -> Column -> Filter - toFilter tb ftb c fc = Filter (colName c, Nothing) (Operation False (Equals, VForeignKey (QualifiedIdentifier s tb) (ForeignKey fc{colTable=(colTable fc){tableName=ftb}}))) + toFilter tb ftb c fc = Filter (colName c, Nothing) (Operation False ("=", VForeignKey (QualifiedIdentifier s tb) (ForeignKey fc{colTable=(colTable fc){tableName=ftb}}))) unicodeStatement :: Text -> HE.Params a -> HD.Result b -> Bool -> H.Query a b unicodeStatement = H.statement . T.encodeUtf8 @@ -407,39 +407,37 @@ pgFmtSelectItem table (f@(_, jp), Nothing, alias) = pgFmtField table f <> pgFmtA pgFmtSelectItem table (f@(_, jp), Just cast, alias) = "CAST (" <> pgFmtField table f <> " AS " <> cast <> " )" <> pgFmtAs jp alias pgFmtFilter :: QualifiedIdentifier -> Filter -> SqlFragment -pgFmtFilter table (Filter fld (Operation hasNot_ ex@(op, operand))) = notOp <> " " <> case operand of - VForeignKey fQi (ForeignKey Column{colTable=Table{tableName=fTableName}, colName=fColName}) -> - pgFmtField fQi fld <> " " <> opToSqlFragment op <> " " <> pgFmtColumn (removeSourceCTESchema (qiSchema fQi) fTableName) fColName - _ -> pgFmtField table fld <> " " <> pgFmtExpr ex - where - notOp = if hasNot_ then "NOT" else "" - -pgFmtExpr :: (Operator, Operand) -> SqlFragment -pgFmtExpr ex = - case ex of - (Like, VText val) -> opToSqlFragment Like <> " " <> unknownLiteral (T.map star val) - (ILike, VText val) -> opToSqlFragment ILike <> " " <> unknownLiteral (T.map star val) - (TSearch, VText val) -> opToSqlFragment TSearch <> " " <> "to_tsquery(" <> unknownLiteral val <> ") " - (Is, VText val) -> opToSqlFragment Is <> " " <> whiteList val - (In, VTextL vals) -> exprForIn vals - (NotIn, VTextL vals) -> opToSqlFragment NotIn <> " " <> "(" <> intercalate ", " (map unknownLiteral vals) <> ") " - (op, VText val) -> opToSqlFragment op <> " " <> unknownLiteral val - _ -> "" -- should not happen, all possible combinations are defined in Parsers +pgFmtFilter table (Filter fld (Operation hasNot_ ex)) = notOp <> " " <> case ex of + (op, VText val) -> pgFmtFieldOp op <> " " <> case op of + "like" -> unknownLiteral (T.map star val) + "ilike" -> unknownLiteral (T.map star val) + "@@" -> "to_tsquery(" <> unknownLiteral val <> ") " + "is" -> whiteList val + "isnot" -> whiteList val + _ -> unknownLiteral val + (op, VTextL vals) -> pgFmtIn op vals -- in and notin + (op, VForeignKey fQi (ForeignKey Column{colTable=Table{tableName=fTableName}, colName=fColName})) -> + pgFmtField fQi fld <> " " <> sqlOperator op <> " " <> pgFmtColumn (removeSourceCTESchema (qiSchema fQi) fTableName) fColName where + pgFmtFieldOp op = pgFmtField table fld <> " " <> sqlOperator op + sqlOperator o = HM.lookupDefault "=" o operators + notOp = if hasNot_ then "NOT" else "" star c = if c == '*' then '%' else c unknownLiteral = (<> "::unknown ") . pgFmtLit whiteList :: Text -> SqlFragment whiteList v = fromMaybe (toS (pgFmtLit v) <> "::unknown ") (find ((==) . toLower $ v) ["null","true","false"]) - exprForIn :: [Text] -> SqlFragment - exprForIn vals = - let emptyValForIn = "= any('{}') " in + pgFmtIn :: Operator -> [Text] -> SqlFragment + pgFmtIn op vals = + -- Workaround because for postgresql "col IN ()" is invalid syntax, we instead do "col = any('{}')" + let emptyValForIn o = (if "not" `isInfixOf` o then "NOT " else "") -- handle case of "notin" operator + <> pgFmtField table fld <> " = any('{}') " in case T.null <$> headMay vals of Just isNull -> if isNull && length vals == 1 - then emptyValForIn - else opToSqlFragment In <> " " <> "(" <> intercalate ", " (map unknownLiteral vals) <> ") " - Nothing -> emptyValForIn + then emptyValForIn op + else pgFmtFieldOp op <> "(" <> intercalate ", " (map unknownLiteral vals) <> ") " + Nothing -> emptyValForIn op pgFmtJsonPath :: Maybe JsonPath -> SqlFragment pgFmtJsonPath (Just [x]) = "->>" <> pgFmtLit x diff --git a/src/PostgREST/Types.hs b/src/PostgREST/Types.hs index 3fb0f2204..08ad20685 100644 --- a/src/PostgREST/Types.hs +++ b/src/PostgREST/Types.hs @@ -1,10 +1,9 @@ module PostgREST.Types where import Protolude import qualified GHC.Show -import qualified GHC.Read import Data.Aeson import qualified Data.ByteString.Lazy as BL -import Data.HashMap.Strict as M +import qualified Data.HashMap.Strict as M import Data.Tree import qualified Data.Vector as V import PostgREST.RangeQuery (NonnegRange) @@ -137,66 +136,27 @@ data Proxy = Proxy { , proxyPath :: Text } deriving (Show, Eq) -data Operator = Equals | Gte | Gt | Lte | Lt | Neq | Like | ILike | Is | IsNot | - TSearch | Contains | Contained | In | NotIn deriving (Eq, Enum) - -instance Show Operator where - show op = case op of - Equals -> "eq" - Gte -> "gte" - Gt -> "gt" - Lte -> "lte" - Lt -> "lt" - Neq -> "neq" - Like -> "like" - ILike -> "ilike" - In -> "in" - NotIn -> "notin" - IsNot -> "isnot" - Is -> "is" - TSearch -> "@@" - Contains -> "@>" - Contained -> "<@" - -instance Read Operator where - readsPrec _ op = case op of - "eq" -> [(Equals, "")] - "gte" -> [(Gte, "")] - "gt" -> [(Gt, "")] - "lte" -> [(Lte, "")] - "lt" -> [(Lt, "")] - "neq" -> [(Neq, "")] - "like" -> [(Like, "")] - "ilike" -> [(ILike, "")] - "in" -> [(In, "")] - "notin" -> [(NotIn, "")] - "isnot" -> [(IsNot, "")] - "is" -> [(Is, "")] - "@@" -> [(TSearch, "")] - "@>" -> [(Contains, "")] - "<@" -> [(Contained, "")] - _ -> [] - -opToSqlFragment :: Operator -> SqlFragment -opToSqlFragment op = case op of - Equals -> "=" - Gte -> ">=" - Gt -> ">" - Lte -> "<=" - Lt -> "<" - Neq -> "<>" - Like -> "LIKE" - ILike -> "ILIKE" - In -> "IN" - NotIn -> "NOT IN" - IsNot -> "IS NOT" - Is -> "IS" - TSearch -> "@@" - Contains -> "@>" - Contained -> "<@" - +type Operator = Text +operators :: M.HashMap Operator SqlFragment +operators = M.fromList [ + ("eq", "="), + ("gte", ">="), + ("gt", ">"), + ("lte", "<="), + ("lt", "<"), + ("neq", "<>"), + ("like", "LIKE"), + ("ilike", "ILIKE"), + ("in", "IN"), + ("notin", "NOT IN"), + ("isnot", "IS NOT"), + ("is", "IS"), + ("@@", "@@"), + ("@>", "@>"), + ("<@", "<@")] data Operation = Operation{ hasNot::Bool, expr::(Operator, Operand) } deriving (Eq, Show) data Operand = VText Text | VTextL [Text] | VForeignKey QualifiedIdentifier ForeignKey deriving (Show, Eq) + type FieldName = Text type JsonPath = [Text] type Field = (FieldName, Maybe JsonPath) @@ -204,12 +164,14 @@ type Alias = Text type Cast = Text type NodeName = Text type SelectItem = (Field, Maybe Cast, Maybe Alias) -type Path = [Text] +-- | Path of the embedded levels, e.g "clients.projects.name=eq.." gives Path ["clients", "projects"] +type EmbedPath = [Text] +data Filter = Filter { field::Field, operation::Operation } deriving (Show, Eq) + data ReadQuery = Select { select::[SelectItem], from::[TableName], flt_::[Filter], order::Maybe [OrderTerm], range_::NonnegRange } deriving (Show, Eq) data MutateQuery = Insert { in_::TableName, qPayload::PayloadJSON, returning::[FieldName] } | Delete { in_::TableName, where_::[Filter], returning::[FieldName] } | Update { in_::TableName, qPayload::PayloadJSON, where_::[Filter], returning::[FieldName] } deriving (Show, Eq) -data Filter = Filter { field::Field, operation::Operation } deriving (Show, Eq) type ReadNode = (ReadQuery, (NodeName, Maybe Relation, Maybe Alias)) type ReadRequest = Tree ReadNode type MutateRequest = MutateQuery diff --git a/test/Feature/QuerySpec.hs b/test/Feature/QuerySpec.hs index b6ba3a3f2..1ed6525f9 100644 --- a/test/Feature/QuerySpec.hs +++ b/test/Feature/QuerySpec.hs @@ -772,8 +772,8 @@ spec = do get "/w_or_wo_comma_names?name=in.Williams\"Hebdon, John\"" `shouldRespondWith` [json| [] |] { matchHeaders = [matchContentTypeJson] } - describe "IN empty set" $ do - context "returns an empty result set when no value is present" $ do + describe "IN and NOT IN empty set" $ do + context "returns an empty result for IN when no value is present" $ do it "works for integer" $ get "/items_with_different_col_types?int_data=in." `shouldRespondWith` [json| [] |] { matchHeaders = [matchContentTypeJson] } @@ -799,8 +799,17 @@ spec = do get "/items_with_different_col_types?time_data=in." `shouldRespondWith` [json| [] |] { matchHeaders = [matchContentTypeJson] } + it "returns all results for notin when no value is present" $ + get "/items_with_different_col_types?int_data=notin.&select=int_data" `shouldRespondWith` + [json| [{int_data: 1}] |] { matchHeaders = [matchContentTypeJson] } + + it "returns all results for not.in when no value is present" $ + get "/items_with_different_col_types?int_data=not.in.&select=int_data" `shouldRespondWith` + [json| [{int_data: 1}] |] { matchHeaders = [matchContentTypeJson] } + it "returns an empty result ignoring spaces" $ - get "/items_with_different_col_types?int_data=in. " `shouldRespondWith` 400 + get "/items_with_different_col_types?int_data=in. " `shouldRespondWith` + [json| [] |] { matchHeaders = [matchContentTypeJson] } it "only returns an empty result set if the in value is empty" $ get "/items_with_different_col_types?int_data=in. ,3,4" `shouldRespondWith` 400 diff --git a/test/fixtures/data.sql b/test/fixtures/data.sql index 5eb61ed4e..5c4067001 100644 --- a/test/fixtures/data.sql +++ b/test/fixtures/data.sql @@ -294,6 +294,9 @@ INSERT INTO w_or_wo_comma_names VALUES ('Smith, Joseph'); INSERT INTO w_or_wo_comma_names VALUES ('David White'); INSERT INTO w_or_wo_comma_names VALUES ('Larry Thompson'); +TRUNCATE TABLE items_with_different_col_types CASCADE; +INSERT INTO items_with_different_col_types VALUES (1, null, null, null, null, null, null, null); + -- -- PostgreSQL database dump complete --