Skip to content

Commit 8187f98

Browse files
authored
Merge pull request #2396 from digitallyinduced/improve-request-body-json-errors
Improve requestBodyJSON error messages for different failure modes
2 parents 8d45e95 + 0651484 commit 8187f98

File tree

7 files changed

+170
-10
lines changed

7 files changed

+170
-10
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
- Deprecated Makefile targets — use `nix build` instead ([#2170](https://github.com/digitallyinduced/ihp/pull/2170))
1818
- Replaced `ApplicationContext` with WAI request vault for AutoRefresh ([#2149](https://github.com/digitallyinduced/ihp/pull/2149))
1919
- Used `OsPath` instead of `FilePath` across all packages ([#2246](https://github.com/digitallyinduced/ihp/pull/2246))
20+
- `requestBodyJSON` now returns `IO Aeson.Value` instead of `Aeson.Value`; update call sites to bind it in `do`-notation ([#2396](https://github.com/digitallyinduced/ihp/pull/2396))
2021

2122
### New Features
2223

ihp-datasync/IHP/DataSync/REST/Controller.hs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ instance (
3535
columnTypeLookup <- makeCachedColumnTypeLookup hasqlPool
3636
columnTypes <- columnTypeLookup table
3737

38-
let payload = requestBodyJSON
38+
payload <- requestBodyJSON
3939

4040
case payload of
4141
Object hashMap -> do
@@ -100,12 +100,12 @@ instance (
100100
columnTypeLookup <- makeCachedColumnTypeLookup hasqlPool
101101
columnTypes <- columnTypeLookup table
102102

103-
let payload = requestBodyJSON
104-
|> \case
105-
Object hashMap -> hashMap
106-
_ -> error "Expected JSON object"
103+
payload <- requestBodyJSON
104+
let hashMap = case payload of
105+
Object hm -> hm
106+
_ -> error "Expected JSON object"
107107

108-
let keyValues = payload
108+
let keyValues = hashMap
109109
|> Aeson.toList
110110
|> map (\(key, val) ->
111111
let col = fieldNameToColumnName (Aeson.toText key)

ihp/IHP/ControllerSupport.hs

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ import Data.IORef (IORef, modifyIORef', readIORef)
3535
import Data.ByteString (ByteString)
3636
import qualified Data.ByteString.Lazy as LBS
3737
import Data.Maybe (fromMaybe)
38-
import Control.Exception.Safe (SomeException, fromException, try, catch)
38+
import Control.Exception.Safe (SomeException, fromException, try, catch, throwIO)
3939
import Data.Typeable (Typeable)
4040
import IHP.HaskellSupport
4141
import Network.Wai
@@ -59,7 +59,9 @@ import qualified Data.TMap as TypeMap
5959
import IHP.RequestVault.ModelContext
6060
import IHP.ActionType (setActionType, actionTypeVaultKey, ActionType(..))
6161
import IHP.RequestVault.Helper (lookupRequestVault)
62+
import qualified IHP.Environment as Environment
6263
import qualified Data.Vault.Lazy as Vault
64+
import qualified Data.Text as Text
6365
import System.IO.Unsafe (unsafePerformIO)
6466

6567
type Action' = IO ResponseReceived
@@ -268,11 +270,32 @@ getFiles =
268270
FormBody { files } -> files
269271
_ -> []
270272

271-
requestBodyJSON :: (?request :: Request) => Aeson.Value
273+
requestBodyJSON :: (?request :: Request) => IO Aeson.Value
272274
requestBodyJSON =
273275
case ?request.parsedBody of
274-
JSONBody { jsonPayload = Just value } -> value
275-
_ -> error "Expected JSON body"
276+
JSONBody { jsonPayload = Just value } -> pure value
277+
JSONBody { jsonPayload = Nothing, rawPayload } -> do
278+
let isDev = ?request.frameworkConfig.environment == Environment.Development
279+
let errorMessage = "Expected JSON body, but could not decode the request body"
280+
<> (if LBS.null rawPayload
281+
then ". The request body is empty."
282+
else if isDev
283+
then ". The raw request body was: " <> truncatePayload rawPayload
284+
else ".")
285+
throwResponseException $ responseLBS HTTP.status400 [(hContentType, "application/json")] $
286+
Aeson.encode $ Aeson.object [("error", Aeson.String errorMessage)]
287+
where
288+
truncatePayload payload =
289+
let shown = show payload
290+
maxLen = 200
291+
in if length shown > maxLen
292+
then Text.pack (take maxLen shown) <> "... (truncated)"
293+
else Text.pack shown
294+
FormBody {} ->
295+
throwResponseException $ responseLBS HTTP.status400 [(hContentType, "application/json")] $
296+
Aeson.encode $ Aeson.object [("error", Aeson.String "Expected JSON body, but the request has a form content type. Make sure to set 'Content-Type: application/json' in the request header.")]
297+
where
298+
throwResponseException response = throwIO (ResponseException response)
276299

277300
-- | Returns a custom config parameter
278301
--
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
{-|
2+
Module: Test.ControllerSupportSpec
3+
-}
4+
module Test.ControllerSupportSpec where
5+
6+
import IHP.Prelude
7+
import Test.Hspec
8+
import IHP.ControllerSupport (requestBodyJSON)
9+
import IHP.Controller.Response (ResponseException(..))
10+
import IHP.Environment (Environment (..))
11+
import qualified IHP.FrameworkConfig as FrameworkConfig
12+
import qualified IHP.RequestVault as RequestVault
13+
import Wai.Request.Params.Middleware (RequestBody (..), requestBodyVaultKey)
14+
import qualified Data.Vault.Lazy as Vault
15+
import qualified Data.Aeson as Aeson
16+
import qualified Network.Wai as Wai
17+
import qualified Control.Exception as Exception
18+
import qualified Data.ByteString.Lazy as LBS
19+
import qualified Data.ByteString as BS
20+
import Network.HTTP.Types (status400)
21+
import System.IO.Unsafe (unsafePerformIO)
22+
import Data.ByteString.Builder (toLazyByteString)
23+
24+
tests = do
25+
describe "IHP.ControllerSupport" do
26+
describe "requestBodyJSON" do
27+
it "should return parsed JSON value for valid JSONBody" do
28+
let jsonValue = Aeson.object [("name", Aeson.String "test")]
29+
let requestBody = JSONBody { jsonPayload = Just jsonValue, rawPayload = "{\"name\":\"test\"}" }
30+
request <- buildRequest requestBody Development
31+
let ?request = request
32+
result <- requestBodyJSON
33+
result `shouldBe` jsonValue
34+
35+
it "should return 400 for FormBody" do
36+
let requestBody = FormBody { params = [], files = [], rawPayload = "" }
37+
request <- buildRequest requestBody Development
38+
let ?request = request
39+
result <- Exception.try requestBodyJSON
40+
case result of
41+
Left (ResponseException response) -> do
42+
Wai.responseStatus response `shouldBe` status400
43+
let body = responseBody response
44+
body `shouldSatisfy` bodyContains "form content type"
45+
Right _ -> expectationFailure "Expected ResponseException"
46+
47+
it "should return 400 for JSONBody with empty body" do
48+
let requestBody = JSONBody { jsonPayload = Nothing, rawPayload = "" }
49+
request <- buildRequest requestBody Development
50+
let ?request = request
51+
result <- Exception.try requestBodyJSON
52+
case result of
53+
Left (ResponseException response) -> do
54+
Wai.responseStatus response `shouldBe` status400
55+
let body = responseBody response
56+
body `shouldSatisfy` bodyContains "request body is empty"
57+
Right _ -> expectationFailure "Expected ResponseException"
58+
59+
it "should return 400 for JSONBody with invalid JSON" do
60+
let requestBody = JSONBody { jsonPayload = Nothing, rawPayload = "not valid json" }
61+
request <- buildRequest requestBody Development
62+
let ?request = request
63+
result <- Exception.try requestBodyJSON
64+
case result of
65+
Left (ResponseException response) -> do
66+
Wai.responseStatus response `shouldBe` status400
67+
let body = responseBody response
68+
body `shouldSatisfy` bodyContains "not valid json"
69+
Right _ -> expectationFailure "Expected ResponseException"
70+
71+
it "should truncate long payloads in dev mode" do
72+
let longPayload = LBS.pack (replicate 500 65) -- 500 bytes of 'A'
73+
let requestBody = JSONBody { jsonPayload = Nothing, rawPayload = longPayload }
74+
request <- buildRequest requestBody Development
75+
let ?request = request
76+
result <- Exception.try requestBodyJSON
77+
case result of
78+
Left (ResponseException response) -> do
79+
let body = responseBody response
80+
body `shouldSatisfy` bodyContains "truncated"
81+
body `shouldSatisfy` bodyContains "raw request body was"
82+
Right _ -> expectationFailure "Expected ResponseException"
83+
84+
it "should omit raw payload in production mode" do
85+
let requestBody = JSONBody { jsonPayload = Nothing, rawPayload = "not valid json" }
86+
request <- buildRequest requestBody Production
87+
let ?request = request
88+
result <- Exception.try requestBodyJSON
89+
case result of
90+
Left (ResponseException response) -> do
91+
Wai.responseStatus response `shouldBe` status400
92+
let body = responseBody response
93+
body `shouldSatisfy` (not . bodyContains "not valid json")
94+
body `shouldSatisfy` (not . bodyContains "raw request body was")
95+
Right _ -> expectationFailure "Expected ResponseException"
96+
97+
bodyContains :: BS.ByteString -> LBS.ByteString -> Bool
98+
bodyContains needle haystack = BS.isInfixOf needle (LBS.toStrict haystack)
99+
100+
buildRequest :: RequestBody -> Environment -> IO Wai.Request
101+
buildRequest requestBody environment = do
102+
frameworkConfig <- FrameworkConfig.buildFrameworkConfig (FrameworkConfig.option environment)
103+
pure Wai.defaultRequest
104+
{ Wai.vault = Vault.insert RequestVault.frameworkConfigVaultKey frameworkConfig
105+
$ Vault.insert requestBodyVaultKey requestBody Vault.empty
106+
}
107+
108+
-- | Extract the body from a WAI Response (works for responseLBS responses)
109+
responseBody :: Wai.Response -> LBS.ByteString
110+
responseBody response =
111+
let (_, _, withBody) = Wai.responseToStream response
112+
in unsafePerformIO $ withBody $ \streamingBody -> do
113+
ref <- newIORef mempty
114+
streamingBody
115+
(\chunk -> modifyIORef ref (<> chunk))
116+
(pure ())
117+
builder <- readIORef ref
118+
pure (toLazyByteString builder)

ihp/Test/Test/Main.hs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import qualified Test.FileStorage.ControllerFunctionsSpec
2121
import qualified Test.PGListenerSpec
2222
import qualified Test.MockingSpec
2323
import qualified Test.HasqlEncoderSpec
24+
import qualified Test.ControllerSupportSpec
2425
import qualified Test.AutoRefreshSpec
2526

2627
main :: IO ()
@@ -43,4 +44,5 @@ main = hspec do
4344
Test.PGListenerSpec.tests
4445
Test.MockingSpec.tests
4546
Test.HasqlEncoderSpec.tests
47+
Test.ControllerSupportSpec.tests
4648
Test.AutoRefreshSpec.tests

ihp/ihp.cabal

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,5 +288,6 @@ test-suite tests
288288
Test.Controller.CookieSpec
289289
Test.PGListenerSpec
290290
Test.MockingSpec
291+
Test.ControllerSupportSpec
291292
Test.HasqlEncoderSpec
292293
Test.AutoRefreshSpec

wai-request-params/Test/Wai/Request/Params/MiddlewareSpec.hs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,21 @@ spec = do
117117
response <- runSession (makeRequestWithBody "POST" [(hContentType, "application/jsonFOO")] body) app
118118
cs (simpleBody response) `shouldBe` ("FormBody params=0 files=0" :: String)
119119

120+
it "returns JSONBody with Nothing payload for invalid JSON POST" $ do
121+
let body = "not valid json{{"
122+
response <- runSession (makeRequestWithBody "POST" [(hContentType, "application/json")] body) app
123+
cs (simpleBody response) `shouldBe` ("JSONBody payload=Nothing" :: String)
124+
125+
it "returns FormBody for POST without Content-Type" $ do
126+
let body = "{\"name\": \"test\"}"
127+
response <- runSession (makeRequestWithBody "POST" [] body) app
128+
cs (simpleBody response) `shouldBe` ("FormBody params=0 files=0" :: String)
129+
130+
it "returns JSONBody with Nothing payload for empty body with application/json" $ do
131+
let body = ""
132+
response <- runSession (makeRequestWithBody "POST" [(hContentType, "application/json")] body) app
133+
cs (simpleBody response) `shouldBe` ("JSONBody payload=Nothing" :: String)
134+
120135
describe "raw body preservation (getRequestBody)" $ do
121136
it "preserves raw body for form-encoded POST requests" $ do
122137
let body = "name=test&value=123"

0 commit comments

Comments
 (0)