TRUNK-6519: Fix NPE when validating Role with null name #5684
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a NullPointerException that occurred in RoleValidator when validating a Role object with a null role name. The fix adds a null check before attempting to validate for trailing spaces, and updates the Javadoc to accurately reflect the validation behavior (fails validation instead of throwing NPE).
Changes:
- Added null check in RoleValidator.validate() before trailing-space validation to prevent NPE
- Enabled previously commented test case that validates null role name behavior
- Updated Javadoc from "throw NullPointerException" to "fail validation" to reflect correct behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| api/src/main/java/org/openmrs/validator/RoleValidator.java | Added null check before trailing space validation and updated Javadoc to reflect proper validation behavior instead of NPE |
| api/src/test/java/org/openmrs/validator/RoleValidatorTest.java | Enabled null role name test case and applied whitespace/formatting cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // reject any role that has a leading or trailing space | ||
| if (!role.getRole().equals(role.getRole().trim())) { | ||
| if (role.getRole() != null && !role.getRole().equals(role.getRole().trim())) { |
There was a problem hiding this comment.
Consider storing the result of role.getRole() in a local variable to avoid multiple method calls. While not critical, this would improve readability and marginally improve performance by calling the method only once instead of three times.
| if (role.getRole() != null && !role.getRole().equals(role.getRole().trim())) { | |
| String roleName = role.getRole(); | |
| if (roleName != null && !roleName.equals(roleName.trim())) { |
There was a problem hiding this comment.
Kindly for readability, is it okay to store role.getRole() in a local variable instead of calling it multiple times.
There was a problem hiding this comment.
I’ve updated the code to store role.getRole() in a local variable for improved readability.
There was a problem hiding this comment.
I’ve applied the readability improvements where the repeated calls occurred and cleaned up the related Javadoc.
If there’s any other specific section you’d like me to update for consistency, please let me know.
|
@dineshsuthar123 Kindly do you have a Jira issue for this PR? if yes attach the ticket link on this PR. |
| public boolean supports(Class<?> c) { | ||
| return c.equals(Role.class); | ||
| } | ||
There was a problem hiding this comment.
Can we discard these formatting changes?
| ValidateUtil.validateFieldLengths(errors, obj.getClass(), "role", "description"); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Same applies here and the rest of the files.
Thanks for the reminder |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
Can we make the PR and commit titles same as the ticket title? |
Yes, that works |
dineshsuthar123
changed the title
Thanks for your patience 🙏 |
3 participants
Summary
Fixes a NullPointerException in RoleValidator.validate() when validating a Role with a null role name.
Changes
Testing