5.0.1: revert Role.privileges to EAGER (fix LazyInitializationException footgun)

[devondragon]

Summary

• Reverts Role.privileges to FetchType.EAGER (it was changed to LAZY in 5.0.0).
• User.roles stays LAZY + the @EntityGraph auth query (UserRepository.findWithRolesByEmail) — that is where the meaningful per-user N+1 win is.
• Updates CHANGELOG.md ([5.0.1]) and MIGRATION.md (lazy-fetch section).

Why

The 5.0.0 LAZY change made role.getPrivileges() throw LazyInitializationException when accessed outside an open transaction/session — the single most natural navigation on a Role — for a negligible gain (privileges are small, static reference data with no bulk-load path). Reverting removes the footgun while keeping the real performance improvement. This is a non-breaking relaxation: code written against 5.0.0 continues to work unchanged.

Test Plan

• Library ./gradlew clean test green (incl. UserRepositoryEntityGraphTest — User.roles still asserts LAZY; query-count bound unaffected).
• Demo app full ./gradlew test green 3/3 against 5.0.1-SNAPSHOT; AdminRoleAccessControlTest (out-of-transaction getPrivileges()) passes.

[claude]

Code Review

Overall: This is a clean, well-reasoned patch. The revert decision is correct — Role.privileges is genuinely static reference data, so the LazyInitializationException footgun wasn't worth the marginal benefit. The docs are excellent. A few items worth flagging:

---

Comment length in Role.java (style violation)

The added comment block in Role.java is 5 lines:

// EAGER by design: privileges are small, static reference data and there is no hot path that loads many Roles at
// once, so eager-loading them is cheap. Keeping this EAGER lets consumers call role.getPrivileges() outside a
// transaction without a LazyInitializationException. The user-load N+1 is addressed by User.roles being LAZY (see
// User), and the authentication path still fetches the full graph in one query via
// UserRepository.findWithRolesByEmail. (Role.privileges was briefly LAZY in 5.0.0; reverted in 5.0.1.)

CLAUDE.md is explicit: "one short line max" and "Don't write multi-paragraph docstrings or multi-line comment blocks." The reasoning itself is worth capturing (it's a non-obvious design decision), but it should be condensed to a single line, e.g.:

// EAGER: privileges are static reference data; no bulk-load path exists, and consumers need getPrivileges() outside transactions.

The historical note about 5.0.0/5.0.1 is already in MIGRATION.md and the git log — no need to repeat it inline.

---

Stale test class Javadoc

UserRepositoryEntityGraphTest.java line 24 still reads:

"This is the regression guard for the EAGER → LAZY switch on User.roles and Role.privileges."

Since Role.privileges is back to EAGER, this is now inaccurate. Should be updated to "...on User.roles" only.

Similarly, the shouldNotInitializeRolesWhenLoadedViaPlainFindByEmail test comment says "proving the EAGER → LAZY switch took effect" — still correct for User.roles, but ambiguous now. Worth tightening to name the collection explicitly.

---

Missing regression test for the bug being fixed

The existing tests (shouldAccessRolesAndPrivilegesWithoutLazyInitializationExceptionAfterDetach, etc.) all go through findWithRolesByEmail (the entity graph path). None of them verify the specific scenario this PR fixes: role.getPrivileges() working when the role itself was loaded by any plain repository call (not via the entity graph).

A minimal regression test would be:

@Test
void shouldLoadPrivilegesEagerlyOnRoleOutsideTransaction() {
    persistUserWithRolesAndPrivileges("eager-priv@test.com");

    // Load role directly (not via the entity graph user finder)
    User user = userRepository.findByEmail("eager-priv@test.com");
    // Detach before accessing roles (open transaction makes this available)
    Role role = userRepository.findById(/* role id */)...;
    entityManager.getEntityManager().detach(role);

    // Must not throw LazyInitializationException — this was the 5.0.0 bug
    assertThatCode(() -> role.getPrivileges().size()).doesNotThrowAnyException();
    assertThat(Hibernate.isInitialized(role.getPrivileges())).isTrue();
}

Without a dedicated test, the regression can silently re-appear if FetchType.EAGER is changed back again. The demo app's AdminRoleAccessControlTest covers this end-to-end, but the library's own test suite should own the regression.

---

Minor: roles.privileges in @EntityGraph is now redundant

With Role.privileges back to EAGER, the attributePaths = {"roles", "roles.privileges"} in findWithRolesByEmail loads privileges twice (once from EAGER, once from the entity graph hint). Hibernate handles this gracefully (it deduplicates), but the roles.privileges path is now misleading — it implies privileges require explicit fetching. Keeping it is harmless and preserves explicitness, but dropping it to {"roles"} would be more accurate.

---

What's working well

• The fix itself is correct. Reverting to EAGER for Role.privileges while keeping LAZY for User.roles is the right tradeoff split.
• MIGRATION.md is excellent — the 5.0.0 → 5.0.1 callout note is exactly what consumers upgrading in two steps need.
• CHANGELOG.md entry is clear and self-contained.
• The JavaDoc in UserRepository is accurate after the update.
• Test plan in the PR description is thorough and transparent about what was and wasn't tested in the library vs. demo app.

[Copilot]

Pull request overview

This PR reverts the Role.privileges association back to FetchType.EAGER to eliminate a common LazyInitializationException hazard introduced by the 5.0.0 switch to LAZY, while keeping the main performance win of User.roles remaining LAZY and being explicitly fetched on the authentication path via an @EntityGraph.

Changes:

• Reverted Role.privileges from LAZY back to EAGER and documented the design rationale in Role.
• Updated UserRepository Javadoc to reflect the intended usage with User.roles still being LAZY.
• Updated MIGRATION.md and CHANGELOG.md to document the 5.0.1 behavior and guidance.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java	Updates Javadoc guidance around lazy roles vs eager privileges and the entity-graph finder.
src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java	Reverts privileges fetch to EAGER and adds rationale explaining the tradeoff.
MIGRATION.md	Updates upgrade guidance to reflect User.roles lazy behavior and the 5.0.0 → 5.0.1 reversal for privileges.
CHANGELOG.md	Adds 5.0.1 entry documenting the revert and migration reference.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.