[DSD-9789] Cherrypicked 5 commits#195
Merged
Prafulrakhade merged 5 commits intomosip:release-1.4.xfrom Mar 18, 2026
Merged
Conversation
Signed-off-by: Rajapandi M <138785181+rajapandi1234@users.noreply.github.com>
* ES-2914 Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Fixed review comments Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Fixed review comments Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Fixed review comments Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Fixed review comments Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> --------- Signed-off-by: ase-101 <sunkadaeanusha@gmail.com>
* Fixed mock identity validation issue removed unnecessary configurations Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Removed unnecessary configurations Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Fixed review comment Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Fixed review comment Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Fixed review comments Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> --------- Signed-off-by: ase-101 <sunkadaeanusha@gmail.com>
* fix: idrepo update identity workaround Signed-off-by: Nandhukumar <nandhukumare@gmail.com> * ES-2955 | fix: idrepo update identity workaround Signed-off-by: Nandhukumar <nandhukumare@gmail.com> --------- Signed-off-by: Nandhukumar <nandhukumare@gmail.com>
Signed-off-by: ase-101 <sunkadaeanusha@gmail.com>
WalkthroughThe PR refactors identity validation and UI specification handling across two plugins. Mock plugin replaces hardcoded field checks with JSON Schema validation, while the identity plugin switches from JSONPath-based parsing to JSON pointer-based fetching with master data integration, supported by domain-based configuration properties throughout. Changes
Sequence DiagramssequenceDiagram
participant Client
participant MockPlugin as Mock Plugin Service
participant SchemaEndpoint as Identity Schema Endpoint
participant Validator as JSON Schema Validator
Client->>MockPlugin: validate(action, profileDto)
MockPlugin->>MockPlugin: Check cached schema
alt Schema not cached
MockPlugin->>SchemaEndpoint: Fetch identity schema
SchemaEndpoint-->>MockPlugin: Return schema
MockPlugin->>MockPlugin: Cache schema
end
MockPlugin->>Validator: Load schema into validator
MockPlugin->>Validator: Validate profileDto.identity
Validator-->>MockPlugin: Return validation errors
alt Has errors
MockPlugin-->>Client: Throw InvalidProfileException
else No errors
MockPlugin-->>Client: Validation passed
end
sequenceDiagram
participant Client
participant IdentityPlugin as Identity Plugin Service
participant UISpecEndpoint as UI Spec Endpoint
participant JSONPointer as JSON Pointer Parser
participant MasterDataService as Master Data Service
participant DynamicFields as Dynamic Fields Endpoint
participant DocTypes as Doc Types Endpoint
Client->>IdentityPlugin: getUISpecification()
IdentityPlugin->>UISpecEndpoint: Fetch response via REST
UISpecEndpoint-->>IdentityPlugin: Return full response
IdentityPlugin->>JSONPointer: Extract spec using pointer
JSONPointer-->>IdentityPlugin: Return located spec
IdentityPlugin->>MasterDataService: fetchAllowedValuesFromMasterDataService()
MasterDataService->>DynamicFields: Fetch paginated dynamic fields
DynamicFields-->>MasterDataService: Return fields
MasterDataService->>DocTypes: Fetch document types & categories
DocTypes-->>MasterDataService: Return doc types
MasterDataService-->>IdentityPlugin: Return aggregated values
IdentityPlugin->>IdentityPlugin: Enrich spec with language, allowed values, maxUploadFileSize
IdentityPlugin-->>Client: Return enriched UI spec
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mock-plugin/src/test/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImplTest.java (1)
303-316:⚠️ Potential issue | 🟠 MajorMake this negative-path test fail when no exception is thrown.
Without an
Assert.fail(...)here, the test passes even if the identifier mismatch check regresses andcreateProfile()returns normally.Suggested patch
- try{ - mockProfileRegistryPlugin.createProfile("requestId", profileDto); - }catch (ProfileException e){ - Assert.assertEquals(ErrorConstants.IDENTIFIER_MISMATCH,e.getMessage()); - } + try { + mockProfileRegistryPlugin.createProfile("requestId", profileDto); + Assert.fail("Expected InvalidProfileException"); + } catch (InvalidProfileException e) { + Assert.assertEquals(ErrorConstants.IDENTIFIER_MISMATCH, e.getMessage()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mock-plugin/src/test/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImplTest.java` around lines 303 - 316, The test createProfile_withInValidRequestAndProfileDto_thenFail in MockProfileRegistryPluginImplTest currently swallows the case where no exception is thrown; after invoking mockProfileRegistryPlugin.createProfile("requestId", profileDto) inside the try block, add an assertion to fail the test (e.g., Assert.fail("Expected ProfileException for identifier mismatch")) so the test fails if createProfile(...) returns normally; keep the existing catch verifying ErrorConstants.IDENTIFIER_MISMATCH in the ProfileException.
🧹 Nitpick comments (6)
mosip-identity-plugin/src/main/resources/application.properties (1)
58-63: Consider externalizing fixed schema/domain literals for masterdata URLs.Line 58 and Line 63 still hardcode
esignet-signupandlanguages=eng. Making them configurable would improve portability across deployments/tenants.♻️ Proposed refactor
+# Masterdata UI/domain configuration +mosip.signup.mosipid.ui-spec.domain-name=${MOSIP_SIGNUP_UI_SPEC_DOMAIN:esignet-signup} +mosip.signup.mosipid.default-language=${MOSIP_SIGNUP_DEFAULT_LANGUAGE:eng} + -mosip.signup.mosipid.get-ui-spec.endpoint=${mosip.esignet.masterdata.domain}/v1/masterdata/uispec/esignet-signup/latest?type=schema +mosip.signup.mosipid.get-ui-spec.endpoint=${mosip.esignet.masterdata.domain}/v1/masterdata/uispec/${mosip.signup.mosipid.ui-spec.domain-name}/latest?type=schema ... -mosip.signup.mosipid.doc-types-category.endpoint=${mosip.esignet.masterdata.domain}/v1/masterdata/applicanttype/000/languages?languages=eng +mosip.signup.mosipid.doc-types-category.endpoint=${mosip.esignet.masterdata.domain}/v1/masterdata/applicanttype/000/languages?languages=${mosip.signup.mosipid.default-language}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mosip-identity-plugin/src/main/resources/application.properties` around lines 58 - 63, The properties mosip.signup.mosipid.get-ui-spec.endpoint and mosip.signup.mosipid.doc-types-category.endpoint currently embed fixed literals ("esignet-signup" and "languages=eng"); make those parts configurable by introducing new properties (e.g., mosip.esignet.uiSpec.path and mosip.esignet.docTypes.languages or similar) and reference them via placeholders in the endpoint values so deployments/tenants can override the UI spec path and language list without changing the URL line; update any code/config consumers that build these endpoints to use the new properties (keep the existing property keys to minimize changes but replace hardcoded segments with the new placeholders).mosip-identity-plugin/src/main/java/io/mosip/signup/plugin/mosipid/service/IdrepoProfileRegistryPluginImpl.java (1)
689-737: Remove unused variabletotalItems.The variable
totalItemsis declared on line 693 but never used in the method, making it dead code.🧹 Proposed fix
private void fetchAndProcessDynamicFields(ObjectNode result) { int pageNumber = 0; int pageSize = 10; int totalPages = 1; - int totalItems = 0; while (pageNumber < totalPages) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mosip-identity-plugin/src/main/java/io/mosip/signup/plugin/mosipid/service/IdrepoProfileRegistryPluginImpl.java` around lines 689 - 737, Remove the dead local variable totalItems from the method fetchAndProcessDynamicFields: delete the declaration "int totalItems = 0;" since it is never read or used; ensure no other references to totalItems exist in fetchAndProcessDynamicFields so the method compiles cleanly (no other behavioral changes required).mosip-identity-plugin/src/test/java/io/mosip/signup/plugin/mosipid/service/IdrepoProfileRegistryPluginImplTest.java (3)
74-74:dynamicFieldsBaseUrltest value lacks pagination placeholders.The production code in
buildDynamicFieldsUrl()usesString.format(dynamicFieldsBaseUrl, pageNumber, pageSize), which expects the URL to contain%dplaceholders. The test setsdynamicFieldsBaseUrlto"http://mock/api/dynamicFields"without placeholders, which will causeString.formatto return the URL unchanged (no parameters interpolated). While this happens to work becauseMockito.contains("dynamic")is used for matching, it doesn't accurately test the production behavior.🔧 Proposed fix
- ReflectionTestUtils.setField(idrepoProfileRegistryPlugin, "dynamicFieldsBaseUrl","http://mock/api/dynamicFields"); + ReflectionTestUtils.setField(idrepoProfileRegistryPlugin, "dynamicFieldsBaseUrl","http://mock/api/dynamicFields?pageNumber=%d&pageSize=%d");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mosip-identity-plugin/src/test/java/io/mosip/signup/plugin/mosipid/service/IdrepoProfileRegistryPluginImplTest.java` at line 74, The test sets dynamicFieldsBaseUrl without pagination placeholders so buildDynamicFieldsUrl (which calls String.format(dynamicFieldsBaseUrl, pageNumber, pageSize)) isn't exercised correctly; update the test to set dynamicFieldsBaseUrl to a format string containing two %d placeholders (e.g., "http://mock/api/dynamicFields?page=%d&size=%d") via ReflectionTestUtils.setField on idrepoProfileRegistryPlugin, then assert or match the final URL produced by buildDynamicFieldsUrl (or interactions that use it) to include the interpolated pageNumber and pageSize values instead of relying on a generic Mockito.contains("dynamic") match.
590-619: Test name is misleading.The test
fetchAllowedValues_FromMasterDataService_withInactiveDynamicField_thenFaildoesn't test a failure scenario—it verifies that inactive dynamic fields are correctly filtered out, resulting in an empty result. Consider renaming to better reflect the actual behavior being tested.📝 Suggested name
- public void fetchAllowedValues_FromMasterDataService_withInactiveDynamicField_thenFail() { + public void fetchAllowedValues_FromMasterDataService_withInactiveDynamicField_thenExcluded() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mosip-identity-plugin/src/test/java/io/mosip/signup/plugin/mosipid/service/IdrepoProfileRegistryPluginImplTest.java` around lines 590 - 619, Rename the test method fetchAllowedValues_FromMasterDataService_withInactiveDynamicField_thenFail in IdrepoProfileRegistryPluginImplTest to a name that reflects the expected behavior (for example fetchAllowedValues_FromMasterDataService_withInactiveDynamicField_returnsEmptyResult or fetchAllowedValues_filtersOutInactiveDynamicFields_andReturnsEmpty), and update any references or test annotations accordingly so the test name accurately describes that inactive dynamic fields are filtered out and an empty JsonNode is returned.
622-672: Test name is misleading (same issue).Similar to the previous test, this test verifies filtering behavior rather than a failure. Consider renaming for clarity.
📝 Suggested name
- public void fetchAllowedValues_FromMasterDataService_withInactiveDocumentTypesAndCategories_thenFail() { + public void fetchAllowedValues_FromMasterDataService_withInactiveDocumentTypesAndCategories_thenExcluded() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mosip-identity-plugin/src/test/java/io/mosip/signup/plugin/mosipid/service/IdrepoProfileRegistryPluginImplTest.java` around lines 622 - 672, The test method fetchAllowedValues_FromMasterDataService_withInactiveDocumentTypesAndCategories_thenFail is misnamed because it asserts filtering behavior (returns empty result) rather than a failure; rename the test to something like fetchAllowedValues_FromMasterDataService_withOnlyInactiveDocumentTypesAndCategories_returnsEmpty or fetchAllowedValues_FromMasterDataService_filtersOutInactiveAndReturnsEmpty, update the test method name accordingly and any references (e.g., in IDE/run configurations) so it clearly describes that idrepoProfileRegistryPlugin.fetchAllowedValuesFromMasterDataService is expected to filter out inactive entries and produce an empty JsonNode in this scenario.mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java (1)
16-19: Add an explicitcom.networkntdependency to avoid transitive dependency fragility.This class directly uses
JsonSchema,JsonSchemaFactory,SpecVersion, andValidationMessagefromcom.networknt.schema(lines 16–19, 92–103), but the pom.xml does not declare an explicit dependency. These types are arriving transitively from one of the provided dependencies (signup-integration-apioresignet-integration-api). Adding an explicitjson-schema-validatordependency makes the module more maintainable and resilient to upstream dependency-graph changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java` around lines 16 - 19, The module is relying on com.networknt.schema types transitively—add an explicit dependency to the module's pom.xml for com.networknt:json-schema-validator (pin to a compatible version used by the project) so JsonSchema, JsonSchemaFactory, SpecVersion and ValidationMessage used in MockProfileRegistryPluginImpl are provided directly; update the module pom to include that dependency (or a managed version in the parent) and run mvn -U dependency:tree to verify the artifact is present and no longer coming only transitively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java`:
- Around line 97-109: Validate the incoming action against ACTIONS before any
schema lazy-loading to avoid hitting identitySchemaEndpoint on invalid requests:
move the ACTIONS.contains(action) check (currently logging error and throwing
InvalidProfileException with ErrorConstants.INVALID_ACTION) to occur before the
synchronized block that initializes schema (which calls
request(identitySchemaEndpoint, HttpMethod.GET, ...) and
JsonSchemaFactory.getSchema). Keep the existing error log and throw behavior but
perform it prior to any calls to request(...) or schema initialization.
---
Outside diff comments:
In
`@mock-plugin/src/test/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImplTest.java`:
- Around line 303-316: The test
createProfile_withInValidRequestAndProfileDto_thenFail in
MockProfileRegistryPluginImplTest currently swallows the case where no exception
is thrown; after invoking mockProfileRegistryPlugin.createProfile("requestId",
profileDto) inside the try block, add an assertion to fail the test (e.g.,
Assert.fail("Expected ProfileException for identifier mismatch")) so the test
fails if createProfile(...) returns normally; keep the existing catch verifying
ErrorConstants.IDENTIFIER_MISMATCH in the ProfileException.
---
Nitpick comments:
In
`@mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java`:
- Around line 16-19: The module is relying on com.networknt.schema types
transitively—add an explicit dependency to the module's pom.xml for
com.networknt:json-schema-validator (pin to a compatible version used by the
project) so JsonSchema, JsonSchemaFactory, SpecVersion and ValidationMessage
used in MockProfileRegistryPluginImpl are provided directly; update the module
pom to include that dependency (or a managed version in the parent) and run mvn
-U dependency:tree to verify the artifact is present and no longer coming only
transitively.
In
`@mosip-identity-plugin/src/main/java/io/mosip/signup/plugin/mosipid/service/IdrepoProfileRegistryPluginImpl.java`:
- Around line 689-737: Remove the dead local variable totalItems from the method
fetchAndProcessDynamicFields: delete the declaration "int totalItems = 0;" since
it is never read or used; ensure no other references to totalItems exist in
fetchAndProcessDynamicFields so the method compiles cleanly (no other behavioral
changes required).
In `@mosip-identity-plugin/src/main/resources/application.properties`:
- Around line 58-63: The properties mosip.signup.mosipid.get-ui-spec.endpoint
and mosip.signup.mosipid.doc-types-category.endpoint currently embed fixed
literals ("esignet-signup" and "languages=eng"); make those parts configurable
by introducing new properties (e.g., mosip.esignet.uiSpec.path and
mosip.esignet.docTypes.languages or similar) and reference them via placeholders
in the endpoint values so deployments/tenants can override the UI spec path and
language list without changing the URL line; update any code/config consumers
that build these endpoints to use the new properties (keep the existing property
keys to minimize changes but replace hardcoded segments with the new
placeholders).
In
`@mosip-identity-plugin/src/test/java/io/mosip/signup/plugin/mosipid/service/IdrepoProfileRegistryPluginImplTest.java`:
- Line 74: The test sets dynamicFieldsBaseUrl without pagination placeholders so
buildDynamicFieldsUrl (which calls String.format(dynamicFieldsBaseUrl,
pageNumber, pageSize)) isn't exercised correctly; update the test to set
dynamicFieldsBaseUrl to a format string containing two %d placeholders (e.g.,
"http://mock/api/dynamicFields?page=%d&size=%d") via
ReflectionTestUtils.setField on idrepoProfileRegistryPlugin, then assert or
match the final URL produced by buildDynamicFieldsUrl (or interactions that use
it) to include the interpolated pageNumber and pageSize values instead of
relying on a generic Mockito.contains("dynamic") match.
- Around line 590-619: Rename the test method
fetchAllowedValues_FromMasterDataService_withInactiveDynamicField_thenFail in
IdrepoProfileRegistryPluginImplTest to a name that reflects the expected
behavior (for example
fetchAllowedValues_FromMasterDataService_withInactiveDynamicField_returnsEmptyResult
or fetchAllowedValues_filtersOutInactiveDynamicFields_andReturnsEmpty), and
update any references or test annotations accordingly so the test name
accurately describes that inactive dynamic fields are filtered out and an empty
JsonNode is returned.
- Around line 622-672: The test method
fetchAllowedValues_FromMasterDataService_withInactiveDocumentTypesAndCategories_thenFail
is misnamed because it asserts filtering behavior (returns empty result) rather
than a failure; rename the test to something like
fetchAllowedValues_FromMasterDataService_withOnlyInactiveDocumentTypesAndCategories_returnsEmpty
or fetchAllowedValues_FromMasterDataService_filtersOutInactiveAndReturnsEmpty,
update the test method name accordingly and any references (e.g., in IDE/run
configurations) so it clearly describes that
idrepoProfileRegistryPlugin.fetchAllowedValuesFromMasterDataService is expected
to filter out inactive entries and produce an empty JsonNode in this scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d1d421f3-7304-4e97-9b5a-ce3ad056e20f
📒 Files selected for processing (8)
README.mdmock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.javamock-plugin/src/main/resources/application.propertiesmock-plugin/src/test/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImplTest.javamosip-identity-plugin/src/main/java/io/mosip/signup/plugin/mosipid/dto/Password.javamosip-identity-plugin/src/main/java/io/mosip/signup/plugin/mosipid/service/IdrepoProfileRegistryPluginImpl.javamosip-identity-plugin/src/main/resources/application.propertiesmosip-identity-plugin/src/test/java/io/mosip/signup/plugin/mosipid/service/IdrepoProfileRegistryPluginImplTest.java
...-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes