feat: SDK-based biometric quality evaluation with SBI fallback - #785
feat: SDK-based biometric quality evaluation with SBI fallback#785mishradev1 wants to merge 1 commit into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR implements SDK-based biometric quality evaluation with SBI fallback in the registration client. It adds configuration-driven timeout enforcement, validates SDK scores against existing thresholds, introduces eight error scenarios for invalid scores and timeouts, and includes audit logging that does not interrupt capture flow. ChangesSDK-Based Biometric Quality Evaluation Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Signed-off-by: Dev Mishra <mishradev222004@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java`:
- Around line 181-190: The sdkScore is currently a primitive double from
getSDKScoreWithTimeout which will NPE if the SDK returned null; change handling
in BioServiceImpl:getSDKScoreWithTimeout() call site to use a boxed Double (or
keep double but first retrieve a Double) and explicitly check for null before
range validation — if null, log the error, call logAuditEvent("QUALITY_EVAL",
bioAttribute, "SDK_INVALID_SCORE", "SDK returned null score", null, "SDK") and
throw the REG_BIOMETRIC_SDK_INVALID_SCORE RegBaseCheckedException; if non-null
continue with the existing ValueRange validation and error path for out-of-range
values.
- Around line 122-125: In BioServiceImpl, change the SBI quality-score
validation so it only runs when sdkEnabled is false and use direct double
comparisons instead of casting to long: check biometricsDto.getQualityScore() >=
0.0 && biometricsDto.getQualityScore() <=
RegistrationConstants.MAX_BIO_QUALITY_SCORE, and if it fails throw the existing
RegBaseCheckedException with
RegistrationExceptionConstants.REG_BIOMETRIC_QUALITY_SCORE_RANGE_ERROR; ensure
this replaces the current ValueRange.of(...).isValidValue((long)
biometricsDto.getQualityScore()) check and references sdkEnabled and
biometricsDto.getQualityScore() so SDK-enabled flows aren’t blocked.
In
`@registration/registration-services/src/test/java/io/mosip/registration/bio/service/test/BioServiceTest.java`:
- Around line 322-391: Current tests only call getSDKScore() and check
ApplicationContext values; update them to exercise the full capture path by
invoking captureModality(...) on bioService with a mocked capture response so
the SDK vs SBI selection, timeout mapping, and qualityScore replacement are
actually executed. Specifically, in the SDK-enabled, disabled, and exception
scenarios: mock the capture provider (BioProviderImpl_V_0_9 via
bioAPIFactory.getBioProvider) to return a controlled capture result (successful
capture with attribute ISO and modality, and a failing capture for exception
test), call bioService.captureModality(...) and assert the returned
BiometricsDto fields (qualityScore replaced with SDK value, modalityName,
attributeISO present) or that the correct error code/exception is propagated;
also add a test that sets RegistrationConstants.SDK_QUALITY_EVALUATION_TIMEOUT
in ApplicationContext and verifies captureModality respects the timeout mapping.
Use existing symbols captureModality, getSDKScore, BioProviderImpl_V_0_9,
bioAPIFactory, BiometricsDto, and RegistrationConstants to locate code.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e98276a-9a19-4fe3-87c0-d43ab74aa680
📒 Files selected for processing (4)
registration/registration-services/src/main/java/io/mosip/registration/constants/RegistrationConstants.javaregistration/registration-services/src/main/java/io/mosip/registration/exception/RegistrationExceptionConstants.javaregistration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.javaregistration/registration-services/src/test/java/io/mosip/registration/bio/service/test/BioServiceTest.java
| // Validate SBI quality score is in valid range | ||
| if (!ValueRange.of(0, RegistrationConstants.MAX_BIO_QUALITY_SCORE).isValidValue((long) biometricsDto.getQualityScore())) | ||
| throw new RegBaseCheckedException(RegistrationExceptionConstants.REG_BIOMETRIC_QUALITY_SCORE_RANGE_ERROR.getErrorCode(), | ||
| RegistrationExceptionConstants.REG_BIOMETRIC_QUALITY_SCORE_RANGE_ERROR.getErrorMessage()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "BioServiceImpl.java" --exec wc -l {}Repository: mosip/registration-client
Length of output: 183
🏁 Script executed:
cat -n registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java | head -200Repository: mosip/registration-client
Length of output: 10151
🏁 Script executed:
cat -n registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java | sed -n '100,250p'Repository: mosip/registration-client
Length of output: 8613
🏁 Script executed:
# Search for sdkEnabled variable usage
rg "sdkEnabled" registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java -B 2 -A 2Repository: mosip/registration-client
Length of output: 391
🏁 Script executed:
# Search for RegistrationConstants.MAX_BIO_QUALITY_SCORE definition
rg "MAX_BIO_QUALITY_SCORE" registration/ -A 1Repository: mosip/registration-client
Length of output: 1233
Skip SBI validation when SDK is enabled; use direct double comparisons for range checks.
Line 123 validates the SBI quality score unconditionally, throwing an exception before the SDK evaluation path (line 137) can execute. When sdkEnabled=true, the SBI score should not block SDK evaluation since it is replaced at line 196. Additionally, both validations cast double to long before range checking, which truncates fractional values and incorrectly allows out-of-range scores like -0.5 (casts to 0) or 100.5 (casts to 100) to pass validation.
Condition the SBI validation on !sdkEnabled since the SBI score is only used when SDK is disabled. Replace the casting-based validation with direct double comparisons to preserve fractional precision.
🤖 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
`@registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java`
around lines 122 - 125, In BioServiceImpl, change the SBI quality-score
validation so it only runs when sdkEnabled is false and use direct double
comparisons instead of casting to long: check biometricsDto.getQualityScore() >=
0.0 && biometricsDto.getQualityScore() <=
RegistrationConstants.MAX_BIO_QUALITY_SCORE, and if it fails throw the existing
RegBaseCheckedException with
RegistrationExceptionConstants.REG_BIOMETRIC_QUALITY_SCORE_RANGE_ERROR; ensure
this replaces the current ValueRange.of(...).isValidValue((long)
biometricsDto.getQualityScore()) check and references sdkEnabled and
biometricsDto.getQualityScore() so SDK-enabled flows aren’t blocked.
| double sdkScore = getSDKScoreWithTimeout(biometricsDto); | ||
|
|
||
| // Validate SDK score is in valid range | ||
| if (!ValueRange.of(0, RegistrationConstants.MAX_BIO_QUALITY_SCORE).isValidValue((long) sdkScore)) { | ||
| LOGGER.error("SDK returned invalid quality score {} for attribute: {}", sdkScore, bioAttribute); | ||
| logAuditEvent("QUALITY_EVAL", bioAttribute, "SDK_INVALID_SCORE", | ||
| "SDK returned score out of valid range: " + sdkScore, sdkScore, "SDK"); | ||
| throw new RegBaseCheckedException( | ||
| RegistrationExceptionConstants.REG_BIOMETRIC_SDK_INVALID_SCORE.getErrorCode(), | ||
| RegistrationExceptionConstants.REG_BIOMETRIC_SDK_INVALID_SCORE.getErrorMessage()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "BioServiceImpl.java" | head -20Repository: mosip/registration-client
Length of output: 179
🏁 Script executed:
cd registration/registration-services && find . -type f -name "BioServiceImpl.java"Repository: mosip/registration-client
Length of output: 144
🏁 Script executed:
rg -l "BioServiceImpl" --type javaRepository: mosip/registration-client
Length of output: 177
🏁 Script executed:
wc -l registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.javaRepository: mosip/registration-client
Length of output: 181
🏁 Script executed:
sed -n '170,260p' registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.javaRepository: mosip/registration-client
Length of output: 4352
🏁 Script executed:
sed -n '1,450p' registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java | grep -n "getSDKScore" -A 15 | head -50Repository: mosip/registration-client
Length of output: 2458
🏁 Script executed:
sed -n '300,450p' registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.javaRepository: mosip/registration-client
Length of output: 6202
Auto-unboxing null SDK score causes incorrect exception path.
When getSDKScore returns null (Map.get on missing key), auto-unboxing to primitive double throws NullPointerException before reaching line 184. This NPE is caught as a generic Exception and wrapped into REG_BIOMETRIC_QUALITY_CHECK_ERROR, preventing the explicit REG_BIOMETRIC_SDK_INVALID_SCORE path from being reached. The method should return Double (boxed) or explicitly check for null to match the feature's intended error classification.
🤖 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
`@registration/registration-services/src/main/java/io/mosip/registration/service/bio/impl/BioServiceImpl.java`
around lines 181 - 190, The sdkScore is currently a primitive double from
getSDKScoreWithTimeout which will NPE if the SDK returned null; change handling
in BioServiceImpl:getSDKScoreWithTimeout() call site to use a boxed Double (or
keep double but first retrieve a Double) and explicitly check for null before
range validation — if null, log the error, call logAuditEvent("QUALITY_EVAL",
bioAttribute, "SDK_INVALID_SCORE", "SDK returned null score", null, "SDK") and
throw the REG_BIOMETRIC_SDK_INVALID_SCORE RegBaseCheckedException; if non-null
continue with the existing ValueRange validation and error path for out-of-range
values.
| @Test | ||
| public void sdkQualityEvaluationEnabledTest() throws BiometricException { | ||
| // Enable SDK quality check | ||
| ApplicationContext.map().put(RegistrationConstants.QUALITY_CHECK_WITH_SDK, RegistrationConstants.ENABLE); | ||
|
|
||
| Map<BiometricType, Float> qualityMap = new HashMap<>(); | ||
| qualityMap.put(BiometricType.FACE, Float.valueOf("85.0")); | ||
| BioProviderImpl_V_0_9 providerImpl_v_0_9 = Mockito.mock(BioProviderImpl_V_0_9.class); | ||
| Mockito.when(bioAPIFactory.getBioProvider(Mockito.any(), Mockito.any())).thenReturn(providerImpl_v_0_9); | ||
| Mockito.when(providerImpl_v_0_9.getModalityQuality(Mockito.any(), Mockito.any())).thenReturn(qualityMap); | ||
|
|
||
| BiometricsDto biometricsDto = new BiometricsDto(); | ||
| biometricsDto.setBioAttribute("face"); | ||
| biometricsDto.setQualityScore(70.0); | ||
| biometricsDto.setAttributeISO(new byte[]{1, 2, 3}); | ||
| biometricsDto.setModalityName(Modality.FACE.name()); | ||
|
|
||
| double sdkScore = bioService.getSDKScore(biometricsDto); | ||
| Assert.assertEquals(85.0, sdkScore, 0); | ||
|
|
||
| // Cleanup | ||
| ApplicationContext.map().remove(RegistrationConstants.QUALITY_CHECK_WITH_SDK); | ||
| } | ||
|
|
||
| @Test | ||
| public void sdkQualityDisabledFallbackToSbiTest() { | ||
| // Ensure SDK is disabled | ||
| ApplicationContext.map().put(RegistrationConstants.QUALITY_CHECK_WITH_SDK, RegistrationConstants.DISABLE); | ||
|
|
||
| boolean sdkEnabled = RegistrationConstants.ENABLE.equalsIgnoreCase( | ||
| (String) ApplicationContext.map().getOrDefault( | ||
| RegistrationConstants.QUALITY_CHECK_WITH_SDK, RegistrationConstants.DISABLE)); | ||
| Assert.assertFalse("SDK should be disabled, SBI fallback expected", sdkEnabled); | ||
|
|
||
| // Cleanup | ||
| ApplicationContext.map().remove(RegistrationConstants.QUALITY_CHECK_WITH_SDK); | ||
| } | ||
|
|
||
| @Test(expected = BiometricException.class) | ||
| public void sdkQualityEvaluationExceptionTest() throws BiometricException { | ||
| BioProviderImpl_V_0_9 providerImpl_v_0_9 = Mockito.mock(BioProviderImpl_V_0_9.class); | ||
| Mockito.when(bioAPIFactory.getBioProvider(Mockito.any(), Mockito.any())).thenReturn(providerImpl_v_0_9); | ||
| Mockito.when(providerImpl_v_0_9.getModalityQuality(Mockito.any(), Mockito.any())) | ||
| .thenThrow(new BiometricException("SDK_ERR", "SDK evaluation failed")); | ||
|
|
||
| BiometricsDto biometricsDto = new BiometricsDto(); | ||
| biometricsDto.setBioAttribute("face"); | ||
| biometricsDto.setQualityScore(70.0); | ||
| biometricsDto.setAttributeISO(new byte[]{1, 2, 3}); | ||
| biometricsDto.setModalityName(Modality.FACE.name()); | ||
|
|
||
| bioService.getSDKScore(biometricsDto); | ||
| } | ||
|
|
||
| @Test | ||
| public void sdkTimeoutConfigurationTest() { | ||
| // Test default timeout when not configured | ||
| Integer timeout = ApplicationContext.getIntValueFromApplicationMap( | ||
| RegistrationConstants.SDK_QUALITY_EVALUATION_TIMEOUT); | ||
| Assert.assertNull("Default timeout config should be null", timeout); | ||
|
|
||
| // Test configured timeout | ||
| ApplicationContext.map().put(RegistrationConstants.SDK_QUALITY_EVALUATION_TIMEOUT, "5000"); | ||
| timeout = ApplicationContext.getIntValueFromApplicationMap( | ||
| RegistrationConstants.SDK_QUALITY_EVALUATION_TIMEOUT); | ||
| Assert.assertEquals(Integer.valueOf(5000), timeout); | ||
|
|
||
| // Cleanup | ||
| ApplicationContext.map().remove(RegistrationConstants.SDK_QUALITY_EVALUATION_TIMEOUT); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
These tests never hit the new SDK capture path.
They only call getSDKScore() or inspect ApplicationContext, so regressions in captureModality(), SDK/SBI source selection, timeout mapping, or qualityScore replacement would still pass. Please drive the assertions through captureModality() with mocked capture responses and assert the returned BiometricsDto state/error code.
🤖 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
`@registration/registration-services/src/test/java/io/mosip/registration/bio/service/test/BioServiceTest.java`
around lines 322 - 391, Current tests only call getSDKScore() and check
ApplicationContext values; update them to exercise the full capture path by
invoking captureModality(...) on bioService with a mocked capture response so
the SDK vs SBI selection, timeout mapping, and qualityScore replacement are
actually executed. Specifically, in the SDK-enabled, disabled, and exception
scenarios: mock the capture provider (BioProviderImpl_V_0_9 via
bioAPIFactory.getBioProvider) to return a controlled capture result (successful
capture with attribute ISO and modality, and a failing capture for exception
test), call bioService.captureModality(...) and assert the returned
BiometricsDto fields (qualityScore replaced with SDK value, modalityName,
attributeISO present) or that the correct error code/exception is propagated;
also add a test that sets RegistrationConstants.SDK_QUALITY_EVALUATION_TIMEOUT
in ApplicationContext and verifies captureModality respects the timeout mapping.
Use existing symbols captureModality, getSDKScore, BioProviderImpl_V_0_9,
bioAPIFactory, BiometricsDto, and RegistrationConstants to locate code.
1 participant
Problem
Currently, the registration client validates biometric quality using only the SBI-provided quality score compared against configurable thresholds. There is no support for SDK-based quality evaluation, which can provide more accurate and standardized quality assessment during biometric capture.
Solution
This PR enables SDK-based biometric quality evaluation during capture in the registration client, while maintaining seamless fallback to SBI quality scores when the SDK is disabled in configuration.
When SDK is enabled (
mosip.registration.quality_check_with_sdk=Y):When SDK is disabled (default behavior):
Key Changes
BioServiceImpl.javaevaluateQualityWithSdk()for SDK-based evaluation with timeout support, corrupt data detection, and audit loggingRegistrationExceptionConstants.javaRegistrationConstants.javaSDK_QUALITY_EVALUATION_TIMEOUTconfiguration keyBioServiceTest.javaHow It Works
Scenario Coverage
Configuration
mosip.registration.quality_check_with_sdkNmosip.registration.sdk_quality_evaluation_timeout10000(ms)Design Decisions
SDK score replaces
qualityScorefield — When SDK succeeds, the SDK score is written to bothsdkScoreandqualityScoreinBiometricsDto. This ensures the existing threshold validation logic ingetCapturedBiometrics()andaddAllBiometrics()works without modification, reusing the same configurable thresholds.No silent SBI fallback on SDK failure — Per the acceptance criteria, when SDK is configured but fails, the system blocks and prompts re-capture rather than silently falling back to SBI. This prevents potentially low-quality biometrics from being accepted.
Configurable timeout with
ExecutorService— SDK evaluation runs in a separate thread with configurable timeout to prevent UI freezing on SDK delays.Audit logging — All quality evaluations, fallbacks, and errors are logged for audit purposes via
logAuditEvent().Type of Change
Checklist
Related Issue
Fixes #770
Summary by CodeRabbit
Release Notes
New Features
Tests