Backend/fix/ondc_compliance_changes#1230
Conversation
WalkthroughExpand Beckn response and error handling: add NACK support to AckResponse, convert exception flows to ONDC-style NACK payloads returned at HTTP 200, introduce a response-wrapping middleware, and add an ONDC-specific error-code hook on the Beckn error typeclass. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SignatureAuth as Signature Auth
participant Handler as Request Handler
participant Flow as Beckn FlowHandler
participant Middleware as Response Wrapper
Client->>SignatureAuth: HTTP Beckn request
SignatureAuth->>Flow: run via becknAuthHandler
alt auth fails
Flow-->>SignatureAuth: throwBecknNack200 (NACK payload)
SignatureAuth-->>Middleware: ServerError (200 + NACK body)
else auth succeeds
SignatureAuth->>Handler: call endpoint
Handler->>Flow: run via becknApiHandler
alt handler throws
Flow-->>Handler: returns Nack AckResponse
else success
Flow-->>Handler: returns Ack AckResponse
end
Handler-->>Middleware: response body (possibly ack envelope)
end
Middleware->>Middleware: shouldWrap? decode and maybe wrap as {"response": ...}
Middleware-->>Client: HTTP 200 with (possibly wrapped) ONDC body
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/mobility-core/src/Kernel/Types/Beckn/Ack.hs (1)
30-67:⚠️ Potential issue | 🟡 MinorOpenAPI schema is permissive for
NACK— error is required by ONDC but marked optional here.For the envelope, the spec requires
errorwhenmessage.ack.status == "NACK". The current schema listserroras a top-level property but not in therequiredlist, which matches the optional case forACKbut under-constrainsNACK. AoneOf/discriminator formulation would be more precise, though the current one is a faithful "superset" schema and not wrong — flagging as minor/spec-alignment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/Types/Beckn/Ack.hs` around lines 30 - 67, The OpenAPI schema for AckResponse currently allows ACK and NACK but does not require the top-level error field when message.ack.status == "NACK"; update the schema generation in ToSchema AckResponse so NACK cases mandate error: either model two alternatives with oneOf (or a discriminator) — one variant with message.ack.status == "ACK" and no required error, and a second variant with message.ack.status == "NACK" that includes error in required — adjust the existing statusSchema, messageSchema and errorSchema assembly (refer to AckResponse, statusSchema, messageSchema, errorSchema, ackSchema) to emit the oneOf variants rather than a single permissive object. Ensure the resulting NamedSchema keeps property names ("message","error") but enforces error required for the NACK variant.
🧹 Nitpick comments (6)
lib/mobility-core/src/Kernel/Types/Error/BaseError/HTTPError/BecknAPIError.hs (2)
67-76: Encoder is lossy for_typeandpath— intentional, worth a test.The encoder drops
_typeandpath(comment explains why), and the decoder always reconstructs_type = INTERNAL_ERROR. This meanstoJSON . fromJSONis not an identity on the Haskell side for anyErrorwhose_typewas notINTERNAL_ERROR. Fine per ONDC spec, but add a round-trip test if you have a Beckn-types test module — it'll lock the ONDC wire shape against future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/Types/Error/BaseError/HTTPError/BecknAPIError.hs` around lines 67 - 76, The JSON encoder for BecknAPIError (ToJSON instance for BecknAPIError) intentionally drops Error._type and Error.path and the decoder reconstructs _type = INTERNAL_ERROR, so add a unit test in the Beckn-types test module that exercises round-trip serialization (toJSON . fromJSON or encode/decode) for BecknAPIError/ Error values to lock the ONDC wire shape: create sample Error values (including non-INTERNAL_ERROR _type and non-empty path), encode them via the existing ToJSON/FromJSON pipeline and assert the resulting JSON matches the expected ONDC wire shape and that decoding produces the documented reconstructed form (i.e., _type becomes INTERNAL_ERROR and path is absent), so future changes that alter the lossy behavior will be caught.
46-66: Parser is now strict about ONDC-shape; non-ONDC error bodies will fail to decode.Previously
BecknAPIErrorcould decode a broader range of error payloads. With the new parser, any upstream that returns a non-ONDC error body (e.g., an HTML 502 from a gateway, or a legacy Beckn peer still emitting the old shape) will fail to decode asBecknAPIErrorand fall through as a generic parse failure. This is consistent with the spec intent, but practically it can hide root causes in logs.Consider a softer fallback — e.g., if the
errorobject is missing, synthesizeError.Errorwith the raw body asmessageand a placeholder code — so upstream misbehaviour is still observable via the same path:♻️ Proposed fallback for non-ONDC-shaped error bodies
parseJSON (Object v) = do inner <- (v .: "response") <|> pure v - errObj <- inner .: "error" - errCode <- errObj .: "code" - errMsg <- errObj .:? "message" + errObj <- inner .:? "error" + (errCode, errMsg) <- case errObj of + Just eo -> (,) <$> eo .: "code" <*> eo .:? "message" + Nothing -> pure ("UNKNOWN", Just "Non-ONDC error body") pure $🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/Types/Error/BaseError/HTTPError/BecknAPIError.hs` around lines 46 - 66, The FromJSON instance for BecknAPIError (the parseJSON function) currently assumes an ONDC-shaped "error" object and fails otherwise; modify parseJSON so it first extracts inner = (v .: "response") <|> pure v, then try to get errObj <- inner .:? "error"; if errObj is Just use existing extraction of code/message, else synthesize an Error.Error for BecknAPIError with a placeholder code (e.g. "UNKNOWN" or 0), path = Nothing, and the raw body (serialize inner to a Text/String) as Error.message so non-ONDC bodies (HTML, legacy shapes) are preserved and decodable via BecknAPIError; keep the error-producing typeMismatch branch for non-Object inputs.lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hs (3)
84-96: Minor:extractBapIdFromBodyre-decodes the JSON body.If this is invoked from a caller that already decoded the body (likely the
shouldWrappredicate or a parallel auth middleware), you pay forA.eitherDecodetwice. Consider exposing a variant that takesA.Valueto avoid redundant decoding on the hot path. Not blocking — only worth doing if this runs per-request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hs` around lines 84 - 96, The current extractBapIdFromBody re-decodes the JSON ByteString; add a new helper extractBapIdFromValue :: A.Value -> Maybe Text that extracts "context" -> "bap_id" from an already-parsed A.Value (mirroring the existing object traversal), then implement extractBapIdFromBody to decode once and call extractBapIdFromValue; update hot-path callers (e.g. shouldWrap or auth middleware) to use extractBapIdFromValue when they already hold an A.Value to avoid double-decoding.
14-14: Scope the deprecation suppression or add a pointer comment.A file-level
-Wno-error=deprecationshides all deprecation warnings in this module indefinitely. Against the repo's-Wall -Werrorstance (per coding guidelines), this is worth (a) scoping down to the specific symbol via a comment / inline pragma, or (b) adding a one-line note identifying which API is deprecated so the next person to touch this file can remove the pragma when the dependency is bumped.As per coding guidelines: "All code must compile without warnings under GHC flags
-Wall -Werror— treat all warnings as errors", so globally waving off deprecations deserves an explicit justification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hs` at line 14, Replace the file-level blanket pragma "{-# OPTIONS_GHC -Wno-error=deprecations `#-`}" in BecknResponseWrapper.hs with a scoped justification: remove the global pragma and either (a) add a one-line comment next to the specific import or use-site that names the deprecated symbol/API being relied on and a TODO to remove the workaround when that dependency is bumped (e.g., "uses DeprecatedFoo from libX; suppress deprecation until libX@vY; remove when upgraded"), or (b) if suppression is absolutely required, move a minimal comment + local pragma as close as possible to the single import/usage that triggers the warning and include the same TODO and reference to the deprecated symbol, ensuring the rest of the module remains compiled with normal deprecation warnings.
49-59: Replayable body is single-shot — document or make it truly idempotent.
rebindRequestBodyreturnsBS.emptyon every read after the first. WAI's contract forrequestBodyis that it's called repeatedly until it returns empty (signalling end-of-stream), so the first reader will see the body correctly as a single chunk followed by empty. However, any middleware or handler that tries to read the body twice (e.g., a second middleware layer) will see an empty body on its first read, silently.If nothing else in the stack reads the body twice, this is fine as-is — worth a one-line comment pinning that invariant. If you want full re-readability, keep the
IORefcursor semantics but reset it between requests, or replace withvault-based caching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hs` around lines 49 - 59, The current rebindRequestBody makes requestBody single-shot by using the IORef consumed inside replay, so any second reader calling Wai.requestBody will immediately see BS.empty; either add a one-line comment in rebindRequestBody explaining this single-shot invariant (referencing rebindRequestBody, replay, consumed and Wai.requestBody) or make it truly replayable by caching the full body and returning a fresh per-reader cursor: replace the consumed Bool with a cached strict ByteString (BL.toStrict body) and expose a requestBody action that serves that cached bytestring as the first chunk and empty thereafter for each independent reader (or alternatively store the cached bytes in the request Vault and provide a read helper), so multiple middleware layers can re-read the body reliably.lib/mobility-core/src/Kernel/Types/Beckn/Ack.hs (1)
77-115: Add a comment explaining the fallback behavior or include a test fixture to document it.The
(v .: "response") <|> pure vpattern correctly handles both wrapped and unwrapped bodies, but has a subtle edge case: if a body contains a"response"key that holds an object lacking"message", the parser won't backtrack to try the outer object—it will simply fail. This is fine in practice since middleware only wraps valid ack envelopes, but the current comments don't explain this behavior. Consider either adding an inline comment clarifying the limitations of the fallback or including a test fixture that documents the expected parse behavior for both wrapped and unwrapped forms.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/Types/Beckn/Ack.hs` around lines 77 - 115, Add an inline comment (or a small test fixture) near the FromJSON instance for AckResponse / parseJSON explaining that the fallback (v .: "response" <|> pure v) accepts both wrapped and unwrapped bodies but will not backtrack if "response" exists and lacks a "message" field (so parsing will fail in that edge case); reference the inner variable and the subsequent extraction of "message" -> "ack" -> "status" when documenting the limitation, or alternatively add unit tests that assert successful parsing for both wrapped ({ "response": {...} }) and unwrapped bodies and that a body with "response" present but missing "message" fails to parse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/mobility-core/src/Kernel/Utils/Error/FlowHandling.hs`:
- Around line 297-303: The current someExceptionToBecknApiError funnels
HTTPException-derived errors through toBecknAPIError which can emit non-numeric
ONDC codes (via toErrorCode) and leak uppercase strings into NACK payloads;
update someExceptionToBecknApiError to inspect the resulting BecknAPIError (or
the error's toOndcErrorCode) and if the ONDC code is missing or non-numeric,
replace it with a sensible numeric fallback (e.g., "31001"); reference
HTTPException, toBecknAPIError, toOndcErrorCode, toErrorCode and ensure the
fallback is applied before returning from someExceptionToBecknApiError so
callers like handleIfUpBeckn receive a numeric ONDC code.
In `@lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hs`:
- Around line 30-41: The middleware wrapBecknResponse currently reads the
response via Wai.responseToStream and then calls responseLBS with the original
headers, which can leave stale Content-Length or Transfer-Encoding headers and
break clients after wrapIfAckEnvelope modifies the body; update the
response-handling branch in wrapBecknResponse (the code that destructures
(status, headers, withBody) and builds final) to filter out "Content-Length" and
"Transfer-Encoding" (and any other hop-by-hop headers you deem necessary) from
headers before calling Wai.responseLBS so Warp will recompute framing for the
modified final body.
---
Outside diff comments:
In `@lib/mobility-core/src/Kernel/Types/Beckn/Ack.hs`:
- Around line 30-67: The OpenAPI schema for AckResponse currently allows ACK and
NACK but does not require the top-level error field when message.ack.status ==
"NACK"; update the schema generation in ToSchema AckResponse so NACK cases
mandate error: either model two alternatives with oneOf (or a discriminator) —
one variant with message.ack.status == "ACK" and no required error, and a second
variant with message.ack.status == "NACK" that includes error in required —
adjust the existing statusSchema, messageSchema and errorSchema assembly (refer
to AckResponse, statusSchema, messageSchema, errorSchema, ackSchema) to emit the
oneOf variants rather than a single permissive object. Ensure the resulting
NamedSchema keeps property names ("message","error") but enforces error required
for the NACK variant.
---
Nitpick comments:
In `@lib/mobility-core/src/Kernel/Types/Beckn/Ack.hs`:
- Around line 77-115: Add an inline comment (or a small test fixture) near the
FromJSON instance for AckResponse / parseJSON explaining that the fallback (v .:
"response" <|> pure v) accepts both wrapped and unwrapped bodies but will not
backtrack if "response" exists and lacks a "message" field (so parsing will fail
in that edge case); reference the inner variable and the subsequent extraction
of "message" -> "ack" -> "status" when documenting the limitation, or
alternatively add unit tests that assert successful parsing for both wrapped ({
"response": {...} }) and unwrapped bodies and that a body with "response"
present but missing "message" fails to parse.
In
`@lib/mobility-core/src/Kernel/Types/Error/BaseError/HTTPError/BecknAPIError.hs`:
- Around line 67-76: The JSON encoder for BecknAPIError (ToJSON instance for
BecknAPIError) intentionally drops Error._type and Error.path and the decoder
reconstructs _type = INTERNAL_ERROR, so add a unit test in the Beckn-types test
module that exercises round-trip serialization (toJSON . fromJSON or
encode/decode) for BecknAPIError/ Error values to lock the ONDC wire shape:
create sample Error values (including non-INTERNAL_ERROR _type and non-empty
path), encode them via the existing ToJSON/FromJSON pipeline and assert the
resulting JSON matches the expected ONDC wire shape and that decoding produces
the documented reconstructed form (i.e., _type becomes INTERNAL_ERROR and path
is absent), so future changes that alter the lossy behavior will be caught.
- Around line 46-66: The FromJSON instance for BecknAPIError (the parseJSON
function) currently assumes an ONDC-shaped "error" object and fails otherwise;
modify parseJSON so it first extracts inner = (v .: "response") <|> pure v, then
try to get errObj <- inner .:? "error"; if errObj is Just use existing
extraction of code/message, else synthesize an Error.Error for BecknAPIError
with a placeholder code (e.g. "UNKNOWN" or 0), path = Nothing, and the raw body
(serialize inner to a Text/String) as Error.message so non-ONDC bodies (HTML,
legacy shapes) are preserved and decodable via BecknAPIError; keep the
error-producing typeMismatch branch for non-Object inputs.
In `@lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hs`:
- Around line 84-96: The current extractBapIdFromBody re-decodes the JSON
ByteString; add a new helper extractBapIdFromValue :: A.Value -> Maybe Text that
extracts "context" -> "bap_id" from an already-parsed A.Value (mirroring the
existing object traversal), then implement extractBapIdFromBody to decode once
and call extractBapIdFromValue; update hot-path callers (e.g. shouldWrap or auth
middleware) to use extractBapIdFromValue when they already hold an A.Value to
avoid double-decoding.
- Line 14: Replace the file-level blanket pragma "{-# OPTIONS_GHC
-Wno-error=deprecations `#-`}" in BecknResponseWrapper.hs with a scoped
justification: remove the global pragma and either (a) add a one-line comment
next to the specific import or use-site that names the deprecated symbol/API
being relied on and a TODO to remove the workaround when that dependency is
bumped (e.g., "uses DeprecatedFoo from libX; suppress deprecation until libX@vY;
remove when upgraded"), or (b) if suppression is absolutely required, move a
minimal comment + local pragma as close as possible to the single import/usage
that triggers the warning and include the same TODO and reference to the
deprecated symbol, ensuring the rest of the module remains compiled with normal
deprecation warnings.
- Around line 49-59: The current rebindRequestBody makes requestBody single-shot
by using the IORef consumed inside replay, so any second reader calling
Wai.requestBody will immediately see BS.empty; either add a one-line comment in
rebindRequestBody explaining this single-shot invariant (referencing
rebindRequestBody, replay, consumed and Wai.requestBody) or make it truly
replayable by caching the full body and returning a fresh per-reader cursor:
replace the consumed Bool with a cached strict ByteString (BL.toStrict body) and
expose a requestBody action that serves that cached bytestring as the first
chunk and empty thereafter for each independent reader (or alternatively store
the cached bytes in the request Vault and provide a read helper), so multiple
middleware layers can re-read the body reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61978098-3998-401f-86a9-b268c8c8afe2
📒 Files selected for processing (8)
lib/mobility-core/mobility-core.caballib/mobility-core/src/Kernel/Types/Beckn/Ack.hslib/mobility-core/src/Kernel/Types/Error.hslib/mobility-core/src/Kernel/Types/Error/BaseError/HTTPError/BecknAPIError.hslib/mobility-core/src/Kernel/Utils/Error/BaseError/HTTPError/BecknAPIError.hslib/mobility-core/src/Kernel/Utils/Error/FlowHandling.hslib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hslib/mobility-core/src/Kernel/Utils/Servant/SignatureAuth.hs
| -- | Pure SomeException → BecknAPIError conversion used by 'becknApiHandler' to | ||
| -- produce a 'Nack' response without throwing. Keeps the Beckn protocol-return path | ||
| -- exception-free. | ||
| someExceptionToBecknApiError :: SomeException -> BecknAPIError | ||
| someExceptionToBecknApiError exc | ||
| | Just (HTTPException err) <- fromException exc = toBecknAPIError err | ||
| | otherwise = toBecknAPIError . InternalError $ show exc |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ONDC TRV10 v2.1.0 generic internal error code for BPP failures
💡 Result:
The generic internal error code for BPP (Seller Network Participant, SNP) failures in ONDC protocols, including TRV10 v2.1.0 (mobility ride-hailing domain), is 31001. This code is labeled "SNP|Internal Error" with the description "Cannot process request due to internal error, please retry". Error codes are standardized across ONDC protocol network extensions and apply to TRV10 as no domain-specific overrides were found in mobility specifications.
Citations:
- 1: https://github.com/ONDC-Official/developer-docs/blob/main/protocol-network-extension/error-codes.md
- 2: TRV11 - v2.x - Feature Backlog ONDC-Official/mobility-specification#196
- 3: https://github.com/ONDC-Official/mobility-specification
🏁 Script executed:
#!/bin/bash
# Find the error handling file and related error code definitions
find . -type f -name "*.hs" | grep -i error | head -20Repository: nammayatri/shared-kernel
Length of output: 1456
🏁 Script executed:
#!/bin/bash
# Search for toBecknAPIError and toOndcErrorCode definitions
rg "toBecknAPIError|toOndcErrorCode" --type hs -B 2 -A 5 | head -100Repository: nammayatri/shared-kernel
Length of output: 5102
🏁 Script executed:
#!/bin/bash
# Look at the actual FlowHandling.hs file structure around the suspect function
cat -n lib/mobility-core/src/Kernel/Utils/Error/FlowHandling.hs | head -310 | tail -50Repository: nammayatri/shared-kernel
Length of output: 2296
🏁 Script executed:
#!/bin/bash
# Find IsBecknAPIError instances and check toOndcErrorCode coverage
rg "instance IsBecknAPIError" --type hs -A 3 | head -80Repository: nammayatri/shared-kernel
Length of output: 2034
🏁 Script executed:
#!/bin/bash
# Check toErrorCode implementation for InternalError and ServerUnavailable
rg "data InternalError|data ServerUnavailable" --type hs -A 10 | head -60Repository: nammayatri/shared-kernel
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search for toErrorCode implementation
rg "toErrorCode.*InternalError|toErrorCode.*ServerUnavailable" --type hs -B 2 -A 2Repository: nammayatri/shared-kernel
Length of output: 421
🏁 Script executed:
#!/bin/bash
# Look at the Error.hs to see error code definitions
cat -n lib/mobility-core/src/Kernel/Types/Error.hs | head -100Repository: nammayatri/shared-kernel
Length of output: 4457
🏁 Script executed:
#!/bin/bash
# Check InternalError definition and toErrorCode
rg "data InternalError" --type hs -A 5 -B 2Repository: nammayatri/shared-kernel
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Find all toErrorCode implementations to see which error types lack toOndcErrorCode
rg "instance IsHTTPError" --type hs -A 10 | grep -A 5 "toErrorCode"Repository: nammayatri/shared-kernel
Length of output: 42998
🏁 Script executed:
#!/bin/bash
# Check BecknAPIError.hs for the complete toBecknAPIError implementation and IsBecknAPIError typeclass
cat -n lib/mobility-core/src/Kernel/Types/Error/BaseError/HTTPError/BecknAPIError.hsRepository: nammayatri/shared-kernel
Length of output: 3417
🏁 Script executed:
#!/bin/bash
# Check if there are any other instances with custom toOndcErrorCode
rg "toOndcErrorCode" --type hs -B 1 -A 1Repository: nammayatri/shared-kernel
Length of output: 1365
Non-ONDC-compliant error codes will leak into NACK payloads for exceptions without an explicit toOndcErrorCode override.
ONDC v2.1.0 mandates numeric error.code values (e.g., "90201", "10001"). For any HTTPException type without an override of toOndcErrorCode, toBecknAPIError falls back to toErrorCode — e.g., "INTERNAL_ERROR", "DB_UNKNOWN_ERROR", "SERVER_UNAVAILABLE", "REDIS_ERROR", etc. These uppercase codes are emitted directly in the wire NACK payload, violating the spec and likely causing BAPs to reject the envelope on strict schema validation.
Only ServiceabilityError currently overrides toOndcErrorCode (returning "90201"); all other error types use the default Nothing, triggering the fallback.
This applies here and indirectly via handleIfUpBeckn (line 280), which funnels ServerUnavailable through the same path during drains.
Options, in order of preference:
- Add a sensible default mapping inside
someExceptionToBecknApiErrorto emit a numeric ONDC fallback code (e.g.,"31001"— ONDC TRV10's SNP internal error code) when the mapped code is non-numeric. - Make
toOndcErrorCodemandatory for any type that declaresIsBecknAPIError, catching missing mappings at compile time.
🔒 Proposed minimal fallback
someExceptionToBecknApiError :: SomeException -> BecknAPIError
someExceptionToBecknApiError exc
- | Just (HTTPException err) <- fromException exc = toBecknAPIError err
- | otherwise = toBecknAPIError . InternalError $ show exc
+ | Just (HTTPException err) <- fromException exc = ensureOndcCode (toBecknAPIError err)
+ | otherwise = ensureOndcCode . toBecknAPIError . InternalError $ show exc
+ where
+ -- Fallback to ONDC SNP internal-error code if the mapped code is not numeric
+ -- (i.e., upstream didn't override toOndcErrorCode).
+ ensureOndcCode (BecknAPIError e)
+ | T.all isDigit (code e) = BecknAPIError e
+ | otherwise = BecknAPIError e { code = "31001" } -- ONDC TRV10 SNP|Internal Error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/Utils/Error/FlowHandling.hs` around lines 297 -
303, The current someExceptionToBecknApiError funnels HTTPException-derived
errors through toBecknAPIError which can emit non-numeric ONDC codes (via
toErrorCode) and leak uppercase strings into NACK payloads; update
someExceptionToBecknApiError to inspect the resulting BecknAPIError (or the
error's toOndcErrorCode) and if the ONDC code is missing or non-numeric, replace
it with a sensible numeric fallback (e.g., "31001"); reference HTTPException,
toBecknAPIError, toOndcErrorCode, toErrorCode and ensure the fallback is applied
before returning from someExceptionToBecknApiError so callers like
handleIfUpBeckn receive a numeric ONDC code.
| wrapBecknResponse :: (BL.ByteString -> IO Bool) -> Wai.Middleware | ||
| wrapBecknResponse shouldWrap app req respond = do | ||
| bodyBs <- consumeRequestBody req | ||
| reqWithReplayableBody <- rebindRequestBody req bodyBs | ||
| wrap <- shouldWrap bodyBs | ||
| if wrap | ||
| then app reqWithReplayableBody $ \resp -> do | ||
| let (status, headers, withBody) = Wai.responseToStream resp | ||
| respBody <- consumeStreamingBody withBody | ||
| let final = fromMaybe respBody (wrapIfAckEnvelope respBody) | ||
| respond $ Wai.responseLBS status headers final | ||
| else app reqWithReplayableBody respond |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Sanity-check: is wrapBecknResponse composed after any middleware that sets
# Content-Length (prometheus, logging, etc.)? Locate call sites.
rg -nP --type=hs -C3 '\bwrapBecknResponse\b' -g '!**/.stack-work/**' -g '!**/dist*/**'Repository: nammayatri/shared-kernel
Length of output: 1568
🏁 Script executed:
# Read the full file to understand the implementation
cat -n lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hsRepository: nammayatri/shared-kernel
Length of output: 4337
🏁 Script executed:
# Search for all imports and actual usage of this middleware in the codebase
rg -nP --type=hs 'wrapBecknResponse' -A2 -B2Repository: nammayatri/shared-kernel
Length of output: 1187
🏁 Script executed:
# Check if there are any existing tests or examples that show how this middleware is used
fd -t f -e hs . | xargs rg -l 'BecknResponseWrapper' | head -20Repository: nammayatri/shared-kernel
Length of output: 134
🌐 Web query:
WAI responseLBS Content-Length header behavior automatic computation
💡 Result:
In Haskell's WAI (Web Application Interface), the responseLBS function creates a Response from a strict status, response headers, and a lazy ByteString (LBS stands for Lazy ByteString). The function itself does not automatically add a Content-Length header; it wraps the lazy ByteString into a ResponseBuilder, passing the provided headers unchanged. The actual Content-Length header behavior is handled by the WAI server backend (e.g., Warp). Warp inspects the Response type and headers: - Applications and middlewares must NOT provide Content-Length, Content-Range, or Transfer-Encoding headers, as the server inserts them when necessary. - For responseLBS (which becomes ResponseBuilder), Warp uses Transfer-Encoding: chunked for HTTP/1.1 responses without a Content-Length. - Warp automatically adds Content-Length if possible (e.g., for known fixed-size bodies like files via responseFile), but for ResponseBuilder/responseLBS, it typically uses chunked encoding instead of computing the full length upfront to avoid strict evaluation of the lazy ByteString. - If a Content-Length header is manually added to the headers passed to responseLBS, it may cause inconsistencies, as noted in Warp source comments. This design prioritizes performance and streaming, avoiding forcing the entire lazy body into memory to compute the length. For fixed-length responses, use responseBuilder with a strict Builder or responseFile. Gzip middleware and other processors may remove or adjust Content-Length if encoding changes the body size.
Citations:
- 1: https://hackage.haskell.org/package/wai-3.0.2.3/docs/Network-Wai.html
- 2: https://hackage.haskell.org/package/wai/docs/Network-Wai.html
- 3: https://stackoverflow.com/questions/35834514/what-does-lbs-mean-in-the-context-of-wai
- 4: https://hackage.haskell.org/package/warp-3.4.7/docs/src/Network.Wai.Handler.Warp.Response.html
🏁 Script executed:
# Check Wai documentation or ghcjs-dom comments about responseLBS
rg -n 'responseLBS' lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hs -B5 -A5Repository: nammayatri/shared-kernel
Length of output: 669
🏁 Script executed:
# Search for any configuration or middleware composition that might set Content-Length before this middleware
rg -n 'Content-Length|Transfer-Encoding' --type=hs | head -20Repository: nammayatri/shared-kernel
Length of output: 556
Critical: Content-Length and Transfer-Encoding headers must be stripped before calling responseLBS.
When the wrapped application (e.g., Servant) returns a response, Wai.responseToStream extracts its headers unchanged. If that response includes a Content-Length header (which Servant/Warp commonly emit), and the middleware then modifies the body by wrapping it in {"response": <original>} (via wrapIfAckEnvelope), the body size increases but the Content-Length header remains unchanged. When Wai.responseLBS passes these mismatched headers to Warp, Warp honors the provided Content-Length value, causing the client to receive a truncated or hung response.
Per WAI/Warp design, applications should not provide Content-Length or Transfer-Encoding headers; however, when they do, any middleware that modifies the body must remove these headers to avoid inconsistencies.
🔒 Proposed fix — strip hop-by-hop and content-framing headers
then app reqWithReplayableBody $ \resp -> do
let (status, headers, withBody) = Wai.responseToStream resp
respBody <- consumeStreamingBody withBody
let final = fromMaybe respBody (wrapIfAckEnvelope respBody)
- respond $ Wai.responseLBS status headers final
+ sanitizedHeaders =
+ filter
+ (\(h, _) -> h /= "Content-Length" && h /= "Transfer-Encoding")
+ headers
+ respond $ Wai.responseLBS status sanitizedHeaders final
else app reqWithReplayableBody respond(Warp will recompute Content-Length or use chunked encoding as appropriate for the modified body.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hs` around
lines 30 - 41, The middleware wrapBecknResponse currently reads the response via
Wai.responseToStream and then calls responseLBS with the original headers, which
can leave stale Content-Length or Transfer-Encoding headers and break clients
after wrapIfAckEnvelope modifies the body; update the response-handling branch
in wrapBecknResponse (the code that destructures (status, headers, withBody) and
builds final) to filter out "Content-Length" and "Transfer-Encoding" (and any
other hop-by-hop headers you deem necessary) from headers before calling
Wai.responseLBS so Warp will recompute framing for the modified final body.
blaze-198
force-pushed
the
Backend/fix/ondc_compliance_changes
branch
from
23c76ea to
0b5ae1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/mobility-core/src/Kernel/Types/Beckn/Ack.hs (1)
37-67:⚠️ Potential issue | 🟡 MinorThe published schema still allows invalid ACK/NACK combinations.
Lines 37-38 require only
"message"and leave"error"optional for all status values, but the parser at lines 88-91 makes"error"mandatory when"status": "NACK". This mismatch will mislead code generators and API consumers into creating invalid clients that send NACK responses without the requirederrorobject, which the parser rejects. Model ACK and NACK as separate schemas usingoneOf/discriminator or similar to accurately reflect thaterroris required only for NACK.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mobility-core/src/Kernel/Types/Beckn/Ack.hs` around lines 37 - 67, The published schema currently makes "error" optional for all statuses but the parser enforces error required for NACK; fix by modeling ACK and NACK as distinct schemas and using oneOf (or discriminator) at the top-level instead of a single schema with optional "error". Concretely: define ackSchema (status:"ACK") and nackSchema (status:"NACK" and required "error" referencing errorSchema), then replace the single combined schema that uses messageSchema + optional error with a oneOf [ackMessageSchema, nackMessageSchema] (or a discriminator) so the OpenAPI schema matches the parser behavior; reference the existing statusSchema, ackSchema, messageSchema, errorSchema identifiers when creating the new nackSchema and the top-level oneOf.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/mobility-core/src/Kernel/Types/Beckn/Ack.hs`:
- Around line 107-114: In toJSON (Nack) change the construction so the "message"
key is only emitted when BError.message err is Just something instead of
serializing Nothing as null; specifically, update the toJSON (Nack) body that
currently uses BError.code and BError.message to conditionally include the
"message" field (e.g. build the inner object with BError.code and append the
"message" pair only when BError.message err is present), ensuring the FromJSON
expectation (using .:? elsewhere) matches the produced JSON.
In
`@lib/mobility-core/src/Kernel/Types/Error/BaseError/HTTPError/BecknAPIError.hs`:
- Around line 68-75: The toJSON implementation for Be cknAPIError is emitting
"error.message": null when Error.message is Nothing; update the toJSON
(BecknAPIError err) code to build the inner "error" object conditionally: always
include "code" (Error.code err) but only add the "message" key when
Error.message err is Just value (omit the key for Nothing). Change the
construction inside toJSON (BecknAPIError err) (or the helper that builds the
error object) to assemble a list of key/value pairs that appends the "message"
pair only when present (e.g., via maybe/catMaybes) and then pass that to object
for the "error" field so consumers don’t receive a null message.
In `@lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hs`:
- Around line 36-40: The middleware currently always drains the streaming
response via consumeStreamingBody (invoked after Wai.responseToStream) which
buffers every response; change the logic so you only consume/buffer when
wrapIfAckEnvelope indicates wrapping is required: call Wai.responseToStream to
get (status, headers, withBody), then attempt a lightweight check for whether
the body needs wrapping (e.g., by peeking headers or otherwise deciding without
draining); if wrapIfAckEnvelope indicates Nothing, forward the original
streaming response unchanged using Wai.responseStream or Wai.responseSource with
the original withBody to preserve streaming/backpressure; only call
consumeStreamingBody and apply wrapIfAckEnvelope when you know wrapping is
required, then respond with Wai.responseLBS for the wrapped payload. Ensure you
reference the current symbols: app, reqWithReplayableBody, Wai.responseToStream,
withBody (the streaming body), consumeStreamingBody, wrapIfAckEnvelope, respond,
Wai.responseLBS/responseStream.
- Line 14: Replace the deprecated direct field update of Wai.requestBody (the
expression `pure r { Wai.requestBody = replay }`) with the provided setter
`Wai.setRequestBodyChunks replay r` where the request is reconstructed; update
the code inside the function that performs the replay so it calls
`Wai.setRequestBodyChunks replay r` instead of mutating `requestBody`, and then
remove the top-module pragma `{-# OPTIONS_GHC -Wno-error=deprecations `#-`}` since
the deprecation is resolved.
---
Outside diff comments:
In `@lib/mobility-core/src/Kernel/Types/Beckn/Ack.hs`:
- Around line 37-67: The published schema currently makes "error" optional for
all statuses but the parser enforces error required for NACK; fix by modeling
ACK and NACK as distinct schemas and using oneOf (or discriminator) at the
top-level instead of a single schema with optional "error". Concretely: define
ackSchema (status:"ACK") and nackSchema (status:"NACK" and required "error"
referencing errorSchema), then replace the single combined schema that uses
messageSchema + optional error with a oneOf [ackMessageSchema,
nackMessageSchema] (or a discriminator) so the OpenAPI schema matches the parser
behavior; reference the existing statusSchema, ackSchema, messageSchema,
errorSchema identifiers when creating the new nackSchema and the top-level
oneOf.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77a8fad5-55e4-44fb-a139-f2c1fd4773e7
📒 Files selected for processing (8)
lib/mobility-core/mobility-core.caballib/mobility-core/src/Kernel/Types/Beckn/Ack.hslib/mobility-core/src/Kernel/Types/Error.hslib/mobility-core/src/Kernel/Types/Error/BaseError/HTTPError/BecknAPIError.hslib/mobility-core/src/Kernel/Utils/Error/BaseError/HTTPError/BecknAPIError.hslib/mobility-core/src/Kernel/Utils/Error/FlowHandling.hslib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hslib/mobility-core/src/Kernel/Utils/Servant/SignatureAuth.hs
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/mobility-core/src/Kernel/Utils/Error/BaseError/HTTPError/BecknAPIError.hs
- lib/mobility-core/src/Kernel/Utils/Servant/SignatureAuth.hs
- lib/mobility-core/mobility-core.cabal
| toJSON (Nack err) = | ||
| object | ||
| [ "message" .= object ["ack" .= object ["status" .= ("NACK" :: Text)]], | ||
| "error" | ||
| .= object | ||
| [ "code" .= BError.code err, | ||
| "message" .= BError.message err | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/mobility-core/src/Kernel/Types/Beckn/Ack.hs | head -150Repository: nammayatri/shared-kernel
Length of output: 5068
🏁 Script executed:
fd -type f BError.hs | head -10Repository: nammayatri/shared-kernel
Length of output: 239
🏁 Script executed:
find . -name "*BError*" -type fRepository: nammayatri/shared-kernel
Length of output: 50
🏁 Script executed:
find . -path "*/Kernel/Types/Beckn/Error*" -type fRepository: nammayatri/shared-kernel
Length of output: 120
🏁 Script executed:
cat -n ./lib/mobility-core/src/Kernel/Types/Beckn/Error.hs | head -100Repository: nammayatri/shared-kernel
Length of output: 2133
Emit optional error message only when present; don't serialize Nothing as null.
Line 113 serializes BError.message err directly, which causes Nothing to become "message": null in the JSON output. However, the FromJSON parser uses optional field parsing (.:? on line 91), and the schema declares "message" as optional (only "code" is required on line 66). For consistency, omit the field entirely when the message is absent rather than emitting null.
Suggested fix
toJSON (Nack err) =
object
[ "message" .= object ["ack" .= object ["status" .= ("NACK" :: Text)]],
"error"
- .= object
- [ "code" .= BError.code err,
- "message" .= BError.message err
- ]
+ .= object
+ ( [ "code" .= BError.code err
+ ]
+ <> maybe [] (\msg -> ["message" .= msg]) (BError.message err)
+ )
]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/Types/Beckn/Ack.hs` around lines 107 - 114, In
toJSON (Nack) change the construction so the "message" key is only emitted when
BError.message err is Just something instead of serializing Nothing as null;
specifically, update the toJSON (Nack) body that currently uses BError.code and
BError.message to conditionally include the "message" field (e.g. build the
inner object with BError.code and append the "message" pair only when
BError.message err is present), ensuring the FromJSON expectation (using .:?
elsewhere) matches the produced JSON.
| toJSON (BecknAPIError err) = | ||
| object | ||
| [ "message" .= object ["ack" .= object ["status" .= ("NACK" :: Text)]], | ||
| "error" | ||
| .= object | ||
| [ "code" .= Error.code err, | ||
| "message" .= Error.message err | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if file exists and read the relevant section
fd -t f "BecknAPIError.hs" | head -1Repository: nammayatri/shared-kernel
Length of output: 146
🏁 Script executed:
# Read the file to see the actual code and context
cat -n lib/mobility-core/src/Kernel/Types/Error/BaseError/HTTPError/BecknAPIError.hsRepository: nammayatri/shared-kernel
Length of output: 3418
🏁 Script executed:
# Find the Error type definition to check the message field type
rg -A 10 "data Error" lib/mobility-core/src/Kernel/Types/Error/BaseError/HTTPError/ --type hsRepository: nammayatri/shared-kernel
Length of output: 50
🏁 Script executed:
# Find the Error type definition
fd -t f "Error.hs" lib/mobility-core/src/Kernel/Types/Beckn/ | head -5Repository: nammayatri/shared-kernel
Length of output: 118
🏁 Script executed:
# Search for Error type definition in the Beckn types
rg -A 20 "^data Error\b" lib/mobility-core/src/Kernel/Types/Beckn/ --type hsRepository: nammayatri/shared-kernel
Length of output: 1657
🏁 Script executed:
# Verify the aeson pattern for conditional object fields
# Search for similar patterns with maybe and object construction
rg -B 3 -A 3 "maybe \[\]" lib/mobility-core/src/ --type hs | head -30Repository: nammayatri/shared-kernel
Length of output: 2834
🏁 Script executed:
# Check aeson imports and verify the object API in this file
rg "from Data.Aeson" lib/mobility-core/src/Kernel/Types/Error/BaseError/HTTPError/BecknAPIError.hsRepository: nammayatri/shared-kernel
Length of output: 50
🌐 Web query:
aeson Haskell maybe null serialization optional field
💡 Result:
In Haskell's aeson library, use Maybe for optional fields in data types. Aeson provides built-in FromJSON and ToJSON instances for Maybe a when a has instances. For manual FromJSON instances on records: - Use o .:? "field" :: Parser (Maybe a) for optional fields (returns Nothing if key missing or value fails to parse as a). - To handle explicit null as Nothing (especially post-aeson 0.10), use fmap join (o .:? "field") or o .:? "field" >>= parseJSON for Maybe (Maybe a), or o .:? "field" .!= Nothing. - For required fields, use o .: "field". Example: data Person = Person { name :: String, age :: Maybe Int } deriving Show instance FromJSON Person where parseJSON = withObject "Person" $ \o -> Person <$> o .: "name" <*> o .:? "age" decode "{"name":"Joe"}" :: Maybe Person yields Just (Person "Joe" Nothing). decode "{"name":"Joe","age":30}" yields Just (Person "Joe" (Just 30)). decode "{"name":"Joe","age":null}" also yields Just (Person "Joe" Nothing) due to Maybe's parseJSON handling Null. For automatic derivation (GHC.Generics): deriving (Generic) instance ToJSON Person where toJSON = genericToJSON defaultOptions { omitNothingFields = True } instance FromJSON Person where parseJSON = genericParseJSON defaultOptions { omitNothingFields = True, allowOmittedFields = True } omitNothingFields (default False) omits Nothing Maybe fields during encoding (no "field":null). allowOmittedFields (recent, default True) allows missing fields during parsing, defaulting Maybe fields to Nothing. In aeson 2.2+, supports more types like First/Last, newtypes. For Template Haskell: $(deriveJSON defaultOptions { omitNothingFields = True } ''Person) encode (Person "Joe" Nothing) yields "{"name":"Joe"}" (no age field).
Citations:
- 1: https://hackage.haskell.org/package/aeson/docs/Data-Aeson.html
- 2: https://www.stackage.org/haddock/nightly-2026-03-25/aeson-2.2.3.0/Data-Aeson.html
- 3: Breaking changes on 0.10 regarding Null field parsing on Maybe types haskell/aeson#287
- 4: https://stackoverflow.com/questions/47100569/omitting-nothing-null-fields-in-haskells-aeson
- 5: omitNothingFields not used in Generic Decode haskell/aeson#571
- 6: https://hackage.haskell.org/package/aeson-2.2.0.0/changelog
- 7: https://williamyaoh.com/posts/2019-10-19-a-cheatsheet-to-json-handling.html
Don't emit error.message: null in the NACK payload.
Line 74 serializes a Maybe Text directly with .=, so Nothing becomes JSON null. For this envelope, absent and null are not equivalent; consumers expecting an optional string-valued field typically require it to be omitted when unavailable, not present with a non-string value. Build the error object conditionally instead of always including the field.
Suggested fix
toJSON (BecknAPIError err) =
object
- [ "message" .= object ["ack" .= object ["status" .= ("NACK" :: Text)]],
- "error"
- .= object
- [ "code" .= Error.code err,
- "message" .= Error.message err
- ]
- ]
+ [ "message" .= object ["ack" .= object ["status" .= ("NACK" :: Text)]],
+ "error"
+ .= object
+ ( [ "code" .= Error.code err
+ ]
+ <> maybe [] (\msg -> ["message" .= msg]) (Error.message err)
+ )
+ ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| toJSON (BecknAPIError err) = | |
| object | |
| [ "message" .= object ["ack" .= object ["status" .= ("NACK" :: Text)]], | |
| "error" | |
| .= object | |
| [ "code" .= Error.code err, | |
| "message" .= Error.message err | |
| ] | |
| toJSON (BecknAPIError err) = | |
| object | |
| [ "message" .= object ["ack" .= object ["status" .= ("NACK" :: Text)]], | |
| "error" | |
| .= object | |
| ( [ "code" .= Error.code err | |
| ] | |
| <> maybe [] (\msg -> ["message" .= msg]) (Error.message err) | |
| ) | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@lib/mobility-core/src/Kernel/Types/Error/BaseError/HTTPError/BecknAPIError.hs`
around lines 68 - 75, The toJSON implementation for Be cknAPIError is emitting
"error.message": null when Error.message is Nothing; update the toJSON
(BecknAPIError err) code to build the inner "error" object conditionally: always
include "code" (Error.code err) but only add the "message" key when
Error.message err is Just value (omit the key for Nothing). Change the
construction inside toJSON (BecknAPIError err) (or the helper that builds the
error object) to assemble a list of key/value pairs that appends the "message"
pair only when present (e.g., via maybe/catMaybes) and then pass that to object
for the "error" field so consumers don’t receive a null message.
|
|
||
| General Public License along with this program. If not, see <https://www.gnu.org/licenses/>. | ||
| -} | ||
| {-# OPTIONS_GHC -Wno-error=deprecations #-} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hsRepository: nammayatri/shared-kernel
Length of output: 4338
🏁 Script executed:
cd lib/mobility-core && find . -name "*.cabal" -o -name "package.yaml" | head -5Repository: nammayatri/shared-kernel
Length of output: 105
🏁 Script executed:
grep -A 5 -B 5 "wai" lib/mobility-core/mobility-core.cabal | head -30Repository: nammayatri/shared-kernel
Length of output: 547
🏁 Script executed:
cat lib/mobility-core/package.yaml | grep -A 10 "wai"Repository: nammayatri/shared-kernel
Length of output: 623
🏁 Script executed:
cat lib/mobility-core/package.yaml | grep -B 2 -A 2 "^ - wai$"Repository: nammayatri/shared-kernel
Length of output: 121
🏁 Script executed:
find . -name "stack.yaml" -o -name "cabal.project" | xargs cat 2>/dev/null | grep -i "wai" | head -10Repository: nammayatri/shared-kernel
Length of output: 50
🌐 Web query:
WAI Haskell responseToStream deprecated wai package
💡 Result:
No, responseToStream is not deprecated in the WAI Haskell package. It is an active function in the current version (3.2.4, latest as of Stackage Nightly 2026-04-05 and Hackage), introduced since version 3.0.0. The official documentation explicitly describes its purpose: "Converting the body information in Response to a StreamingBody." It is referenced in docs as the standard way to modify response bodies, e.g., in middleware like GZIP from wai-extra. The only deprecation in recent changelogs (3.2.2) is for requestBody in favor of getRequestBodyChunk, not responseToStream. Source code confirms its presence without deprecation warnings. Users should continue using it, often with responseStream for creating streaming responses, wrapped in bracket for resource safety.
Citations:
- 1: https://hackage-content.haskell.org/package/wai-3.2.4/docs/Network-Wai.html
- 2: https://hackage.haskell.org/package/wai/docs/Network-Wai.html
- 3: https://www.stackage.org/haddock/nightly-2026-03-17/wai-3.2.4/Network-Wai.html
- 4: http://hackage.haskell.org/package/wai-3.2.4/changelog
- 5: https://www.stackage.org/haddock/nightly-2026-02-15/wai-3.2.4/Network-Wai.html
- 6: https://www.stackage.org/package/wai
- 7: http://github.com/yesodweb/wai
- 8: https://github.com/yesodweb/wai/blob/master/CONTRIBUTING.md
- 9: https://hackage.haskell.org/package/wai-3.2.4/changelog
🌐 Web query:
WAI Haskell consumeRequestBodyStrict deprecated
💡 Result:
No, consumeRequestBodyStrict is not deprecated in WAI. It was added in wai-3.2.3 as a synonym for strictRequestBody to explicitly signal the non-idempotent nature of consuming the request body. It remains available in the current version (wai-3.2.4). The deprecations in WAI relate to requestBody (deprecated in favor of getRequestBodyChunk for reading) and the older requestBody field setter (use setRequestBodyChunks instead).
Citations:
- 1: https://hackage.haskell.org/package/wai-3.2.4/changelog
- 2: https://hackage.haskell.org/package/wai/docs/Network-Wai.html
- 3: https://hackage.haskell.org/package/wai/docs/src/Network.Wai.html
- 4: https://hackage.haskell.org/package/wai-3.2.4/docs/Network-Wai-Internal.html
- 5: https://www.stackage.org/package/wai
- 6: https://www.stackage.org/haddock/nightly-2026-03-20/wai-3.2.4/Network-Wai.html
🏁 Script executed:
grep -r "deprecated" lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hsRepository: nammayatri/shared-kernel
Length of output: 50
🏁 Script executed:
cd lib/mobility-core && ghc -Wall -fno-code src/Kernel/Utils/Servant/BecknResponseWrapper.hs 2>&1 | head -50Repository: nammayatri/shared-kernel
Length of output: 110
🏁 Script executed:
rg "Wai\." lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hsRepository: nammayatri/shared-kernel
Length of output: 526
🏁 Script executed:
rg "requestBody" lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hs -B 2 -A 2Repository: nammayatri/shared-kernel
Length of output: 218
🌐 Web query:
WAI Haskell requestBody field deprecated getRequestBodyChunk
💡 Result:
Yes, the requestBody field in WAI's Request type is deprecated. It was deprecated in wai version 3.2.2 (2019) because its name is misleading—it returns only the next chunk of the request body, not the entire body. Developers should use getRequestBodyChunk instead for reading the body. To read the request body, loop calling getRequestBodyChunk request until it returns an empty ByteString: do chunk <- getRequestBodyChunk request if BS.null chunk then return else process chunk For setting the requestBody field (e.g., in middleware) without deprecation warnings, use setRequestBodyChunks, added in wai 3.2.4 (2023). As of the latest version (wai 3.2.4, current in 2026 Stackage nightlies), the deprecation remains in place; it has not been removed.
Citations:
- 1: https://hackage.haskell.org/package/wai/docs/Network-Wai.html
- 2: https://hackage.haskell.org/package/wai-3.2.4/docs/Network-Wai-Internal.html
- 3: https://www.stackage.org/haddock/nightly-2026-02-15/wai-3.2.4/Network-Wai.html
- 4: https://hackage.haskell.org/package/wai-3.2.4/changelog
- 5: Deprecate requestBody yesodweb/wai#726
- 6: wai: add getRequestBodyChunk to silence requestBody deprecation warning yesodweb/wai#949
- 7: https://hackage.haskell.org/package/wai
Replace the deprecated requestBody field setter with setRequestBodyChunks.
Line 59 uses the deprecated Wai.requestBody field (deprecated in wai 3.2.2). Replace pure r {Wai.requestBody = replay} with Wai.setRequestBodyChunks replay r to fix the deprecation without suppressing the warning module-wide. Once fixed, remove the pragma at line 14.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hs` at line
14, Replace the deprecated direct field update of Wai.requestBody (the
expression `pure r { Wai.requestBody = replay }`) with the provided setter
`Wai.setRequestBodyChunks replay r` where the request is reconstructed; update
the code inside the function that performs the replay so it calls
`Wai.setRequestBodyChunks replay r` instead of mutating `requestBody`, and then
remove the top-module pragma `{-# OPTIONS_GHC -Wno-error=deprecations `#-`}` since
the deprecation is resolved.
| then app reqWithReplayableBody $ \resp -> do | ||
| let (status, headers, withBody) = Wai.responseToStream resp | ||
| respBody <- consumeStreamingBody withBody | ||
| let final = fromMaybe respBody (wrapIfAckEnvelope respBody) | ||
| respond $ Wai.responseLBS status headers final |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hsRepository: nammayatri/shared-kernel
Length of output: 4338
This buffers every response when wrapping is enabled, not just ACK/NACK payloads.
When wrap is True, the middleware unconditionally drains the streaming response via consumeStreamingBody at line 38. For responses that don't need wrapping (when wrapIfAckEnvelope returns Nothing), the already-buffered respBody is sent via responseLBS, consuming O(response size) memory unnecessarily. This removes streaming benefits and backpressure handling for non-ACK responses passing through this middleware.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mobility-core/src/Kernel/Utils/Servant/BecknResponseWrapper.hs` around
lines 36 - 40, The middleware currently always drains the streaming response via
consumeStreamingBody (invoked after Wai.responseToStream) which buffers every
response; change the logic so you only consume/buffer when wrapIfAckEnvelope
indicates wrapping is required: call Wai.responseToStream to get (status,
headers, withBody), then attempt a lightweight check for whether the body needs
wrapping (e.g., by peeking headers or otherwise deciding without draining); if
wrapIfAckEnvelope indicates Nothing, forward the original streaming response
unchanged using Wai.responseStream or Wai.responseSource with the original
withBody to preserve streaming/backpressure; only call consumeStreamingBody and
apply wrapIfAckEnvelope when you know wrapping is required, then respond with
Wai.responseLBS for the wrapped payload. Ensure you reference the current
symbols: app, reqWithReplayableBody, Wai.responseToStream, withBody (the
streaming body), consumeStreamingBody, wrapIfAckEnvelope, respond,
Wai.responseLBS/responseStream.
1 participant
Type of Change
Description
Additional Changes
Motivation and Context
How did you test it?
Checklist
./dev/format-all-files.shSummary by CodeRabbit
New Features
Enhancements