Remove deprecated classes and static singleton-based architecture
This commit is contained in:
anthorob committed
1 parent
54a27883d1
commit
b8786c8dc9
133 files changed
+4984
-1038
No files matched your search
@@ -0,0 +1,302 @@
|
||||
---
|
||||
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<Lifecycle> components = new ArrayList<>();
|
||||
public <T> 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.
|
||||
@@ -0,0 +1,214 @@
|
||||
---
|
||||
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/` |
|
||||
| `framework/NetNeutralityBot.java` | remove or merge into App |
|
||||
| `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/<category>/`, `tasks/<category>/`, `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 `framework/NetNeutralityBot.java` into `core/` or merge into App if it's just a shell
|
||||
- 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
|
||||
Reference in new issue
Block a user