Skip to content

TRUNK-6519: Fix NPE when validating Role with null name #5684

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dineshsuthar123 wants to merge 3 commits into
base: master
Choose a base branch
from
Open

TRUNK-6519: Fix NPE when validating Role with null name #5684

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
35d70bc
TRUNK-XXXX: Fix NPE in RoleValidator when role name is null
dineshsuthar123 Jan 19, 2026
861dd6e
Update api/src/main/java/org/openmrs/validator/RoleValidator.java
dineshsuthar123 Jan 19, 2026
5cc217c
TRUNK-XXXX: Improve readability by caching role name
dineshsuthar123 Jan 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions api/src/main/java/org/openmrs/validator/RoleValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
*/
@Handler(supports = { Role.class }, order = 50)
public class RoleValidator implements Validator {

/**
* Determines if the command object being submitted is a valid type
*
Expand All @@ -32,19 +32,22 @@ public class RoleValidator implements Validator {
public boolean supports(Class<?> c) {
return c.equals(Role.class);
}
Copy link
Contributor

@jwnasambu jwnasambu Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we discard these formatting changes?


/**
* Checks the form object for any inconsistencies/errors
*
* @see org.springframework.validation.Validator#validate(java.lang.Object,
* org.springframework.validation.Errors)
* <strong>Should</strong> throw NullPointerException if role is null
* <strong>Should</strong> fail validation if role is empty or whitespace
* <strong>Should</strong> pass validation if description is null or empty or whitespace
* <strong>Should</strong> fail validation if role has leading or trailing space
* <strong>Should</strong> pass validation if all required fields have proper values
* <strong>Should</strong> pass validation if field lengths are correct
* <strong>Should</strong> fail validation if field lengths are not correct
* <strong>Should</strong> fail validation if role is null
* <strong>Should</strong> fail validation if role is empty or whitespace
* <strong>Should</strong> pass validation if description is null or empty
* or whitespace
* <strong>Should</strong> fail validation if role has leading or trailing
* space
* <strong>Should</strong> pass validation if all required fields have
* proper values
* <strong>Should</strong> pass validation if field lengths are correct
* <strong>Should</strong> fail validation if field lengths are not correct
*/
@Override
public void validate(Object obj, Errors errors) {
Expand All @@ -53,9 +56,9 @@ public void validate(Object obj, Errors errors) {
errors.rejectValue("role", "error.general");
} else {
ValidationUtils.rejectIfEmptyOrWhitespace(errors, "role", "error.role");

// 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())) {
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if (role.getRole() != null && !role.getRole().equals(role.getRole().trim())) {
String roleName = role.getRole();
if (roleName != null && !roleName.equals(roleName.trim())) {

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@jwnasambu jwnasambu Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kindly for readability, is it okay to store role.getRole() in a local variable instead of calling it multiple times.

Copy link
Author

@dineshsuthar123 dineshsuthar123 Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve updated the code to store role.getRole() in a local variable for improved readability.

Copy link
Author

@dineshsuthar123 dineshsuthar123 Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

errors.rejectValue("role", "error.trailingSpaces");
}
}
Expand All @@ -65,5 +68,5 @@ public void validate(Object obj, Errors errors) {
ValidateUtil.validateFieldLengths(errors, obj.getClass(), "role", "description");
}
}
Copy link
Contributor

@jwnasambu jwnasambu Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same applies here and the rest of the files.


}
49 changes: 25 additions & 24 deletions api/src/test/java/org/openmrs/validator/RoleValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
* Tests methods on the {@link RoleValidator} class.
*/
public class RoleValidatorTest extends BaseContextSensitiveTest {

/**
* @see RoleValidator#validate(Object,Errors)
*/
Expand All @@ -32,22 +32,21 @@ public void validate_shouldFailValidationIfRoleIsNullOrEmptyOrWhitespace() {
Role role = new Role();
role.setRole(null);
role.setDescription("some text");
//TODO: change/fix this test when it is decided whether to change the validator behavior to avoid throwing an NPE
Errors errors = new BindException(role, "type");
//new RoleValidator().validate(role, errors);
//Assert.assertTrue(errors.hasFieldErrors("role"));
new RoleValidator().validate(role, errors);
assertTrue(errors.hasFieldErrors("role"));

role.setRole("");
errors = new BindException(role, "role");
new RoleValidator().validate(role, errors);
assertTrue(errors.hasFieldErrors("role"));

role.setRole(" ");
errors = new BindException(role, "role");
new RoleValidator().validate(role, errors);
assertTrue(errors.hasFieldErrors("role"));
}

/**
* @see RoleValidator#validate(Object,Errors)
*/
Expand All @@ -56,22 +55,22 @@ public void validate_shouldPassValidationIfDescriptionIsNullOrEmptyOrWhitespace(
Role role = new Role();
role.setRole("Bowling race car driver");
role.setDescription(null);

Errors errors = new BindException(role, "type");
new RoleValidator().validate(role, errors);
assertFalse(errors.hasFieldErrors("description"));

role.setDescription("");
errors = new BindException(role, "role");
new RoleValidator().validate(role, errors);
assertFalse(errors.hasFieldErrors("description"));

role.setDescription(" ");
errors = new BindException(role, "role");
new RoleValidator().validate(role, errors);
assertFalse(errors.hasFieldErrors("description"));
}

/**
* @see RoleValidator#validate(Object,Errors)
*/
Expand All @@ -80,19 +79,19 @@ public void validate_shouldFailValidationIfRoleHasLeadingOrTrailingSpace() {
Role role = new Role();
role.setDescription("some text");
role.setRole(" Bowling race car driver");

Errors errors = new BindException(role, "type");
new RoleValidator().validate(role, errors);
assertTrue(errors.hasFieldErrors("role"));
assertEquals("error.trailingSpaces", errors.getFieldError("role").getCode());

role.setRole("Bowling race car driver ");
errors = new BindException(role, "role");
new RoleValidator().validate(role, errors);
assertTrue(errors.hasFieldErrors("role"));
assertEquals("error.trailingSpaces", errors.getFieldError("role").getCode());
}

/**
* @see RoleValidator#validate(Object,Errors)
*/
Expand All @@ -101,13 +100,13 @@ public void validate_shouldPassValidationIfAllRequiredFieldsHaveProperValues() {
Role role = new Role();
role.setRole("Bowling race car driver");
role.setDescription("You don't bowl or race fast cars");

Errors errors = new BindException(role, "type");
new RoleValidator().validate(role, errors);

assertFalse(errors.hasErrors());
}

/**
* @see RoleValidator#validate(Object,Errors)
*/
Expand All @@ -116,27 +115,29 @@ public void validate_shouldPassValidationIfFieldLengthsAreCorrect() {
Role role = new Role();
role.setRole("Bowling race car driver");
role.setDescription("description");

Errors errors = new BindException(role, "type");
new RoleValidator().validate(role, errors);

assertFalse(errors.hasErrors());
}

/**
* @see RoleValidator#validate(Object,Errors)
*/
@Test
public void validate_shouldFailValidationIfFieldLengthsAreNotCorrect() {
Role role = new Role();
role
.setRole("too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text");
.setRole(
"too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text");
role
.setDescription("too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text");

.setDescription(
"too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text too long text");

Errors errors = new BindException(role, "type");
new RoleValidator().validate(role, errors);

assertTrue(errors.hasFieldErrors("role"));
}
}
Loading