feat/face-compare-api-addition#1331
Conversation
WalkthroughA new face comparison endpoint is introduced across the Idfy client library and verification interface. Request and response types are defined at both the Idfy provider level and the generic interface level. The Idfy client implements the HTTP call via Servant. An adapter bridges interface types to the Idfy client, and a dispatcher routes requests to Idfy or returns "Not Implemented" for other providers. ChangesFace Comparison Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant IdfyClient
participant IdfyService
Client->>IdfyClient: faceCompare(FaceCompareRequest)
IdfyClient->>IdfyService: POST /v3/tasks/sync/compare/face
IdfyService-->>IdfyClient: FaceCompareResponse
IdfyClient-->>Client: FaceCompareResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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. Comment |
|
@ClaudeWeb please review |
|
I'll fetch the PR details, diff, and run a security scan in parallel.Let me try using the GitHub MCP tools instead.Now let me get the changed files and look at the diff.Now let me look at the surrounding context in the existing source files to compare patterns.Now let me check the existing Idfy request/response types and the Interface/Idfy.hs for patterns around PR Review:
|
|
Checked ClaudeWeb comments . nothing critical |
RahulGupta0706
force-pushed
the
feat/face-compare-api-addition
branch
from
c35c0ce to
3caec28
Compare
|
@ClaudeWeb Can you review, if errors are handled properly ? |
|
Let me do a focused error handling review across all changed files.Let me fetch Error Handling Review —
|
| Layer | Verdict |
|---|---|
Client.hs HTTP transport errors |
✓ callApiUnwrappingApiError handles 4xx/5xx/network |
decrypt cfg.apiKey / cfg.accountId |
✓ throws on decryption failure |
buildIdfyRequest (GUID gen) |
✓ monadic, no silent failure path |
Interface.hs dispatcher throwError "Not Implemented!" |
✓ non-Idfy configs fail loudly |
JSON decode of FaceCompareResponseData |
✓ all fields Maybe, Servant throws on parse failure |
RahulGupta0706
force-pushed
the
feat/face-compare-api-addition
branch
2 times, most recently
from
0e06507 to
a7dd9d8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hs`:
- Line 26: IdfyCfg's faceCompareRetryLimit is currently Maybe Int with no
validation; update the decode/constructor to enforce a non-negative bounded
value (e.g., change the field to Maybe Natural/Word or validate in the FromJSON
instance for IdfyCfg) and apply a max cap constant (e.g.,
MAX_FACE_COMPARE_RETRIES) or reject out-of-range values, then ensure faceCompare
still uses retryLimit = fromMaybe 1 cfg.faceCompareRetryLimit but with the
validated/clamped value so negative or excessively large inputs are prevented;
modify the FromJSON for IdfyCfg (or a smart constructor) to perform this check
and return a parse error or clamp to the cap.
🪄 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: 2886cb79-21cf-47f5-88de-12f63591df37
📒 Files selected for processing (7)
lib/mobility-core/src/Kernel/External/Verification/Idfy/Client.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Request.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Response.hslib/mobility-core/src/Kernel/External/Verification/Interface.hslib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hslib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Client.hs
- lib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Request.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Response.hs
- lib/mobility-core/src/Kernel/External/Verification/Interface.hs
- lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs
| secret :: EncryptedField 'AsEncrypted Text, | ||
| url :: BaseUrl | ||
| url :: BaseUrl, | ||
| faceCompareRetryLimit :: Maybe Int |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all uses of faceCompareRetryLimit and whether any non-negative/bounded validation exists.
rg -n -C3 '\bfaceCompareRetryLimit\b|fromMaybe\s+1\s+cfg\.faceCompareRetryLimit|callWithRetry\s*\(' \
lib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hs \
lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hsRepository: nammayatri/shared-kernel
Length of output: 2361
🏁 Script executed:
#!/bin/bash
# Inspect callWithRetry definition and any handling of retryLimit/attemptsLeft <= 0
rg -n "callWithRetry|attemptsLeft" lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs -nRepository: nammayatri/shared-kernel
Length of output: 212
🏁 Script executed:
#!/bin/bash
# Show the surrounding implementation area for callWithRetry
sed -n '470,560p' lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hsRepository: nammayatri/shared-kernel
Length of output: 3378
🏁 Script executed:
#!/bin/bash
# Inspect Config.hs around the IdfyCfg definition and the faceCompareRetryLimit field
sed -n '1,120p' lib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hsRepository: nammayatri/shared-kernel
Length of output: 1269
🏁 Script executed:
#!/bin/bash
# Search whole repo for any other uses/validation of faceCompareRetryLimit
rg -n "\bfaceCompareRetryLimit\b" -S .Repository: nammayatri/shared-kernel
Length of output: 310
Constrain faceCompareRetryLimit to a non-negative, bounded value
IdfyCfg defines faceCompareRetryLimit :: Maybe Int with derived FromJSON (no validation), and the only consumer is retryLimit = fromMaybe 1 cfg.faceCompareRetryLimit in faceCompare. Negative values make attemptsLeft > 0 false immediately (no retries, early failure), while very large values can amplify external-call retries and latency. Add non-negative/bounded validation at the config decode boundary (e.g., Maybe Natural/Word plus a max cap) and/or clamp/explicitly reject out-of-range values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hs` at line
26, IdfyCfg's faceCompareRetryLimit is currently Maybe Int with no validation;
update the decode/constructor to enforce a non-negative bounded value (e.g.,
change the field to Maybe Natural/Word or validate in the FromJSON instance for
IdfyCfg) and apply a max cap constant (e.g., MAX_FACE_COMPARE_RETRIES) or reject
out-of-range values, then ensure faceCompare still uses retryLimit = fromMaybe 1
cfg.faceCompareRetryLimit but with the validated/clamped value so negative or
excessively large inputs are prevented; modify the FromJSON for IdfyCfg (or a
smart constructor) to perform this check and return a parse error or clamp to
the cap.
a7dd9d8 to
06bb283
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs`:
- Around line 551-553: The code returns FaceCompareResp {faceComparedData = Just
result} for any Just result; update the case handling for resp.result to first
verify the face-detection flags on the returned result (the detection fields on
result) are present and true, and only then set faceComparedData to Just result;
if any detection flag is false or missing, throw/return InternalError instead of
returning faceComparedData. Ensure this validation is implemented where
resp.result is matched (the Just result branch) and that the error uses the
existing InternalError type/constructor used elsewhere in this module.
- Around line 548-555: The current retry loop regenerates idfyReq (and its
task_id) each attempt in callWithRetry by calling buildIdfyRequest(req.driverId
reqData) inside the recursion; change the logic to build idfyReq once before
starting retries and pass that same idfyReq into callWithRetry (or convert
callWithRetry to accept an idfyReq parameter) so Idfy.faceCompare always
receives the same payload; update references to idfyReq inside callWithRetry and
keep the same retry/decrement logic and error handling around Idfy.faceCompare
and the attemptsLeft parameter.
🪄 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: cb64f86a-75d1-4562-a88c-c08cada6e8d3
📒 Files selected for processing (7)
lib/mobility-core/src/Kernel/External/Verification/Idfy/Client.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Request.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Response.hslib/mobility-core/src/Kernel/External/Verification/Interface.hslib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hslib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Client.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Request.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Response.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hs
- lib/mobility-core/src/Kernel/External/Verification/Interface.hs
- lib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs
| let callWithRetry attemptsLeft = do | ||
| idfyReq <- buildIdfyRequest req.driverId reqData | ||
| resp <- Idfy.faceCompare apiKey accountId url idfyReq | ||
| case resp.result of | ||
| Just result -> pure FaceCompareResp {faceComparedData = Just result} | ||
| Nothing | ||
| | attemptsLeft > 0 -> callWithRetry (attemptsLeft - 1) | ||
| | otherwise -> throwError $ InternalError "Please contact customer support, as face match cannot be done." |
There was a problem hiding this comment.
Avoid regenerating task_id on retries; reuse one request payload.
Line 549 currently rebuilds the request per retry, generating a new GUID each time. For external compare APIs this can create duplicate non-idempotent operations. Build idfyReq once before retry recursion and reuse it across attempts.
Suggested fix
faceCompare cfg req = do
let url = cfg.url
retryLimit = fromMaybe 1 cfg.faceCompareRetryLimit
apiKey <- decrypt cfg.apiKey
accountId <- decrypt cfg.accountId
let reqData =
Idfy.FaceCompareRequestBody
{ document1 = req.selfieImage,
document2 = req.documentImage
}
- let callWithRetry attemptsLeft = do
- idfyReq <- buildIdfyRequest req.driverId reqData
+ idfyReq <- buildIdfyRequest req.driverId reqData
+ let callWithRetry attemptsLeft = do
resp <- Idfy.faceCompare apiKey accountId url idfyReq
case resp.result of
Just result -> pure FaceCompareResp {faceComparedData = Just result}
Nothing
| attemptsLeft > 0 -> callWithRetry (attemptsLeft - 1)
| otherwise -> throwError $ InternalError "Please contact customer support, as face match cannot be done."
callWithRetry retryLimit🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs` around
lines 548 - 555, The current retry loop regenerates idfyReq (and its task_id)
each attempt in callWithRetry by calling buildIdfyRequest(req.driverId reqData)
inside the recursion; change the logic to build idfyReq once before starting
retries and pass that same idfyReq into callWithRetry (or convert callWithRetry
to accept an idfyReq parameter) so Idfy.faceCompare always receives the same
payload; update references to idfyReq inside callWithRetry and keep the same
retry/decrement logic and error handling around Idfy.faceCompare and the
attemptsLeft parameter.
| case resp.result of | ||
| Just result -> pure FaceCompareResp {faceComparedData = Just result} | ||
| Nothing |
There was a problem hiding this comment.
Gate success on face-detection validity before returning match data.
At Line 552, success is returned for any Just result, but face-compare outputs are unreliable when one or both images have no detected face. Please enforce a validation guard on the detection flags and throw InternalError when detection is false/missing instead of returning faceComparedData.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs` around
lines 551 - 553, The code returns FaceCompareResp {faceComparedData = Just
result} for any Just result; update the case handling for resp.result to first
verify the face-detection flags on the returned result (the detection fields on
result) are present and true, and only then set faceComparedData to Just result;
if any detection flag is false or missing, throw/return InternalError instead of
returning faceComparedData. Ensure this validation is implemented where
resp.result is matched (the Just result branch) and that the error uses the
existing InternalError type/constructor used elsewhere in this module.
| deriving (Show, Generic, FromJSON, ToJSON, ToSchema) | ||
|
|
||
| data FaceCompareReq = FaceCompareReq | ||
| { selfieImage :: Text, |
There was a problem hiding this comment.
We can keep generic names. Order doesn't matter anyway
4 participants
Type of Change
Description
Additional Changes
Motivation and Context
How did you test it?
Checklist
./dev/format-all-files.shSummary by CodeRabbit
New Features
Configuration