Skip to content

Refactor: Replace brittle constraint-name string matching with proactive repository checks in AuthService #41

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
Copilot wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Refactor: Replace brittle constraint-name string matching with proactive repository checks in AuthService #41

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
ff94c91
Initial plan
Copilot Apr 5, 2026
ce57ed9
Refactor: remove brittle string-based exception checking in AuthService
Copilot Apr 5, 2026
4630af8
Restore generic DataIntegrityViolationException safety net in setUser…
Copilot Apr 5, 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
17 changes: 5 additions & 12 deletions src/main/java/com/fredmaina/chatapp/Auth/services/AuthService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,12 @@ public class AuthService {
@CacheEvict(value = "usernameCheck", key = "#request.username.toLowerCase()")
public AuthResponse signUp(SignUpRequest request) {
AuthResponse authResponse = new AuthResponse();
if (request.getUsername() != null && userRepository.findByUsernameIgnoreCase(request.getUsername()).isPresent()) {
authResponse.setMessage("Username already exists (case-insensitive)");
if (request.getUsername() != null && userRepository.existsByUsernameIgnoreCase(request.getUsername())) {
authResponse.setMessage("Username already exists");
Comment thread
fred-maina marked this conversation as resolved.

Copilot AI Apr 5, 2026

Copy link

Choose a reason for hiding this comment

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

Changing the duplicate-username message from "Username already exists (case-insensitive)" to "Username already exists" will break any callers that branch on the exact message text. For example, AuthController.register currently returns HTTP 409 only when the message equals the old string, so duplicate usernames will now likely return 400 instead of 409. Consider keeping the message stable or updating the controller to use the new message (or better: return/propagate a dedicated error code/status instead of matching on message strings).

Suggested change
authResponse.setMessage("Username already exists");
authResponse.setMessage("Username already exists (case-insensitive)");

Copilot uses AI. Check for mistakes.
Comment on lines 49 to +53

Copilot AI Apr 5, 2026

Copy link

Choose a reason for hiding this comment

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

@CacheEvict uses key "#request.username.toLowerCase()" which will throw a SpEL evaluation error if signUp is ever called with a null username (and the method body already allows for null via request.getUsername() != null checks). To make this null-safe, add a condition = "#request.username != null" (or otherwise guard the key expression) so the cache eviction doesn't fail before the method executes.

Copilot uses AI. Check for mistakes.
authResponse.setSuccess(false);
return authResponse;
}
if (userRepository.findByEmail(request.getEmail()).isPresent()) {
if (userRepository.existsByEmail(request.getEmail())) {
authResponse.setMessage("Email already exists");
authResponse.setSuccess(false);
return authResponse;
Expand All @@ -80,14 +80,8 @@ public AuthResponse signUp(SignUpRequest request) {
authResponse.setUser(user);
authResponse.setToken(token);
} catch (DataIntegrityViolationException e) {
if (e.getMessage().contains("users_email_key")) {
authResponse.setMessage("Email already exists");
} else if (e.getMessage().contains("idx_user_username") || e.getMessage().contains("users_username_key")) {
authResponse.setMessage("Username already exists");
} else {
log.error("Data integrity violation during sign up: {}", e.getMessage());
authResponse.setMessage("Data integrity violation");
}
log.error("Data integrity violation during sign up: {}", e.getMessage());

Copilot AI Apr 5, 2026

Copy link

Choose a reason for hiding this comment

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

In the DataIntegrityViolationException handler, the error is logged without the exception object, which drops the stack trace and makes production debugging harder. Consider logging e as the throwable (and optionally using a stable identifier rather than only e.getMessage()).

Suggested change
log.error("Data integrity violation during sign up: {}", e.getMessage());
log.error("Data integrity violation during sign up: {}", e.getMessage(), e);

Copilot uses AI. Check for mistakes.
authResponse.setMessage("Data integrity violation");

Copilot AI Apr 5, 2026

Copy link

Choose a reason for hiding this comment

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

After removing constraint-name matching, this catch-all now always returns the generic message "Data integrity violation". Since the PR intent is to keep this as a race-condition safety net, you can still provide a user-friendly duplicate email/username message by re-checking existsByUsernameIgnoreCase/existsByEmail inside the catch and only falling back to the generic message if neither exists. This keeps the logic resilient without depending on driver/constraint strings.

Suggested change
authResponse.setMessage("Data integrity violation");
if (request.getUsername() != null && userRepository.existsByUsernameIgnoreCase(request.getUsername())) {
authResponse.setMessage("Username already exists");
} else if (userRepository.existsByEmail(request.getEmail())) {
authResponse.setMessage("Email already exists");
} else {
authResponse.setMessage("Data integrity violation");
}

Copilot uses AI. Check for mistakes.
authResponse.setSuccess(false);
} catch (Exception e) {
log.error("Unexpected error during sign up: {}", e.getMessage(), e);
Expand Down Expand Up @@ -240,7 +234,6 @@ public AuthResponse setUsername(String email, String username) {
.message("Username set successfully")
.build();
} catch (DataIntegrityViolationException e) {

log.error("Data integrity violation while setting username for email {}: {}", email, e.getMessage());

Copilot AI Apr 5, 2026

Copy link

Choose a reason for hiding this comment

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

Similar to signUp, this DataIntegrityViolationException log line only records e.getMessage() and omits the throwable, which can hide the stack trace and root cause. Consider including e in the log call so the full exception is captured.

Suggested change
log.error("Data integrity violation while setting username for email {}: {}", email, e.getMessage());
log.error("Data integrity violation while setting username for email {}: {}", email, e.getMessage(), e);

Copilot uses AI. Check for mistakes.
return AuthResponse.builder()
.message("Username already taken or another data issue occurred.")
Expand Down
11 changes: 4 additions & 7 deletions src/test/java/com/fredmaina/chatapp/Auth/Services/AuthServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.*;
import org.springframework.dao.DataIntegrityViolationException;
import org.springframework.security.crypto.password.PasswordEncoder;

import java.util.Optional;
Expand Down Expand Up @@ -68,14 +67,13 @@ void testSignUp_duplicateEmail() {
request.setPassword("mypassword");
request.setEmail("fred@example.com");

Copilot AI Apr 5, 2026

Copy link

Choose a reason for hiding this comment

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

testSignUp_duplicateEmail only stubs existsByEmail, implicitly relying on Mockito's default false return for the preceding existsByUsernameIgnoreCase call. To make the test intent clearer and avoid coupling to Mockito defaults, consider explicitly stubbing existsByUsernameIgnoreCase("fredmaina123") to false (or verifying it was called).

Suggested change
when(userRepository.existsByUsernameIgnoreCase("fredmaina123")).thenReturn(false);

Copilot uses AI. Check for mistakes.
when(passwordEncoder.encode(anyString())).thenReturn("encodedPassword");
when(userRepository.save(any(User.class))).thenThrow(
new DataIntegrityViolationException("users_email_key"));
when(userRepository.existsByEmail("fred@example.com")).thenReturn(true);

AuthResponse response = authService.signUp(request);

assertFalse(response.isSuccess());
assertEquals("Email already exists", response.getMessage());
verify(userRepository, never()).save(any(User.class));
}

@Test
Expand All @@ -85,14 +83,13 @@ void testSignUp_duplicateUsername() {
request.setPassword("mypassword");
request.setEmail("fred@example.com");

when(passwordEncoder.encode(anyString())).thenReturn("encodedPassword");
when(userRepository.save(any(User.class))).thenThrow(
new DataIntegrityViolationException("users_username_key"));
when(userRepository.existsByUsernameIgnoreCase("fredmaina123")).thenReturn(true);

AuthResponse response = authService.signUp(request);

assertFalse(response.isSuccess());
assertEquals("Username already exists", response.getMessage());
verify(userRepository, never()).save(any(User.class));
}

@Test
Expand Down