Skip to content

fix: improve user API message handling and fix detached entity bug #249

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

Merged
devondragon merged 3 commits into from
Jan 26, 2026
Merged

fix: improve user API message handling and fix detached entity bug #249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f3009e3
fix(api): re-fetch user from database in updatePassword endpoint
devondragon Jan 26, 2026
50e0a03
feat(i18n): add default message fallbacks for missing translations
devondragon Jan 26, 2026
556ecd0
fix: address PR feedback for user API improvements
devondragon Jan 26, 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
28 changes: 19 additions & 9 deletions src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,19 @@ public ResponseEntity<JSONResponse> updateUserAccount(@AuthenticationPrincipal D
@Valid @RequestBody UserProfileUpdateDto profileUpdateDto,
HttpServletRequest request, Locale locale) {
validateAuthenticatedUser(userDetails);
User user = userDetails.getUser();
// Re-fetch user from database to ensure we have an attached entity
User user = userService.findUserByEmail(userDetails.getUser().getEmail());
if (user == null) {
log.error("User not found in database: {}", userDetails.getUser().getEmail());
return buildErrorResponse(messages.getMessage("message.user.not-found", null, "User not found", locale), 1, HttpStatus.BAD_REQUEST);
}
user.setFirstName(profileUpdateDto.getFirstName());
user.setLastName(profileUpdateDto.getLastName());
userService.saveRegisteredUser(user);

logAuditEvent("ProfileUpdate", "Success", "User profile updated", user, request);

return buildSuccessResponse(messages.getMessage("message.update-user.success", null, locale), null);
return buildSuccessResponse(messages.getMessage("message.update-user.success", null, "Profile updated successfully", locale), null);
}

/**
Expand Down Expand Up @@ -206,7 +211,7 @@ public ResponseEntity<JSONResponse> savePassword(@Valid @RequestBody SavePasswor
// are not a concern. Constant-time comparison is only needed when comparing
// against stored credentials, which is handled by Spring's PasswordEncoder.
if (!savePasswordDto.getNewPassword().equals(savePasswordDto.getConfirmPassword())) {
return buildErrorResponse(messages.getMessage("message.password.mismatch", null, locale), 1,
return buildErrorResponse(messages.getMessage("message.password.mismatch", null, "Passwords do not match", locale), 1,
HttpStatus.BAD_REQUEST);
}

Expand All @@ -216,14 +221,14 @@ public ResponseEntity<JSONResponse> savePassword(@Valid @RequestBody SavePasswor

if (tokenResult != UserService.TokenValidationResult.VALID) {
String messageKey = "auth.message." + tokenResult.getValue();
return buildErrorResponse(messages.getMessage(messageKey, null, locale), 2, HttpStatus.BAD_REQUEST);
return buildErrorResponse(messages.getMessage(messageKey, null, "Invalid or expired token", locale), 2, HttpStatus.BAD_REQUEST);
}

// Get user by token
Optional<User> userOptional = userService.getUserByPasswordResetToken(savePasswordDto.getToken());

if (userOptional.isEmpty()) {
return buildErrorResponse(messages.getMessage("auth.message.invalid", null, locale), 3,
return buildErrorResponse(messages.getMessage("auth.message.invalid", null, "Invalid token", locale), 3,
HttpStatus.BAD_REQUEST);
}

Expand All @@ -246,7 +251,7 @@ public ResponseEntity<JSONResponse> savePassword(@Valid @RequestBody SavePasswor

logAuditEvent("PasswordReset", "Success", "Password reset completed", user, request);

return buildSuccessResponse(messages.getMessage("message.reset-password.success", null, locale),
return buildSuccessResponse(messages.getMessage("message.reset-password.success", null, "Password has been reset successfully", locale),
"/user/login.html");

} catch (Exception ex) {
Expand All @@ -272,7 +277,12 @@ public ResponseEntity<JSONResponse> savePassword(@Valid @RequestBody SavePasswor
public ResponseEntity<JSONResponse> updatePassword(@AuthenticationPrincipal DSUserDetails userDetails,
@Valid @RequestBody PasswordDto passwordDto, HttpServletRequest request, Locale locale) {
validateAuthenticatedUser(userDetails);
User user = userDetails.getUser();
// Re-fetch user from database to ensure we have an attached entity
User user = userService.findUserByEmail(userDetails.getUser().getEmail());
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

The updatePassword method now calls userService.findUserByEmail(), but the existing unit tests don't mock this call. This will likely cause test failures. The tests should be updated to mock findUserByEmail to return the test user.

Copilot uses AI. Check for mistakes.
if (user == null) {
log.error("User not found in database: {}", userDetails.getUser().getEmail());
Comment on lines +280 to +283
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

The re-fetching of the user entity creates an inconsistency with other endpoints like updateUserAccount (line 158) which still use userDetails.getUser() directly. If detached entities are truly a concern for updatePassword, the same issue should exist for updateUserAccount which also calls userService.saveRegisteredUser(). Either both should re-fetch, or neither should. Consider whether this fix is actually necessary or if it indicates a broader issue with entity management.

Suggested change
// Re-fetch user from database to ensure we have an attached entity
User user = userService.findUserByEmail(userDetails.getUser().getEmail());
if (user == null) {
log.error("User not found in database: {}", userDetails.getUser().getEmail());
// Use the User entity from the authenticated user details for consistency with other endpoints
User user = userDetails.getUser();
if (user == null) {
log.error("Authenticated user has no associated User entity");

Copilot uses AI. Check for mistakes.
return buildErrorResponse(messages.getMessage("message.user.not-found", null, "User not found", locale), 1, HttpStatus.BAD_REQUEST);
}

try {
// Verify old password is correct
Expand All @@ -293,10 +303,10 @@ public ResponseEntity<JSONResponse> updatePassword(@AuthenticationPrincipal DSUs
userService.changeUserPassword(user, passwordDto.getNewPassword());
logAuditEvent("PasswordUpdate", "Success", "User password updated", user, request);

return buildSuccessResponse(messages.getMessage("message.update-password.success", null, locale), null);
return buildSuccessResponse(messages.getMessage("message.update-password.success", null, "Password updated successfully", locale), null);
} catch (InvalidOldPasswordException ex) {
logAuditEvent("PasswordUpdate", "Failure", "Invalid old password", user, request);
return buildErrorResponse(messages.getMessage("message.update-password.invalid-old", null, locale), 1,
return buildErrorResponse(messages.getMessage("message.update-password.invalid-old", null, "Invalid old password", locale), 1,
HttpStatus.BAD_REQUEST);
} catch (Exception ex) {
log.error("Unexpected error during password update.", ex);
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public ModelAndView showChangePasswordPage(final HttpServletRequest request, fin
String redirectString = "redirect:" + forgotPasswordChangeURI;
return new ModelAndView(redirectString, model);
} else {
String messageKey = AUTH_MESSAGE_PREFIX + result.name().toLowerCase();
String messageKey = AUTH_MESSAGE_PREFIX + result.getValue();
model.addAttribute("messageKey", messageKey);
return new ModelAndView("redirect:/index.html", model);
}
Expand Down Expand Up @@ -137,7 +137,7 @@ public ModelAndView confirmRegistration(final HttpServletRequest request, final
return new ModelAndView(redirectString, model);
}

model.addAttribute("messageKey", AUTH_MESSAGE_PREFIX + result.toString());
model.addAttribute("messageKey", AUTH_MESSAGE_PREFIX + result.getValue());
model.addAttribute("expired", result == TokenValidationResult.EXPIRED);
model.addAttribute("token", token);
log.debug("UserAPI.confirmRegistration: failed. Token not found or expired.");
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/com/digitalsanctuary/spring/user/web/GlobalMessageControllerAdvice.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ public void addMessageFromKey(WebRequest request, org.springframework.ui.Model m
String messageKey = request.getParameter("messageKey");
if (messageKey != null) {
Locale locale = request.getLocale();
String message = messages.getMessage(messageKey, null, locale);
// Use the messageKey itself as the default if no translation is found
String message = messages.getMessage(messageKey, null, messageKey, locale);
model.addAttribute("message", message);
}
}
Expand Down
16 changes: 10 additions & 6 deletions src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,8 @@ public Object resolveArgument(org.springframework.core.MethodParameter parameter
.setControllerAdvice(new TestExceptionHandler())
.build();

when(messageSource.getMessage(eq("message.update-password.success"), any(), any(Locale.class)))
when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser);
when(messageSource.getMessage(eq("message.update-password.success"), any(), any(), any(Locale.class)))
.thenReturn("Password updated successfully");
when(userService.checkIfValidOldPassword(any(User.class), eq("oldPassword"))).thenReturn(true);

Expand Down Expand Up @@ -443,7 +444,8 @@ public Object resolveArgument(org.springframework.core.MethodParameter parameter
.setControllerAdvice(new TestExceptionHandler())
.build();

when(messageSource.getMessage(eq("message.update-password.invalid-old"), any(), any(Locale.class)))
when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser);
when(messageSource.getMessage(eq("message.update-password.invalid-old"), any(), any(), any(Locale.class)))
.thenReturn("Invalid old password");
when(userService.checkIfValidOldPassword(any(User.class), eq("wrongPassword"))).thenReturn(false);

Expand Down Expand Up @@ -509,7 +511,8 @@ public Object resolveArgument(org.springframework.core.MethodParameter parameter
.setControllerAdvice(new TestExceptionHandler())
.build();

when(messageSource.getMessage(eq("message.update-user.success"), any(), any(Locale.class)))
when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser);
when(messageSource.getMessage(eq("message.update-user.success"), any(), any(), any(Locale.class)))
.thenReturn("Profile updated successfully");
when(userService.saveRegisteredUser(any(User.class))).thenReturn(testUser);

Expand All @@ -522,8 +525,8 @@ public Object resolveArgument(org.springframework.core.MethodParameter parameter
.andExpect(jsonPath("$.success").value(true))
.andExpect(jsonPath("$.messages[0]").value("Profile updated successfully"));

verify(userService).saveRegisteredUser(argThat(user ->
user.getFirstName().equals("UpdatedFirst") &&
verify(userService).saveRegisteredUser(argThat(user ->
user.getFirstName().equals("UpdatedFirst") &&
user.getLastName().equals("UpdatedLast")
));
}
Expand Down Expand Up @@ -741,7 +744,8 @@ public Object resolveArgument(org.springframework.core.MethodParameter parameter
.setControllerAdvice(new TestExceptionHandler())
.build();

when(messageSource.getMessage(eq("message.update-user.success"), any(), any(Locale.class)))
when(userService.findUserByEmail(testUser.getEmail())).thenReturn(testUser);
when(messageSource.getMessage(eq("message.update-user.success"), any(), any(), any(Locale.class)))
.thenReturn("Profile updated successfully");
when(userService.saveRegisteredUser(any(User.class))).thenReturn(testUser);

Expand Down
Loading