diff --git a/.gitignore b/.gitignore index bed7c67..d457e02 100644 --- a/.gitignore +++ b/.gitignore @@ -33,6 +33,7 @@ hs_err_pid* .idea/ *.iml *.iws +.junie/plans # Mac .DS_Store diff --git a/.junie/plans/consolidate-json-on-jackson.md b/.junie/plans/consolidate-json-on-jackson.md deleted file mode 100644 index 23dee58..0000000 --- a/.junie/plans/consolidate-json-on-jackson.md +++ /dev/null @@ -1,259 +0,0 @@ ---- -sessionId: session-260925-072402-zipx ---- - -# Requirements - -### Overview & Goals -The project currently depends on **two** JSON libraries declared in `pom.xml`: - -- `org.json:json:20250517` (line 220) — used widely -- `com.google.code.gson:gson:2.14.0` (line 225) — used in exactly two places - -Goal: standardize on **one** JSON library across the whole codebase and remove the others from `pom.xml`. - -**Recommendation: Jackson** (`com.fasterxml.jackson`). See the *Library Comparison* tab for the rationale — the decisive point is that Jackson **is already on the compile classpath** (2.21.2, transitively via JDA 6.4.1 and Javalin 7.2.3), so consolidating on it removes *both* declared dependencies instead of one. - -### Scope - -**In scope** -- Add an explicit, version-managed `jackson-databind` dependency. -- Migrate all `org.json` usages (`JSONObject` / `JSONArray`) to Jackson `JsonNode` / `ObjectNode` / `ArrayNode`. -- Migrate the two Gson usages to Jackson. -- Remove `org.json:json` and `com.google.code.gson:gson` from `pom.xml`. -- Update the two tests that build `JSONObject` payloads. - -**Out of scope** -- Changing the *persisted JSON format* stored in the database or `state.json` — the on-the-wire/on-disk shape must stay byte-compatible in meaning so existing persisted tasks keep loading. -- Refactoring unrelated logic in the touched files (e.g. the dead loop in `BotState.saveGuildsState`). -- Replacing JDA's or Sentry's internal JSON handling. - -### Functional Requirements -- The bot compiles and all existing tests pass with only one JSON library declared. -- Tasks persisted before the change still restore correctly (same field names: `id`, `lockId`, `guildId`, `channelId`, `messageId`, `emojiMapping`). -- The Javalin HTTP server keeps serializing/deserializing request and response bodies identically. -- `Settings.getGitVersion()` and `CryptoPriceCommand` keep reading the same fields from the same remote APIs. - -### Non-Functional Requirements -- No new transitive dependency conflicts in the shaded jar. -- Jackson version stays aligned with the one JDA brings in, to avoid split-version issues. - -# Library Comparison - -### The three candidates - -| | `org.json` | Gson | **Jackson** | -|---|---|---|---| -| Already on classpath? | declared | declared | **yes, transitively (JDA + Javalin)** | -| Deps removable if chosen | 1 (gson) | 1 (org.json) | **2 (both)** | -| Javalin 7 integration | none (manual) | custom `GsonJsonMapper` needed | **built-in `JavalinJackson`** | -| Tree/DOM API (needed by this codebase) | `JSONObject` | weak (`JsonObject`, clunky) | `ObjectNode` / `JsonNode` | -| POJO binding | none | good | excellent | -| Current usage count | ~9 files | 2 call sites | 0 | - -### Why Jackson wins here -1. **It is already a compile dependency.** Verified via `mvn dependency:tree`: - - `net.dv8tion:JDA:6.4.1` → `jackson-core:2.21.2`, `jackson-databind:2.21.2` - - `io.javalin:javalin:7.2.3` → `jersey-media-json-jackson` → jackson modules - - So Jackson ships in the jar today whether we use it or not. Choosing `org.json` or Gson means keeping a *redundant* second parser in the artifact forever. -2. **Javalin has first-class Jackson support.** `GsonJsonMapper.java` exists purely to bridge Gson into Javalin; with Jackson that whole class is deleted and Javalin's default mapper takes over. -3. **Jackson covers both usage styles present in the codebase**: the tree-style manipulation `org.json` is used for (`ObjectNode`/`ArrayNode`) *and* the POJO serialization Gson is used for (`writeValueAsString`). - -### Why not the other two -- **Gson**: the smaller migration on paper is actually the *bigger* one — all ~9 `org.json` files would move to Gson's awkward tree API, and Jackson would still be shipped unused. -- **`org.json`**: only 2 call sites to change (trivial), but it is the weakest library of the three (no POJO binding, `new JSONObject(bean)` reflection is fragile), it has no Javalin integration, and Jackson still ships unused. Choose this only if minimal diff outweighs everything else. - -### Cost -Jackson is the largest diff (~11 files) but the only option that actually reduces what is bundled. - -# Technical Design - -### Current Implementation - -**Gson (2 sites — trivial):** -- `http/server/GsonJsonMapper.java` — implements Javalin's `JsonMapper` over a static `Gson`. -- `http/WebModule.java:256` — `RequestBody.create(new Gson().toJson(params), ...)` for POST bodies. - -**`org.json` (9 files):** -- `core/tasks/PersistableTask.java` — interface declares `void restore(JSONObject payload)`. -- `core/tasks/impl/AbstractPersistableTask.java:69-80` — `restore(JSONObject)` no-op and `toPayload()` using **`new JSONObject(this)` bean introspection** plus `id` / `lockId`. -- `core/tasks/impl/EmojiTask.java:39-51` — `restore` reads `guildId`, `channelId`, `messageId`, and the `emojiMapping` object. -- `core/tasks/persistence/PersistableTaskRegistry.java`, `db/service/TaskService.java` — parse/handle payload JSON. -- `http/WebModule.java:133-143` — `getAsObject()` / `getAsArray()` returning `JSONObject` / `JSONArray`. -- `utils/Settings.java:104-109` — `getAsArray()`, `isNull(0)`, `getJSONObject(0).getString("tag_name")`. -- `command/crypto/CryptoPriceCommand.java` — reads the crypto API response. -- `bootstrap/BotState.java:46-81` — builds a `state.json` dump. -- Tests: `PersistableTaskRegistryTest`, `EmojiReactionRoleTaskPersistenceTest`. - -`tasks/reaction/EmojiReactionRoleTask.java` itself has **no** JSON imports — it is affected only indirectly through `EmojiTask`. - -### Key Decisions -1. **Jackson as the single library**, pinned to the version JDA supplies (2.21.2) via an explicit `` on `jackson-databind` so the build does not silently depend on a transitive. -2. **One shared `ObjectMapper`** in a small `utils/Json.java` holder — `ObjectMapper` is thread-safe and expensive to create; today the code does `new Gson()` per request. -3. **Replace `new JSONObject(this)` bean introspection with explicit payload building.** Jackson's `valueToTree(this)` would walk every getter on the task hierarchy (including ones returning JDA objects) and can blow up. Instead `toPayload()` builds an `ObjectNode` explicitly and tasks with extra state override a `writePayload(ObjectNode)` hook — mirroring the existing `restore` hook. This keeps the emitted field set identical to today's (`id`, `lockId`, `guildId`, `channelId`, `messageId`, `emojiMapping`, `name`) but makes it deterministic. -4. **Delete `GsonJsonMapper`** and rely on Javalin's built-in Jackson mapper rather than porting the class to Jackson. -5. **Keep tree-style code tree-style.** No POJO/DTO redesign in this task — `JSONObject` → `ObjectNode`, `JSONArray` → `ArrayNode`, `optString(k, null)` → `node.path(k).asText(null)`. - -### Data Models / Contracts - -```java -// utils/Json.java (new) -public final class Json { - private static final ObjectMapper MAPPER = new ObjectMapper(); - public static ObjectMapper mapper(); - public static ObjectNode object(); // MAPPER.createObjectNode() - public static ArrayNode array(); - public static ObjectNode readObject(String json); // wraps JsonProcessingException - public static ArrayNode readArray(String json); - public static String write(Object value); -} -``` - -Interface change in `PersistableTask`: -```java -- void restore(JSONObject payload); -+ void restore(ObjectNode payload); -``` - -New hook in `AbstractPersistableTask`: -```java -protected void writePayload(ObjectNode payload) { /* no-op */ } - -@Override -public String toPayload() { - ObjectNode payload = Json.object(); - payload.put("id", getId()); - payload.put("lockId", getLockId()); - payload.put("name", getName()); - writePayload(payload); - return payload.toString(); -} -``` - -`EmojiTask` gains the symmetric writer: -```java -@Override protected void writePayload(ObjectNode payload) { - payload.put("guildId", guildId); - payload.put("channelId", channelId); - payload.put("messageId", messageId); - ObjectNode mapping = payload.putObject("emojiMapping"); - emojiMapping.forEach(mapping::put); -} -``` - -`WebModule` response accessors: -```java -public ObjectNode getAsObject(); // Json.readObject(body) -public ArrayNode getAsArray(); // Json.readArray(body) -``` -Both wrap Jackson's checked `JsonProcessingException` into an unchecked exception so the existing call sites' signatures are unchanged. - -### File Structure - -**Added** -- `src/main/java/me/bobert/netneutrality/utils/Json.java` - -**Removed** -- `src/main/java/me/bobert/netneutrality/http/server/GsonJsonMapper.java` - -**Modified** -- `pom.xml` — drop `org.json:json` + `com.google.code.gson:gson`, add `jackson-databind` -- `core/tasks/PersistableTask.java` -- `core/tasks/impl/AbstractPersistableTask.java` -- `core/tasks/impl/EmojiTask.java` -- `core/tasks/persistence/PersistableTaskRegistry.java` -- `db/service/TaskService.java` -- `http/WebModule.java` -- wherever `GsonJsonMapper` was registered on the Javalin config (`http/server/*`) -- `utils/Settings.java` -- `command/crypto/CryptoPriceCommand.java` -- `bootstrap/BotState.java` -- `src/test/java/.../PersistableTaskRegistryTest.java` -- `src/test/java/.../EmojiReactionRoleTaskPersistenceTest.java` - -### Architecture Diagram - -```mermaid -graph LR - A[Task persistence] --> J[Json utils - ObjectMapper] - B[WebModule HTTP client] --> J - C[Settings / CryptoPriceCommand] --> B - D[BotState dump] --> J - E[Javalin server] --> F[JavalinJackson built-in] - J --> G[jackson-databind] - F --> G -``` - -### Risks -- **Behavioural drift in `toPayload()`**: `new JSONObject(this)` reflects over *all* getters. If any persisted consumer relies on an incidental field, the explicit writer could drop it. Mitigation: inspect the DB payload column / existing tests and keep the field set identical. -- **Null handling differs**: `JSONObject.put(k, null)` removes the key; `ObjectNode.put(k, (String) null)` writes a JSON `null`. `EmojiTask.restore` uses `optString(k, null)`, so writers must skip nulls to stay equivalent. -- **`JSONArray.isNull(0)` has no direct Jackson twin** — `Settings.getGitVersion()` must instead check `array.isEmpty() || !array.get(0).hasNonNull("tag_name")`. -- **Checked exceptions**: Jackson parsing throws `JsonProcessingException`; must be wrapped to avoid rippling `throws` through call sites. -- **Shade plugin**: confirm no relocation/filter rules in `pom.xml` reference `org.json` or `gson`. - -# Testing - -### Validation Approach -- `./mvnw -o clean verify` must pass with the two dependencies removed — a leftover `org.json`/`gson` import then becomes a hard compile error, which is the primary completeness check. -- Grep the whole `src` tree for `org.json`, `com.google.gson`, `JSONObject`, `JSONArray`, `Gson` and confirm zero hits. -- `./mvnw -o dependency:tree` to confirm `org.json:json` and `com.google.code.gson:gson` are fully gone (not even transitive) and only one Jackson version is resolved. - -### Key Scenarios -- **Task round-trip**: `EmojiReactionRoleTaskPersistenceTest` — a task is persisted via `toPayload()` and restored via `restore(...)` with `guildId` / `channelId` / `messageId` / `emojiMapping` intact. -- **Registry restore**: `PersistableTaskRegistryTest` — a payload built in the test is resolved to the right task type and hydrated. -- **Backward compatibility**: add an assertion that a payload string in the *old* `org.json` format still restores correctly, proving pre-existing DB rows keep working. -- **HTTP client**: `WebModule.getAsObject()` / `getAsArray()` parse a sample body; the POST path produces the same JSON body as Gson did for a `Map`. - -### Edge Cases -- Missing / null `guildId` in a payload → `restore` yields `null`, not the string `"null"`. -- Empty array from the GitLab releases endpoint → `Settings.getGitVersion()` returns `"0.0.0"` instead of throwing. -- Malformed response body → the wrapped unchecked exception surfaces with a useful message rather than an NPE. - -### Test Changes -- Update the two existing test files to build payloads with `ObjectNode`. -- Add a legacy-payload restore test in `EmojiReactionRoleTaskPersistenceTest`. -- Optionally add a small `JsonTest` covering `Json.readObject` / `readArray` error wrapping. - -# Delivery Steps - -### ✓ Step 1: Introduce Jackson and the shared Json utility -Jackson is an explicit dependency and a single shared `ObjectMapper` is available to the codebase. - -- Add `com.fasterxml.jackson.core:jackson-databind` to `pom.xml`, pinned to the version already resolved via JDA (2.21.2), next to the existing `org.json`/`gson` entries (which stay for now so the build keeps compiling). -- Create `src/main/java/me/bobert/netneutrality/utils/Json.java` holding a static thread-safe `ObjectMapper` plus helpers: `mapper()`, `object()`, `array()`, `readObject(String)`, `readArray(String)`, `write(Object)`. -- Have `readObject`/`readArray` wrap Jackson's checked `JsonProcessingException` in an unchecked exception so existing call-site signatures stay unchanged. -- Verify no shade-plugin relocation or filter rules in `pom.xml` reference `org.json` or `gson`. - -### ✓ Step 2: Migrate the Gson call sites to Jackson -Gson is no longer referenced anywhere in the source tree. - -- Delete `http/server/GsonJsonMapper.java` and remove its registration from the Javalin configuration, relying on Javalin 7's built-in Jackson `JsonMapper` instead. -- Replace `new Gson().toJson(params)` at `http/WebModule.java:256` with `Json.write(params)`, reusing the shared mapper instead of allocating per request. -- Confirm Javalin request/response serialization still behaves identically for the existing endpoints. - -### ✓ Step 3: Migrate the HTTP client and its consumers -`WebModule` returns Jackson nodes and its callers read them. - -- Change `WebModule.getAsObject()` to return `ObjectNode` and `getAsArray()` to return `ArrayNode` (lines 133-143), parsing through `Json`. -- Update `utils/Settings.java:104-109`: replace `JSONArray`/`isNull(0)`/`getJSONObject(0).getString("tag_name")` with an `ArrayNode` emptiness check and `get(0).path("tag_name").asText()`, preserving the `"0.0.0"` fallback. -- Update `command/crypto/CryptoPriceCommand.java` to read the crypto API response via `JsonNode` accessors. -- Remove the now-unused `org.json` imports from these three files. - -### ✓ Step 4: Migrate the task persistence layer -Tasks serialize and restore through Jackson while keeping the existing on-disk payload shape. - -- Change `PersistableTask.restore(JSONObject)` to `restore(ObjectNode)`. -- In `AbstractPersistableTask`, replace the `new JSONObject(this)` bean-introspection `toPayload()` with an explicit `ObjectNode` builder writing `id`, `lockId` and `name`, plus a new `protected void writePayload(ObjectNode)` hook; keep `restore` a no-op default. -- In `EmojiTask`, rewrite `restore` (lines 39-51) using `payload.path(...).asText(null)` and iteration over the `emojiMapping` node, and implement `writePayload` to emit `guildId`, `channelId`, `messageId` and `emojiMapping`, skipping nulls to match `optString(k, null)` semantics. -- Update `PersistableTaskRegistry` and `db/service/TaskService` to parse stored payload strings via `Json.readObject`. -- Confirm `tasks/reaction/EmojiReactionRoleTask` still compiles unchanged against the new `EmojiTask` contract. - -### ✓ Step 5: Update tests and drop the old dependencies -`org.json` and Gson are removed from `pom.xml` and the full build passes. - -- Rewrite payload construction in `PersistableTaskRegistryTest` and `EmojiReactionRoleTaskPersistenceTest` using `ObjectNode`. -- Add a regression test restoring a payload written in the legacy `org.json` format to prove pre-existing database rows still load. -- Migrate the `state.json` dump in `bootstrap/BotState.java:46-81` to `ObjectNode`/`ArrayNode`. -- Remove the `org.json:json` and `com.google.code.gson:gson` dependencies from `pom.xml`. -- Run `./mvnw -o clean verify`, then `dependency:tree` to confirm both libraries are fully absent and a single Jackson version is resolved. \ No newline at end of file diff --git a/.junie/plans/db-package-best-practices.md b/.junie/plans/db-package-best-practices.md deleted file mode 100644 index 7a480e8..0000000 --- a/.junie/plans/db-package-best-practices.md +++ /dev/null @@ -1,309 +0,0 @@ ---- -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` | 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`. -- 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` 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 { - protected final SessionProvider sessions; - private final Class type; - protected AbstractRepository(SessionProvider sessions, Class type) - public List findAll() // fromSession + criteria/HQL - public Optional 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 read(Function work) // sf.fromSession - protected R write(Function work) // sf.fromTransaction - protected void writeVoid(Consumer work) // sf.inTransaction -} -``` -Repositories and their key queries: -- `GuildRepository extends AbstractRepository`. -- `BlacklistRepository`: - - `findByGuildId` - - `findEnabledWords(guildId)` returns `Set` - - `enableWord(OGuild, creatorId, word)` - - `deleteWord(guildId, word)` -- `GuildSettingRepository`: - - `findByGuild` - - `upsertOrDelete(guildId, name, value, default)`, which queries the setting directly instead of using the lazy collection. -- `TaskRepository`. -- `MessageRepository`: - - `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 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 get(GSettings key); - Map snapshot(); // unmodifiable -} - -public interface GuildServiceProvider { - GuildService get(String guildId); - void evict(String guildId); - Map 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`, 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. \ No newline at end of file diff --git a/.junie/plans/decouple-command-processor.md b/.junie/plans/decouple-command-processor.md deleted file mode 100644 index 32d3fc0..0000000 --- a/.junie/plans/decouple-command-processor.md +++ /dev/null @@ -1,178 +0,0 @@ ---- -sessionId: session-260910-075746-nw8q ---- - -# Requirements - -### Overview & Goals -Refactor `CommandProcessor` to break its tightly coupled `dispatch()` method into composable, extensible pieces so that new preconditions, permission strategies, and result handlers can be added without modifying the processor itself. - -### Scope -**In Scope** -- Decompose `CommandProcessor.dispatch()` into a pipeline of single-responsibility components -- Eliminate `instanceof` checks for `InteractionContext` / `SlashCommandContext` inside the processor -- Preserve all existing behavior and keep the existing test suite green - -**Out of Scope** -- Changes to the `Command` interface or individual command implementations -- Changes to `CommandContext` subclass hierarchy -- Changes to `CommandContainer`, `CommandRegistry`, or `CommandExecutionResult` - -# Technical Design - -### Current Implementation -`CommandProcessor.dispatch()` (~80 lines, `CommandProcessor.java:48-129`) performs **five concerns** sequentially: -1. **Command lookup** — resolve command from registry -2. **Precondition checks** — guild-only, slash-only, via `instanceof` checks against `InteractionContext` -3. **Permission check** — `canExecute()` with `instanceof SlashCommandContext` -4. **Usage/argument validation** — `checkUsage()` + `checkOptions()`, again branching on `instanceof InteractionContext` -5. **Execution & result handling** — execute command, handle TASK/ERROR/WARN/NOACTION results, catch exceptions - -The `instanceof` checks (lines 66, 197, 220) tightly couple the processor to specific context subtypes, making it impossible to add new context types without modifying the processor. - -### Key Decisions -1. **Introduce a `CommandPrecondition` interface** — each check (guild-only, slash-only, permissions) becomes a standalone, testable precondition that can be composed into an ordered list. This replaces the inline if-chains and eliminates instanceof checks from the processor. -2. **Move context-type-specific logic into the context itself** — instead of `instanceof` checks in the processor, add polymorphic methods to `CommandContext` (e.g., `supportsSlashOnly()`, `canSkipPermissionCheck()`, `validateOptions()`). Each context subclass overrides as needed. -3. **Extract result handling into a `ResultHandler`** — the if-chain for TASK/ERROR/WARN/NOACTION becomes a dedicated component, making it easy to add new result types. -4. **Keep changes incremental** — refactor in place without changing the public API of `CommandProcessor` (constructor + `dispatch()` remain the same). The pipeline is internal. - -### Proposed Changes - -#### 1. `CommandPrecondition` interface -```java -// New file: core/command/precondition/CommandPrecondition.java -public interface CommandPrecondition { - /** Returns null if passed, or an error message to reply with. */ - String check(CommandContext context, Command command); -} -``` -Concrete implementations: -- `GuildOnlyPrecondition` — replaces lines 60-64 -- `SlashCommandOnlyPrecondition` — replaces lines 66-69, uses `context.supportsSlashOnly()` instead of instanceof -- `PermissionPrecondition` — replaces `canExecute()` (lines 215-229), uses `context.canSkipPermissionCheck()` instead of instanceof - -#### 2. Polymorphic methods on `CommandContext` -Add to `CommandContext`: -- `boolean isInteraction()` — default `false`, overridden to `true` in `InteractionContext` -- `boolean canSkipPermissionCheck()` — default `false`, overridden in `SlashCommandContext` - -This eliminates all `instanceof` checks from the processor. - -#### 3. `CommandResultHandler` -```java -// New file: core/command/CommandResultHandler.java -public class CommandResultHandler { - void handle(CommandContext context, CommandExecutionResult result, TaskManager taskManager); -} -``` -Encapsulates the TASK/ERROR/WARN/NOACTION/message reply logic from lines 100-120. - -#### 4. Refactored `CommandProcessor.dispatch()` -The method becomes: -```java -public void dispatch() { - CommandContainer container = resolveCommand(); - if (container == null) return; - - Command command = container.getCommand(); - context.setCommand(command); - configureSentry(command); - - for (CommandPrecondition precondition : preconditions) { - String error = precondition.check(context, command); - if (error != null) { context.replyEphemeral(error); return; } - } - - if (!usageValidator.validate(context, container)) return; - - try { - CommandExecutionResult result = command.execute(context); - resultHandler.handle(context, result, taskManager); - } catch (...) { /* existing error handling */ } -} -``` - -### Architecture Diagram -```mermaid -graph LR - D[dispatch] --> R[resolveCommand] - R --> P[Preconditions Loop] - P --> GuildOnly - P --> SlashOnly - P --> Permission - P --> UV[UsageValidator] - UV --> E[command.execute] - E --> RH[ResultHandler] -``` - -### File Structure -| File | Action | -|------|--------| -| `core/command/precondition/CommandPrecondition.java` | **New** — interface | -| `core/command/precondition/GuildOnlyPrecondition.java` | **New** | -| `core/command/precondition/SlashCommandOnlyPrecondition.java` | **New** | -| `core/command/precondition/PermissionPrecondition.java` | **New** | -| `core/command/CommandResultHandler.java` | **New** | -| `core/command/UsageValidator.java` | **New** — extracted from `checkUsage()`/`checkOptions()` | -| `core/command/CommandProcessor.java` | **Modified** — delegate to above components | -| `core/context/CommandContext.java` | **Modified** — add `isInteraction()`, `canSkipPermissionCheck()` | -| `core/context/InteractionContext.java` | **Modified** — override `isInteraction()` | -| `context/command/SlashCommandContext.java` | **Modified** — override `canSkipPermissionCheck()` | -| `CommandProcessorTest.java` | **Modified** — update construction, add unit tests for new components | - -### Risks -- **Behavioral regression** — the refactor must preserve exact reply messages and ordering. Mitigated by the existing 1000+ line test suite. -- **Precondition ordering matters** — guild-only must run before permission check. The list order is explicit and documented. - -# Testing - -### Validation Approach -- All existing `CommandProcessorTest` tests must pass unchanged (behavior preservation). -- New unit tests for each precondition, the result handler, and the usage validator in isolation. - -### Key Scenarios -- Preconditions short-circuit in order (guild-only before permissions) -- Custom precondition can be added to the list and is invoked -- ResultHandler correctly dispatches TASK to TaskManager, logs ERROR/WARN -- UsageValidator handles all subcommand/group/option combinations - -### Edge Cases -- Null command from container -- Empty precondition list (all checks pass) -- InteractionContext skips option validation in UsageValidator - -# Delivery Steps - -### ✓ Step 1: Add polymorphic methods to CommandContext hierarchy -CommandContext subclasses expose their capabilities without requiring instanceof checks. - -- Add `isInteraction()` (default `false`) and `canSkipPermissionCheck()` (default `false`) to `CommandContext.java` -- Override `isInteraction()` to return `true` in `InteractionContext.java` -- Override `canSkipPermissionCheck()` to return `true` in `SlashCommandContext.java` -- Verify existing tests still compile and pass - -### ✓ Step 2: Extract preconditions into CommandPrecondition interface and implementations -Precondition checks become standalone, composable components. - -- Create `CommandPrecondition` interface in `core/command/precondition/` -- Implement `GuildOnlyPrecondition` — checks `command.isGuildOnly()` and `context.isFromGuild()` -- Implement `SlashCommandOnlyPrecondition` — checks `command.isSlashCommandOnly()` and `context.isInteraction()` -- Implement `PermissionPrecondition` — replaces `canExecute()`, uses `context.canSkipPermissionCheck()` instead of instanceof -- Add unit tests for each precondition - -### ✓ Step 3: Extract UsageValidator and CommandResultHandler -Usage validation and result handling become dedicated classes. - -- Extract `checkUsage()` and `checkOptions()` into `UsageValidator.java`, replacing `instanceof InteractionContext` with `context.isInteraction()` -- Extract result-handling if-chain (TASK/ERROR/WARN/NOACTION + reply) into `CommandResultHandler.java` -- Add unit tests for both classes - -### ✓ Step 4: Refactor CommandProcessor to use the new components -CommandProcessor.dispatch() becomes a thin orchestrator delegating to the extracted components. - -- Replace inline precondition checks with a `List` loop -- Replace `checkUsage()`/`checkOptions()` calls with `UsageValidator` -- Replace result-handling if-chain with `CommandResultHandler` -- Remove the now-unused private methods (`canExecute`, `checkUsage`, `checkOptions`) -- Keep constructor backward-compatible or provide a builder/factory for custom precondition lists -- Run full `CommandProcessorTest` suite to confirm behavioral equivalence \ No newline at end of file diff --git a/.junie/plans/di-lifecycle-refactor.md b/.junie/plans/di-lifecycle-refactor.md deleted file mode 100644 index c7c95b1..0000000 --- a/.junie/plans/di-lifecycle-refactor.md +++ /dev/null @@ -1,302 +0,0 @@ ---- -sessionId: session-260826-092009-17wb ---- - -# Requirements - -### Overview & Goals - -Replace the singleton/static-heavy bootstrap of the bot with an explicit, deterministic **lifecycle-managed application** driven by a small hand-rolled `ApplicationContext` and a `Lifecycle` contract. Dependencies are wired through **constructors** (including for reflectively-loaded commands and tasks), and **interfaces** are introduced for the key services so the code becomes testable, maintainable, and mockable — without adopting Spring Boot or any DI framework. - -The current design (documented in `docs/architecture.md`) uses: -- `NetNeutrality` as a static singleton holding `instance` and `jda`, with a manual, order-sensitive `startup()`/`shutdown()` chain. -- Static singletons everywhere: `Settings.getInstance()`, `SessionManager` (static `getSessionFactory`/`shutdown`), `CommandManager` (static `init`/maps), `TaskManager.getInstance()`, `MusicManager` (static factory + static `playerManager`), `ThreadManager` (static executors), `ScheduledTaskManager` (static), `GuildService.get(...)` (static cache). -- `ReflectionHelper` instantiating commands/tasks via **no-arg constructors**, which forces them to reach out to singletons. - -### Scope - -**In Scope** -- A `Lifecycle` interface (`start()`/`stop()`/`priority()`) and an `ApplicationContext` that owns instances, wires them, and drives ordered startup/shutdown. -- Converting the core singletons to constructor-injected **instances**. -- Introducing **interfaces for key services** (settings, persistence/session, executors, command registry, task manager, music service, guild-service provider) — decision confirmed with the user. -- Reworking `ReflectionHelper` + command/task base classes to use **constructor injection** (decision confirmed). -- Symmetric, deterministic startup/shutdown replacing `NetNeutrality.startup()/shutdown()` and the JVM shutdown hook. -- **Reorganizing the package layout** into `bootstrap` / `framework` / `platform` / `feature` / `data` / `shared` (see the Package Structure tab), done in the same effort as the DI refactor so imports are touched once. This includes renaming the entry point `NetNeutrality` → `App`. - -**Out of Scope** -- Introducing Spring Boot, Guice, or Dagger (explicitly rejected — manual context chosen). -- Full interface-per-service/repository abstraction (only key services get interfaces). -- Dropping the `O*`/`G*` entity name prefixes (optional cosmetic churn, deferred). -- Behavioral/feature changes to commands, tasks, music, or the web API — this is a structural refactor only. -- Database schema / Flyway migration changes. - -### User Stories -- As a maintainer, I want services created and wired in one explicit place so the dependency graph and startup order are obvious and not implicit. -- As a maintainer, I want symmetric `start()`/`stop()` for every component so shutdown reliably tears down what startup created, in reverse order. -- As a developer, I want commands/tasks to receive their dependencies via constructor so they no longer call `getInstance()` and can be unit-tested with mocks. -- As a developer, I want key services behind interfaces so I can mock them in tests (building on the existing JUnit 5 + Mockito setup). - -### Functional Requirements -- The bot must start and shut down with identical externally-observable behavior to today (same ordered steps from `docs/architecture.md`). -- Startup order must be preserved: persistence/session → migrations → executors → command/task registries → music sources → JDA → web API → presence. -- Shutdown must run in reverse of startup: cancel scheduled tasks → disconnect music players → shut down executors → shut down JDA → release the Hibernate registry. -- Commands and tasks discovered by `ReflectionHelper` must continue to auto-load ("drop a class in the package" model) but now receive dependencies via constructor. -- No public command/task behavior changes. - -### Non-Functional Requirements -- **No new runtime dependencies** (manual container only). -- **Incremental & low-risk**: migrate one area at a time using temporary static shims that delegate to the context, so the build stays green throughout. -- **Compatibility**: fat-JAR packaging and `main` entry point (`me.bobert.netneutrality.NetNeutrality`) remain unchanged. -- **Testability**: instance + interface-based services must be mockable with Mockito. - -# Technical Design - -### Current Implementation - -- **Bootstrap** — `NetNeutrality` (`src/main/java/me/bobert/netneutrality/NetNeutrality.java`): static `instance`, static `jda`, `main()` builds `Settings` implicitly, constructor calls `startup()` which chains `SessionManager.getSessionFactory()` → `DatabaseMigration.migrate()` → `CommandManager.init()` → `MusicManager.initPlayerManager()` → `TaskManager.getInstance().init()` → `initJDA()` → `CommandManager.registerSlashCommands(jda)` → `new EventsHandler()` → `new WebApi(port)` → presence → shutdown hook. `shutdown()` runs `ScheduledTaskManager.cancelAll()` → `MusicManager.disconnectPlayers()` → `ThreadManager.shutdownExecutor()` → `disconnect()` → `SessionManager.shutdown()`. -- **Singletons/statics**: `Settings` (`utils/Settings.java`), `SessionManager` (`core/db/SessionManager.java`, all-static), `CommandManager` (`core/command/CommandManager.java`, static `commands` map + static `init`), `TaskManager` (`core/tasks/TaskManager.java`, `getInstance()`), `MusicManager` (`music/MusicManager.java`, static `managers` map + static `playerManager` + static `get`/`initPlayerManager`/`disconnectPlayers`), `ThreadManager` (`core/thread/ThreadManager.java`, static executors), `ScheduledTaskManager`, `GuildService` (`db/service/GuildService.java`, static `services` cache). -- **Reflective instantiation** — `ReflectionHelper.loadClassesFromPackage(...)` calls `s.getConstructor().newInstance()` (no-arg), used by `CommandManager.initCommands()` and `TaskManager.loadBuiltInTasks()/loadScheduledTasks()`. -- **Consumers of singletons** — ~110 call sites across commands (`command/**`), tasks (`tasks/**`), contexts (`context/**`, `core/context/**`), HTTP handlers (`http/server/**`), `EventsHandler`, repositories/services (`db/**`), etc. - -### Key Decisions (confirmed with user) -1. **Manual `ApplicationContext` + `Lifecycle`** — no DI framework; the context owns instances, wires them via constructors, and drives ordered `start()`/`stop()`. -2. **Constructor injection for commands/tasks** — `ReflectionHelper` resolves a constructor that accepts the context (or specific dependencies) and passes them in; commands/tasks store deps as final fields instead of calling `getInstance()`. -3. **Lifecycle + key service interfaces** — introduce a `Lifecycle` interface plus interfaces for the main services (settings, persistence/session, executors, command registry, task manager, music service, guild-service provider). Repositories and minor helpers stay concrete. -4. **Package restructure to `bootstrap`/`framework`/`platform`/`feature`/`data`/`shared`** — separate code by reason-of-change (engine vs. cross-cutting infra vs. concrete features vs. persistence model vs. leaf helpers), consolidate each feature vertical (e.g. music) into one package, and dissolve the catch-all `utils`. Performed right after the DI wiring, via the IDE's safe "Move Class" refactor, so the ~110 import updates happen once. Full details in the Package Structure tab. - -### Proposed Changes - -**1. Lifecycle contract** — new `me.bobert.netneutrality.bootstrap.Lifecycle`: -```java -public interface Lifecycle { - void start() throws Exception; - default void stop() {} - default int priority() { return 0; } // lower = started earlier, stopped later -} -``` - -**2. Application context** — new `me.bobert.netneutrality.bootstrap.ApplicationContext`: -```java -public final class ApplicationContext { - private final List components = new ArrayList<>(); - public T register(T component) { - if (component instanceof Lifecycle l) components.add(l); - return component; - } - public void start() throws Exception { - components.sort(Comparator.comparingInt(Lifecycle::priority)); - for (Lifecycle c : components) c.start(); - } - public void stop() { - var reversed = new ArrayList<>(components); - Collections.reverse(reversed); - reversed.forEach(Lifecycle::stop); - } -} -``` -The context also exposes typed getters for wired services so `ReflectionHelper`/`EventsHandler`/contexts can resolve dependencies. - -**3. Convert singletons to instances behind interfaces** (priority values in parentheses set start order): -- `SettingsProvider` ← `Settings` (config; constructed first, not itself a `Lifecycle`). -- `SessionProvider`/persistence ← `SessionManager` instance implementing `Lifecycle` (10): `start()` builds the `SessionFactory` + runs `DatabaseMigration.migrate()`; `stop()` destroys the registry. -- `ExecutorProvider` ← `ThreadManager` instance implementing `Lifecycle` (20): `stop()` = current `shutdownExecutor()`. -- `CommandRegistry` ← `CommandManager` instance implementing `Lifecycle` (30): `start()` = `init()`; `registerSlashCommands(jda)` called after JDA is ready. -- `TaskManager` interface ← impl implementing `Lifecycle` (30): `start()` = current `init()`. -- Scheduled tasks ← `ScheduledTaskManager` instance implementing `Lifecycle`: `stop()` = `cancelAll()`. -- `MusicService` ← new factory owning the shared `AudioPlayerManager` + per-guild `MusicManager` map, implementing `Lifecycle` (40): `start()` = `initPlayerManager()`; `stop()` = `disconnectPlayers()`. Per-guild `MusicManager` becomes a plain instance created by the factory. -- `GuildServiceProvider` ← registry owning the guild cache (replaces static `GuildService.get`). -- `WebApi` becomes a `Lifecycle` (60): `start()` binds the HTTP server; `stop()` stops it. -- JDA is built by the context (replacing static `jda`) and exposed via a getter/provider. - -**4. Rework reflective instantiation** — extend `ReflectionHelper` so `loadClassesFromPackage` accepts a factory/`ApplicationContext` and instantiates via a constructor that takes the needed dependencies (falling back to no-arg for classes that declare it). `CommandManager.initCommands()` and `TaskManager.loadBuiltInTasks()/loadScheduledTasks()` pass the context so commands/tasks are constructor-injected. - -**5. Migrate consumers off `getInstance()`** — command/task base classes (`core/tasks/impl/AbstractTask.java`, `AbstractStepTask`, `AbstractPersistableTask`, `core/command/impl/MusicCommand.java`) and contexts (`core/context/AbstractContext.java`, `context/**`) take dependencies via constructor. `EventsHandler` is constructed with the context and passes services when building contexts. During migration, keep **temporary static shims** (e.g. `Settings.getInstance()` delegating to the context instance) so untouched call sites keep compiling; remove them at the end. - -**6. New bootstrap** — `NetNeutrality.main()` builds the `ApplicationContext`, constructs and `register()`s services in dependency order, calls `context.start()`, and installs **one** shutdown hook calling `context.stop()`. Static `instance`/`jda` are removed. - -### Data Models / Contracts -Key new interfaces (illustrative signatures): -```java -interface SettingsProvider { String get(String key); String get(String key, String def); String getPrefix(); } -interface SessionProvider extends Lifecycle { org.hibernate.SessionFactory getSessionFactory(); } -interface ExecutorProvider extends Lifecycle { java.util.concurrent.ExecutorService getExecutor(String name); java.util.concurrent.ScheduledExecutorService getScheduledExecutor(); } -interface CommandRegistry extends Lifecycle { CommandContainer getCommandContainer(String name); void registerSlashCommands(JDA jda); } -interface TaskManager extends Lifecycle { Task getTask(String id); void insert(CommandContext ctx, Task task); void handle(TaskContext ctx); /* ... */ } -interface MusicService extends Lifecycle { MusicManager get(net.dv8tion.jda.api.entities.Guild guild); } -interface GuildServiceProvider { GuildService get(String guildId); } -``` - -### File Structure -New DI classes are created directly in their **target** packages (from the Package Structure tab) so they are not moved twice. -- **Added**: `bootstrap/Lifecycle.java`, `bootstrap/ApplicationContext.java`; service interfaces `platform/config/SettingsProvider.java`, `platform/persistence/SessionProvider.java`, `platform/concurrency/ExecutorProvider.java`, `framework/command/CommandRegistry.java`, `framework/task/TaskManager.java` [interface] + impl, `feature/music/MusicService.java`, `data/service/GuildServiceProvider.java`. -- **Moved / renamed (package restructure)**: `core` → `framework` (engine only); `core.db` + `db` → `platform.persistence` + `data`; `core.thread` → `platform.concurrency`; `hooks` → `platform.event`; `http` → `platform.web`; `utils` dissolved into `platform.config` / `platform.persistence` / `bootstrap` / `shared.util`; `NetNeutrality` → `App`. See the Package Structure tab. -- **Modified**: `App.java` (bootstrap), `ReflectionHelper.java` (constructor injection), all converted managers/services, command/task base classes, `EventsHandler.java`, `WebApi.java`, HTTP handlers, `getInstance()` call sites, and `pom.xml` / `Dockerfile` / `startup.sh` for the `mainClass` rename. -- **Docs**: update `docs/architecture.md` (startup/shutdown + package map) and `docs/utilities-and-bootstrap.md` (bootstrap) to reflect the context and new layout. - -### Architecture Diagram -```mermaid -graph TD - Main["NetNeutrality.main()"] --> Ctx[ApplicationContext] - Ctx -->|constructs & wires| Settings[SettingsProvider] - Ctx -->|register + start/stop| Session[SessionProvider] - Ctx -->|register + start/stop| Exec[ExecutorProvider] - Ctx -->|register + start/stop| Cmd[CommandRegistry] - Ctx -->|register + start/stop| Tasks[TaskManager] - Ctx -->|register + start/stop| Music[MusicService] - Ctx -->|register + start/stop| Web[WebApi] - Ctx -->|builds| JDA[JDA] - Cmd -->|constructor-injects| Refl[ReflectionHelper] - Tasks -->|constructor-injects| Refl - Refl -->|ctor deps| Impl[Commands / Tasks] - JDA -->|events| Events[EventsHandler] - Events -->|builds contexts with deps| Cmd - Events --> Tasks -``` - -### Risks -- **Large call-site surface (~110 usages)** — mitigated by temporary static shims delegating to context instances, migrating area-by-area, keeping the build green. -- **Startup ordering regressions** — mitigated by encoding order via `Lifecycle.priority()` and mirroring the exact sequence from `docs/architecture.md`. -- **Reflective constructor resolution** — commands/tasks must expose a consistent injectable constructor; mitigate by supporting a no-arg fallback and validating all classes load at startup. -- **Per-guild `MusicManager` vs shared factory** — clarify that the shared `AudioPlayerManager` and cache move to the `MusicService`, while per-guild `MusicManager` stays a lightweight instance. -- **`SessionManager.System.exit(1)` on DB failure** — preserve fail-fast behavior inside the new `SessionProvider.start()`. - -# Package Structure - -### Current Layout Problems -The tree today mixes three organizing principles at once: abstraction-vs-impl (`core.command`↔`command`, `core.context`↔`context`, `core.tasks`↔`tasks`), infra-vs-data (`core.db`↔`db`), and no split at all (`music`, `http`, `utils`, `hooks`). Consequences: -- `core` is simultaneously "the engine" and "misc" — it holds the framework sub-packages **and** loose classes (`LogHandler`, `BotState`). -- A single feature is scattered: music lives in `core.command.impl.MusicCommand`, `command.music`, `context.MusicChannelContext`, `tasks.music`, and `music/` — five places. -- `utils` is a catch-all mixing config (`Settings`), reflection (`ReflectionHelper`), JPA glue (`JpaUtils`), and formatting (`MessageFunction`, `UserFunction`). -- Naming drift: `db.entities` (plural) vs. the singular convention elsewhere. - -### Guiding Principle -Separate code by the reason it changes: -- **framework** — the reusable bot engine (command/task/context machinery); rarely changes, knows nothing about specific commands. -- **platform** — cross-cutting technical services (persistence, concurrency, config, logging, event dispatch, web); changes when tech/tooling changes. -- **feature** — concrete commands + tasks + contexts + domain logic, grouped **by feature vertical**; changes most often. -- **data** — the persistence model (entities, repositories, services). -- **shared** — genuinely cross-feature leaf helpers. - -### Target Layout -``` -me.bobert.netneutrality -├── App.java // was NetNeutrality (main + bootstrap entry) -├── bootstrap/ // ApplicationContext, Lifecycle, ReflectionHelper (composition root) -├── framework/ // the reusable engine (was core, minus the misc) -│ ├── command/ // Command, CommandProcessor, CommandRegistry, options, build, impl bases -│ ├── context/ // Context hierarchy, reply/ -│ └── task/ // Task, TaskManager, StepTask/PersistableTask, persistence/, trait/, impl bases -├── platform/ // cross-cutting technical services (horizontals) -│ ├── config/ // Settings / SettingsProvider -│ ├── persistence/ // SessionProvider, DatabaseMigration, Repository base, JpaUtils -│ ├── concurrency/ // ExecutorProvider, ScheduledTaskManager, BaseThreadFactory -│ ├── logging/ // LogHandler -│ ├── event/ // EventsHandler (was hooks) -│ └── web/ // WebApi, handlers, WebModule (was http) -├── feature/ // concrete features, one vertical per package -│ ├── music/ // MusicService/MusicManager, QueueManager, scheduler, source/, + music commands/tasks/contexts -│ ├── moderation/ // blacklist, purge, admin commands + related services -│ ├── reaction/ // reaction commands + emoji-role tasks -│ ├── git/ // report-issue command + task -│ ├── crypto/ -│ └── help/ -├── data/ // the persistence model (was the "db" half of the split) -│ ├── entity/ // O* entities (entities → entity, singular) -│ ├── repository/ -│ └── service/ // GuildService, GuildServiceProvider, BlacklistService, … -├── shared/util/ // MiscUtils, MessageFunction, UserFunction, GuildUtils, ContextUtils -└── exceptions/ // unchanged (already coherent) -``` - -### Key Moves -- `core` → `framework`, now containing **only** the engine (no `LogHandler`, no `BotState`, no `db`, no `thread`). -- `core.db` (infra) → `platform.persistence`; `db` (model) → `data` — resolves the "`db` means two things" problem. `db.entities` → `data.entity`. -- `core.thread` → `platform.concurrency`; `hooks` → `platform.event`; `http` → `platform.web`. -- `utils` dissolved: config → `platform.config`, JPA glue → `platform.persistence`, reflection → `bootstrap`, formatting/misc → `shared.util`. -- All music code consolidates under `feature.music`; the other verticals group their command/task/context/service triplets similarly. -- `LogHandler` → `platform.logging`; `BotState` → `bootstrap` (composition-root-adjacent state). - -### Sequencing & Rationale -The repackage is done **immediately after the DI wiring, in the same effort**: the DI refactor already renames managers (`CommandManager`→`CommandRegistry`, `SessionManager`→`SessionProvider`, `ThreadManager`→`ExecutorProvider`) and touches imports at ~110 call sites, so combining them avoids editing the same imports twice. Moves use the IDE's safe **"Move Class"** refactor (references update automatically), one subsystem at a time, building after each. New DI classes are created directly in their target packages rather than moved later. - -### Lower-Risk Variant (optional fallback) -If full feature-first grouping feels too large in one pass, a high-value subset fixes ~80% of the confusion with far fewer moves: rename `core` → `framework`, extract `platform` (evict `LogHandler`, `BotState`, `core.db`, `core.thread`, `hooks`, `http`), and dissolve `utils` — while leaving the concrete `command`/`task`/`context` packages where they are. The trade-off is that a single feature stays spread across those layer packages. - -### Risks -- **Move touches nearly every import** — mitigated by IDE "Move Class" (auto-updates references), one subsystem per step, and a build after each move. -- **Main-class rename `NetNeutrality` → `App`** — must update the Shade `mainClass` in `pom.xml`, the `Dockerfile`/`startup.sh` entry, and docs atomically, or the fat JAR won't launch. -- **Feature-first churn** — grouping music/moderation/etc. is the largest move; the lower-risk variant above is the fallback if needed. - -# Testing - -### Validation Approach -The refactor is behavior-preserving, so validation focuses on: (a) the project compiles and the fat JAR builds after each stage, (b) existing tests still pass, and (c) new unit tests prove services are now mockable/instance-based. Use the existing Maven + JUnit 5 + Mockito setup (`./mvnw test`); Checkstyle runs in the `test` phase and must stay green. - -### Key Scenarios -- **Build & Checkstyle** — `./mvnw package` succeeds after every stage (shade fat JAR, main class unchanged). -- **ApplicationContext ordering** — a unit test registers fake `Lifecycle` components with different `priority()` values and asserts `start()` runs ascending and `stop()` runs descending. -- **Command/task loading** — assert `CommandManager`/`TaskManager` still discover and instantiate all implementations via `ReflectionHelper` with constructor injection (no `getInstance()` calls at runtime). -- **Existing tests** — `CommandProcessorTest`, `SettingsTest`, and any others continue to pass; update them to use injected instances/mocks instead of wrestling with singletons. - -### Edge Cases -- Missing `NETNEUTRALITY_BOT_TOKEN` → startup still fails fast with a clear error. -- DB connection failure inside `SessionProvider.start()` → preserves current fail-fast (Sentry capture in non-dev + exit). -- A command/task class that only declares a no-arg constructor → `ReflectionHelper` still loads it via the fallback path. -- Shutdown with services that never fully started → `stop()` must be null-safe (mirroring current guards like `jda != null`, `registry != null`). - -### Test Changes -- **Add**: `ApplicationContextTest` (ordering + symmetric start/stop), a `ReflectionHelper` constructor-injection test. -- **Update**: `SettingsTest` and `CommandProcessorTest` to construct instances/mocks of the new interfaces instead of relying on static singletons. -- **Skip**: no integration test against a live Discord gateway or real MySQL (out of scope). - -# Delivery Steps - -### ✓ Step 1: Introduce Lifecycle contract and ApplicationContext bootstrap -The app boots through an `ApplicationContext` that drives ordered start/stop, while behavior stays identical via temporary shims. - -- Add `me.bobert.netneutrality.bootstrap.Lifecycle` interface with `start()`, `stop()` (default no-op), and `priority()` (default 0, lower = started earlier / stopped later). -- Add `me.bobert.netneutrality.bootstrap.ApplicationContext` that registers components, sorts by `priority()`, starts ascending and stops descending, and exposes typed getters for wired services. -- Refactor `NetNeutrality.main()`/`startup()` to build the context and register the existing (still static) steps as thin `Lifecycle` adapters, install a single shutdown hook calling `context.stop()`, and remove static `instance`/`jda` (temporarily bridged where needed). -- Add `ApplicationContextTest` asserting ascending start and descending stop ordering. - -### ✓ Step 2: Convert foundational services to instances behind interfaces -Configuration, persistence, and executors are constructor-injected instances behind interfaces, wired into the context with correct lifecycle order. - -- Introduce `SettingsProvider` implemented by `Settings` (instance, constructed first); keep `Settings.getInstance()` as a temporary shim delegating to the context instance. -- Convert `core/db/SessionManager` into a `SessionProvider` instance implementing `Lifecycle` (priority ~10): `start()` builds the `SessionFactory` and runs `DatabaseMigration.migrate()`, `stop()` destroys the registry; preserve fail-fast on DB errors. -- Convert `core/thread/ThreadManager` into an `ExecutorProvider` instance implementing `Lifecycle` (priority ~20) with `stop()` = current `shutdownExecutor()`. -- Update `JpaUtils`/`Repository`/`db/**` call sites andhihims to use the injected providers. - -### ✓ Step 3: Convert domain managers (command, task, scheduled, music, guild) to instances behind interfaces -The command/task/music/guild managers are instance-based `Lifecycle` components behind interfaces, registered in the context. - -- Introduce `CommandRegistry` (impl = `CommandManager`, priority ~30): `start()` = `init()`; `registerSlashCommands(jda)` invoked after JDA is ready. -- Introduce a `TaskManager` interface + impl (priority ~30) implementing `Lifecycle` with `start()` = current `init()`; convert `ScheduledTaskManager` to an instance whose `stop()` = `cancelAll()`. -- Introduce `MusicService` factory owning the shared `AudioPlayerManager` and per-guild `MusicManager` map (priority ~40): `start()` = `initPlayerManager()`, `stop()` = `disconnectPlayers()`; make per-guild `MusicManager` a plain instance created by the factory. -- Introduce `GuildServiceProvider` owning the guild cache, replacing static `GuildService.get(...)`. -- Keep static shims on each so untouched call sites keep compiling. - -### ✓ Step 4: Enable constructor injection for reflectively-loaded commands and tasks -Commands and tasks receive their dependencies via constructor instead of calling `getInstance()`. - -- Extend `ReflectionHelper.loadClassesFromPackage` to accept the `ApplicationContext` and instantiate via a constructor taking the required dependencies, with a no-arg fallback for classes that declare it. -- Update `CommandManager.initCommands()` and `TaskManager.loadBuiltInTasks()/loadScheduledTasks()` to pass the context. -- Update command/task base classes (`core/tasks/impl/AbstractTask`, `AbstractStepTask`, `AbstractPersistableTask`, `core/command/impl/MusicCommand`) and contexts (`core/context/AbstractContext`, `context/**`, `EventsHandler`) to store injected dependencies as final fields. -- Add a `ReflectionHelper` test proving constructor injection and no-arg fallback both work. - -### ✓ Step 5: Remove static shims, finalize interfaces, and update tests/docs -All singletons/`getInstance()` accessors are gone; services are wired only through the context, and tests/docs reflect the new model. - -- Migrate remaining `getInstance()`/static call sites (commands, tasks, HTTP handlers in `http/server/**`, `LogHandler`, `BotState`) to injected interfaces and delete the temporary static shims and singleton fields. -- Convert `WebApi` into a `Lifecycle` component (priority ~60) with `start()`/`stop()` for the HTTP server. -- Update `SettingsTest` and `CommandProcessorTest` to use injected instances/mocks of the new interfaces; ensure `./mvnw package` (including Checkstyle) is green. -- Update `docs/architecture.md` (startup/shutdown lifecycle) and `docs/utilities-and-bootstrap.md` (bootstrap) to describe the `ApplicationContext` + `Lifecycle` design. - -### ✓ Step 6: Reorganize packages into bootstrap/framework/platform/feature/data/shared -The codebase is regrouped by reason-of-change into the target layout (see the Package Structure tab), with all references updated and the build green. - -- Rename the entry point `NetNeutrality` → `App`, and update the Shade `mainClass` in `pom.xml`, plus the `Dockerfile`/`startup.sh` launch entry. -- Using the IDE's safe "Move Class" refactor, one subsystem per step: `core` → `framework` (engine only); `core.db` + `db` → `platform.persistence` + `data` (`db.entities` → `data.entity`); `core.thread` → `platform.concurrency`; `hooks` → `platform.event`; `http` → `platform.web`. -- Dissolve `utils`: `Settings`/`SettingsProvider` → `platform.config`, `JpaUtils` → `platform.persistence`, `ReflectionHelper` → `bootstrap`, formatting/misc helpers → `shared.util`; move `LogHandler` → `platform.logging` and `BotState` → `bootstrap`. -- Consolidate each feature vertical (music, moderation, reaction, git, crypto, help) into `feature.*`, pulling its commands/tasks/contexts/services together. -- Build and run tests after each subsystem move; update `docs/architecture.md` "Package map" to the new layout. \ No newline at end of file diff --git a/.junie/plans/improve-music-manager.md b/.junie/plans/improve-music-manager.md deleted file mode 100644 index 0e602a9..0000000 --- a/.junie/plans/improve-music-manager.md +++ /dev/null @@ -1,95 +0,0 @@ ---- -sessionId: session-260905-173932-36te ---- - -# Requirements - -### Overview & Goals -Improve the `MusicManager` class for better maintainability, testability, and separation of concerns. - -### Scope -**In Scope:** -- Refactor `MusicManager` to reduce responsibilities -- Extract vote-skip logic into a dedicated service -- Fix synchronization strategy -- Improve error handling -- Eliminate magic numbers - -**Out of Scope:** -- Changing `QueueManager` or `TrackScheduler` internals (beyond reducing circular refs) -- Changing external behavior or command interfaces - -# Technical Design - -### Current Issues - -1. **Vote-skip business logic in MusicManager** (lines 113–145): The `voteSkip()` method contains complex voting logic (threshold calculation, member counting, skip decisions) that doesn't belong in a player manager. Magic number `2.1` is used as a vote divisor with no explanation. - -2. **Circular dependencies**: `MusicManager` → `TrackScheduler` → `QueueManager` → `TrackScheduler` → `MusicManager`. This makes testing and reasoning about the code difficult. - -3. **Overly broad `synchronized` methods**: Nearly every public method is `synchronized` on `this`, even simple getters like `getCurrentlyPlaying()` and `isCurrentlyPlaying()`. This creates unnecessary contention. Methods like `getQueueSize()` aren't synchronized at all, creating inconsistency. - -4. **Poor error handling in `connect()`** (line 80): Throws a bare `RuntimeException` when `audioChannel` is null instead of using `IllegalArgumentException` or returning a `CommandExecutionResult`. - -5. **`baseVolume` parsed from String without error handling** (line 44): `Integer.parseInt()` can throw `NumberFormatException` with no fallback. - -### Proposed Changes - -#### 1. Extract `VoteSkipService` -- New class: `me.bobert.netneutrality.music.VoteSkipService` -- Moves all vote-skip logic out of `MusicManager` -- Replace magic `2.1` with a named constant `VOTE_THRESHOLD_DIVISOR` or configurable guild setting -- `MusicManager.voteSkip()` delegates to the new service - -#### 2. Reduce circular dependencies -- `TrackScheduler` and `QueueManager` should not need to call back into `MusicManager` for `disconnect()`. Instead, use a callback/listener interface (e.g., `MusicManagerCallback`) that `TrackScheduler` holds, breaking the direct circular reference. - -#### 3. Fix synchronization -- Remove `synchronized` from simple delegating methods (`getCurrentlyPlaying()`, `isCurrentlyPlaying()`, `resume()`, `stopTrack()`) — `AudioPlayer` is already thread-safe. -- Keep `synchronized` only on methods that coordinate multiple state changes (`connect()`, `disconnect()`, `play()`). -- Make `getQueueSize()` consistent with the chosen strategy. - -#### 4. Improve error handling -- `connect()`: Replace `RuntimeException` with `IllegalArgumentException("audioChannel must not be null")` -- `baseVolume`: Add try-catch with a sensible default (e.g., 50) and a warning log. - -#### 5. Extract constants -- `VOTE_THRESHOLD_DIVISOR = 2.1` (or make configurable) -- `MIN_MEMBERS_FOR_VOTE = 3` -- `DEFAULT_VOLUME = 50` - -### File Structure -| File | Action | -|------|--------| -| `music/MusicManager.java` | Modify — remove vote logic, fix sync, fix error handling | -| `music/VoteSkipService.java` | **New** — extracted vote-skip logic | -| `music/MusicManagerCallback.java` | **New** — interface to break circular dependency | -| `music/TrackScheduler.java` | Modify — use callback interface instead of direct `MusicManager` reference where possible | -| `music/QueueManager.java` | Modify — remove `getMusicManager()` call chain for disconnect | - -# Delivery Steps - -### ✓ Step 1: Extract VoteSkipService and constants -Vote-skip logic is isolated in a new service class and magic numbers are replaced with named constants. - -- Create `VoteSkipService.java` in `me.bobert.netneutrality.music` -- Move the vote-skip logic from `MusicManager.voteSkip()` into `VoteSkipService.voteSkip(CommandContext, QueueManager, AudioChannel)` -- Define constants: `VOTE_THRESHOLD_DIVISOR`, `MIN_MEMBERS_FOR_VOTE` -- Update `MusicManager.voteSkip()` to delegate to the new service -- Add `DEFAULT_VOLUME` constant and wrap `baseVolume` parsing in try-catch with fallback - -### ✓ Step 2: Fix synchronization and error handling -Synchronization is applied only where needed and error handling is improved. - -- Remove `synchronized` from simple delegating methods: `getCurrentlyPlaying()`, `isCurrentlyPlaying()`, `resume()`, `stopTrack()`, `setVolume()` -- Keep `synchronized` on `connect()`, `disconnect()`, `play()`, `isNotConnectedTo()` -- Replace `RuntimeException` in `connect()` with `IllegalArgumentException` -- Ensure `getQueueSize()` follows the same synchronization strategy as other methods - -### ✓ Step 3: Reduce circular dependencies between MusicManager, TrackScheduler, and QueueManager -The circular reference chain is broken using a callback interface. - -- Create `MusicManagerCallback.java` interface with method `void onQueueEmpty()` (or similar) to replace direct `MusicManager.disconnect()` calls from `QueueManager` -- Have `MusicManager` implement the callback interface -- Update `TrackScheduler` constructor to accept the callback interface instead of the full `MusicManager` where only disconnect/connection behavior is needed -- Update `QueueManager.clearQueue()` to use the callback instead of `scheduler.getMusicManager().disconnect()` \ No newline at end of file diff --git a/.junie/plans/improve-task-manager.md b/.junie/plans/improve-task-manager.md deleted file mode 100644 index 49dce0c..0000000 --- a/.junie/plans/improve-task-manager.md +++ /dev/null @@ -1,213 +0,0 @@ ---- -sessionId: session-260919-132150-1vmv ---- - -# Requirements - -### Overview & Goals -The `TaskManager` subsystem in Net Neutrality manages the in-memory registry, execution dispatch, locking, persistence, and lifecycle transitions for Discord interaction tasks (buttons, modals, reactions, step-by-step flows, and scheduled background tasks). - -This plan addresses several bugs, synchronization oversights, memory leak potentials, and architectural discrepancies between `docs/tasks.md` and `TaskManagerImpl.java`: -1. **Fix Parent-Child Lifecycle Propagation**: Correctly handle `REMOVE_SELF_AND_CHILDREN` when triggered by child tasks (`WithParent`). -2. **Synchronize DB Persistence on Removal**: Ensure `PersistableTask` entries are purged from the database via `TaskService.unpersistTask()` when removed. -3. **Prevent Memory Leaks in Pending Tasks**: Unify `pendingTasks` tracking and ensure `TaskCleanup` cleans up expired pending step tasks. -4. **Fix Concurrency & Lock Cleanup Hazards**: Eliminate race conditions where lock eviction during task execution allows duplicate concurrent runs. -5. **Optimize Lookup Performance**: Replace eager full-collection queries with short-circuiting streams for partial ID lookups. -6. **Improve Test Coverage**: Introduce thorough unit tests covering all task lifecycle paths, locking behavior, and persistence hooks. - -### Scope -- **In Scope**: - - `me.bobert.netneutrality.core.tasks.TaskManager` interface. - - `me.bobert.netneutrality.core.tasks.TaskManagerImpl` implementation. - - `me.bobert.netneutrality.scheduled.cleanup.TaskCleanup` expiration handling. - - Interaction deletion error-handling callbacks on `InteractionHook`. - - Comprehensive unit test suite for `TaskManagerImpl`. -- **Out of Scope**: - - Refactoring concrete task business logic (e.g., individual music or reaction commands). - - Schema changes to the `OTask` entity or database migrations. - -### Functional Requirements -- **FR-1**: When any child task implementing `WithParent` returns `Lifecycle.REMOVE_SELF_AND_CHILDREN`, `TaskManager` must remove the parent task and all associated child tasks from the registry. -- **FR-2**: When any `PersistableTask` is removed from `TaskManager` (via lifecycle completion, manual removal, or cancellation), `TaskManager` must call `taskService.unpersistTask()` so the task is deleted from persistent storage. -- **FR-3**: Pending step tasks must be tracked reliably, exposed for expiration checks, and cleaned up by `TaskCleanup` if abandoned. -- **FR-4**: Concurrent task invocations sharing the same `lockId` must be serialized safely without lock collisions or premature lock disposal. -- **FR-5**: `findTaskByPartialId` must short-circuit upon finding the first match without allocating full intermediate lists. -- **FR-6**: Interaction deletion via `Deletable` must gracefully handle expired hooks and Discord API errors asynchronously. - -# Technical Design - -### Current Implementation -`TaskManagerImpl` (`me.bobert.netneutrality.core.tasks.TaskManagerImpl`) implements `TaskManager` and `Lifecycle`. It maintains: -- `tasks`: `ConcurrentHashMap` for active tasks. -- `pendingTasks`: `CopyOnWriteArrayList` for unassigned step tasks. -- `deletableTasksContext`: `ConcurrentHashMap` for tracking interaction hooks to delete. -- `tasksLock`: `ConcurrentHashMap` for per-task/per-parent synchronization. -- `taskService`: `TaskService` for loading and persisting `PersistableTask` entities. - -### Key Issues & Decisions - -#### 1. Parent/Child Task Lifecycle Resolution -- **Issue**: `handleTaskLifecycle` only checks `if (task instanceof WithChildren)` for `REMOVE_SELF_AND_CHILDREN`. When a button task (which implements `WithParent

`, such as `LoadTrackFromPlaylistTask`) returns `REMOVE_SELF_AND_CHILDREN`, the check evaluates to `false`, leaving the parent and sibling buttons active in memory. -- **Decision**: In `handleTaskLifecycle`, check if the task implements `WithParent`. If so, obtain the parent task (`taskWithParent.getParentTask()`) and remove the parent along with all of its child tasks (`parent.getChildTasks()`). If the task directly implements `WithChildren`, remove all its child tasks and itself. - -#### 2. Automatic Persistence Synchronization -- **Issue**: `docs/tasks.md` specifies that persistable tasks removed after execution are deleted from the database. Currently `TaskManagerImpl.removeTask` only removes from `tasks` map and never calls `taskService.unpersistTask(persistableTask)`. -- **Decision**: In `removeTask(Task task)`, check `if (task instanceof PersistableTask persistableTask)` and invoke `taskService.unpersistTask(persistableTask)`. - -#### 3. Pending Task Registry & Cleanup -- **Issue**: `pendingTasks` is stored in a `CopyOnWriteArrayList`. `getTasks()` ignores it, preventing `TaskCleanup` from pruning expired step tasks. `removeTask()` also ignores `pendingTasks`. -- **Decision**: Store `pendingTasks` in a `ConcurrentHashMap` (or unify with active tasks / pending lookup). Update `getTasks()` or provide a method to include pending tasks in cleanup routines. Update `removeTask(Task task)` to remove from `pendingTasks` as well as `tasks`. - -#### 4. Concurrency & Lock Management -- **Issue**: `removeTask` calls `tasksLock.remove(task.getLockId())` immediately. If another thread is waiting or about to acquire the lock for a sibling task, it can get a new lock instance from `computeIfAbsent`, breaking mutual exclusion. -- **Decision**: Maintain lock instances safely; do not remove locks eagerly while in the middle of executing handlers. Persistable tasks with state should also participate in locking if needed, or maintain clear locking semantics. - -#### 5. Lookup API Optimization -- **Issue**: `findTaskByPartialId` calls `findTasksByPartialId`, instantiating and populating a full `ArrayList` before taking the first element. -- **Decision**: Implement `findTaskByPartialId` with `.filter(...).findFirst()` directly on the stream. - -### Architecture & Component Interaction Diagram - -```mermaid -graph TD - subgraph Discord Events - BE[ButtonInteractionEvent] --> EH[EventsHandler] - ME[ModalInteractionEvent] --> EH - RE[MessageReactionAddEvent] --> EH - MSG[MessageReceivedEvent] --> EH - end - - subgraph TaskManager Subsystem - EH -->|TaskContext| TM[TaskManagerImpl] - TM -->|Acquire Lock| TL[tasksLock Map] - TM -->|Lookup Task| TR[tasks & pendingTasks Maps] - TM -->|Execute| T[Task.execute] - T -->|Return Lifecycle| TM - TM -->|Cascade Cleanup| LC[handleTaskLifecycle] - LC -->|WithParent / WithChildren| RM[removeTask] - RM -->|Unpersist| TS[TaskService] - TS -->|Delete| DB[(Database / OTask)] - RM -->|Deletable| DH[InteractionHook.deleteOriginal] - end - - subgraph Background Maintenance - TC[TaskCleanup] -->|Periodic Poll| TM - TM -->|Prune Expired| RM - end -``` - -### Proposed Code Changes & Contracts - -#### `TaskManagerImpl.java` -```java -private void handleTaskLifecycle(Task task, Task.Lifecycle status) { - if (status == null) { - throw new IllegalArgumentException("Task lifecycle status cannot be null"); - } - - switch (status) { - case REMOVE_SELF -> removeTask(task); - case REMOVE_SELF_AND_CHILDREN -> { - if (task instanceof WithParent taskWithParent) { - Task parent = taskWithParent.getParentTask(); - if (parent instanceof WithChildren parentWithChildren) { - for (Task child : parentWithChildren.getChildTasks()) { - removeTask(child); - } - } - removeTask(parent); - } else if (task instanceof WithChildren taskWithChildren) { - for (Task child : taskWithChildren.getChildTasks()) { - removeTask(child); - } - removeTask(task); - } else { - removeTask(task); - } - } - case KEEP, PARENT, BUILTIN -> { - // Maintained in active registry - } - } -} - -@Override -public void removeTask(Task task) { - if (task == null) return; - String id = task.getId(); - boolean removed = (tasks.remove(id) != null); - removed |= (pendingTasks.remove(task) != null); - - if (removed) { - logger.debug("Removing task: {}", id); - if (task instanceof PersistableTask persistableTask) { - taskService.unpersistTask(persistableTask); - } - } -} -``` - -### File Structure Changes -- **Modified**: `src/main/java/me/bobert/netneutrality/core/tasks/TaskManagerImpl.java` -- **Modified**: `src/main/java/me/bobert/netneutrality/scheduled/cleanup/TaskCleanup.java` -- **Added**: `src/test/java/me/bobert/netneutrality/core/tasks/TaskManagerImplTest.java` - -# Testing - -### Validation Approach -Automated unit tests will be created in `src/test/java/me/bobert/netneutrality/core/tasks/TaskManagerImplTest.java` utilizing JUnit 5, AssertJ, and Mockito to validate all functional requirements without requiring live Discord connections or database instances. - -### Key Scenarios -1. **Task Registration & Lookup**: - - Insert standard `Task`, verify `getTask(id)` and `hasTask(id)` return `true`. - - Insert duplicate task ID, verify rejected with user notification. -2. **Parent / Child Lifecycle (`REMOVE_SELF_AND_CHILDREN`)**: - - Create parent task implementing `WithChildren` and child task implementing `WithParent`. - - Execute child task returning `REMOVE_SELF_AND_CHILDREN`. - - Verify parent task and all sibling child tasks are removed from the registry. -3. **Persistable Task DB Synchronization**: - - Insert `PersistableTask`, verify `TaskService.persistTask` is called. - - Remove `PersistableTask` via lifecycle completion or `removeTask()`, verify `TaskService.unpersistTask` is invoked. -4. **Step Task Flow**: - - Insert `StepTask` into pending registry. - - Reassign ID via `activateTask(task, oldId)`, verify task moves from pending to active registry under new ID. -5. **Partial ID Search**: - - Query `findTaskByPartialId` and verify it matches correct instance without scanning beyond first match. -6. **Task Expiration & Cleanup**: - - Mock expired active and pending tasks, trigger `TaskCleanup.run()`, verify expired tasks are pruned from both collections. -7. **Concurrency & Locking**: - - Simulate concurrent executions for tasks sharing a `lockId`, confirming mutual exclusion. - -# Delivery Steps - -### ✓ Step 1: Fix Task Lifecycle Hierarchy and DB Persistence Synchronization -`TaskManagerImpl` correctly propagates parent-child lifecycle actions and synchronizes task deletions with `TaskService`. - -- Update `handleTaskLifecycle` in `TaskManagerImpl.java` to support `REMOVE_SELF_AND_CHILDREN` for tasks implementing `WithParent`, resolving the parent task and removing both the parent and all sibling child tasks. -- Modify `removeTask(Task task)` in `TaskManagerImpl.java` to invoke `taskService.unpersistTask(persistableTask)` when `task instanceof PersistableTask`, ensuring persisted records in the database are purged when tasks finish. -- Refactor `deleteInteraction(Task task)` in `TaskManagerImpl.java` to cleanly handle `Deletable` logic for both standalone and parented tasks, adding failure logging to `InteractionHook.deleteOriginal().queue()`. - -### ✓ Step 2: Refactor Pending Task Management and Concurrency Locking -Pending tasks and active task locks are managed safely without race conditions or memory leaks. - -- Refactor `pendingTasks` in `TaskManagerImpl.java` to use a `ConcurrentHashMap` keyed by task ID for O(1) lookups and consistent state management. -- Update `removeTask(Task task)`, `getTask(String id)`, and `getTasks()` in `TaskManagerImpl.java` to include pending tasks so they can be retrieved, removed, and cleaned up when expired. -- Update `TaskCleanup.java` to ensure expired pending tasks are identified and cleaned up. -- Improve lock management in `TaskManagerImpl.handle(TaskContext context)` so that lock eviction does not interfere with concurrently waiting threads acquiring new lock instances. - -### ✓ Step 3: Optimize Task Query Performance and Diagnostic Logging -Task lookup methods use short-circuiting stream pipelines, and diagnostic logging is corrected. - -- Optimize `findTaskByPartialId` in `TaskManagerImpl.java` to use `.findFirst()` directly rather than allocating a full list via `findTasksByPartialId`. -- Fix typos in log messages and warnings across `TaskManagerImpl.java` (e.g. `"Aquiring lock"`, `"childs"`). -- Ensure consistent ephemeral fallback messages and handling when unacknowledged interactions or missing tasks are encountered. - -### ✓ Step 4: Add Comprehensive Unit Tests for Task Management -A complete unit test suite validates `TaskManagerImpl` functionality across all task types, lifecycles, and concurrency scenarios. - -- Create `src/test/java/me/bobert/netneutrality/core/tasks/TaskManagerImplTest.java` using JUnit 5 and Mockito. -- Implement tests for standard task registration (`insert`), lookup (`getTask`, `findTaskByPartialId`), and removal (`removeTask`). -- Implement tests for parent-child tasks (`WithChildren`, `WithParent`), verifying lock inheritance and `REMOVE_SELF_AND_CHILDREN` cascading removals. -- Implement tests for `StepTask` lifecycle (pending insertion, ID reassignment via `activateTask`, and expiration). -- Implement tests for `PersistableTask` lifecycle, verifying automatic database persistence on insert and unpersisting on removal. -- Implement tests verifying thread synchronization and lock acquisition during task execution. \ No newline at end of file diff --git a/.junie/plans/jakarta-data-repositories.md b/.junie/plans/jakarta-data-repositories.md deleted file mode 100644 index 5bac8ab..0000000 --- a/.junie/plans/jakarta-data-repositories.md +++ /dev/null @@ -1,117 +0,0 @@ ---- -sessionId: session-260925-095225-4rh7 ---- - -# Requirements - -### Overview & Goals -Swap the hand-written HQL in `db/repository/*` for **Jakarta Data 1.0** repository interfaces. Hibernate's `hibernate-processor` generates the implementations at compile time. Queries become compile-time checked and the boilerplate goes away. The public repository API used by `App`, services and tests stays the same. - -### Scope -**In scope** -- Build setup: `jakarta.data-api` plus `hibernate-processor` as an annotation processor. -- Generated query interfaces for `OGuild`, `OGuildSetting`, `OTask`, `OBlacklist` and `OLogMessage`. -- The existing repository classes become thin transactional facades over the generated code. -- `AbstractRepository` is removed. - -**Out of scope** -- Moving to Spring or CDI. -- Schema or Flyway changes. -- Changing any service behaviour. - -### Functional Requirements -- Every current public method keeps the same signature and behaviour. This includes `findOrCreate`, `upsertOrDelete`, `upsert`, `enableWord`, `deleteWord`, `deleteByTaskId`, `findEnabledWords`, `findAll`, `findById`, `save` and `deleteById`. -- Each read-modify-write operation still runs in a single transaction. On error the transaction rolls back and the exception is rethrown. -- An invalid query must fail `mvn compile`. - -# Technical Design - -### Current Implementation -- `AbstractRepository` gives each repository `read`/`write`/`writeVoid` helpers over a stateful `Session`, using `SessionFactory.fromSession`/`fromTransaction`. -- The repositories build HQL inline (for example `TaskRepository.findByTaskId`) or use named queries (`settings_findByName`). -- They are wired in `App.java` (lines 122-124 and 138) as `new XRepository(sessions)`. Tests construct them the same way. - -### Key Decisions -1. **Generated interfaces run on a `StatelessSession`.** This is the Hibernate Data default: the generated `*_` class takes a `StatelessSession` in its constructor. There's no container, so transactions stay our responsibility. -2. **The facade pattern keeps the public API.** Classes like `GuildSettingRepository` stay as the public API. Each call opens a stateless transaction and creates the generated DAO. `App`, the services and the tests don't change. -3. **No dirty checking, so writes are explicit.** Code that changes a loaded entity must call `@Update`. New rows are written with `@Insert`, and removals use `@Delete` or a `@Query("delete ...")`. `session.getReference(OGuild…)` becomes a detached `OGuild` with only its id set, which is enough for the foreign key on insert. - -### Proposed Changes -- `db/TransactionRunner.java` (new) replaces `AbstractRepository`. It exposes ` R inTx(Function)` using `SessionFactory.fromStatelessTransaction`, and `read(...)` using `fromStatelessSession`. -- `db/data/*Queries.java` (new) holds the `@Repository` interfaces, for example: -```java -@Repository -public interface TaskQueries extends CrudRepository { - @Find Optional byTaskId(String taskId); - @Query("delete from OTask where taskId = :taskId") int deleteByTaskId(String taskId); -} -@Repository -public interface GuildSettingQueries extends CrudRepository { - @Query("where guild.guildId = :guildId") List byGuild(String guildId); - @Query("where guild.guildId = :id and name = :name") List byName(String id, String name); -} -``` -- Repository facades, for example `upsertOrDelete`: -```java -tx.inTx(ss -> { var q = new GuildSettingQueries_(ss); var ex = q.byName(id, s.getName()); - if (reset) { q.deleteAll(ex); return null; } - if (ex.isEmpty()) q.insert(newSetting(id, s, value)); else { ex.forEach(e -> e.setValue(value)); q.updateAll(ex); } - return null; }); -``` -- `BlacklistQueries` covers `byGuild` (ordered by id), `enabledWords` (a `select o.word` projection) and `findWord` (case-insensitive `upper()`). -- `MessageQueries` holds the same kind of migration for `MessageRepository`. -- `pom.xml` gets: - - `jakarta.data:jakarta.data-api:1.0.1` - - `maven-compiler-plugin` with `annotationProcessorPaths` pointing to `org.hibernate.orm:hibernate-processor:7.1.9.Final` - - The shade plugin keeps working unchanged. - -### Risks -- **Lazy associations:** stateless sessions don't lazy-load outside the session. The facades must return fully initialised data, which the current code already assumes since its sessions are closed anyway. -- **`save()` semantics:** `merge` becomes `upsert`, available on `CrudRepository`. -- **Named query `settings_findByName`:** it's replaced by `@Query`. The `@NamedQuery` on `OGuildSetting` can be removed. - -# Testing - -### Validation Approach -Run the existing suites unchanged with `./mvnw test`: `AbstractRepositoryTest`, `*RepositoryTest`, `GuildServiceTest` and `BlacklistServiceTest`. They cover the public repository behaviour. - -### Key Scenarios -- `upsertOrDelete`: insert, update and reset-to-default delete. -- `TaskRepository.upsert` followed by `deleteByTaskId`. -- `enableWord` returns `CREATED`, then `UNCHANGED`, and `ENABLED` after the word has been disabled. -- `findOrCreate` is idempotent. - -### Edge Cases -- A failing write rolls back. -- An HQL typo breaks `mvn compile`, which confirms the processor is active. - -### Test Changes -- Rename `AbstractRepositoryTest` to `TransactionRunnerTest`, or rework it so it no longer depends on the removed base class. - -# Delivery Steps - -### ✓ Step 1: Add Jakarta Data and hibernate-processor to the build -The project compiles with the annotation processor generating the metamodel and repository classes. -- Add the `jakarta.data-api` dependency to `pom.xml`. -- Configure `maven-compiler-plugin` with `annotationProcessorPaths` pointing to `hibernate-processor` 7.1.9.Final. -- Add `db/TransactionRunner` with stateless read and transaction helpers. -- Check that `./mvnw compile` produces classes like `OGuild_`. - -### ✓ Step 2: Migrate Guild and Task repositories -`GuildRepository` and `TaskRepository` delegate to the generated `GuildQueries_` and `TaskQueries_`. -- Create `db/data/GuildQueries` and `db/data/TaskQueries`. -- Rewrite `findOrCreate`, `upsert`, `findByTaskId` and `deleteByTaskId` using explicit `@Insert`/`@Update`. -- Run `GuildServiceTest` and `TaskRepositoryTest`. - -### ✓ Step 3: Migrate GuildSetting, Blacklist and Message repositories -All remaining repositories use the generated queries, with transactional logic kept in the facades. -- Create `GuildSettingQueries`, `BlacklistQueries` and `MessageQueries`. -- Port `upsertOrDelete`, `enableWord` and `deleteWord` to explicit writes. -- Remove the `settings_findByName` named query. -- Run the matching repository and service tests. - -### ✓ Step 4: Remove AbstractRepository and finish -`AbstractRepository` is gone and the full test suite passes. -- Delete `db/AbstractRepository.java` and move the generic `findAll`/`findById`/`save`/`deleteById` onto `CrudRepository`. -- Rework `AbstractRepositoryTest` into `TransactionRunnerTest`. -- Run `./mvnw verify`, including checkstyle and the shaded jar. \ No newline at end of file diff --git a/.junie/plans/javalin-webapi-refactor.md b/.junie/plans/javalin-webapi-refactor.md deleted file mode 100644 index 18849e4..0000000 --- a/.junie/plans/javalin-webapi-refactor.md +++ /dev/null @@ -1,188 +0,0 @@ ---- -sessionId: session-260923-111902-5uwc ---- - -# Requirements - -### Overview & Goals -Replace the hand-rolled `com.sun.net.httpserver` stack in `me.bobert.netneutrality.http.server` with Javalin, so the bot's embedded API needs far fewer custom classes and gains correct status codes, JSON handling and error responses. Additionally simplify the outbound `WebModule` HTTP client. - -### Scope -**In scope** -- `WebApi` becomes a thin Javalin-based `Lifecycle` component (same priority 70, same `NETNEUTRALITY_API_PORT` setting). -- Delete `AbstractHandler`, `RootHandler`, `ActionHandler`, `GuildHandler`; their logic moves into small route-registration units. -- Standardise the API layer on **Gson** — remove `org.json` (`JSONObject`/`JSONArray`) usage from the HTTP server package. -- Correct HTTP semantics: 404 for unknown guilds/paths, 405 for wrong methods, 500 on exceptions, UTF-8 safe bodies. -- Simplify `WebModule` (outbound OkHttp wrapper): extract the static cache, remove duplicated/unused members. - -**Out of scope** -- Changing the public URL shape of existing endpoints (`/`, `/actions/*`, `/guilds/*`) — paths and payload keys stay backwards compatible. -- Authentication/authorisation for the API. -- Migrating `org.json` usage elsewhere in the project (e.g. task classes using `WebModule.getAsObject()`). - -### Functional Requirements -- `GET /` → HTML liveness page (unchanged text). -- `GET /actions/status` → `{"health":...,"ping":...}`. -- `POST /actions/shutdown` → runs the shutdown hook and exits (accept `GET` too for compatibility if desired). -- `GET /actions/settings/current/{guildId}` → `{"status":"ok","settings":{...}}`, or 404 + `{"status":"error"}` for an unknown guild. -- `POST /actions/settings/reload/{guildId}` → reloads the guild setting cache, `{"status":"true"}`; 404 for an unknown guild. -- `GET /guilds/owner` → array of `{guild_id, owner_id}`. -- `GET /guilds/channels/{guildId}` and `GET /guilds/roles/{guildId}` → arrays of id/name objects; 404 for an unknown guild. -- Requests arriving before JDA is ready must return 503 rather than throwing an NPE. - -### Non-Functional Requirements -- Startup/shutdown stays deterministic through `ApplicationContext` — `stop()` must release the port. -- Shaded jar must still run (`maven-shade-plugin`); Javalin/Jetty service files need merging. -- No behavioural regression for existing consumers of the endpoints. - -# Technical Design - -### Current Implementation -- `WebApi.java` (92 lines) implements `Lifecycle` (priority 70), creates an `HttpServer`, registers three contexts and a fixed pool built from `BaseThreadFactory("web-api")`. Constructed in `App.java:154`. -- `AbstractHandler` offers `sendStringResponse`/`sendJsonResponse`; both hardcode status 200 and use `str.length()` instead of byte length (breaks non-ASCII). -- `ActionHandler` / `GuildHandler` route via `if (path.startsWith(...))` chains and parse ids with `getRequestURI().toString().split("/")`; they mix Gson and `org.json`, never check the HTTP method, and call `jda.getGuildById()` twice in places. -- `WebModule` is an unrelated *outbound* OkHttp wrapper with a builder, a static LRU cache and a `CachedResponse` record; it builds a brand-new `OkHttpClient` per request and has three redundant timeout setters plus unused `elapsedTime`/`errorMessage` accessors. - -### Key Decisions -1. **Javalin** (6.x, JDK 17+ / Java 25 compatible) as the embedded server — removes routing, method matching, error mapping and content-type handling code. -2. **Gson only** in the HTTP layer: register a `GsonJsonMapper` on the Javalin config so `ctx.json(obj)` works with plain records/maps; `org.json` disappears from `http/server`. -3. **Route groups instead of handler classes**: `ActionRoutes` and `GuildRoutes` expose a single `static void register(Javalin app, deps...)`, so `AbstractHandler`, `RootHandler`, `ActionHandler`, `GuildHandler` are all deleted (4 classes → 2 tiny registrars). -4. **Guild lookup guard**: one private helper resolves `{guildId}` → `Guild` or throws `NotFoundResponse`, replacing repeated null checks. -5. **WebModule**: keep the builder API used by callers, but move the cache into a package-private `ResponseCache`, share a single static `OkHttpClient` and derive per-request timeouts with `newBuilder()`. - -### Proposed Changes -**`http/server/WebApi.java`** -```java -public class WebApi implements Lifecycle { - private Javalin app; - @Override public int priority() { return 70; } - @Override public void start() { - int port = Integer.parseInt(settings.get("NETNEUTRALITY_API_PORT", "9000")); - app = Javalin.create(cfg -> cfg.jsonMapper(new GsonJsonMapper())); - app.get("/", ctx -> ctx.html(LIVENESS_HTML)); - ActionRoutes.register(app, jdaSupplier, guildServices, shutdownHook); - GuildRoutes.register(app, jdaSupplier); - app.exception(Exception.class, (e, ctx) -> { log.error(...); ctx.status(500).json(Map.of("status","error")); }); - app.start(port); - } - @Override public void stop() { if (app != null) { app.stop(); app = null; } } -} -``` - -**`http/server/GuildRoutes.java`** — `/guilds/owner`, `/guilds/channels/{guildId}`, `/guilds/roles/{guildId}` mapped to `List>` and returned via `ctx.json(...)`. - -**`http/server/ActionRoutes.java`** — `/actions/status`, `/actions/shutdown`, `/actions/settings/current/{guildId}`, `/actions/settings/reload/{guildId}`. - -**`http/server/GsonJsonMapper.java`** — ~15 lines implementing Javalin's `JsonMapper` over a shared `Gson`. - -**`http/WebModule.java`** — shared client, cache extracted, redundant setters/accessors dropped; public builder methods used by callers remain. - -### Data Models / Contracts -Small records replace ad-hoc JSON building, serialized by Gson with the existing key names: -```java -record GuildOwner(String guild_id, String owner_id) {} -record NamedEntity(String channel_id, String channel_name) {} -record StatusResponse(String health, String ping) {} -``` - -### File Structure -``` -src/main/java/me/bobert/netneutrality/http/ - WebModule.java (modified) - server/WebApi.java (rewritten, Javalin) - server/GsonJsonMapper.java (new) - server/ActionRoutes.java (new) - server/GuildRoutes.java (new) - server/AbstractHandler.java (deleted) - server/RootHandler.java (deleted) - server/ActionHandler.java (deleted) - server/GuildHandler.java (deleted) -pom.xml (+ io.javalin:javalin, shade transformer) -``` - -### Architecture Diagram -```mermaid -graph LR -App-->ApplicationContext -ApplicationContext-->WebApi -WebApi-->Javalin -WebApi-->GsonJsonMapper -Javalin-->ActionRoutes -Javalin-->GuildRoutes -ActionRoutes-->GuildServiceProvider -GuildRoutes-->JDA -``` - -### Risks -- **Shaded jar**: Jetty/Javalin rely on `META-INF/services`; the shade plugin needs `ServicesResourceTransformer`, otherwise startup fails only in the packaged build. -- **Jar size / SLF4J binding**: Jetty logs through SLF4J; project already has log4j + slf4j — check for duplicate bindings. -- **JDA not ready**: `jdaSupplier.get()` may be null; guard with a 503 mapping. -- `System.exit(0)` in the shutdown route must run after the response is flushed. - -# Testing - -### Validation Approach -Build with `./mvnw -q verify` (checkstyle included), then start the app/`WebApi` on an ephemeral port and exercise every endpoint with `curl`, comparing payload shape against the pre-refactor behaviour. - -### Key Scenarios -- `GET /` returns 200 and the HTML liveness text. -- `GET /actions/status` returns 200 `application/json` with `health` and `ping`. -- `GET /guilds/owner` returns a JSON array with `guild_id`/`owner_id` keys. -- `GET /guilds/channels/{id}` and `/guilds/roles/{id}` return arrays with `channel_id`/`channel_name`. -- `GET /actions/settings/current/{id}` returns `{"status":"ok","settings":{...}}`. - -### Edge Cases -- Unknown guild id → 404 with `{"status":"error"}` (previously 200). -- Unknown path → 404 (previously echoed the path with 200). -- Wrong method on a route → 405. -- Non-ASCII content in settings values round-trips correctly (old `str.length()` bug). -- `Lifecycle.stop()` frees the port — a second `start()` on the same port succeeds. - -### Test Changes -- Add a lightweight JUnit test starting `WebApi` on port 0 with a mocked `Supplier` and `GuildServiceProvider`, hitting the routes with Javalin's test client or `java.net.http.HttpClient`. -- Add a `WebModule` test covering cache hit/expiry after the cache extraction. - -# Delivery Steps - -### ✓ Step 1: Add Javalin dependency and Gson JSON mapper -The project builds with Javalin available and a Gson-backed JSON mapper ready for use. - -- Add `io.javalin:javalin` (6.x) to `pom.xml` dependencies. -- Add `ServicesResourceTransformer` to the `maven-shade-plugin` configuration so Jetty/Javalin service files survive shading. -- Create `http/server/GsonJsonMapper.java` implementing Javalin's `JsonMapper` over a shared `Gson` instance. -- Verify `./mvnw -q package` succeeds and the shaded jar still starts. - -### ✓ Step 2: Rewrite WebApi as a Javalin Lifecycle component -`WebApi` starts and stops a Javalin server while keeping its Lifecycle contract and the root endpoint. - -- Replace the `HttpServer`/`ExecutorService` fields with a `Javalin app` field. -- Build the instance in `start()` with the `GsonJsonMapper`, reading `NETNEUTRALITY_API_PORT` from `SettingsProvider` as today. -- Register `GET /` returning the liveness HTML and delete `RootHandler.java`. -- Add global `exception` and `error` handlers returning 500/404 JSON, plus a 503 guard when `jdaSupplier.get()` is null. -- Implement `stop()` via `app.stop()`; keep `priority() == 70` and the `App.java` construction site unchanged. - -### ✓ Step 3: Migrate guild endpoints to GuildRoutes -All `/guilds/*` endpoints are served by Javalin route lambdas with proper status codes and Gson serialization. - -- Create `http/server/GuildRoutes.register(Javalin, Supplier)`. -- Map `GET /guilds/owner`, `GET /guilds/channels/{guildId}`, `GET /guilds/roles/{guildId}` using path params instead of `split("/")`. -- Return `List` payloads via `ctx.json(...)`, preserving the existing `guild_id`/`owner_id`/`channel_id`/`channel_name` keys. -- Resolve guilds through a shared helper that throws `NotFoundResponse` for unknown ids (single `getGuildById` call). -- Delete `GuildHandler.java`. - -### ✓ Step 4: Migrate action endpoints to ActionRoutes -All `/actions/*` endpoints run on Javalin, with `org.json` fully removed from the HTTP server package. - -- Create `http/server/ActionRoutes.register(Javalin, Supplier, GuildServiceProvider, Runnable)`. -- Map `/actions/status`, `/actions/shutdown`, `/actions/settings/current/{guildId}`, `/actions/settings/reload/{guildId}`. -- Replace `JSONObject`/`JSONArray` with Gson-serialized maps/records keeping the same response keys. -- Ensure the shutdown route flushes its response before running the hook and exiting. -- Delete `ActionHandler.java` and `AbstractHandler.java`, and add endpoint tests starting `WebApi` on port 0 with mocked JDA/guild services. - -### ✓ Step 5: Simplify the outbound WebModule client -`WebModule` keeps its builder API but sheds duplicated state and its inlined cache. - -- Extract the static LRU cache and `CachedResponse` into a package-private `ResponseCache` class in `http/`. -- Share a single static `OkHttpClient` and derive per-request timeouts with `newBuilder()` instead of constructing a client per build. -- Collapse the redundant `timeout`/`connectTimeout`/`readTimeout`/`writeTimeout` setters and remove unused accessors (`getErrorMessage`, `elapsedTime` exposure) after checking call sites. -- Add a unit test covering cache hit, expiry and POST bypass; verify existing `WebModule` callers still compile. \ No newline at end of file diff --git a/.junie/plans/migrate-music-to-lavalink.md b/.junie/plans/migrate-music-to-lavalink.md deleted file mode 100644 index c02164c..0000000 --- a/.junie/plans/migrate-music-to-lavalink.md +++ /dev/null @@ -1,249 +0,0 @@ ---- -sessionId: session-260923-114918-ce7w ---- - -# Requirements - -### Overview & Goals - -Replace the in-process **Lavaplayer** audio stack with a remote **Lavalink v4** node driven by the official `dev.arbjerg:lavalink-client`. Audio decoding, source resolution and voice sending move out of the bot process to the Lavalink node, which can then be extended with server-side plugins (YouTube source, LavaSrc, etc.) without touching the bot. - -All dependency/plugin examples from the Lavalink docs are Gradle-based and will be **translated to Maven** (`pom.xml` `` / ``); the Lavalink node itself is configured via a YAML file mounted into the container, not via any build script. - -### Scope - -**In Scope** -- Swap `dev.arbjerg:lavaplayer` + `dev.lavalink.youtube:v2` for `dev.arbjerg:lavalink-client` in `pom.xml`. -- Rewrite the `me.bobert.netneutrality.music` package against Lavalink: `MusicServiceImpl`, `MusicManager`, `TrackScheduler`, `QueueManager`, `TrackInfo`, `TrackLoadingHandler`, `StuckTrackRecoveryHandler`; delete `AudioPlayerSendHandler`. -- Route voice connections through Lavalink's `JDAVoiceUpdateListener` / `DirectAudioController` instead of `Guild#getAudioManager()`. -- Add a `lavalink` service plus `lavalink/application.yml` to `docker-compose.yml`, wired to the existing `ejs-api` (yt-cipher) container. -- Update `MessageFunction`, `LoadPlaylistTask`, `LoadTrackFromPlaylistTask` and the music command classes to the new track type. -- Update the affected tests and the docs under `docs/`. - -**Out of Scope** -- Porting `music/source/UdioAudioSourceManager` to a Lavalink server plugin — captured as a documented follow-up. Udio playback is temporarily unavailable; the existing code stays in the repo as the porting reference. -- Any change to command UX, queue semantics, DJ rules or vote-skip behaviour. -- Audio filters/equalizer, node load balancing across multiple nodes, and session resuming (Lavalink supports these; not enabled here). - -### User Stories -- As a bot operator, I want audio to be served by a separate Lavalink node so that I can add source plugins and restart the bot without interrupting the audio pipeline's configuration. -- As a user, I want `/play`, `/skip`, `/queue`, `/volume`, `/stop`, `/resume`, `/shuffle` and vote-skip to behave exactly as before the migration. -- As a maintainer, I want YouTube cipher/poToken configuration to live in the node's `application.yml` rather than in Java code. - -### Functional Requirements -- All 13 commands in `command/music/` keep their current signatures, replies and `CommandExecutionResult` semantics. -- `SourceFinder` keeps classifying URLs and prefixing searches with `ytsearch:`; identifiers are passed to Lavalink's `loadItem` unchanged. -- Queue ordering, duplicate rejection, auto-start on first enqueue, auto-disconnect on empty queue and the two-strike stuck-track recovery are preserved. -- If the Lavalink node is unreachable at startup, the bot still starts; music commands reply with a clear error instead of crashing. -- Udio URLs reply with a "temporarily unsupported" message rather than failing silently. - -### Non-Functional Requirements -- The bot no longer ships native audio encoders: the `jdave-api` / `jdave-native-*` dependencies and the `AudioModuleConfig` DAVE wiring in `App.initJDA()` are removed, since DAVE is handled by the node (Lavalink ≥ 4.2.0). -- New settings are read through the existing `SettingsProvider` and documented in `bot.properties.example`. -- Checkstyle (`checkstyle.xml`, bound to the `test` phase) must keep passing. - -# Technical Design - -### Current Implementation - -- `MusicServiceImpl` (priority-40 `Lifecycle`, registered in `App.buildContext()` line 140) owns a `DefaultAudioPlayerManager` and registers YouTube (with `setRemoteCipher(...)`), SoundCloud, Udio and HTTP source managers in `start()`; `stop()` clears queues and calls `player.destroy()`. -- `MusicManager` creates an `AudioPlayer` per guild, sets `AudioPlayerSendHandler` on `guild.getAudioManager()`, opens/closes the voice connection itself, and calls `playerManager.loadItemOrdered(this, identifier, new TrackLoadingHandler(...))`. -- `TrackScheduler extends AudioEventAdapter` and receives `onTrackStart/End/Exception/Stuck` callbacks directly from the player. -- `QueueManager` holds a `Deque` and drives playback with `player.startTrack(track.makeClone(), false)` / `player.stopTrack()`. -- `StuckTrackRecoveryHandler` relies on `AudioTrack#makeClone`, `getPosition()` and `setPosition()`. -- `com.sedmelluq.*` types appear in 14 main sources and 5 tests (`QueueManagerTest`, `TrackInfoTest`, `TrackSchedulerTest`, `VoteSkipServiceTest`, `UdioAudioSourceManagerTest`). -- `pom.xml` already declares the `https://maven.lavalink.dev/releases` repository (lines 136–140). -- `docker-compose.yml` runs `db`, `bot` and `ejs_signature_api` (yt-cipher) on the `netneutrality` network; there is no Lavalink node. - -### Key Decisions - -1. **Official client** — `dev.arbjerg:lavalink-client` (v4-only). `VoiceDispatchInterceptor` is verified present in the bundled JDA 6.4.1 jar, so `JDAVoiceUpdateListener` attaches cleanly. -2. **Central event dispatcher** — `MusicServiceImpl` holds the single subscription to the client's event flux and routes each event to the owning guild's `TrackScheduler`. One `Disposable` to dispose in `stop()`. -3. **Synchronous facade** — `MusicManager` subscribes to the returned `Mono`s internally; command classes keep their current synchronous signatures. Failures are reported via `LogHandler` and the attached `MusicChannelContext`. -4. **Lavalink `Track` everywhere** — `dev.arbjerg.lavalink.protocol.v4.Track` replaces `AudioTrack` in `TrackInfo`, `QueueManager`, `MessageFunction` and the playlist tasks. `Track` is an immutable data class, so `makeClone()` disappears and `setPosition` becomes `player.setPosition(...)` on the Lavalink player. -5. **Node in compose** — a `lavalink` service with a mounted `application.yml`; YouTube cipher/poToken config moves out of `MusicServiceImpl.start()` into that YAML. -6. **Udio deferred** — `music/source/**` is excluded from compilation-relevant wiring and the `Udio` branch of `SourceFinder.getIdentifier` returns `null` with a friendly reply; the classes remain for the future plugin port. - -### Proposed Changes - -**Maven (`pom.xml`)** — translated from the docs' Gradle snippets: -```xml - - dev.arbjerg - lavalink-client - 3.4.0 - -``` -resolved from the already-present `arbjergDev-releases` repository. Removed: `dev.arbjerg:lavaplayer`, `dev.lavalink.youtube:v2`, `club.minnced:jdave-*`. - -**`MusicServiceImpl`** -- Field `LavalinkClient client = new LavalinkClient(Helpers.getUserIdFromToken(token))`. -- `start()` registers one node (`client.addNode(...)` with URI, password and name from settings) and opens the dispatcher subscription: - `client.on(TrackStartEvent.class)`, `TrackEndEvent`, `TrackExceptionEvent`, `TrackStuckEvent`, `WebSocketClosedEvent` → look up `managers.get(event.getGuildId())` → forward to its `TrackScheduler`. -- `stop()` disposes the subscriptions, clears queues, destroys players and closes the client. -- Exposes `LavalinkClient getClient()` in place of `getPlayerManager()`. - -**`App`** -- `initJDA()` adds `.setVoiceDispatchInterceptor(new JDAVoiceUpdateListener(client))` and drops `.setAudioModuleConfig(new AudioModuleConfig().withDaveSessionFactory(new JDaveSessionFactory()))`. -- Ordering: the `LavalinkClient` must exist before JDA is built, which the existing priorities already guarantee (music = 40, JDA = 60). -- Dev log levels switch from `com.sedmelluq.discord` / `dev.lavalink.youtube` to `dev.arbjerg.lavalink`. - -**`MusicManager`** -- Holds `LavalinkClient client` and resolves `Link link = client.getOrCreateLink(guildId)`. -- `connect(AudioChannel)` → `jda.getDirectAudioController().connect(channel)` + `setSelfDeafened`; `disconnect()` → `getDirectAudioController().disconnect(guild)` and `link.destroyPlayer()`. -- `isConnected()` / `isNotConnectedTo(...)` read `guild.getSelfMember().getVoiceState().getChannel()` — **`Guild#getAudioManager()` must not be used** with Lavalink. -- `play(...)` → `link.loadItem(identifier).subscribe(new TrackLoadingHandler(...))`. -- `getCurrentlyPlaying()` returns the cached `Track` from the last `TrackStartEvent` (the REST player state is not synchronously readable). -- `setVolume` / `stopTrack` → `link.getPlayer().flatMap(p -> p.setVolume(v).setEncodedTrack(null))` subscribed internally. - -**`TrackScheduler`** -- No longer extends `AudioEventAdapter`; exposes `onTrackStart(Track)`, `onTrackEnd(Track, AudioTrackEndReason)`, `onTrackException`, `onTrackStuck(Track, long)` called by the dispatcher. -- The existing `endReason` branching maps 1:1 onto Lavalink's `AudioTrackEndReason` (`FINISHED`, `LOAD_FAILED`, `STOPPED`, `REPLACED`, `CLEANUP`). - -**`QueueManager`** -- `player.getPlayingTrack() == null` becomes a `volatile Track currentTrack` maintained by scheduler events (avoids a blocking REST call inside the `synchronized (queue)` blocks). -- `startTrack(track, noInterrupt)` becomes `link.createOrUpdatePlayer().setTrack(track).setNoReplace(...)`; `makeClone()` calls are dropped since `Track` is immutable. - -**`TrackLoadingHandler`** — implements lavalink-client's `AbstractAudioLoadResultHandler`: `ontrackLoaded` / `onPlaylistLoaded` / `onSearchResultLoaded` / `noMatches` / `loadFailed(LoadFailed)`. `onSearchResultLoaded` takes the first result, matching today's `ytsearch:` behaviour. - -**`StuckTrackRecoveryHandler`** — re-queues the same immutable `Track` and restores position via `player.setPosition(lastPosition)`; the second consecutive strike still skips. - -### Data Models / Contracts -```java -// MusicManager (synchronous facade, unchanged call sites) -public synchronized void play(String query, CommandContext ctx, boolean silent); -public synchronized void stopTrack(); -public synchronized void setVolume(int volume); -public synchronized Track getCurrentlyPlaying(); - -// TrackScheduler (invoked by the dispatcher) -void onTrackStart(Track track); -void onTrackEnd(Track track, AudioTrackEndReason endReason); -void onTrackStuck(Track track, long thresholdMs); - -// TrackInfo -public record TrackInfo(Track track, Member requester) { } -``` - -New settings (read via `SettingsProvider`, documented in `bot.properties.example`): -`NETNEUTRALITY_LAVALINK_URI` (default `ws://lavalink:2333`), `NETNEUTRALITY_LAVALINK_PASSWORD`, `NETNEUTRALITY_LAVALINK_NAME`. - -### File Structure - -| Path | Change | -|---|---| -| `pom.xml` | swap lavaplayer/youtube-v2/jdave → `lavalink-client` | -| `docker-compose.yml` | add `lavalink` service | -| `lavalink/application.yml` | **new** — node + youtube-source plugin + yt-cipher config | -| `bot.properties.example` | new Lavalink settings | -| `music/MusicServiceImpl.java` | client lifecycle + event dispatcher | -| `music/MusicManager.java` | `Link`/`DirectAudioController` based | -| `music/TrackScheduler.java` | plain event sink | -| `music/QueueManager.java`, `TrackInfo.java`, `TrackLoadingHandler.java`, `StuckTrackRecoveryHandler.java` | `Track` type + REST player calls | -| `music/AudioPlayerSendHandler.java` | **deleted** | -| `music/SourceFinder.java` | Udio → unsupported | -| `App.java` | voice interceptor, drop DAVE wiring, log levels | -| `shared/util/MessageFunction.java`, `tasks/music/LoadPlaylistTask.java`, `tasks/music/LoadTrackFromPlaylistTask.java` | `Track` type | -| `src/test/.../music/*` | updated fixtures | -| `docs/music.md`, `docs/architecture.md`, `docs/README.md` | rewritten sections | - -### Architecture Diagram -```mermaid -graph LR -JDA[JDA gateway] -->|voice state| VIL[JDAVoiceUpdateListener] -VIL --> C[LavalinkClient] -C -->|REST and WS| N[Lavalink node] -N --> V[Discord voice] -CMD[music commands] --> MM[MusicManager] -MM -->|Link ops| C -MM --> QM[QueueManager] -C -->|event flux| DISP[MusicServiceImpl dispatcher] -DISP --> TS[TrackScheduler] -TS --> QM -``` - -### Risks -- **Voice API confusion**: any remaining `guild.getAudioManager()` call silently breaks playback; all call sites (`MusicManager`, `MusicUtils`, `BotState`) must be swept. -- **Blocking on reactor**: subscribing inside `synchronized` blocks in `QueueManager` risks deadlock — hence the cached `currentTrack` instead of a REST read. -- **Node availability**: `MusicServiceImpl.start()` must not throw when the node is down, or `App.startup()` exits with code 1. -- **Feature regressions**: Udio support pauses; direct-MP3 and SoundCloud must be re-verified against the node's enabled sources. -- **DAVE**: removing JDave requires Lavalink ≥ 4.2.0 — the compose image must be pinned accordingly. - -# Testing - -### Validation Approach -- `./mvnw -q verify` must compile and pass Checkstyle plus the updated unit tests. -- Unit tests keep using mocks: `QueueManagerTest` and `TrackSchedulerTest` mock the Lavalink `Link`/`LavalinkPlayer` and assert on the interactions instead of on `AudioPlayer`. -- A grep sweep confirms no `com.sedmelluq` / `dev.lavalink.youtube` import survives outside `music/source/**`. -- Manual smoke run with `docker compose up` to confirm the bot reaches the node and plays a YouTube URL. - -### Key Scenarios -- Enqueue on an empty queue starts playback immediately; enqueue while playing appends. -- `TrackEndEvent(FINISHED)` advances to the next track; an empty queue triggers `onQueueEmpty()` → disconnect. -- `skip(n)`, `shuffle`, `clearQueue`, `removeTracksFromUser` and `removeFromQueue(index)` return the same `CommandExecutionResult` messages as before. -- `ytsearch:` queries resolve through `onSearchResultLoaded` and queue the first hit. -- Volume from `GSettings.MUSIC_VOLUME` is applied on connect, with the 50 fallback on a malformed value. - -### Edge Cases -- Lavalink node unreachable at startup → bot boots, music commands reply with an error. -- Node disconnects mid-playback → `WebSocketClosedEvent` is logged and the guild state is cleaned up. -- Stuck track: first event re-queues and seeks, second consecutive event skips. -- Udio URL → explicit "temporarily unsupported" reply. -- Duplicate track enqueue is still rejected by `TrackInfo` equality. - -### Test Changes -- Update `QueueManagerTest`, `TrackSchedulerTest`, `TrackInfoTest`, `VoteSkipServiceTest` to the Lavalink `Track` type and mocked players. -- Keep `UdioAudioSourceManagerTest` as-is while the source classes remain in the repo for the future plugin port. - -# Delivery Steps - -### ✓ Step 1: Swap Maven dependencies and stand up a Lavalink node -The project builds against `lavalink-client` and `docker compose up` starts a reachable Lavalink v4 node. - -- Replace `dev.arbjerg:lavaplayer`, `dev.lavalink.youtube:v2` and the four `club.minnced:jdave-*` entries in `pom.xml` with `dev.arbjerg:lavalink-client` (Gradle docs snippet translated to Maven, using the existing `arbjergDev-releases` repository). -- Add a `lavalink` service to `docker-compose.yml` on the `netneutrality` network, pinned to an image with DAVE support (≥ 4.2.0), with the bot depending on it. -- Create `lavalink/application.yml` mounted into the node: server port/password, the youtube-source plugin, and the remote cipher pointed at the existing `ejs-api` container plus poToken/visitorData. -- Add `NETNEUTRALITY_LAVALINK_URI`, `_PASSWORD` and `_NAME` to `bot.properties.example`. - -### ✓ Step 2: Wire LavalinkClient into the bootstrap and JDA voice handling -The bot connects to the node on startup and routes Discord voice through Lavalink instead of JDA's audio manager. - -- Rework `MusicServiceImpl` to own a `LavalinkClient`, registering the configured node in `start()` without throwing when it is unreachable. -- Dispose the client and its subscriptions in `stop()`, keeping the priority-40 `Lifecycle` contract used by `App.buildContext()`. -- Add `.setVoiceDispatchInterceptor(new JDAVoiceUpdateListener(client))` in `App.initJDA()` and remove the `AudioModuleConfig`/`JDaveSessionFactory` wiring. -- Update the dev-mode log levels in `App.main` from `com.sedmelluq.discord`/`dev.lavalink.youtube` to `dev.arbjerg.lavalink`. -- Replace `getPlayerManager()` with `getClient()` and fix `BotState`'s usage. - -### ✓ Step 3: Rebuild MusicManager on Links and the DirectAudioController -Per-guild connect, disconnect and player control go through Lavalink while command signatures stay unchanged. - -- Replace the `AudioPlayer` field in `MusicManager` with a `Link` obtained from `client.getOrCreateLink(guildId)`. -- Implement `connect`/`disconnect` via `jda.getDirectAudioController()` and `link.destroyPlayer()`, deleting the `guild.getAudioManager()` calls and the `AudioPlayerSendHandler` class. -- Base `isConnected()` and `isNotConnectedTo(...)` on `getSelfMember().getVoiceState().getChannel()`, and update `MusicUtils.connect`/`checkIfConnected` accordingly. -- Keep the synchronous facade: `stopTrack`, `setVolume` and `play` subscribe to the returned `Mono`s internally and report failures through `LogHandler`. -- Apply the `GSettings.MUSIC_VOLUME` base volume on connect with the existing default-50 fallback. - -### ✓ Step 4: Migrate queue, track loading and message rendering to the Lavalink Track type -Tracks are represented by `dev.arbjerg.lavalink.protocol.v4.Track` end to end and loading uses Lavalink's REST resolver. - -- Change `TrackInfo` to wrap `Track` + `Member`, preserving equality-based duplicate rejection. -- Update `QueueManager` to drive playback with `createOrUpdatePlayer().setTrack(...)`/`setNoReplace(...)`, dropping `makeClone()` and replacing `player.getPlayingTrack()` checks with a scheduler-maintained `volatile Track currentTrack`. -- Reimplement `TrackLoadingHandler` on `AbstractAudioLoadResultHandler`, mapping `onSearchResultLoaded` to the first hit to preserve the `ytsearch:` behaviour. -- Update `MessageFunction`, `LoadPlaylistTask` and `LoadTrackFromPlaylistTask` to the new track type. -- Make `SourceFinder` return no identifier for Udio URLs and have `MusicManager.play` reply that Udio is temporarily unsupported. - -### ✓ Step 5: Route Lavalink events to TrackScheduler and restore stuck-track recovery -Track lifecycle events from the node drive queue advancement, error reporting and stuck recovery exactly as before. - -- Add a central dispatcher in `MusicServiceImpl` subscribing once to `TrackStartEvent`, `TrackEndEvent`, `TrackExceptionEvent`, `TrackStuckEvent` and `WebSocketClosedEvent`, routing by guild id to the cached `MusicManager`. -- Convert `TrackScheduler` from an `AudioEventAdapter` subclass into a plain sink with `onTrackStart`/`onTrackEnd`/`onTrackException`/`onTrackStuck`, keeping the existing `FINISHED`/`LOAD_FAILED`/`STOPPED` branching and `MusicChannelContext` attachment. -- Rework `StuckTrackRecoveryHandler` to re-queue the immutable `Track` and seek with `player.setPosition(...)`, retaining the retry-then-skip rule. -- Update `QueueManagerTest`, `TrackSchedulerTest`, `TrackInfoTest` and `VoteSkipServiceTest` to mock `Link`/`LavalinkPlayer`, and verify `./mvnw verify` passes Checkstyle and the suite. - -### ✓ Step 6: Update documentation and record the Udio plugin follow-up -The docs describe the Lavalink-based architecture and the deferred Udio work is explicit. - -- Rewrite the Lavaplayer sections of `docs/music.md` and `docs/architecture.md` to cover the node, the client, the dispatcher and the new settings. -- Refresh the music mentions in `docs/README.md`, `docs/threading.md` and `docs/utilities-and-bootstrap.md`. -- Document how to run the node locally and how to add server-side plugins via `lavalink/application.yml`. -- Add a follow-up note stating that `music/source/UdioAudioSourceManager` and `UdioAudioTrack` remain only as the reference for a future Lavalink server plugin and that Udio playback is disabled until then. \ No newline at end of file diff --git a/.junie/plans/persistable-task-registry-refactor.md b/.junie/plans/persistable-task-registry-refactor.md deleted file mode 100644 index e5f22ed..0000000 --- a/.junie/plans/persistable-task-registry-refactor.md +++ /dev/null @@ -1,280 +0,0 @@ ---- -sessionId: session-260925-070147-1a6u ---- - -# Requirements - -### Overview & Goals - -The persistable-task system currently requires a hand-maintained resource file, `src/main/resources/persistable-tasks.json`, that duplicates each task class's fully-qualified name, constructor parameter names and parameter types. Adding a persistable task means editing code *and* JSON; changing a constructor or moving a package silently breaks restore at runtime with only a log line. - -This is not hypothetical — the file is **already broken**: - -| Declared in JSON | Actual code | -|---|---| -| `me.bobert.netneutrality.feature.reaction.task.EmojiReactionRoleTask` | `me.bobert.netneutrality.tasks.reaction.EmojiReactionRoleTask` | -| ctor `(String, String, String, Map)` | ctor `(TaskManager, String, String, String, Map)` | - -So today the only registered persistable task **cannot be restored at all**: `Class.forName` fails on the stale package, and even with the right package `getConstructor` would miss because the DI refactor prepended `TaskManager`. On top of that, `EmojiTask.emojiMapping` is populated only by the "auto loading" constructor, so any mismatch loses the entire emoji→role mapping. - -The goal: **delete `persistable-tasks.json`** and make persistable tasks self-describing and self-registering, consistent with how `BuiltInTask` and `ScheduledTask` are already discovered via `ReflectionHelper` + `ApplicationContext`. - -### Scope - -**In scope** -- Remove `persistable-tasks.json` and `TaskRegistry`/`TaskFactory`'s metadata-driven reflection. -- Classpath-scan discovery of `PersistableTask` implementations. -- A `restore(JSONObject)` contract implemented by each task, replacing reflective constructor matching. -- A stable logical type id persisted in `tasks.task_class` instead of the FQCN, with a Flyway migration. -- Fix the `EmojiReactionRoleTask` restore breakage and the `emojiMapping` data loss. -- Symmetric payload serialization (what is written is what is read back). - -**Out of scope** -- Replacing `org.json` with Jackson. -- Changing `Task`/`StepTask`/`BuiltInTask` lifecycles or `TaskManagerImpl` concurrency behaviour. -- Reworking `Repository`/`TaskRepository` or the Hibernate setup. -- The unrelated issues tracked in `.junie/plans/improve-task-manager.md`. - -### User Stories - -- As a bot developer, I want to add a new persistable task by writing **one class** and nothing else, so I cannot forget to update a mapping file. -- As a bot developer, I want a constructor-signature change or package move to be caught by the **compiler or a test**, not by a runtime log at startup. -- As a server operator, I want tasks persisted before a restart — including their emoji mappings — to come back intact after a restart. - -### Functional Requirements - -- **FR-1** `persistable-tasks.json` is deleted; no resource file is read during persistable-task loading. -- **FR-2** Persistable task types are discovered by scanning `me.bobert.netneutrality` for concrete `PersistableTask` subtypes, the same package root already used by `ReflectionHelper.loadTasks`. -- **FR-3** Each persistable task exposes a stable `typeId()` (defaulting to its existing `getName()`), unique across all discovered types; a duplicate id fails fast at startup with a clear error. -- **FR-4** Each persistable task implements `restore(JSONObject payload)` to rebuild its own state; instantiation happens through the existing DI path (`ReflectionHelper.instantiate` with `ApplicationContext`). -- **FR-5** `toPayload()` and `restore()` are symmetric: a task serialized and restored yields an equal logical state, including `emojiMapping`. -- **FR-6** `tasks.task_class` stores the `typeId`; existing rows are migrated from FQCN (including the stale `feature.reaction.task.*` value) to the new id. -- **FR-7** A row whose `typeId` is unknown is logged and skipped without aborting the load of remaining tasks — current behaviour is preserved. -- **FR-8** `TaskService.persistTask` refuses to persist a task whose type is not registered, as it does today. - -### Non-Functional Requirements - -- Classpath scanning happens **once** at startup (`TaskManagerImpl.init()`), not per task row. -- No new runtime dependency is introduced; `org.reflections` and `org.json` are already in use. -- Existing persisted rows remain loadable after deployment — the migration must be the only manual step. - -# Technical Design - -### Current Implementation - -``` -App.main - └─ TaskManagerImpl(context) // holds ApplicationContext, creates TaskService(this) - └─ init() - ├─ loadBuiltInTasks() // ReflectionHelper.loadTasks(BuiltInTask, context, ..) ← scanning + DI - └─ taskService.loadTasks() - ├─ TaskRegistry.loadTaskConfig() // reads persistable-tasks.json - └─ for each OTask row: - TaskRegistry.getTaskCreator(oTask.taskClass) // keyed by FQCN - └─ TaskFactory.createTaskInstance(class, params, types, payload) - ├─ Class.forName(className) - ├─ map "string"/"int"/"map" → Class[] - ├─ pull each named value out of the payload - └─ clazz.getConstructor(types).newInstance(args) -``` - -Key files: -- `core/tasks/persistence/TaskRegistry.java` — parses the JSON into `Map`. -- `core/tasks/persistence/TaskFactory.java` — `ParamType` enum (only `string`/`int`/`map`) + reflective ctor call. -- `core/tasks/PersistableTask.java` — `insert`/`save`/`delete`/`toPayload`. -- `core/tasks/impl/AbstractPersistableTask.java` — `toPayload()` = `new JSONObject(this)` (bean getters). -- `db/service/TaskService.java` — `loadTasks`/`persistTask`/`unpersistTask`, `validateTask` checks `id`/`lockId`. -- `db/entities/OTask.java` — `task_id` (unique), `task_class`, `payload` (`@Lob`). -- `bootstrap/ReflectionHelper.java` — `loadClassesFromPackage` + `instantiate(type, ApplicationContext)`. - -The asymmetry is the root cause: **writing** uses bean introspection over all getters, **reading** uses a hand-declared subset of constructor params. Nothing keeps the two in sync. - -### Key Decisions - -1. **Self-restoring tasks** (confirmed with user). A task is created via the normal DI constructor and then told to hydrate itself from the payload. This eliminates parameter metadata entirely and makes the round trip visible in one class. -2. **Reuse `ReflectionHelper` + `ApplicationContext`** rather than a bespoke scanner, so persistable tasks follow the exact same discovery convention as `BuiltInTask` and `ScheduledTask`. -3. **Stable `typeId` in the DB** (confirmed with user). `getName()` already returns `"emoji-reaction-role"`, so `typeId()` defaults to `getName()` and no task needs a new literal. Package moves and class renames stop being breaking changes. -4. **Prototype-per-type registry.** The scan produces one throwaway instance per type used only to read `typeId()`; each restore creates a *fresh* instance via `ReflectionHelper.instantiate` so no state leaks between rows. -5. **Keep `org.json`.** Jackson was considered and rejected — it would touch every task's field mapping for no gain here. - -### Proposed Changes - -**A. Contract — `core/tasks/PersistableTask.java`** - -```java -public interface PersistableTask extends Task { - /** Stable logical type id stored in tasks.task_class. Defaults to getName(). */ - default String typeId() { return getName(); } - - /** Rebuild this instance's state from a payload previously produced by toPayload(). */ - void restore(JSONObject payload); - - void insert(CommandContext context); - void save(); - void delete(); - String toPayload(); -} -``` - -**B. `core/tasks/persistence/PersistableTaskRegistry.java`** (replaces `TaskRegistry`, no longer static) - -```java -public final class PersistableTaskRegistry { - private final Map> typesById; - private final ApplicationContext context; - - public void scan(); // ReflectionHelper.loadTasks(PersistableTask.class, context, ..) - public boolean isRegistered(String typeId); - public PersistableTask create(String typeId, JSONObject payload); // instantiate + restore -} -``` - -- `scan()` skips abstract types (`AbstractPersistableTask`, `EmojiTask` are filtered by the existing `Modifier.isAbstract` check in `loadClassesFromPackage`). -- Duplicate `typeId` → `IllegalStateException` at startup, naming both classes. -- `create()` calls `ReflectionHelper.instantiate(clazz, context)` then `task.restore(payload)`. - -**C. Delete** `TaskFactory.java` (and its `ParamType` enum) and `src/main/resources/persistable-tasks.json`. - -**D. `AbstractPersistableTask`** - -- `toPayload()` keeps `new JSONObject(this)` but the base class becomes responsible for guaranteeing `id` and `lockId` are present, since `TaskService.validateTask` reads both. -- Add a no-op `restore(JSONObject)` default so leaf tasks only override when they carry extra state. - -**E. `EmojiTask` / `EmojiReactionRoleTask` — the actual bug fix** - -- `EmojiTask.restore(payload)` reads `guildId`/`channelId`/`messageId` and repopulates `emojiMapping` from `payload.getJSONObject("emojiMapping")`. -- Because `guildId`/`channelId`/`messageId` are `final` and set by the ctor, they become non-final assigned-on-restore fields, or the restore path uses a dedicated protected initializer — whichever keeps `getId()` (`String.format(EmojiTaskContext.ID, guildId, messageId)`) correct **before** the task is handed to `TaskManager`. -- The vestigial "constructor for auto loading" overload in `EmojiReactionRoleTask` is removed; it exists only to serve `TaskFactory`. - -**F. `db/service/TaskService.java`** - -- Takes the `PersistableTaskRegistry` (constructed by `TaskManagerImpl`, which already owns the `ApplicationContext`). -- `loadTasks()` drops `TaskRegistry.loadTaskConfig()` and resolves by `oTask.getTaskClass()` treated as a **type id**. -- `persistTask()` writes `task.typeId()` instead of `task.getClass().getName()`, and guards with `registry.isRegistered(...)`. -- `validateTask` is unchanged. - -**G. Migration — `src/main/resources/db/migration/V2__task_type_id.sql`** - -```sql -UPDATE tasks SET task_class = 'emoji-reaction-role' - WHERE task_class IN ( - 'me.bobert.netneutrality.feature.reaction.task.EmojiReactionRoleTask', - 'me.bobert.netneutrality.tasks.reaction.EmojiReactionRoleTask' - ); -``` - -Column stays `task_class` (renaming it is cosmetic and would churn `OTask`); a comment documents that it now holds a logical id. - -### File Structure - -| File | Change | -|---|---| -| `src/main/resources/persistable-tasks.json` | **deleted** | -| `core/tasks/persistence/TaskFactory.java` | **deleted** | -| `core/tasks/persistence/TaskRegistry.java` | **replaced** by `PersistableTaskRegistry.java` | -| `core/tasks/PersistableTask.java` | + `typeId()`, + `restore(JSONObject)` | -| `core/tasks/impl/AbstractPersistableTask.java` | default `restore`, payload guarantees | -| `core/tasks/impl/EmojiTask.java` | implements `restore`, hydrates `emojiMapping` | -| `tasks/reaction/EmojiReactionRoleTask.java` | drop auto-loading ctor | -| `core/tasks/TaskManagerImpl.java` | build + scan registry, pass to `TaskService` | -| `db/service/TaskService.java` | registry injection, type-id keying | -| `src/main/resources/db/migration/V2__task_type_id.sql` | **new** | -| `docs/tasks.md`, `docs/persistence.md` | document the new flow, drop JSON references | - -### Architecture Diagram - -```mermaid -graph LR - A[TaskManagerImpl.init] --> B[PersistableTaskRegistry.scan] - B --> C[Reflections: PersistableTask subtypes] - C --> D[typeId to Class map] - A --> E[TaskService.loadTasks] - E --> F[TaskRepository.findAll] - F --> G[registry.create typeId payload] - D --> G - G --> H[ReflectionHelper.instantiate DI] - H --> I[task.restore payload] - I --> J[TaskManager.insertPersistableTask] -``` - -### Risks - -- **Final-field hydration in `EmojiTask`.** Restoring after construction means `guildId`/`messageId` cannot stay `final`. Mitigation: hydrate inside `PersistableTaskRegistry.create()` *before* the instance escapes, and assert `getId()` matches `OTask.task_id` in `validateTask` (already done today). -- **Scan picks up unintended subtypes.** Any concrete `PersistableTask` in `me.bobert.netneutrality` becomes registered. Mitigation: duplicate-id detection plus a test asserting the registered set. -- **Migration ordering.** Flyway runs before `TaskManagerImpl.init()` in the existing startup lifecycle, so V2 is applied before any load; worth re-verifying in `App.main` wiring. -- **DI constructor ambiguity.** `ReflectionHelper.instantiate` picks the longest fully-resolvable constructor; removing the auto-loading overload leaves exactly one candidate for `EmojiReactionRoleTask`, which is the intent. - -# Testing - -### Validation Approach - -Compile and run the existing suite (`TaskManagerImplTest`, `ReflectionInjectionTest`) to confirm no regression, then add focused unit tests around the new registry and the round trip. The key invariant — *a task can be serialized and restored without loss* — becomes a test rather than a hand-checked JSON file. - -### Key Scenarios - -- **Discovery**: `PersistableTaskRegistry.scan()` over `me.bobert.netneutrality` registers `emoji-reaction-role` and does **not** register the abstract `AbstractPersistableTask` / `EmojiTask`. -- **Round trip**: build an `EmojiReactionRoleTask` with a non-empty `emojiMapping`, call `toPayload()`, feed it to `registry.create("emoji-reaction-role", payload)`, and assert `getId()`, `getLockId()`, `guildId`, `channelId`, `messageId` and the full `emojiMapping` all match. This is the regression test for the current data-loss bug. -- **DI**: the restored instance has a non-null `taskManager` injected via `ApplicationContext`, proving the DI path is used. -- **Persist keying**: `TaskService.persistTask` stores `emoji-reaction-role` in `OTask.task_class`, not an FQCN. -- **End-to-end load**: a stubbed `TaskRepository` returning one row with the new type id results in `taskManager.insertPersistableTask` being called once. - -### Edge Cases - -- Unknown `typeId` in a row → logged, skipped, remaining rows still load. -- Duplicate `typeId` across two classes → startup fails fast with both class names in the message. -- Payload missing `emojiMapping` → restores to an empty map rather than throwing. -- `validateTask` mismatches (`id`, payload `id`, payload `lockId`) still reject the row. -- No `persistable-tasks.json` on the classpath → nothing references it; a test asserts the resource is absent so it cannot be reintroduced by accident. - -### Test Changes - -- New `PersistableTaskRegistryTest` — discovery, duplicate detection, unknown id. -- New `EmojiReactionRoleTaskPersistenceTest` — payload round trip including emoji mapping. -- Update `TaskManagerImplTest` where it constructs the task service / registry. -- Remove any assertions tied to `TaskFactory.ParamType`. - -# Delivery Steps - -### ✓ Step 1: Introduce the self-describing PersistableTask contract -`PersistableTask` carries its own type id and restore logic, with a compiling default for existing tasks. - -- Add `default String typeId() { return getName(); }` to `core/tasks/PersistableTask.java`. -- Add `void restore(JSONObject payload)` to the interface. -- Give `AbstractPersistableTask` a no-op `restore` default so only tasks with extra state override it. -- Harden `AbstractPersistableTask.toPayload()` to guarantee `id` and `lockId` are always present, since `TaskService.validateTask` reads both. -- Leave `TaskRegistry`/`TaskFactory` in place at this stage so the build stays green. - -### ✓ Step 2: Add the scanning PersistableTaskRegistry and retire the JSON -Persistable task types are discovered from the classpath and `persistable-tasks.json` is gone. - -- Create `core/tasks/persistence/PersistableTaskRegistry` with `scan()`, `isRegistered(String)` and `create(String typeId, JSONObject payload)`. -- Implement `scan()` on top of `ReflectionHelper.loadTasks(PersistableTask.class, context, ..)`, mapping `typeId()` to the concrete class and throwing on duplicate ids naming both classes. -- Implement `create()` as `ReflectionHelper.instantiate(clazz, context)` followed by `task.restore(payload)`. -- Delete `core/tasks/persistence/TaskFactory.java`, `core/tasks/persistence/TaskRegistry.java` and `src/main/resources/persistable-tasks.json`. -- Add `PersistableTaskRegistryTest` covering discovery of `emoji-reaction-role`, exclusion of abstract types, and duplicate-id failure. - -### ✓ Step 3: Fix EmojiTask restore and the lost emoji mapping -An `EmojiReactionRoleTask` survives a persist/restore cycle with its emoji mapping intact. - -- Implement `restore(JSONObject)` in `core/tasks/impl/EmojiTask.java`, hydrating `guildId`, `channelId`, `messageId` and repopulating `emojiMapping` from the payload. -- Adjust the field declarations so restore-after-construction is possible while `getId()` stays correct before the instance escapes the registry. -- Remove the now-dead "constructor for auto loading" overload from `tasks/reaction/EmojiReactionRoleTask.java`. -- Handle a missing `emojiMapping` key by restoring an empty map instead of throwing. -- Add `EmojiReactionRoleTaskPersistenceTest` asserting a full round trip including a non-empty mapping and a DI-injected `taskManager`. - -### ✓ Step 4: Key persistence by stable type id and migrate existing rows -`tasks.task_class` holds a logical type id, decoupling stored rows from package structure. - -- Change `TaskService.persistTask` to write `task.typeId()` instead of `task.getClass().getName()`, guarded by `registry.isRegistered(...)`. -- Change `TaskService.loadTasks` to resolve rows through `PersistableTaskRegistry.create(...)` and drop the `TaskRegistry.loadTaskConfig()` call. -- Inject the registry into `TaskService` from `TaskManagerImpl`, which already owns the `ApplicationContext`, and trigger `scan()` during `TaskManagerImpl.init()`. -- Add `src/main/resources/db/migration/V2__task_type_id.sql` rewriting both the stale `feature.reaction.task.*` and current `tasks.reaction.*` FQCNs to `emoji-reaction-role`. -- Keep unknown-type rows as logged-and-skipped, and update `TaskManagerImplTest` for the new construction signature. - -### ✓ Step 5: Update the task and persistence documentation -The docs describe the scan-and-restore flow with no references to the deleted resource file. - -- Rewrite the persistable-task section of `docs/tasks.md` to describe `typeId()`, `restore()` and classpath discovery, replacing the `TaskRegistry`/`TaskFactory` JSON description. -- Add a short "how to add a persistable task" recipe showing that a single class is sufficient. -- Update the `TaskService` bullet and the `OTask` row in `docs/persistence.md` to state that `task_class` stores a logical type id. -- Note the `V2__task_type_id.sql` migration in the schema section of `docs/persistence.md`. \ No newline at end of file diff --git a/.junie/plans/queue-scheduler-improvements.md b/.junie/plans/queue-scheduler-improvements.md deleted file mode 100644 index 58ac950..0000000 --- a/.junie/plans/queue-scheduler-improvements.md +++ /dev/null @@ -1,132 +0,0 @@ ---- -sessionId: session-260906-073726-sbxb ---- - -# Requirements - -### Overview & Goals -Improve the code quality, correctness, and maintainability of `QueueManager`, `TrackScheduler`, `TrackInfo`, and `VoteSkipService` — the core music queue and playback scheduling subsystem. - -### Scope -**In Scope:** -- Fix thread-safety issues (check-then-act race conditions in `QueueManager`) -- Simplify confusing queue insertion logic in `QueueManager.queue()` -- Separate concerns: extract command-result formatting from queue data operations -- Fix `TrackInfo` using wrong logger (`QueueManager.class` instead of `TrackInfo.class`) -- Clean up `TrackScheduler.onTrackStuck()` complexity -- Add unit tests for `QueueManager`, `TrackScheduler`, `TrackInfo`, and `VoteSkipService` -- Expose skip-vote state through `TrackInfo` more cleanly (encapsulate the `Set` skips) - -**Out of Scope:** -- Changes to `MusicManager`, `MusicServiceImpl`, or audio source/loading classes -- New user-facing features or commands -- Changes to `MusicManagerCallback` interface - -# Technical Design - -### Current Implementation - -**`QueueManager`** (`src/main/java/me/bobert/netneutrality/music/QueueManager.java`, 286 lines): -- Manages a `LinkedBlockingDeque` with manual `synchronized` blocks -- Multiple methods check state outside the synchronized block, then re-check inside (e.g., `clearQueue()` L81 vs L86, `skip()` L197 vs L203, `shuffle()` L229 vs L233, `removeFromQueue()` L254 vs L259) — the outer checks are racy and misleading -- `queue()` method (L135-158) has confusing branching: `addFirst` + `add` can result in the same track being added twice when `player.getPlayingTrack() == null && !queue.isEmpty()` -- `clearQueue()` calls `callback.onQueueEmpty()` before checking if queue is empty (L79) -- `removeTracksFromUser()` iterates the queue outside synchronized (L97), then again inside (L102) -- `skip(int)` silently resets `nbTrack` to 1 if it exceeds queue size (L209) — surprising behavior -- Mixes queue data operations with `CommandExecutionResult` formatting - -**`TrackScheduler`** (`src/main/java/me/bobert/netneutrality/music/TrackScheduler.java`, 175 lines): -- `onTrackStuck()` (L129-173) has deeply nested logic with inline async recovery — hard to follow and test -- Circular dependency: creates `QueueManager` in constructor, which takes `this` back -- `context` field is accessed from multiple threads but only `attachContext`/`detachContext` are synchronized — `getContext()`, `getDj()` are not - -**`TrackInfo`** (`src/main/java/me/bobert/netneutrality/music/TrackInfo.java`, 83 lines): -- Logger incorrectly uses `QueueManager.class` (L26) instead of `TrackInfo.class` -- `getSkips()` returns mutable `Set` — breaks encapsulation -- Duplicate copyright block (L1-5 and L7-11) - -**`VoteSkipService`** (`src/main/java/me/bobert/netneutrality/music/VoteSkipService.java`, 57 lines): -- Relatively clean, but directly calls `queueManager.skip()` mixing vote logic with queue mutation -- `info.addSkip()` uses `context.getGuildUser()` but `info.getSkips().size()` check uses raw set — no race protection - -### Proposed Changes - -#### QueueManager -1. **Remove all outer unsynchronized checks** — move all logic inside a single `synchronized` block per method to eliminate check-then-act races -2. **Rewrite `queue()` method** — simplify to: add to queue, if nothing playing start the head track. Remove the confusing `addFirst`/`add` dual-path -3. **Fix `clearQueue()`** — move `callback.onQueueEmpty()` call to after the queue is confirmed non-empty and cleared -4. **Fix `skip(int)`** — when `nbTrack > queue.size()`, skip all remaining tracks instead of silently resetting to 1 -5. **Fix `removeTracksFromUser()`** — do all filtering inside synchronized block -6. **Return queue operation results as data** (e.g., number removed, track name) and let callers build `CommandExecutionResult` — or at minimum, document that this class returns command results - -#### TrackScheduler -1. **Extract stuck-track recovery** into a dedicated `StuckTrackRecoveryHandler` class to reduce `onTrackStuck()` complexity -2. **Make `context` access thread-safe** — synchronize `getContext()` and `getDj()`, or use `volatile` -3. **Add `lastStuckTrack` reset** on track start or successful recovery to prevent stale state - -#### TrackInfo -1. **Fix logger** — change to `TrackInfo.class` -2. **Remove duplicate copyright block** -3. **Encapsulate skips** — return `Collections.unmodifiableSet(skips)` from `getSkips()`, add `getSkipCount()` method - -#### VoteSkipService -1. **Synchronize vote-skip check** — ensure `addSkip` + threshold check is atomic (or accept eventual consistency and document it) - -### File Structure -All changes in `src/main/java/me/bobert/netneutrality/music/`: -- **Modified:** `QueueManager.java`, `TrackScheduler.java`, `TrackInfo.java`, `VoteSkipService.java` -- **New:** `StuckTrackRecoveryHandler.java` -- **New tests:** `src/test/java/me/bobert/netneutrality/command/music/QueueManagerTest.java`, `TrackSchedulerTest.java`, `TrackInfoTest.java`, `VoteSkipServiceTest.java` - -# Testing - -### Validation Approach -Add unit tests for all four classes using JUnit 5 and Mockito (already in project dependencies via the existing test classes). - -### Key Scenarios -- **QueueManager:** queue/dequeue ordering, skip single/multiple, shuffle preserves current track, removeFromQueue bounds, clearQueue idempotency, removeTracksFromUser correctness -- **TrackInfo:** skip vote tracking, duplicate skip prevention, unmodifiable skips set, equals/hashCode -- **VoteSkipService:** threshold calculation, skip when below min members, vote counting, duplicate vote handling -- **TrackScheduler:** onTrackEnd triggers correct queue action per end reason, onTrackStuck recovery flow - -### Edge Cases -- Queue with single track: skip, shuffle, remove -- Skip count exceeding queue size -- Empty queue operations -- Concurrent vote-skip from same user - -# Delivery Steps - -### ✓ Step 1: Fix TrackInfo issues -TrackInfo has correct logger, clean copyright, and encapsulated skip state. - -- Fix logger from `QueueManager.class` to `TrackInfo.class` (line 26) -- Remove duplicate copyright block (lines 7-11) -- Change `getSkips()` to return `Collections.unmodifiableSet(skips)` -- Add `getSkipCount()` convenience method -- Add `TrackInfoTest.java` covering skip tracking, duplicate prevention, equals/hashCode, and unmodifiable set enforcement - -### ✓ Step 2: Fix QueueManager thread-safety and queue logic -QueueManager has correct synchronization and simplified queue insertion. - -- Remove all outer unsynchronized pre-checks in `clearQueue()`, `skip()`, `shuffle()`, `removeFromQueue()`, `removeTracksFromUser()` — consolidate into single synchronized blocks -- Rewrite `queue(AudioTrack, Member)` to eliminate the confusing `addFirst`/`add` dual-path: simply add to tail, then start playback if nothing is playing -- Fix `clearQueue()` to call `callback.onQueueEmpty()` only after confirming queue was non-empty -- Fix `skip(int)` to skip all remaining when `nbTrack > queue.size()` instead of silently resetting to 1 -- Fix `removeTracksFromUser()` to do all filtering inside synchronized block -- Add `QueueManagerTest.java` covering queue ordering, skip, shuffle, remove, clear, and edge cases - -### ✓ Step 3: Extract stuck-track recovery and fix TrackScheduler thread-safety -TrackScheduler is simplified with extracted recovery logic and thread-safe context access. - -- Create `StuckTrackRecoveryHandler.java` encapsulating the retry-or-skip logic currently in `onTrackStuck()` (lines 129-173) -- Simplify `onTrackStuck()` to delegate to the new handler -- Reset `lastStuckTrack` in `onTrackStart()` to prevent stale state -- Synchronize `getContext()` and `getDj()` methods (or make `context` volatile) -- Add `TrackSchedulerTest.java` covering onTrackEnd behavior per end reason and stuck-track recovery delegation - -### ✓ Step 4: Improve VoteSkipService robustness and add tests -VoteSkipService has documented concurrency behavior and full test coverage. - -- Add documentation comment about the vote-then-check atomicity assumption -- Add `VoteSkipServiceTest.java` covering: threshold math, auto-skip below min members, vote accumulation, duplicate vote from same user, skip trigger when threshold exceeded \ No newline at end of file diff --git a/.junie/plans/remove-settings-singleton.md b/.junie/plans/remove-settings-singleton.md deleted file mode 100644 index ed4f8f5..0000000 --- a/.junie/plans/remove-settings-singleton.md +++ /dev/null @@ -1,91 +0,0 @@ ---- -sessionId: session-260909-154047-1h6n ---- - -# Requirements - -### Overview & Goals -Remove the singleton pattern from `Settings` so it becomes a plain instantiable class. All access should go through constructor injection of `SettingsProvider`, except for the single bootstrap site in `App.main()` which creates the instance. - -### Scope -**In Scope:** -- Remove `static INSTANCE` field and `getInstance()` from `Settings` -- Remove static helper methods `getDiscordInvite()` and `getVersion()` from `Settings` -- Migrate all remaining `Settings.getInstance()` call sites to use injected `SettingsProvider` -- Update the test to construct `Settings` directly - -**Out of Scope:** -- Changing the `SettingsProvider` interface -- Introducing a DI framework -- Changing how properties are loaded - -# Technical Design - -### Current Implementation -- `Settings` is a lazy singleton (`getInstance()`) implementing `SettingsProvider` -- `SettingsProvider` interface already exists and is injected into most classes via `ApplicationContext` -- `App.buildContext()` already registers the Settings instance as `SettingsProvider` in the context -- **Remaining direct singleton usages (9 call sites):** - - `App.java` — `VERSION` static field (line 51), `main()` bootstrap (lines 61-62), `startup()` (line 89), `buildContext()` (line 112), `initJDA()` (line 190) - - `CommandContext.java` — `isMessageFromCreator()` (line 134) - - `WebModule.java` — `createRequest()` (line 289) via `Settings.getVersion()` - - `ReportBug.java` — `executeTask()` (line 34) via `Settings.getDiscordInvite()` - - `SettingsTest.java` — `getMavenVersion()` test (line 17) - -### Proposed Changes - -#### 1. `Settings.java` -- Make constructor `public` -- Remove `static INSTANCE` field, `getInstance()`, `getDiscordInvite()`, `getVersion()` -- Keep `getMavenVersion()` as an instance method -- Add `getDiscordInvite()` as an instance method on `SettingsProvider` (or keep it as a convenience on `Settings`) - -#### 2. `App.java` -- Create `Settings` via `new Settings()` at the top of `main()`; pass it into `App` constructor or store as a field -- Replace `VERSION` static field with an instance field initialized from the settings instance -- Thread the `SettingsProvider` through `startup()`, `buildContext()`, `initJDA()` - -#### 3. `CommandContext.java` -- Add a `SettingsProvider` field, accept it via constructor parameter -- Update `isMessageFromCreator()` to use the injected provider -- Update subclasses (`MessageCommandContext`, `SlashCommandContext`, etc.) to pass it through - -#### 4. `WebModule.java` -- Accept a version string or `SettingsProvider` in the `Builder`; replace `Settings.getVersion()` call - -#### 5. `ReportBug.java` -- Accept `SettingsProvider` via constructor injection; replace `Settings.getDiscordInvite()` with `settings.get("NETNEUTRALITY_BOT_INVITE", ...)` - -#### 6. `SettingsTest.java` -- Use `new Settings()` instead of `Settings.getInstance()` - -### Risks -- `CommandContext` is abstract with multiple subclasses — all constructors need updating -- `WebModule.Builder` is used in many places including inside `Settings.getGitVersion()` itself — the version string approach (pass a `String` rather than the full provider) keeps this simpler - -# Delivery Steps - -### ✓ Step 1: Remove singleton from Settings class -Settings can be instantiated with `new Settings()` and no longer holds static state. - -- Make the `Settings` constructor `public` -- Remove the `static INSTANCE` field and `getInstance()` method -- Remove static helpers `getDiscordInvite()` and `getVersion()` -- Move `getDiscordInvite()` logic to an instance method (or inline at call sites) -- Keep `getMavenVersion()` as-is (already instance method) - -### ✓ Step 2: Migrate App.java bootstrap to explicit instantiation -App creates and threads a single Settings instance through its lifecycle methods. - -- Create `new Settings()` at the start of `main()` and store as a field on `App` -- Replace `Settings.getInstance()` calls in `main()`, `startup()`, `buildContext()`, `initJDA()` with the field -- Replace the `VERSION` static field with an instance field or a local derived from the settings instance - -### ✓ Step 3: Inject SettingsProvider into remaining call sites -CommandContext, WebModule, and ReportBug no longer reach for the singleton. - -- Add `SettingsProvider` constructor parameter to `CommandContext`; update `isMessageFromCreator()` to use it -- Update `CommandContext` subclasses (`MessageCommandContext`, `SlashCommandContext`, etc.) to pass the provider through -- In `WebModule.Builder`, accept a version string parameter; replace `Settings.getVersion()` in `createRequest()` -- In `ReportBug`, add `SettingsProvider` constructor parameter; replace `Settings.getDiscordInvite()` with `settings.get(...)` -- Update `SettingsTest` to use `new Settings()` directly \ No newline at end of file diff --git a/.junie/plans/revert-package-structure.md b/.junie/plans/revert-package-structure.md deleted file mode 100644 index 3bcb076..0000000 --- a/.junie/plans/revert-package-structure.md +++ /dev/null @@ -1,212 +0,0 @@ ---- -sessionId: session-260905-145156-xyio ---- - -# Requirements - -### Overview & Goals - -Revise the package structure section of the lifecycle refactor plan to use a hybrid layout that keeps the new `bootstrap/` package but reverts the rest to the familiar old-style naming and organization. - -### Scope - -**In Scope** -- Reverting `framework/` → `core/` (engine: command, context, task machinery) -- Reverting `platform/` hierarchy → flat old-style packages: `db/` (persistence + entities + repositories + services), `hooks/` (events), `http/` (web API), `music/` (music domain) -- Reverting `feature/` vertical grouping → separate top-level `command/`, `tasks/`, `context/` packages with subcategory folders (e.g. `command/music/`, `tasks/scheduled/`) -- Restoring `utils/` for remaining helpers (Settings, JpaUtils, MusicUtils, ChannelMessagePurger) -- Keeping `bootstrap/` (ApplicationContext, Lifecycle, BotState, ReflectionHelper) — this is new and stays -- Keeping `exceptions/` — unchanged -- Keeping `shared/util/` (ContextUtils, GuildUtils, MessageFunction, MiscUtils, UserFunction) - -**Out of Scope** -- Any behavioral or DI/lifecycle logic changes — this is purely a package restructure -- Changing the DI refactor plan's other sections (Lifecycle contract, ApplicationContext, constructor injection, interfaces) - -### Target Layout - -``` -me.bobert.netneutrality -├── App.java -├── bootstrap/ # NEW — keep as-is -│ ├── ApplicationContext.java -│ ├── BotState.java -│ ├── Lifecycle.java -│ └── ReflectionHelper.java -├── core/ # was framework/ → revert to core/ -│ ├── command/ # Command, CommandProcessor, CommandRegistry, CommandManager, options/, impl/ -│ ├── context/ # Context hierarchy, abstract bases -│ └── tasks/ # Task, TaskManager, StepTask, PersistableTask, persistence/, trait/, impl/ -├── command/ # concrete command implementations (was feature/*/command/) -│ ├── admin/ -│ ├── basic/ -│ ├── bot/ -│ ├── crypto/ -│ ├── custom/ -│ ├── git/ -│ ├── music/ -│ └── react/ -├── context/ # concrete context implementations (was framework/context/command/ + task/) -│ ├── command/ -│ ├── tasks/ -│ └── MusicChannelContext.java -├── tasks/ # concrete task implementations (was feature/*/task/) -│ ├── emoji/ -│ ├── generic/ -│ ├── git/ -│ ├── help/ -│ ├── music/ -│ ├── reaction/ -│ └── scheduled/ -├── db/ # persistence model (was data/ + platform/persistence/) -│ ├── entities/ # O* entities -│ ├── repository/ -│ ├── service/ # GuildService, GuildServiceProvider, BlacklistService, etc. -│ ├── GSettings.java -│ ├── Repository.java -│ ├── DatabaseMigration.java -│ ├── SessionManager.java # SessionProvider impl -│ └── JpaUtils.java -├── hooks/ # was platform/event/ -│ └── EventsHandler.java -├── http/ # was platform/web/ -│ ├── server/ -│ └── WebModule.java -├── music/ # music domain (was feature/music/ minus commands/tasks) -│ ├── AudioPlayerSendHandler.java -│ ├── MusicManager.java -│ ├── MusicService.java -│ ├── QueueManager.java -│ ├── SourceFinder.java -│ ├── source/ -│ ├── TrackInfo.java -│ ├── TrackLoadingHandler.java -│ └── TrackScheduler.java -├── utils/ # remaining helpers not in shared/util/ -│ ├── Settings.java # SettingsProvider impl -│ ├── ChannelMessagePurger.java -│ ├── JpaUtils.java # (or in db/, see note) -│ └── MusicUtils.java -├── shared/util/ # NEW — keep as-is -│ ├── ContextUtils.java -│ ├── GuildUtils.java -│ ├── MessageFunction.java -│ ├── MiscUtils.java -│ └── UserFunction.java -├── exceptions/ # unchanged -└── core/thread/ OR thread/ # ThreadManager, ScheduledTaskManager, BaseThreadFactory -``` - -**Note:** `core/thread/` was the old location for threading. The old layout also had a top-level `thread/` for `MusicManagerCleanupThread`. Both can be restored as they were. - -### Key Moves (current → proposed) - -| Current (new) | Proposed (hybrid) | -|---|---| -| `framework/command/` | `core/command/` | -| `framework/context/` | `core/context/` | -| `framework/task/` | `core/tasks/` | -| `platform/config/Settings.java` | `utils/Settings.java` | -| `platform/persistence/*` | `db/` (SessionManager, DatabaseMigration, JpaUtils, Repository) | -| `platform/concurrency/*` | `core/thread/` | -| `platform/logging/LogHandler.java` | `core/LogHandler.java` | -| `platform/event/EventsHandler.java` | `hooks/EventsHandler.java` | -| `platform/web/*` | `http/*` | -| `data/entity/` | `db/entities/` | -| `data/repository/` | `db/repository/` | -| `data/service/` | `db/service/` | -| `feature/music/command/*` | `command/music/` | -| `feature/music/*.java` (domain) | `music/` | -| `feature/moderation/command/*` | `command/admin/` | -| `feature/moderation/ChannelMessagePurger.java` | `utils/ChannelMessagePurger.java` | -| `feature/reaction/command/*` | `command/react/` | -| `feature/reaction/task/*` | `tasks/reaction/` | -| `feature/git/command/*` | `command/git/` | -| `feature/git/task/*` | `tasks/git/` | -| `feature/crypto/command/*` | `command/crypto/` | -| `feature/help/command/*` | `command/basic/` + `command/bot/` | -| `feature/help/task/*` | `tasks/help/` | -| `feature/custom/command/*` | `command/custom/` | -| `bootstrap/` | `bootstrap/` (no change) | -| `shared/util/` | `shared/util/` (no change) | -| `exceptions/` | `exceptions/` (no change) | - -# Technical Design - -### Current Implementation - -The codebase currently uses the post-refactor layout with: -- `bootstrap/` — ApplicationContext, Lifecycle, BotState, ReflectionHelper -- `framework/` — command, context, task engine -- `platform/` — config, persistence, concurrency, logging, event, web -- `feature/` — music, moderation, reaction, git, crypto, help, custom (feature-vertical grouping) -- `data/` — entity, repository, service -- `shared/util/` — cross-cutting helpers -- `exceptions/` - -All DI/lifecycle wiring (constructor injection, interfaces like `SettingsProvider`, `SessionProvider`, `ExecutorProvider`, `CommandRegistry`, `TaskManager`, `MusicService`, `GuildServiceProvider`) remains intact — only package locations change. - -### Key Decisions -1. **Revert engine package name**: `framework/` → `core/` — matches the original naming the team is familiar with. -2. **Flatten infrastructure**: dissolve `platform/` hierarchy back to `db/`, `hooks/`, `http/`, `core/thread/`, `utils/` — removes the extra nesting that added cognitive overhead. -3. **Separate top-level command/tasks/context**: undo feature-vertical grouping in `feature/` — concrete implementations go back to `command//`, `tasks//`, `context/` as before. -4. **Keep bootstrap/ and shared/util/**: these are genuinely new and useful additions with no old equivalent. -5. **Restore `db/entities/`**: revert `data/entity/` back to `db/entities/` (plural, matching old convention). -6. **Service interfaces stay**: `SettingsProvider`, `SessionProvider`, `ExecutorProvider`, etc. remain — they just live in their reverted package locations (e.g. `SettingsProvider` in `utils/` alongside `Settings`). - -### Proposed Changes - -This is a pure move/rename refactor using IDE safe "Move Class" operations. No logic changes. The DI wiring, lifecycle priorities, constructor injection, and all interfaces remain exactly as they are — only their package declarations and import statements change. - -### Risks -- **Import churn** — same risk as the original restructure; mitigated by IDE "Move Class" refactor, one subsystem at a time, build after each. -- **Interface co-location** — service interfaces (e.g. `SessionProvider`) now live alongside their impls in `db/` or `utils/` rather than in a separate `platform/` package. This is acceptable for this project's size. - -# Delivery Steps - -### ✓ Step 1: Move engine packages from framework/ back to core/ -The `framework/` package is renamed back to `core/` with its original subpackage names. - -- Move `framework/command/` → `core/command/` (all engine classes: Command, CommandProcessor, CommandRegistry, CommandManager, CommandCategory, CommandContainer, CommandExecutionResult, CommandOptionsBuilder, impl/, options/) -- Move `framework/context/` → `core/context/` (Context, AbstractContext, CommandContext, TaskContext, InteractionContext, MessageContext, NoMessageContext, AsyncContext) -- Move `framework/task/` → `core/tasks/` (Task, TaskManager interface + impl, StepTask, PersistableTask, BuiltInTask, ScheduledTask, TaskCleanup, persistence/, trait/, impl/, generic/) -- Move `platform/logging/LogHandler.java` → `core/LogHandler.java` -- Move `platform/concurrency/*` → `core/thread/` (ThreadManager/ExecutorProvider, ScheduledTaskManager, BaseThreadFactory) -- Update all imports; verify build with `./mvnw compile` - -### ✓ Step 2: Move persistence and infrastructure packages back to db/, hooks/, http/, utils/ -Infrastructure packages are flattened back to their original top-level locations. - -- Move `data/entity/` → `db/entities/` (restore plural naming) -- Move `data/repository/` → `db/repository/` -- Move `data/service/` → `db/service/` (GuildService, GuildServiceProvider, GuildSettingService, BlacklistService, TaskService) -- Move `data/GSettings.java` → `db/GSettings.java` -- Move `platform/persistence/` contents → `db/` (SessionManager/SessionProvider, DatabaseMigration, Repository, JpaUtils, SessionProvider interface) -- Move `platform/config/Settings.java` + `SettingsProvider` → `utils/` -- Move `platform/event/EventsHandler.java` → `hooks/EventsHandler.java` -- Move `platform/web/` → `http/` (WebApi, server/, WebModule) -- Move `feature/moderation/ChannelMessagePurger.java` → `utils/ChannelMessagePurger.java` -- Move `feature/music/MusicUtils.java` → `utils/MusicUtils.java` (if it exists) -- Delete empty `platform/` and `data/` directories -- Update all imports; verify build with `./mvnw compile` - -### ✓ Step 3: Restore music domain package and split feature/ into command/, tasks/, context/ -Feature-vertical grouping is dissolved back into separate top-level packages. - -- Move `feature/music/` domain classes (MusicManager, MusicService, QueueManager, AudioPlayerSendHandler, SourceFinder, source/, TrackInfo, TrackLoadingHandler, TrackScheduler, MusicChannelContext) → `music/` -- Move concrete context classes from `framework/context/command/` → `context/command/` and `framework/context/task/` → `context/tasks/`; move MusicChannelContext → `context/MusicChannelContext.java` -- Move `feature/music/command/*` → `command/music/` -- Move `feature/moderation/command/*` → `command/admin/` -- Move `feature/reaction/command/*` → `command/react/` -- Move `feature/git/command/*` → `command/git/` -- Move `feature/crypto/command/*` → `command/crypto/` -- Move `feature/help/command/*` → `command/basic/` and `command/bot/` (matching old categorization) -- Move `feature/custom/command/*` → `command/custom/` -- Move `feature/reaction/task/*` → `tasks/reaction/` -- Move `feature/git/task/*` → `tasks/git/` -- Move `feature/help/task/*` → `tasks/help/` -- Move `feature/music/task/*` → `tasks/music/` -- Move scheduled tasks → `tasks/scheduled/` -- Delete empty `feature/` directory -- Update all imports; verify full build with `./mvnw package` -- Update `docs/architecture.md` package map table to reflect the new hybrid layout \ No newline at end of file diff --git a/.junie/plans/thread-safety-improvements.md b/.junie/plans/thread-safety-improvements.md deleted file mode 100644 index aefb5cc..0000000 --- a/.junie/plans/thread-safety-improvements.md +++ /dev/null @@ -1,99 +0,0 @@ ---- -sessionId: session-260910-054543-zckm ---- - -# Requirements - -### Overview & Goals -Analyze and fix thread-safety issues and improve the `ExecutorProvider` / `ThreadManager` infrastructure and all code that interacts with it. - -### Scope -#### In Scope -- Fix thread-safety bugs in `ThreadManager`, `TaskManagerImpl`, `ChannelMessagePurger`, `MusicManager` -- Improve executor pool configuration (bounded pools, proper shutdown) -- Clean up inconsistent synchronization patterns - -#### Out of Scope -- Rewriting the task/command architecture -- Changing the `QueueManager` synchronization (it's consistent and correct, just verbose) - -# Technical Design - -### Current Issues Found - -#### 1. `ThreadManager` (`core/thread/ThreadManager.java`) -- **Unbounded cached thread pools** — `newCachedThreadPool()` creates unlimited threads; under load (e.g., many guilds purging simultaneously) this can exhaust memory. -- **Shutdown bug (line 66-69)** — If `awaitTermination` throws `InterruptedException` on the *first* executor, the `catch` block exits the loop and remaining executors only get `shutdownNow()` without a prior `shutdown()` call. Each executor should be shut down independently. -- **No daemon threads** — `BaseThreadFactory` doesn't set `thread.setDaemon(true)`, so leaked threads can prevent JVM exit. - -#### 2. `TaskManagerImpl` (`core/tasks/TaskManagerImpl.java`) -- **Null values in `ConcurrentHashMap` (line 178-184)** — `computeIfAbsent` returns `null` for `PersistableTask`/`BuiltInTask`, but `ConcurrentHashMap` does **not** permit null values. This will throw `NullPointerException` at runtime. -- **Mixed synchronization** — `getTasks()` is `synchronized` on `this`, but all mutations use `ConcurrentHashMap` operations (no `synchronized`). The `synchronized` keyword on `getTasks()` provides no actual protection since `insert()`, `removeTask()`, etc. don't synchronize on `this`. -- **`hasTask()` race (line 93-95)** — Check-then-act across two collections (`tasks` and `pendingTasks`) without synchronization. -- **`handle()` lock cleanup race (line 191-194)** — After acquiring the lock, it checks `tasks.containsKey()` and removes from `tasksLock`, but another thread could be waiting on the same lock and will find it removed. - -#### 3. `ChannelMessagePurger` (`utils/ChannelMessagePurger.java`) -- **Double unlock bug** — `purgeMessages()` acquires `lock` via `tryLock()` (line 68) and releases it in `finally` (line 79). But `safePurge()` also calls `lock.unlock()` (line 154), causing an `IllegalMonitorStateException` or releasing a lock that a *different* thread now holds. -- **Non-thread-safe static `HashMap`** (line 40) — `instances` is a plain `HashMap` accessed from multiple threads via `computeIfAbsent`. Should be `ConcurrentHashMap`. -- **Static mutable `ExecutorProvider` binding** — The `volatile` field + `bind()` pattern works but is fragile; a null check race exists between `canLockBeAcquired` callers and `purgeMessages`. - -#### 4. `MusicManager` (`music/MusicManager.java`) -- **Inconsistent synchronization** — `isConnected()`, `connect()`, `disconnect()`, `play()` are `synchronized`, but `isCurrentlyPlaying()` (line 114), `getCurrentlyPlaying()` (line 110), `stopTrack()` (line 138), `resume()` (line 142) are not. If these are called from event threads concurrently with `disconnect()`, they can see stale state. - -#### 5. `BaseThreadFactory` (`core/thread/BaseThreadFactory.java`) -- Threads are not set as daemon threads, which can prevent clean JVM shutdown if an executor leaks. - -### Proposed Changes - -#### `ThreadManager` -- Replace `newCachedThreadPool()` with `newFixedThreadPool()` or a bounded `ThreadPoolExecutor` with a reasonable max (e.g., `availableProcessors() * 2`). -- Fix shutdown loop: wrap each executor's `shutdown()`/`awaitTermination()` in its own try-catch so one failure doesn't skip the rest. -- Consider adding a `getExecutorStatus()` method for observability. - -#### `BaseThreadFactory` -- Set `thread.setDaemon(true)` to prevent JVM hang on unclean shutdown. - -#### `TaskManagerImpl` -- Fix `computeIfAbsent` null return: use a sentinel or simply don't store an entry for tasks that don't need locking (use `get()` + null check instead). -- Remove misleading `synchronized` on `getTasks()` or make all task mutations consistently synchronized. -- Guard `hasTask()` with proper synchronization or accept eventual consistency and document it. -- Fix lock cleanup race in `handle()` — don't remove from `tasksLock` inside the lock-held section; let `removeTask()` handle cleanup. - -#### `ChannelMessagePurger` -- Remove the `lock.unlock()` call on line 154 inside `safePurge()` — the `finally` block in `purgeMessages()` already handles it. -- Change `instances` from `HashMap` to `ConcurrentHashMap`. - -#### `MusicManager` -- Either synchronize all public methods that touch `player`/`AudioManager` state, or remove `synchronized` from methods where JDA already guarantees thread safety and document the threading model. - -# Delivery Steps - -### ✓ Step 1: Fix ThreadManager and BaseThreadFactory -ThreadManager uses bounded pools and has correct shutdown; BaseThreadFactory produces daemon threads. - -- Replace `Executors.newCachedThreadPool()` in `ThreadManager.getExecutor()` with a bounded `ThreadPoolExecutor` (core=0, max=availableProcessors*2, 60s keepAlive, LinkedBlockingQueue). -- Fix `stop()` shutdown loop: wrap each executor's `shutdown()`+`awaitTermination()` in its own try-catch so an `InterruptedException` on one executor doesn't skip the rest. -- In `BaseThreadFactory.newThread()`, add `thread.setDaemon(true)`. -- Verify the `getScheduledExecutor()` pool size is still appropriate. - -### ✓ Step 2: Fix TaskManagerImpl thread-safety bugs -TaskManagerImpl has no null-in-ConcurrentHashMap violations and consistent synchronization. - -- Replace the `computeIfAbsent` that returns null (line 178-184) with a pattern that avoids storing null: check task type first, only call `computeIfAbsent` when a lock is actually needed. -- Remove the misleading `synchronized` keyword from `getTasks()`, or switch to a consistent locking strategy across all mutation methods. -- Fix the lock cleanup race in `handle()`: do not remove from `tasksLock` at line 193; let `removeTask()` be the sole owner of lock cleanup. -- Document the threading contract for `hasTask()` (check-then-act across two collections is inherently racy). - -### ✓ Step 3: Fix ChannelMessagePurger concurrency bugs -ChannelMessagePurger has no double-unlock and uses thread-safe collections. - -- Remove the redundant `lock.unlock()` call on line 154 in `safePurge()` — the `finally` block in `purgeMessages()` (line 79) already releases the lock. -- Change the static `instances` field (line 40) from `HashMap` to `ConcurrentHashMap`. -- Remove the `synchronized` keyword from `countAllChannelMessages()` and `safePurge()` since the `ReentrantLock` already provides mutual exclusion. - -### ✓ Step 4: Harmonize MusicManager synchronization -MusicManager has a consistent and documented threading model. - -- Add `synchronized` to `isCurrentlyPlaying()`, `getCurrentlyPlaying()`, `stopTrack()`, and `resume()` to match the existing pattern on `connect()`/`disconnect()`/`play()`. -- Alternatively, if JDA's `AudioPlayer` is already thread-safe, remove `synchronized` from all methods and add a class-level Javadoc explaining the threading model. -- Investigate which approach is correct by checking Lavaplayer's `AudioPlayer` thread-safety guarantees, then apply consistently. \ No newline at end of file