Merged
Conversation
Signed-off-by: ase-101 <sunkadaeanusha@gmail.com>
Signed-off-by: ase-101 <sunkadaeanusha@gmail.com>
WalkthroughReplaces username-based checks with runtime JSON Schema validation: plugin fetches and caches identity-schema and ui-schema endpoints, validates profileDto.identity against the schema for CREATE/UPDATE (special-cases update error 1028), switches to configurable identifierField, and updates tests/configs to exercise schema-driven flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Plugin as MockProfileRegistryPlugin
participant SchemaAPI as Identity-Schema Endpoint
participant Validator as JsonSchema Validator
Client->>Plugin: validate(action, profileDto)
alt schema not cached
Plugin->>SchemaAPI: GET identity-schema
SchemaAPI-->>Plugin: schema JSON
Plugin->>Plugin: parse -> JsonSchema (cache)
end
Plugin->>Validator: validate(profileDto.identity)
alt valid
Validator-->>Plugin: success
Plugin-->>Client: validation OK
else invalid
Validator-->>Plugin: validation errors
Plugin->>Plugin: map errors -> invalid_<field> (ignore code 1028 on UPDATE)
Plugin-->>Client: return/throw validation errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 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
🧹 Nitpick comments (4)
mock-plugin/src/test/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImplTest.java (1)
130-269: Good test coverage for validation scenarios.The tests appropriately cover valid data, missing required fields, invalid enum values, pattern mismatches, and UPDATE-specific behavior. Consider extracting the repeated mock setup into a helper method to reduce duplication:
♻️ Optional: Extract common mock setup
private void setupIdentitySchemaEndpointMock() throws JsonProcessingException { ReflectionTestUtils.setField(mockProfileRegistryPlugin, "identitySchemaEndpoint", "http://localhost:8080/"); ResponseWrapper<JsonNode> wrapper = new ResponseWrapper<>(); wrapper.setResponse(objectMapper.readTree(IDENTITY_SCHEMA)); ResponseEntity<ResponseWrapper<JsonNode>> responseEntity = new ResponseEntity<>(wrapper, HttpStatus.OK); Mockito.when(restTemplate.exchange( Mockito.eq("http://localhost:8080/"), Mockito.any(HttpMethod.class), Mockito.any(), Mockito.eq(new ParameterizedTypeReference<ResponseWrapper<JsonNode>>() {}) )).thenReturn(responseEntity); }🤖 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 130 - 269, Tests repeat the same identity schema mock setup across validate_* tests; extract that repeated setup into a helper method (e.g., setupIdentitySchemaEndpointMock) and call it from each test to reduce duplication. Move the ReflectionTestUtils.setField call, ResponseWrapper<JsonNode> creation with objectMapper.readTree(IDENTITY_SCHEMA), ResponseEntity construction, and the Mockito.when(restTemplate.exchange(...)) stub into that helper, and update each test to invoke setupIdentitySchemaEndpointMock() before creating the ProfileDto and calling mockProfileRegistryPlugin.validate(...).mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java (3)
106-109: Consider validating action before fetching schema.The action validation occurs after the schema fetch. If the action is invalid, the REST call to fetch the schema has already been made unnecessarily. Moving the action check before schema initialization would be more efficient.
♻️ Suggested reorder
`@Override` public void validate(String action, ProfileDto profileDto) throws InvalidProfileException { + if(!ACTIONS.contains(action)) { + log.error("Invalid action value : {}. Allowed values are CREATE and UPDATE", action); + throw new InvalidProfileException(ErrorConstants.INVALID_ACTION); + } if(schema == null) { ResponseWrapper<JsonNode> responseWrapper = request(identitySchemaEndpoint, HttpMethod.GET ,null, new ParameterizedTypeReference<ResponseWrapper<JsonNode>>() {}); JsonSchemaFactory jsonSchemaFactory = JsonSchemaFactory.getInstance(SpecVersion.VersionFlag.V202012); schema = jsonSchemaFactory.getSchema(responseWrapper.getResponse()); } - if(!ACTIONS.contains(action)) { - log.error("Invalid action value : {}. Allowed values are CREATE and UPDATE", action); - throw new InvalidProfileException(ErrorConstants.INVALID_ACTION); - }🤖 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 106 - 109, In MockProfileRegistryPluginImpl, validate the incoming action against ACTIONS before any schema fetching or schema initialization occurs so you avoid the unnecessary REST call; move the existing check that throws InvalidProfileException (the block referencing log.error and ErrorConstants.INVALID_ACTION) to precede the schema-fetching code path (the method that initializes/loads the profile schema in the same class) so the method returns/throws early on invalid action values like CREATE/UPDATE before invoking the external schema retrieval.
94-104: Thread safety concern with lazy schema initialization.The
schemafield is lazily initialized without synchronization. In a multi-threaded environment (typical for Spring web applications), multiple threads may concurrently detectschema == nulland fetch the schema redundantly.Consider using
@PostConstructfor eager initialization or add synchronization:♻️ Option 1: Eager initialization with `@PostConstruct`
+ `@PostConstruct` + public void init() { + ResponseWrapper<JsonNode> responseWrapper = request(identitySchemaEndpoint, HttpMethod.GET, null, + new ParameterizedTypeReference<ResponseWrapper<JsonNode>>() {}); + JsonSchemaFactory jsonSchemaFactory = JsonSchemaFactory.getInstance(SpecVersion.VersionFlag.V202012); + schema = jsonSchemaFactory.getSchema(responseWrapper.getResponse()); + } + `@Override` public void validate(String action, ProfileDto profileDto) throws InvalidProfileException { - - if(schema == null) { - ResponseWrapper<JsonNode> responseWrapper = request(identitySchemaEndpoint, HttpMethod.GET ,null, - new ParameterizedTypeReference<ResponseWrapper<JsonNode>>() {}); - JsonSchemaFactory jsonSchemaFactory = JsonSchemaFactory.getInstance(SpecVersion.VersionFlag.V202012); - schema = jsonSchemaFactory.getSchema(responseWrapper.getResponse()); - }♻️ Option 2: Double-checked locking
- private JsonSchema schema; + private volatile JsonSchema schema; `@Override` public void validate(String action, ProfileDto profileDto) throws InvalidProfileException { - if(schema == null) { - ResponseWrapper<JsonNode> responseWrapper = request(identitySchemaEndpoint, HttpMethod.GET ,null, - new ParameterizedTypeReference<ResponseWrapper<JsonNode>>() {}); - JsonSchemaFactory jsonSchemaFactory = JsonSchemaFactory.getInstance(SpecVersion.VersionFlag.V202012); - schema = jsonSchemaFactory.getSchema(responseWrapper.getResponse()); + synchronized(this) { + if(schema == null) { + ResponseWrapper<JsonNode> responseWrapper = request(identitySchemaEndpoint, HttpMethod.GET, null, + new ParameterizedTypeReference<ResponseWrapper<JsonNode>>() {}); + JsonSchemaFactory jsonSchemaFactory = JsonSchemaFactory.getInstance(SpecVersion.VersionFlag.V202012); + schema = jsonSchemaFactory.getSchema(responseWrapper.getResponse()); + } + } }🤖 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 94 - 104, The schema field is lazily initialized in validate(...) without synchronization causing race conditions; make initialization thread-safe by either (A) eagerly fetching and assigning schema in a `@PostConstruct` method using the existing identitySchemaEndpoint and request(...) call and JsonSchemaFactory.getSchema(...), or (B) keep lazy init but declare the field volatile and wrap the fetch in a double-checked locking pattern inside validate(String action, ProfileDto profileDto) (check schema == null, synchronized on the class/instance, re-check schema == null, then call request(...) and assign schema). Ensure you reference the existing identitySchemaEndpoint, request(...) helper, JsonSchemaFactory.getInstance(...), and JsonSchema.getSchema(...) when moving or wrapping the initialization.
117-120: Consider extracting error code "1028" to a named constant for clarity.Error code
"1028"correctly identifies required-field validation errors in networknt-schema, but extracting it to a constant would improve code maintainability and self-documentation.Suggested improvement: Extract to a named constant
+ private static final String REQUIRED_FIELD_ERROR_CODE = "1028"; + `@Override` public void validate(String action, ProfileDto profileDto) throws InvalidProfileException { // ... for(ValidationMessage error : errors) { // ... - if(action.equals("UPDATE") && error.getCode().equals("1028")) { + if(action.equals("UPDATE") && error.getCode().equals(REQUIRED_FIELD_ERROR_CODE)) { //Ignore required field validation errors for update action continue; }🤖 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 117 - 120, Replace the magic string "1028" with a descriptive constant to improve readability and maintainability: add a private static final String REQUIRED_FIELD_ERROR_CODE = "1028" (or an int if codes are numeric) in MockProfileRegistryPluginImpl and use that constant in the conditional that checks error.getCode() alongside action.equals("UPDATE"); update any related comments to reference REQUIRED_FIELD_ERROR_CODE so the intent (required-field validation error) is self-documenting.
🤖 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 128-131: The current check uses
profileDto.getIdentity().get(identifierField).asText() which can NPE if the
identity map doesn't contain identifierField; update the condition in
MockProfileRegistryPluginImpl so you first verify identifierField is not null
and profileDto.getIdentity().has(identifierField) (or that get(identifierField)
!= null) before calling asText(), and if the field is missing or its text does
not equal profileDto.getIndividualId() log the same message and throw
InvalidProfileException(ErrorConstants.IDENTIFIER_MISMATCH); ensure you
reference identifierField, profileDto.getIdentity(),
profileDto.getIndividualId(), InvalidProfileException and
ErrorConstants.IDENTIFIER_MISMATCH in the fix.
---
Nitpick comments:
In
`@mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java`:
- Around line 106-109: In MockProfileRegistryPluginImpl, validate the incoming
action against ACTIONS before any schema fetching or schema initialization
occurs so you avoid the unnecessary REST call; move the existing check that
throws InvalidProfileException (the block referencing log.error and
ErrorConstants.INVALID_ACTION) to precede the schema-fetching code path (the
method that initializes/loads the profile schema in the same class) so the
method returns/throws early on invalid action values like CREATE/UPDATE before
invoking the external schema retrieval.
- Around line 94-104: The schema field is lazily initialized in validate(...)
without synchronization causing race conditions; make initialization thread-safe
by either (A) eagerly fetching and assigning schema in a `@PostConstruct` method
using the existing identitySchemaEndpoint and request(...) call and
JsonSchemaFactory.getSchema(...), or (B) keep lazy init but declare the field
volatile and wrap the fetch in a double-checked locking pattern inside
validate(String action, ProfileDto profileDto) (check schema == null,
synchronized on the class/instance, re-check schema == null, then call
request(...) and assign schema). Ensure you reference the existing
identitySchemaEndpoint, request(...) helper, JsonSchemaFactory.getInstance(...),
and JsonSchema.getSchema(...) when moving or wrapping the initialization.
- Around line 117-120: Replace the magic string "1028" with a descriptive
constant to improve readability and maintainability: add a private static final
String REQUIRED_FIELD_ERROR_CODE = "1028" (or an int if codes are numeric) in
MockProfileRegistryPluginImpl and use that constant in the conditional that
checks error.getCode() alongside action.equals("UPDATE"); update any related
comments to reference REQUIRED_FIELD_ERROR_CODE so the intent (required-field
validation error) is self-documenting.
In
`@mock-plugin/src/test/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImplTest.java`:
- Around line 130-269: Tests repeat the same identity schema mock setup across
validate_* tests; extract that repeated setup into a helper method (e.g.,
setupIdentitySchemaEndpointMock) and call it from each test to reduce
duplication. Move the ReflectionTestUtils.setField call,
ResponseWrapper<JsonNode> creation with objectMapper.readTree(IDENTITY_SCHEMA),
ResponseEntity construction, and the Mockito.when(restTemplate.exchange(...))
stub into that helper, and update each test to invoke
setupIdentitySchemaEndpointMock() before creating the ProfileDto and calling
mockProfileRegistryPlugin.validate(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4a01865e-91bd-4bce-8fd9-acc7da06fd3a
📒 Files selected for processing (4)
mock-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/resources/application.properties
...-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: ase-101 <sunkadaeanusha@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java (1)
126-130:⚠️ Potential issue | 🔴 CriticalCritical logic error causes NPE - condition is inverted.
The condition
!profileDto.getIdentity().hasNonNull(identifierField)checks if the field is missing, then proceeds to call.get(identifierField).asText()which will throw NPE sinceget()returnsnullfor missing keys.The intended logic should throw when the field exists but values don't match:
🐛 Proposed fix with corrected logic
`@Override` public ProfileResult createProfile(String requestId, ProfileDto profileDto) throws ProfileException { - if(identifierField != null && !profileDto.getIdentity().hasNonNull(identifierField) - && !profileDto.getIndividualId().equalsIgnoreCase(profileDto.getIdentity().get(identifierField).asText())) { + if(identifierField != null && profileDto.getIdentity().hasNonNull(identifierField) + && !profileDto.getIndividualId().equalsIgnoreCase(profileDto.getIdentity().get(identifierField).asText())) { log.error("{} and userName mismatch", identifierField); throw new InvalidProfileException(ErrorConstants.IDENTIFIER_MISMATCH); }🤖 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 126 - 130, The logic in MockProfileRegistryPluginImpl is inverted and causes an NPE by calling profileDto.getIdentity().get(identifierField).asText() when the field is missing; update the condition to first verify the identifier exists and is non-null (use profileDto.getIdentity().hasNonNull(identifierField) or fetch the JsonNode and null-check it) AND then compare its text to profileDto.getIndividualId() (negate the equalsIgnoreCase) so that you only call .asText() when the field is present; on mismatch, keep throwing InvalidProfileException(ErrorConstants.IDENTIFIER_MISMATCH) and logging via log.error with identifierField.
🤖 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`:
- Line 92: The JsonSchema field "schema" in MockProfileRegistryPluginImpl is
lazily initialized without synchronization causing a race; make the field
volatile and either eagerly load it in a new `@PostConstruct` method (call the
existing logic that fetches from identitySchemaEndpoint) or implement
double-checked locking inside validate() (check schema != null, synchronize on
this, re-check schema, then fetch and assign) so concurrent calls to validate()
do not trigger duplicate HTTP requests or see a partially initialized schema.
---
Duplicate comments:
In
`@mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java`:
- Around line 126-130: The logic in MockProfileRegistryPluginImpl is inverted
and causes an NPE by calling
profileDto.getIdentity().get(identifierField).asText() when the field is
missing; update the condition to first verify the identifier exists and is
non-null (use profileDto.getIdentity().hasNonNull(identifierField) or fetch the
JsonNode and null-check it) AND then compare its text to
profileDto.getIndividualId() (negate the equalsIgnoreCase) so that you only call
.asText() when the field is present; on mismatch, keep throwing
InvalidProfileException(ErrorConstants.IDENTIFIER_MISMATCH) and logging via
log.error with identifierField.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 79ea6b37-3a10-43c1-accd-01af9a6af04e
📒 Files selected for processing (1)
mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java
...-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: ase-101 <sunkadaeanusha@gmail.com>
Signed-off-by: ase-101 <sunkadaeanusha@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java (1)
97-105:⚠️ Potential issue | 🟠 MajorIncomplete double-checked locking pattern.
The
synchronizedblock is missing the inner null-check, so multiple threads that pass the outerif(schema == null)before one acquires the lock will each perform the HTTP fetch redundantly.🔒 Proposed fix with proper double-checked locking
if(schema == null) { synchronized (this) { + if(schema == null) { ResponseWrapper<JsonNode> responseWrapper = request(identitySchemaEndpoint, HttpMethod.GET, null, new ParameterizedTypeReference<ResponseWrapper<JsonNode>>() { }); JsonSchemaFactory jsonSchemaFactory = JsonSchemaFactory.getInstance(SpecVersion.VersionFlag.V202012); schema = jsonSchemaFactory.getSchema(responseWrapper.getResponse()); + } } }🤖 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 97 - 105, The synchronized block in MockProfileRegistryPluginImpl uses a double-checked locking pattern but lacks the inner null-check and proper volatility for the cached variable; add an inner "if (schema == null)" inside the synchronized(this) before performing the HTTP fetch (request(identitySchemaEndpoint,...)) so only the first thread initializes schema, and ensure the field schema is declared volatile to make the double-checked locking safe.
🧹 Nitpick comments (4)
mock-plugin/src/test/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImplTest.java (2)
327-338: Commented-out mock may indicate incomplete test setup.The mock for
mockIdentity.get("individualId")is commented out at line 328. The test passes becausehasNonNull("individualId")returnsfalseby default on the mock, causing the identifier mismatch check to be skipped entirely. Consider either:
- Uncommenting and properly mocking the identifier field to test the actual validation path, or
- Adding a comment explaining why this behavior is intentionally not tested here.
🤖 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 327 - 338, The test currently leaves the mock for mockIdentity.get("individualId") commented out which lets hasNonNull("individualId") remain false and skips the identifier validation path; either uncomment and properly mock mockIdentity.get("individualId") (and/or mockIdentity.hasNonNull("individualId") to return true) so the ProfileDto validation executes the identifier check, or add a brief inline comment next to the commented line explaining why the identifier path is intentionally not exercised; reference the mock object mockIdentity and the ProfileDto setup to locate where to apply the change.
242-269: Missing test for UPDATE action bypassing required-field errors.This test validates that UPDATE fails when a field has an invalid pattern. However, there's no test verifying that UPDATE succeeds when only required fields are missing (the code path at lines 118-120 in the implementation that checks for error code "1028"). Adding such a test would ensure the bypass logic works as intended.
💚 Suggested test case
`@Test` public void validate_withUpdateMissingRequiredFields_thenPass() throws JsonProcessingException { ReflectionTestUtils.setField(mockProfileRegistryPlugin, "identitySchemaEndpoint", "http://localhost:8080/"); String action = "UPDATE"; ResponseWrapper<JsonNode> wrapper = new ResponseWrapper<>(); wrapper.setResponse(objectMapper.readTree(IDENTITY_SCHEMA)); ResponseEntity<ResponseWrapper<JsonNode>> responseEntity = new ResponseEntity<>(wrapper, HttpStatus.OK); Mockito.when(restTemplate.exchange( Mockito.eq("http://localhost:8080/"), Mockito.any(HttpMethod.class), Mockito.any(), Mockito.eq(new ParameterizedTypeReference<ResponseWrapper<JsonNode>>() { }))).thenReturn(responseEntity); // Only individualId provided, missing fullName, phone, password (all required) String userinfo = "{\"individualId\": \"1234567890\"}"; JsonNode mockIdentity = objectMapper.readTree(userinfo); ProfileDto profileDto = new ProfileDto(); profileDto.setIndividualId("1234567890"); profileDto.setIdentity(mockIdentity); // Should pass - required field errors are ignored for UPDATE mockProfileRegistryPlugin.validate(action, profileDto); }🤖 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 242 - 269, Add a new test method (e.g., validate_withUpdateMissingRequiredFields_thenPass) in MockProfileRegistryPluginImplTest that mirrors the existing setup (set ReflectionTestUtils field "identitySchemaEndpoint", mock restTemplate.exchange to return a ResponseWrapper<JsonNode> built from IDENTITY_SCHEMA), set action = "UPDATE", build a ProfileDto with only individualId and identity JSON containing only individualId, then call mockProfileRegistryPlugin.validate(action, profileDto) and assert it does not throw (no Assert.fail); this verifies the UPDATE path that bypasses required-field errors (error code "1028") works as intended.mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java (2)
107-110: Consider validating action before fetching schema.The action validation occurs after the schema fetch. If an invalid action is provided, an unnecessary HTTP request may be made. Moving the action check before the schema initialization would avoid this.
♻️ Proposed reordering
`@Override` public void validate(String action, ProfileDto profileDto) throws InvalidProfileException { + if(!ACTIONS.contains(action)) { + log.error("Invalid action value : {}. Allowed values are CREATE and UPDATE", action); + throw new InvalidProfileException(ErrorConstants.INVALID_ACTION); + } if(schema == null) { synchronized (this) { - ResponseWrapper<JsonNode> responseWrapper = request(identitySchemaEndpoint, HttpMethod.GET, null, + if(schema == null) { + ResponseWrapper<JsonNode> responseWrapper = request(identitySchemaEndpoint, HttpMethod.GET, null, new ParameterizedTypeReference<ResponseWrapper<JsonNode>>() { }); - JsonSchemaFactory jsonSchemaFactory = JsonSchemaFactory.getInstance(SpecVersion.VersionFlag.V202012); - schema = jsonSchemaFactory.getSchema(responseWrapper.getResponse()); + JsonSchemaFactory jsonSchemaFactory = JsonSchemaFactory.getInstance(SpecVersion.VersionFlag.V202012); + schema = jsonSchemaFactory.getSchema(responseWrapper.getResponse()); + } } } - if(!ACTIONS.contains(action)) { - log.error("Invalid action value : {}. Allowed values are CREATE and UPDATE", action); - throw new InvalidProfileException(ErrorConstants.INVALID_ACTION); - }🤖 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 107 - 110, Move the action validation to before any schema fetch/initialization to avoid unnecessary HTTP calls: check ACTIONS.contains(action) first inside MockProfileRegistryPluginImpl (the same block that currently logs via log.error and throws new InvalidProfileException(ErrorConstants.INVALID_ACTION)), and return/throw immediately on invalid action so subsequent schema fetch/initialization code is not executed.
118-121: Useerror.getType()instead of magic error code "1028".The error code
"1028"is a built-in constant from thenetworknt/json-schema-validatorlibrary representing required field validation errors. Instead of coupling to this numeric code, use the library's publicgetType()method, which returns"required"as a string—this is more maintainable and resilient to library changes.Replace:
if(action.equals("UPDATE") && error.getCode().equals("1028"))With:
if(action.equals("UPDATE") && error.getType().equals("required"))🤖 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 118 - 121, In MockProfileRegistryPluginImpl, replace the magic numeric schema error check that uses error.getCode().equals("1028") with a type-based check using error.getType().equals("required") so the UPDATE-path that skips required-field validation uses the schema validator's public type instead of a hard-coded code; update the conditional inside the loop where variables action and error are used (the if(action.equals("UPDATE") && ... ) block) to check error.getType() for "required".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java`:
- Around line 97-105: The synchronized block in MockProfileRegistryPluginImpl
uses a double-checked locking pattern but lacks the inner null-check and proper
volatility for the cached variable; add an inner "if (schema == null)" inside
the synchronized(this) before performing the HTTP fetch
(request(identitySchemaEndpoint,...)) so only the first thread initializes
schema, and ensure the field schema is declared volatile to make the
double-checked locking safe.
---
Nitpick comments:
In
`@mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.java`:
- Around line 107-110: Move the action validation to before any schema
fetch/initialization to avoid unnecessary HTTP calls: check
ACTIONS.contains(action) first inside MockProfileRegistryPluginImpl (the same
block that currently logs via log.error and throws new
InvalidProfileException(ErrorConstants.INVALID_ACTION)), and return/throw
immediately on invalid action so subsequent schema fetch/initialization code is
not executed.
- Around line 118-121: In MockProfileRegistryPluginImpl, replace the magic
numeric schema error check that uses error.getCode().equals("1028") with a
type-based check using error.getType().equals("required") so the UPDATE-path
that skips required-field validation uses the schema validator's public type
instead of a hard-coded code; update the conditional inside the loop where
variables action and error are used (the if(action.equals("UPDATE") && ... )
block) to check error.getType() for "required".
In
`@mock-plugin/src/test/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImplTest.java`:
- Around line 327-338: The test currently leaves the mock for
mockIdentity.get("individualId") commented out which lets
hasNonNull("individualId") remain false and skips the identifier validation
path; either uncomment and properly mock mockIdentity.get("individualId")
(and/or mockIdentity.hasNonNull("individualId") to return true) so the
ProfileDto validation executes the identifier check, or add a brief inline
comment next to the commented line explaining why the identifier path is
intentionally not exercised; reference the mock object mockIdentity and the
ProfileDto setup to locate where to apply the change.
- Around line 242-269: Add a new test method (e.g.,
validate_withUpdateMissingRequiredFields_thenPass) in
MockProfileRegistryPluginImplTest that mirrors the existing setup (set
ReflectionTestUtils field "identitySchemaEndpoint", mock restTemplate.exchange
to return a ResponseWrapper<JsonNode> built from IDENTITY_SCHEMA), set action =
"UPDATE", build a ProfileDto with only individualId and identity JSON containing
only individualId, then call mockProfileRegistryPlugin.validate(action,
profileDto) and assert it does not throw (no Assert.fail); this verifies the
UPDATE path that bypasses required-field errors (error code "1028") works as
intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d6227042-4d87-4996-aaec-2a3ef2e48c8d
📒 Files selected for processing (2)
mock-plugin/src/main/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImpl.javamock-plugin/src/test/java/io/mosip/signup/plugin/mock/service/MockProfileRegistryPluginImplTest.java
Prafulrakhade
approved these changes
Mar 13, 2026
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
3 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
Refactor
Bug Fixes
Chores