Skip to content

Commit 89c06ef

Browse files
devondragonclaude
devondragon
and
claude
committed
Fix Hibernate entity management issue with User collections
- Changed internal User.roles storage from List to Set to prevent Hibernate immutable proxy issues - Maintained backward compatibility by keeping existing getRoles()/setRoles() methods - Added new getRolesAsSet()/setRolesAsSet() methods for direct Set access - All collection methods now use defensive copying to ensure mutability - Fixed DSOidcUserService to properly assign roles to OAuth2 users - Added comprehensive integration tests to verify User entity updates work correctly This fixes the UnsupportedOperationException that was occurring when trying to save User entities after modification, particularly in LoginAttemptService and OAuth2 integration scenarios. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
1 parent b52eff6 commit 89c06ef

File tree

3 files changed

+306
-3
lines changed

3 files changed

+306
-3
lines changed

‎src/main/java/com/digitalsanctuary/spring/user/persistence/model/User.java‎

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,17 @@
11
package com.digitalsanctuary.spring.user.persistence.model;
22

3+
import java.util.ArrayList;
34
import java.util.Date;
5+
import java.util.HashSet;
46
import java.util.List;
7+
import java.util.Set;
58
import org.springframework.data.annotation.CreatedDate;
69
import org.springframework.data.annotation.LastModifiedDate;
710
import org.springframework.data.jpa.domain.support.AuditingEntityListener;
811
import jakarta.persistence.*;
912
import lombok.Data;
13+
import lombok.EqualsAndHashCode;
14+
import lombok.ToString;
1015

1116
/**
1217
* The User Entity. Part of the basic User ->> Role ->> Privilege structure. This is the primary user data object. You can add to this, or add
@@ -92,11 +97,13 @@ public enum Provider {
9297
@Temporal(TemporalType.TIMESTAMP)
9398
private Date lockedDate;
9499

95-
/** The roles. */
100+
/** The roles - stored as Set to avoid Hibernate immutable collection issues */
101+
@ToString.Exclude
102+
@EqualsAndHashCode.Exclude
96103
@ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE}, fetch = FetchType.EAGER)
97104
@JoinTable(name = "users_roles", joinColumns = @JoinColumn(name = "user_id", referencedColumnName = "id"),
98105
inverseJoinColumns = @JoinColumn(name = "role_id", referencedColumnName = "id"))
99-
private List<Role> roles;
106+
private Set<Role> roles = new HashSet<>();
100107

101108
/**
102109
* Instantiates a new user.
@@ -122,4 +129,43 @@ public void setLastActivityDate() {
122129
public String getFullName() {
123130
return firstName + " " + lastName;
124131
}
132+
133+
/**
134+
* Gets the roles as a List for backward compatibility.
135+
*
136+
* @return the roles as a List
137+
*/
138+
public List<Role> getRoles() {
139+
return new ArrayList<>(roles);
140+
}
141+
142+
/**
143+
* Sets the roles from a List for backward compatibility.
144+
* Creates a new HashSet to ensure the collection is mutable.
145+
*
146+
* @param rolesList the roles to set
147+
*/
148+
public void setRoles(List<Role> rolesList) {
149+
this.roles = new HashSet<>(rolesList != null ? rolesList : new ArrayList<>());
150+
}
151+
152+
/**
153+
* Gets the roles as a Set (preferred method).
154+
* Returns a defensive copy to prevent external modification.
155+
*
156+
* @return the roles as a Set
157+
*/
158+
public Set<Role> getRolesAsSet() {
159+
return new HashSet<>(roles);
160+
}
161+
162+
/**
163+
* Sets the roles from a Set (preferred method).
164+
* Creates a new HashSet to ensure the collection is mutable.
165+
*
166+
* @param rolesSet the roles to set
167+
*/
168+
public void setRolesAsSet(Set<Role> rolesSet) {
169+
this.roles = new HashSet<>(rolesSet != null ? rolesSet : new HashSet<>());
170+
}
125171
}

‎src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package com.digitalsanctuary.spring.user.service;
22

3+
import java.util.Arrays;
34
import com.digitalsanctuary.spring.user.persistence.model.User;
5+
import com.digitalsanctuary.spring.user.persistence.repository.RoleRepository;
46
import com.digitalsanctuary.spring.user.persistence.repository.UserRepository;
57
import lombok.RequiredArgsConstructor;
68
import lombok.extern.slf4j.Slf4j;
@@ -41,6 +43,9 @@ public class DSOidcUserService implements OAuth2UserService<OidcUserRequest, Oid
4143

4244
/** The user repository. */
4345
private final UserRepository userRepository;
46+
47+
/** The role repository. */
48+
private final RoleRepository roleRepository;
4449

4550
OidcUserService defaultOidcUserService = new OidcUserService();
4651

@@ -100,7 +105,7 @@ public User handleOidcLoginSuccess(String registrationId, OidcUser oidcUser) {
100105
private User registerNewOidcUser(String registrationId, User user) {
101106
User.Provider provider = User.Provider.valueOf(registrationId.toUpperCase());
102107
user.setProvider(provider);
103-
// user.setRoles(Collections.singletonList(roleRepository.findByName(RoleName.ROLE_USER)));
108+
user.setRoles(Arrays.asList(roleRepository.findByName("ROLE_USER")));
104109
// We will trust OAuth2 providers to provide us with a verified email address.
105110
user.setEnabled(true);
106111
return userRepository.save(user);

‎src/test/java/com/digitalsanctuary/spring/user/service/UserUpdateIntegrationTest.java‎

Lines changed: 252 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,252 @@
1+
package com.digitalsanctuary.spring.user.service;
2+
3+
import static org.assertj.core.api.Assertions.assertThat;
4+
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
5+
6+
import java.util.Arrays;
7+
import java.util.HashSet;
8+
9+
import org.junit.jupiter.api.BeforeEach;
10+
import org.junit.jupiter.api.Test;
11+
import org.springframework.beans.factory.annotation.Autowired;
12+
import org.springframework.boot.test.context.SpringBootTest;
13+
import org.springframework.test.context.TestPropertySource;
14+
import org.springframework.transaction.annotation.Transactional;
15+
16+
import com.digitalsanctuary.spring.user.persistence.model.Role;
17+
import com.digitalsanctuary.spring.user.persistence.model.User;
18+
import com.digitalsanctuary.spring.user.persistence.repository.RoleRepository;
19+
import com.digitalsanctuary.spring.user.persistence.repository.UserRepository;
20+
21+
import jakarta.persistence.EntityManager;
22+
import jakarta.persistence.PersistenceContext;
23+
24+
/**
25+
* Integration test to verify that User entity updates work correctly with Hibernate.
26+
* This test specifically validates the fix for the UnsupportedOperationException
27+
* that was occurring when trying to update User entities with collection fields.
28+
*/
29+
@SpringBootTest
30+
@TestPropertySource(properties = {
31+
"user.registration.sendVerificationEmail=false",
32+
"user.security.failedLoginAttempts=3",
33+
"user.security.accountLockoutDuration=1"
34+
})
35+
class UserUpdateIntegrationTest {
36+
37+
@Autowired
38+
private UserRepository userRepository;
39+
40+
@Autowired
41+
private RoleRepository roleRepository;
42+
43+
@Autowired
44+
private LoginAttemptService loginAttemptService;
45+
46+
@PersistenceContext
47+
private EntityManager entityManager;
48+
49+
private Role userRole;
50+
private Role adminRole;
51+
52+
@BeforeEach
53+
void setUp() {
54+
// Clean up
55+
userRepository.deleteAll();
56+
roleRepository.deleteAll();
57+
58+
// Create roles
59+
userRole = new Role("ROLE_USER", "Basic user role");
60+
userRole = roleRepository.save(userRole);
61+
62+
adminRole = new Role("ROLE_ADMIN", "Administrator role");
63+
adminRole = roleRepository.save(adminRole);
64+
}
65+
66+
@Test
67+
@Transactional
68+
void testUserUpdateWithRolesCollection_NoUnsupportedOperationException() {
69+
// Create user with roles using List (backward compatible method)
70+
User user = new User();
71+
user.setEmail("[email protected]");
72+
user.setFirstName("Test");
73+
user.setLastName("User");
74+
user.setPassword("password");
75+
user.setEnabled(true);
76+
user.setRoles(Arrays.asList(userRole));
77+
78+
user = userRepository.save(user);
79+
assertThat(user.getId()).isNotNull();
80+
assertThat(user.getRoles()).hasSize(1);
81+
82+
// Clear the session to simulate real-world scenario
83+
entityManager.flush();
84+
entityManager.clear();
85+
86+
// Load user again (simulating a new transaction)
87+
User loadedUser = userRepository.findByEmail("[email protected]");
88+
assertThat(loadedUser).isNotNull();
89+
90+
// This should NOT throw UnsupportedOperationException
91+
assertDoesNotThrow(() -> {
92+
loadedUser.setFailedLoginAttempts(3);
93+
loadedUser.setLocked(true);
94+
userRepository.save(loadedUser);
95+
});
96+
97+
// Verify the update worked
98+
entityManager.flush();
99+
entityManager.clear();
100+
101+
User verifiedUser = userRepository.findByEmail("[email protected]");
102+
assertThat(verifiedUser.getFailedLoginAttempts()).isEqualTo(3);
103+
assertThat(verifiedUser.isLocked()).isTrue();
104+
}
105+
106+
@Test
107+
@Transactional
108+
void testLoginAttemptService_CanUpdateUser() {
109+
// Create user with roles
110+
User user = new User();
111+
user.setEmail("[email protected]");
112+
user.setFirstName("Login");
113+
user.setLastName("Test");
114+
user.setPassword("password");
115+
user.setEnabled(true);
116+
user.setRoles(Arrays.asList(userRole));
117+
118+
userRepository.save(user);
119+
120+
// Clear session
121+
entityManager.flush();
122+
entityManager.clear();
123+
124+
// Test login failed - this should work without UnsupportedOperationException
125+
assertDoesNotThrow(() -> {
126+
loginAttemptService.loginFailed("[email protected]");
127+
});
128+
129+
User updatedUser = userRepository.findByEmail("[email protected]");
130+
assertThat(updatedUser.getFailedLoginAttempts()).isEqualTo(1);
131+
132+
// Test multiple failures leading to account lock
133+
assertDoesNotThrow(() -> {
134+
loginAttemptService.loginFailed("[email protected]");
135+
loginAttemptService.loginFailed("[email protected]");
136+
});
137+
138+
updatedUser = userRepository.findByEmail("[email protected]");
139+
assertThat(updatedUser.getFailedLoginAttempts()).isEqualTo(3);
140+
assertThat(updatedUser.isLocked()).isTrue();
141+
142+
// Test login success - should reset attempts
143+
assertDoesNotThrow(() -> {
144+
loginAttemptService.loginSucceeded("[email protected]");
145+
});
146+
147+
updatedUser = userRepository.findByEmail("[email protected]");
148+
assertThat(updatedUser.getFailedLoginAttempts()).isEqualTo(0);
149+
assertThat(updatedUser.isLocked()).isFalse();
150+
}
151+
152+
@Test
153+
@Transactional
154+
void testUserRolesUpdate_UsingNewSetMethods() {
155+
// Create user with roles using the new Set methods
156+
User user = new User();
157+
user.setEmail("[email protected]");
158+
user.setFirstName("Set");
159+
user.setLastName("Test");
160+
user.setPassword("password");
161+
user.setEnabled(true);
162+
user.setRolesAsSet(new HashSet<>(Arrays.asList(userRole)));
163+
164+
user = userRepository.save(user);
165+
166+
// Clear session
167+
entityManager.flush();
168+
entityManager.clear();
169+
170+
// Load and update roles
171+
User loadedUser = userRepository.findByEmail("[email protected]");
172+
173+
// Add admin role using Set method
174+
assertDoesNotThrow(() -> {
175+
var currentRoles = loadedUser.getRolesAsSet();
176+
currentRoles.add(adminRole);
177+
loadedUser.setRolesAsSet(currentRoles);
178+
userRepository.save(loadedUser);
179+
});
180+
181+
// Verify
182+
entityManager.flush();
183+
entityManager.clear();
184+
185+
User verifiedUser = userRepository.findByEmail("[email protected]");
186+
assertThat(verifiedUser.getRoles()).hasSize(2);
187+
assertThat(verifiedUser.getRoles()).extracting(Role::getName)
188+
.containsExactlyInAnyOrder("ROLE_USER", "ROLE_ADMIN");
189+
}
190+
191+
@Test
192+
@Transactional
193+
void testBackwardCompatibility_ListMethodsStillWork() {
194+
// Create user using old List-based API
195+
User user = new User();
196+
user.setEmail("[email protected]");
197+
user.setFirstName("Compat");
198+
user.setLastName("Test");
199+
user.setPassword("password");
200+
user.setEnabled(true);
201+
202+
// Use List-based setter (backward compatible)
203+
user.setRoles(Arrays.asList(userRole, adminRole));
204+
userRepository.save(user);
205+
206+
// Clear session
207+
entityManager.flush();
208+
entityManager.clear();
209+
210+
// Load and verify List-based getter works
211+
User loadedUser = userRepository.findByEmail("[email protected]");
212+
var rolesList = loadedUser.getRoles(); // Returns List<Role>
213+
214+
assertThat(rolesList).isInstanceOf(java.util.List.class);
215+
assertThat(rolesList).hasSize(2);
216+
assertThat(rolesList).extracting(Role::getName)
217+
.containsExactlyInAnyOrder("ROLE_USER", "ROLE_ADMIN");
218+
}
219+
220+
@Test
221+
@Transactional
222+
void testMultipleUpdatesInSameTransaction() {
223+
// Create user
224+
User user = new User();
225+
user.setEmail("[email protected]");
226+
user.setFirstName("Multi");
227+
user.setLastName("Update");
228+
user.setPassword("password");
229+
user.setEnabled(true);
230+
user.setRoles(Arrays.asList(userRole));
231+
232+
user = userRepository.save(user);
233+
final Long userId = user.getId();
234+
235+
// Multiple updates in same transaction should work
236+
assertDoesNotThrow(() -> {
237+
User userToUpdate = userRepository.findById(userId).orElseThrow();
238+
userToUpdate.setFailedLoginAttempts(1);
239+
userRepository.save(userToUpdate);
240+
241+
userToUpdate.setFailedLoginAttempts(2);
242+
userRepository.save(userToUpdate);
243+
244+
userToUpdate.setLocked(true);
245+
userRepository.save(userToUpdate);
246+
});
247+
248+
User finalUser = userRepository.findById(userId).orElseThrow();
249+
assertThat(finalUser.getFailedLoginAttempts()).isEqualTo(2);
250+
assertThat(finalUser.isLocked()).isTrue();
251+
}
252+
}

0 commit comments

Comments
 (0)