Refactor persistable-task system with self-registering tasks

Replaced metadata-driven reflection with self-describing `PersistableTask` types and a new `PersistableTaskRegistry`. Removed `persistable-tasks.json` and `TaskRegistry`, migrating to type-safe DI instantiation and explicit `restore()` methods. Fixed task restoration issues, ensuring `EmojiReactionRoleTask` and its `emojiMapping` now persist and restore correctly. Introduced `typeId()` for stable task identification, updated payload serialization for symmetry, and applied database migration to map FQCNs to logical IDs. Added unit tests for robust type discovery, payload symmetry, and registry behavior, ensuring no breaking changes.
This commit is contained in:
anthorob committed 2026-09-25 07:35:45 -04:00
1 parent 9f2e15a622
commit 05b4a2ab85
19 files changed
+1020 -291

No files matched your search

+259
View File
@@ -0,0 +1,259 @@
---
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 `<dependency>` 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<String,String>`.
### 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.
@@ -0,0 +1,280 @@
---
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<String, TaskCreator>`.
- `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<String, Class<? extends PersistableTask>> 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`.