netneutrality/.junie/plans/db-package-best-practices.md

---
sessionId: session-260925-080906-1d5c
---

# Findings

### Correctness bugs (highest priority)
| # | Where | Problem |
|---|---|---|
| B1 | `BlacklistRepository.delete/add` | `getWordFromDb` loads the entity in its **own session** (which it also closes twice). The caller then calls `session.remove(bl)` in another session on that **detached** entity, which Hibernate rejects with `IllegalArgumentException`. The exception is swallowed by `BlacklistService`, which returns `warn`. |
| B2 | `GuildSettingRepository.setSettingValue` | Runs inside `JpaUtils.getWithSession`, which has **no transaction**, so `merge`/`remove` are never flushed or committed. Updating or resetting a setting is probably lost; only an IDENTITY `persist` happens to insert. |
| B3 | `Repository<O>` | IDs are always `String`, but `OBlacklist.id` / `OGuildSetting.id` are `Integer`, so `deleteBy`/`findById` bind a type-mismatched parameter. `BlacklistRepository.findById` returns `null` outright. |
| B4 | `Repository.save`, `JpaUtils.updateWithTransaction` | Every non-constraint exception is swallowed after `logger.error(e)`, so callers think the write succeeded. `runWithTransaction` never rolls back on failure. |
| B5 | `JpaUtils.tryLoad` | Uses the deprecated `Session.load(Object, id)`. It closes a session it doesn't own and calls back into `GuildService` (a hidden circular dependency). |
| B6 | `MessageRepository.getMessageString` | NPE when the message isn't found. |
| B7 | `SessionManager.getSessionFactory` | Lazy initialization isn't thread-safe. The Hibernate factory is built **before** Flyway migrates, and the schema is never validated. |

### Design / best-practice issues
- **Static service locator.** `JpaUtils.bind` / `bindGuildServiceLookup` (called from `App.java` l.119, l.138) hide dependencies and make repositories untestable.
- **Duplicated CRUD.** Five repositories copy-paste `findAll` / `findById` / `deleteBy`. They use the "lambda + external `ArrayList`" workaround because the helpers can't return values.
- **Duplicated JDBC config.** `SessionManager` and `DatabaseMigration` both build the URL, user and password from settings.
- **Layering.** Repositories take JDA types (`Member`, `Message`, `MessageChannel`), and `MessageRepository.log` carries a `//TODO: move to a service`.
- **Services create their own repositories** with `new`. `TaskService.persistTask`/`unpersistTask` even shadow the repository field it already holds.
- **Caches.** `GuildSettingService.reloadSettingsCache` clears the map and then refills it, so readers can see it empty. `getSettings()` and `GuildServiceProviderImpl.getServices()` leak mutable maps. Guild services are never evicted.
- **Logging mix.** The package uses both Log4j `LogManager` and SLF4J.
- **Entities.** No `equals`/`hashCode`. The field is named `OLoggings`. `OWebUser` is missing from `hibernate.cfg.xml`.
- **Hygiene.** Duplicated copyright headers and a broken `package ...;/*` line in `GuildRepository` and `GuildSettingRepository`. `GSettings` has a `//TODO: description`.
- **No tests** for the `db` package.

# Requirements

### Overview & Goals
Make the persistence layer (`me.bobert.netneutrality.db` and subpackages) correct, testable and idiomatic for Hibernate 7.1 / Java 25. Services become pure domain code with no JDA or command types. User-visible bot behavior stays the same except for the bug fixes (writes actually persist, errors surface).

> Chosen options: **Full redesign**, **injected generic base repository**, **Testcontainers MySQL** tests.

### Scope
**In scope**
- Fixes for bugs B1–B7 (see Findings).
- Replace the static `JpaUtils` with constructor-injected `SessionProvider` and a generic `AbstractRepository<T, ID>`.
- A single `DatabaseConfig` shared by Hibernate and Flyway. Migrate first, then build the factory with `hbm2ddl.auto=validate`.
- Service cleanup: inject repositories, safe cache swaps, no JDA types in repositories.
- **Decouple services from JDA and `CommandExecutionResult`.** Services take plain IDs and return domain results. The callers (`BlacklistCommand`, `ConfigureCommand`, `MusicCommand`, `MusicManager`, `EventsHandler`, `ActionRoutes`, `BotState`, `TaskManagerImpl`, `LoadPlaylistTask`) map those results to replies.
- **Rework the guild-service cache.** `GuildServiceProvider` keys by guild ID only, evicts on `GuildLeaveEvent`, and exposes read-only snapshots.
- Entity hygiene and consistent logging (Log4j API, matching `App` and `SessionManager`).
- Testcontainers-based integration tests.

**Out of scope**
- Switching to Spring or JPA `EntityManager`, or changing the Flyway schema (a migration is added only if validation shows an actual mismatch).
- Changing command UX: slash command names and options, and the reply wording (messages stay equivalent).
- Changes to `CommandExecutionResult` itself or to the command framework (`core.command`).

### Functional Requirements
- Adding or removing a blacklist word persists, and the cache reflects it right away.
- Setting a guild setting to a non-default value inserts or updates it. Setting it to the default or `null` deletes the row. All of this happens in one transaction.
- Persistence failures propagate as exceptions and are rolled back. They are never silently logged.
- When the bot starts: Flyway migrates first, then Hibernate validates the schema. If either fails, startup aborts with a clear error (Sentry is kept for non-dev environments). The password is never logged.
- A missing `OGuild` gets recreated through `GuildService.findOrCreateGuild` instead of the `tryLoad` side channel.
- No class in `db.service` or `db.repository` imports `net.dv8tion.jda.*` or `me.bobert.netneutrality.core.command.*`.
- The blacklist add/remove replies stay equivalent: success, "already exists" and "not found" become distinct outcomes, and unexpected errors are shown as a warning by the command.
- When the bot leaves a guild, its row is deleted and its cached `GuildService` is evicted.

# Technical Design

### Key Decisions
1. **Constructor injection instead of a static locator.** Repositories receive the `SessionProvider` (not the `SessionFactory`), because the factory is only built in `start()`. `JpaUtils` is deleted.
2. **Use Hibernate 7's native helpers** `SessionFactory.fromTransaction` / `inTransaction` / `fromSession`. They handle begin, commit, rollback and close correctly, so no hand-written transaction code is needed.
3. **Generic ID type.** `AbstractRepository<T, ID>` takes the entity class and uses `session.find`, `merge` and `remove` on a managed instance.
4. **Queries and writes run in the same session.** This fixes B1 and B2 by construction.
5. **Migrate, then validate.** Flyway runs before the factory is built, and `hibernate.hbm2ddl.auto=validate` catches drift between entities and schema.
6. **Services return domain outcomes; commands turn them into replies.** Services return small enums/records such as `BlacklistResult { ADDED, REENABLED, ALREADY_EXISTS, REMOVED, NOT_FOUND }` and never `CommandExecutionResult`. They take `String` IDs, not `Member`/`Guild`/`Message`. Unexpected persistence errors propagate, and the command layer catches them and shows a warning.
7. **ID-only provider API.** `GuildServiceProvider.get(Guild)` is removed (callers pass `guild.getId()`). The provider adds `evict(String)`, and `getServices()` becomes `snapshot()`, which returns `Map.copyOf`.

### Data Models / Contracts
```java
public record DatabaseConfig(String url, String user, String password) {
    static DatabaseConfig from(SettingsProvider s) { ... } // MYSQL_* keys, same defaults
}

public abstract class AbstractRepository<T, ID> {
    protected final SessionProvider sessions;
    private final Class<T> type;
    protected AbstractRepository(SessionProvider sessions, Class<T> type)
    public List<T> findAll()                 // fromSession + criteria/HQL
    public Optional<T> findById(ID id)       // session.find
    public T save(T entity)                  // fromTransaction(s -> s.merge(entity))
    public void deleteById(ID id)            // inTransaction: find + remove
    protected <R> R read(Function<Session, R> work)   // sf.fromSession
    protected <R> R write(Function<Session, R> work)  // sf.fromTransaction
    protected void writeVoid(Consumer<Session> work)  // sf.inTransaction
}
```
Repositories and their key queries:
- `GuildRepository extends AbstractRepository<OGuild, String>`.
- `BlacklistRepository<OBlacklist, Integer>`:
  - `findByGuildId`
  - `findEnabledWords(guildId)` returns `Set<String>`
  - `enableWord(OGuild, creatorId, word)`
  - `deleteWord(guildId, word)`
- `GuildSettingRepository<OGuildSetting, Integer>`:
  - `findByGuild`
  - `upsertOrDelete(guildId, name, value, default)`, which queries the setting directly instead of using the lazy collection.
- `TaskRepository<OTask, String>`.
- `MessageRepository<OLogMessage, String>`:
  - `findByMessageId`
  - `updateEditedMessage`
  - `findByUser(... ORDER BY dateSent DESC)`
  - `log(...)` takes plain values (IDs, content, `Instant`) instead of JDA types.

Service contracts after the redesign:
```java
public enum BlacklistResult { ADDED, REENABLED, ALREADY_EXISTS, REMOVED, NOT_FOUND }

public class BlacklistService {
    BlacklistResult add(String creatorUserId, String word);
    BlacklistResult remove(String word);
    List<BlacklistEntry> list();              // record BlacklistEntry(String word, boolean enabled)
    boolean containsBlacklistedWord(String text);
}

public class GuildSettingService {
    void set(GSettings key, @Nullable String value); // null/default => delete
    Optional<String> get(GSettings key);
    Map<String, String> snapshot();                  // unmodifiable
}

public interface GuildServiceProvider {
    GuildService get(String guildId);
    void evict(String guildId);
    Map<String, GuildService> snapshot();
}
```

### Proposed Changes
- **`SessionManager`**
  - Hold a `DatabaseConfig`.
  - `start()` runs `DatabaseMigration.migrate(config)` and then builds the factory eagerly (`synchronized` / `volatile`).
  - `getSessionFactory()` throws if called before `start()`.
  - Remove `JpaUtils.bind`.
  - Add `hibernate.hbm2ddl.auto=validate` to `hibernate.cfg.xml` and map `OWebUser` there if it has a table.
- **`DatabaseMigration`**: takes `DatabaseConfig`.
- **Services** get their dependencies through constructors:
  - `GuildService(String guildId, GuildRepository, BlacklistRepository, GuildSettingRepository)`.
  - `updateDebugMode` becomes `oGuild = guildRepository.save(modified)`.
  - `BlacklistService.reloadBlacklistCache` uses `findEnabledWords` (no `tryLoad`).
  - `GuildSettingService` builds a new map and swaps it into a `volatile` field. `getSettings()` returns an unmodifiable view.
  - `TaskService` receives `TaskRepository` and uses the field. It parses the payload once.
- **`GuildServiceProvider` / `GuildServiceProviderImpl`** follow the ID-only API: the provider is constructed with the repositories, uses `ConcurrentHashMap.computeIfAbsent`, and has `evict` and `snapshot`.
- **Callers updated to the new service API:**
  - `BlacklistCommand` maps `BlacklistResult` to success/warn replies, passes `context.getGuildUser().getId()`, and lists entries through `list()`.
  - `ConfigureCommand` uses `set(...)` / `snapshot()`.
  - `MusicCommand`, `MusicManager`, `MusicServiceImpl`, `ActionRoutes`, `BotState`, `LoadPlaylistTask` and `TaskManagerImpl` use `get(String)` and `snapshot()`.
  - `EventsHandler`: on `GuildLeaveEvent` it calls `deleteGuild()` and then `evict(id)`.
- **`App.java`**:
  - Build the repositories from the registered `SessionManager`.
  - Pass them to `GuildServiceProviderImpl` and `TaskService`.
  - Remove `JpaUtils.bindGuildServiceLookup`.
  - Update the callers of `MessageRepository.log`.
- **Entities**:
  - Add ID-based `equals`/`hashCode` (with a null-safe `getClass`/proxy-aware pattern).
  - Rename `OLoggings` to `loggings`.
- **Cleanup**: fix the headers, standardize on the Log4j API, and document `GSettings`.

### File Structure
- **New**:
  - `db/DatabaseConfig.java`
  - `db/service/BlacklistResult.java`, `db/service/BlacklistEntry.java`
  - `db/AbstractRepository.java` (replaces `db/Repository.java`)
  - `src/test/java/me/bobert/netneutrality/db/**` tests
- **Modified**:
  - `SessionManager`, `SessionProvider` (if needed), `DatabaseMigration`
  - all 5 repositories, all services, `GuildServiceProvider`
  - the callers: `BlacklistCommand`, `ConfigureCommand`, `MusicCommand`, `MusicManager`, `MusicServiceImpl`, `EventsHandler`, `ActionRoutes`, `BotState`, `TaskManagerImpl`, `LoadPlaylistTask`
  - the entities, `hibernate.cfg.xml`, `App.java`, `pom.xml` (test dependencies)
- **Deleted**: `db/JpaUtils.java`, `db/Repository.java`.

### Architecture Diagram
```mermaid
graph LR
App --> SessionManager
SessionManager -->|migrate first| DatabaseMigration
SessionManager -->|builds| SF[SessionFactory]
App --> Repos[AbstractRepository subclasses]
Repos -->|fromTransaction| SF
App --> GSP[GuildServiceProviderImpl]
GSP --> GuildService
GuildService --> BlacklistService
GuildService --> GuildSettingService
BlacklistService --> Repos
GuildSettingService --> Repos
App --> TaskService
TaskService --> Repos
```

### Risks
- **`validate` may fail on existing drift** between the entities and V1/V2. Mitigation: the integration test catches it; fix the mapping or add `V3__*.sql`.
- **Callers relied on swallowed exceptions.** Commands such as `BlacklistCommand` and `ConfigureCommand` catch them and show a warning reply. The listener and `LogHandler` paths are reviewed so errors stay logged and don't crash event handling.
- **Wide caller change set** from the API redesign. Mitigation: do it in its own stage, keep the method semantics identical, and rely on the compiler and `./mvnw test` to catch every call site.
- **Lazy associations accessed outside a session** (for example `OGuild.getBlacklists()` elsewhere). Mitigation: search for the getters and replace them with repository queries.

# Testing

### Validation Approach
- Run `./mvnw test` after each stage. Existing tests (for example `ApplicationContextTest`) must keep passing.
- Add the `org.testcontainers:mysql` and `junit-jupiter` test dependencies.
- Add a shared `MySqlTestBase` that:
  - starts `mysql:8`
  - builds a `DatabaseConfig` / `SettingsProvider` stub
  - starts a real `SessionManager` (Flyway plus validate)
  - uses `@Testcontainers(disabledWithoutDocker = true)`

### Key Scenarios
- `SessionManager` starts against the container: the migrations apply and schema validation passes.
- `BlacklistRepository`: adding a word and then deleting it removes the row (regression for B1). Re-adding a disabled word re-enables it.
- `GuildSettingRepository`: setting, updating and resetting to default each persist the expected state (regression for B2).
- `AbstractRepository`: `findById` / `deleteById` work with `Integer` and `String` IDs (B3). A failing write throws and leaves no partial data (B4).
- `GuildService.findOrCreateGuild` creates the guild once, and a second call returns the existing row.
- `GuildSettingService`: readers never observe an empty cache during a reload (unit test with Mockito).
- `BlacklistService` returns `ADDED`, then `ALREADY_EXISTS`, then `REMOVED`, then `NOT_FOUND` for the matching sequence of calls (container test).
- `BlacklistCommand` maps each `BlacklistResult` (and a thrown exception) to the expected reply type (Mockito unit test).
- `GuildServiceProviderImpl.evict` removes the cached service, and the next `get` builds a new one.
- Architecture check: a test scans `db.service` / `db.repository` classes to confirm there are no `net.dv8tion.jda` or `core.command` imports (a simple source-scan test).

### Edge Cases
- `MessageRepository.findByMessageId` for an unknown ID returns empty (no NPE).
- `SessionManager.getSessionFactory()` before `start()` throws `IllegalStateException`.
- `TaskService.persistTask` with an unregistered type is a no-op.

# Delivery Steps

### ✓ Step 1: Rework session bootstrap and add a generic base repository
SessionManager migrates first, then builds a validated SessionFactory, and repositories can extend a type-safe injected base class.

- Add `db/DatabaseConfig` record built from the `MYSQL_*` settings. Use it in both `SessionManager` and `DatabaseMigration`.
- `SessionManager.start()`:
  - runs Flyway first
  - then builds the factory eagerly and thread-safely
  - keeps the Sentry/dev logic
  - does not log the password
- `getSessionFactory()` throws before `start()`.
- Add `hibernate.hbm2ddl.auto=validate` to `hibernate.cfg.xml` and map `OWebUser` if it has a table.
- Add `AbstractRepository<T, ID>`, which takes a `SessionProvider` and implements:
  - `findAll`
  - `findById` returning `Optional`
  - `save` returning the merged entity
  - `deleteById`
  - `read`/`write` helpers built on `fromSession`/`fromTransaction`/`inTransaction`
- Add the Testcontainers test dependencies and a `MySqlTestBase`, plus a startup and schema validation test.

### ✓ Step 2: Migrate repositories onto the base class and fix persistence bugs
All five repositories use injected sessions with correct ID types, and the blacklist and setting write bugs are fixed and covered by tests.

- Convert `GuildRepository`, `BlacklistRepository`, `GuildSettingRepository`, `TaskRepository` and `MessageRepository` to `AbstractRepository` with proper ID types.
- Blacklist:
  - `enableWord` / `deleteWord` query and modify inside the same transaction (fixes the detached remove).
  - Add `findEnabledWords(guildId)`.
- Guild settings: `upsertOrDelete` runs in a transaction and queries the setting directly.
- Messages:
  - JDA-free `log(...)`
  - `updateEditedMessage`
  - `Optional` lookups
  - ordered `findByUser`
- Remove the swallowed-exception patterns and fix the duplicated headers and package lines.
- Add repository integration tests for the blacklist, settings, ID types and rollback scenarios.

### ✓ Step 3: Refactor services and wiring, and remove JpaUtils
Services receive their repositories through constructors, caches are thread-safe, and the static `JpaUtils` and `Repository` classes are gone.

- `GuildService`:
  - takes its repositories through the constructor
  - `updateDebugMode` uses `save`
  - `findOrCreateGuild` handles missing guilds, replacing `tryLoad`
- `BlacklistService` reloads its cache through `findEnabledWords`.
- `GuildSettingService` swaps in a new cache map and exposes it unmodifiable.
- `TaskService`:
  - uses the injected `TaskRepository` field
  - parses the payload once
- `GuildServiceProviderImpl` is constructed with the repositories and uses `computeIfAbsent`. The API changes come in Step 4.
- Update `App.java`:
  - build the repositories from `SessionManager`
  - remove `JpaUtils.bindGuildServiceLookup`
  - update the `MessageRepository.log` callers
- Delete `JpaUtils.java` and `Repository.java`. Replace any lazy-collection access outside a session with repository queries.
- Add service tests: Mockito for the caches, and the container test for `findOrCreateGuild`.

### ✓ Step 4: Decouple services from JDA and command types, and update callers
Services expose JDA-free, ID-based domain APIs, and every caller maps domain results to replies.

- Add `BlacklistResult` and `BlacklistEntry`. `BlacklistService.add/remove/list` take plain IDs and return domain results instead of `CommandExecutionResult`.
- `GuildSettingService` exposes `set` / `get` / `snapshot`.
- `GuildServiceProvider` becomes ID-only: remove `get(Guild)`, add `evict`, and replace `getServices` with `snapshot`.
- Update the callers:
  - `BlacklistCommand` maps results and exceptions to replies
  - `ConfigureCommand`
  - `MusicCommand`, `MusicManager`, `MusicServiceImpl`
  - `ActionRoutes`, `BotState`
  - `TaskManagerImpl`, `LoadPlaylistTask`
  - `EventsHandler` evicts on guild leave
- Add tests: the `BlacklistService` result sequence (container), `BlacklistCommand` reply mapping (Mockito), provider eviction, and the no-JDA-imports source scan.

### ✓ Step 5: Entity and package hygiene
Entities follow JPA best practices and the package uses consistent conventions, with the full test suite green.

- Add ID-based, proxy-safe `equals`/`hashCode` to the entities.
- Rename `OGuild.OLoggings` to `loggings`.
- Standardize logging on the Log4j API across `db.service`.
- Document the `GSettings` entries by resolving the TODO.
- Re-run schema validation. If entity/schema drift shows up, add a `V3__*.sql` migration or fix the mapping.
- Run `./mvnw test` to confirm the existing and new tests pass.