From 28845e0f4301a57af43535c1ed31c4c7d29a61f2 Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Tue, 8 May 2018 11:49:45 -0500 Subject: [PATCH] Fix accepting misspellings in order syntax --- CHANGELOG.md | 1 + src/PostgREST/Parsers.hs | 29 +++++++++-------- src/PostgREST/Types.hs | 8 ++--- test/Feature/QuerySpec.hs | 65 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b4d76a876..c4412793c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #925, Fix RPC high memory usage by using parametrized query and avoiding json encoding - @steve-chavez - #987, Fix embedding with self-reference foreign key - @steve-chavez - #1044, Fix view parent embedding when having many views - @steve-chavez +- #781, Fix accepting misspelled desc/asc ordering modificators - @onporat, @steve-chavez ### Changed diff --git a/src/PostgREST/Parsers.hs b/src/PostgREST/Parsers.hs index ad257c811..58721ecf3 100644 --- a/src/PostgREST/Parsers.hs +++ b/src/PostgREST/Parsers.hs @@ -148,23 +148,22 @@ pDelimiter :: Parser Char pDelimiter = char '.' "delimiter (.)" pOrder :: Parser [OrderTerm] -pOrder = lexeme pOrderTerm `sepBy` char ',' +pOrder = lexeme pOrderTerm `sepBy1` char ',' pOrderTerm :: Parser OrderTerm -pOrderTerm = - try ( do - c <- pField - d <- optionMaybe (try $ pDelimiter *> ( - try(string "asc" *> pure OrderAsc) - <|> try(string "desc" *> pure OrderDesc) - )) - nls <- optionMaybe (pDelimiter *> ( - try(string "nullslast" *> pure OrderNullsLast) - <|> try(string "nullsfirst" *> pure OrderNullsFirst) - )) - return $ OrderTerm c d nls - ) - <|> OrderTerm <$> pField <*> pure Nothing <*> pure Nothing +pOrderTerm = do + fld <- pField + dir <- optionMaybe $ + try (pDelimiter *> string "asc" $> OrderAsc) <|> + try (pDelimiter *> string "desc" $> OrderDesc) + nls <- optionMaybe pNulls <* pEnd <|> + pEnd $> Nothing + return $ OrderTerm fld dir nls + where + pNulls = try (pDelimiter *> string "nullsfirst" $> OrderNullsFirst) <|> + try (pDelimiter *> string "nullslast" $> OrderNullsLast) + pEnd = try (void $ lookAhead (char ',')) <|> + try eof pLogicTree :: Parser LogicTree pLogicTree = Stmnt <$> try pLogicFilter diff --git a/src/PostgREST/Types.hs b/src/PostgREST/Types.hs index 9ace5a020..2e13aef50 100644 --- a/src/PostgREST/Types.hs +++ b/src/PostgREST/Types.hs @@ -118,13 +118,13 @@ data PrimaryKey = PrimaryKey { data OrderDirection = OrderAsc | OrderDesc deriving (Eq) instance Show OrderDirection where - show OrderAsc = "asc" - show OrderDesc = "desc" + show OrderAsc = "ASC" + show OrderDesc = "DESC" data OrderNulls = OrderNullsFirst | OrderNullsLast deriving (Eq) instance Show OrderNulls where - show OrderNullsFirst = "nulls first" - show OrderNullsLast = "nulls last" + show OrderNullsFirst = "NULLS FIRST" + show OrderNullsLast = "NULLS LAST" data OrderTerm = OrderTerm { otTerm :: Field diff --git a/test/Feature/QuerySpec.hs b/test/Feature/QuerySpec.hs index 2efb54586..1f23b132f 100644 --- a/test/Feature/QuerySpec.hs +++ b/test/Feature/QuerySpec.hs @@ -564,6 +564,8 @@ spec = do { matchStatus = 200 , matchHeaders = ["Content-Range" <:> "0-1/*"] } + + it "by a column desc" $ get "/items?id=lte.2&order=id.desc" `shouldRespondWith` [json| [{"id":2},{"id":1}] |] @@ -608,6 +610,26 @@ spec = do , matchHeaders = ["Content-Range" <:> "0-2/*"] } + it "by two columns with nulls and direction specified" $ + get "/projects?select=client_id,id,name&order=client_id.desc.nullslast,id.desc" + `shouldRespondWith` [json| + [{"client_id":2,"id":4,"name":"OSX"}, + {"client_id":2,"id":3,"name":"IOS"}, + {"client_id":1,"id":2,"name":"Windows 10"}, + {"client_id":1,"id":1,"name":"Windows 7"}, + {"client_id":null,"id":5,"name":"Orphan"}] + |] + { matchStatus = 200 + , matchHeaders = ["Content-Range" <:> "0-4/*"] + } + + it "by a column with no direction or nulls specified" $ + get "/items?id=lte.2&order=id" + `shouldRespondWith` [json| [{"id":1},{"id":2}] |] + { matchStatus = 200 + , matchHeaders = ["Content-Range" <:> "0-1/*"] + } + it "by a json column property asc" $ get "/json?order=data->>id.asc" `shouldRespondWith` [json| [{"data": {"id": 0}}, {"data": {"id": 1, "foo": {"bar": "baz"}}}, {"data": {"id": 3}}] |] @@ -644,7 +666,50 @@ spec = do get "/projects?id=eq.1&select=id, name, client(id, name)&client.order=name.asc" `shouldRespondWith` [str|[{"id":1,"name":"Windows 7","client":{"id":1,"name":"Microsoft"}}]|] + context "order syntax errors" $ do + it "gives meaningful error messages when asc/desc/nulls{first,last} are misspelled" $ do + get "/items?order=id.ac" `shouldRespondWith` + [json|{"details":"unexpected \"c\" expecting \"asc\", \"desc\", \"nullsfirst\" or \"nullslast\"","message":"\"failed to parse order (id.ac)\" (line 1, column 4)"}|] + { matchStatus = 400 + , matchHeaders = [matchContentTypeJson] + } + get "/items?order=id.descc" `shouldRespondWith` + [json|{"details":"unexpected 'c' expecting delimiter (.), \",\" or end of input","message":"\"failed to parse order (id.descc)\" (line 1, column 8)"}|] + { matchStatus = 400 + , matchHeaders = [matchContentTypeJson] + } + get "/items?order=id.nulsfist" `shouldRespondWith` + [json|{"details":"unexpected \"n\" expecting \"asc\", \"desc\", \"nullsfirst\" or \"nullslast\"","message":"\"failed to parse order (id.nulsfist)\" (line 1, column 4)"}|] + { matchStatus = 400 + , matchHeaders = [matchContentTypeJson] + } + get "/items?order=id.nullslasttt" `shouldRespondWith` + [json|{"details":"unexpected 't' expecting \",\" or end of input","message":"\"failed to parse order (id.nullslasttt)\" (line 1, column 13)"}|] + { matchStatus = 400 + , matchHeaders = [matchContentTypeJson] + } + get "/items?order=id.smth34" `shouldRespondWith` + [json|{"details":"unexpected \"s\" expecting \"asc\", \"desc\", \"nullsfirst\" or \"nullslast\"","message":"\"failed to parse order (id.smth34)\" (line 1, column 4)"}|] + { matchStatus = 400 + , matchHeaders = [matchContentTypeJson] + } + it "gives meaningful error messages when nulls{first,last} are misspelled after asc/desc" $ do + get "/items?order=id.asc.nlsfst" `shouldRespondWith` + [json|{"details":"unexpected \"l\" expecting \"nullsfirst\" or \"nullslast\"","message":"\"failed to parse order (id.asc.nlsfst)\" (line 1, column 8)"}|] + { matchStatus = 400 + , matchHeaders = [matchContentTypeJson] + } + get "/items?order=id.asc.nullslasttt" `shouldRespondWith` + [json|{"details":"unexpected 't' expecting \",\" or end of input","message":"\"failed to parse order (id.asc.nullslasttt)\" (line 1, column 17)"}|] + { matchStatus = 400 + , matchHeaders = [matchContentTypeJson] + } + get "/items?order=id.asc.smth34" `shouldRespondWith` + [json|{"details":"unexpected \"s\" expecting \"nullsfirst\" or \"nullslast\"","message":"\"failed to parse order (id.asc.smth34)\" (line 1, column 8)"}|] + { matchStatus = 400 + , matchHeaders = [matchContentTypeJson] + } describe "Accept headers" $ do it "should respond an unknown accept type with 415" $