Model patch lifecycle as a sealed state with a domain/persistence split - #99
Open
jpenilla wants to merge 1 commit into
Open
Model patch lifecycle as a sealed state with a domain/persistence split#99jpenilla wants to merge 1 commit into
jpenilla wants to merge 1 commit into
Conversation
Replace the mutable status enum + nullable columns with a sealed PatchState (Available / InProgress / Completed) as the domain model. Each state carries only the data valid in that state, so invalid combinations are unrepresentable: transitions are exhaustive switches (adding a state breaks compilation), and the previous requireNonNull / null-guard logic disappears. - model.Patch is now a domain record; persistence lives in repository.PatchEntity (state_type discriminator + columns), with PatchCodec as the single conversion point - V3 migrations backfill state_type/started_at from the legacy status column and drop it; responsible_user/duration/last_updated are reused as-is, preserving existing data - Restore DB-level status filtering (getPatchesByStateTypeAndMinecraftVersion) - stats() derives counts and intervals from a single per-user aggregation, removing the two-map invariant
There was a problem hiding this comment.
Pull request overview
This PR refactors the patch lifecycle to use a sealed PatchState domain model and separates persistence into a dedicated JPA PatchEntity with a state_type discriminator, with Flyway V3 migrations to transition existing data.
Changes:
- Introduces sealed
PatchState+StateType, and convertsPatchinto a domainrecordpersisted viaPatchEntityandPatchCodec. - Updates
PatchService,PatchRepository, andApiControllerto operate on the new domain/persistence split and to restore state-based filtering. - Adds Postgres and H2 V3 migrations to backfill
state_type/started_atand drop the legacystatuscolumn.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/resources/db/migration/postgresql/V3__sealed_state.sql | Adds state_type/started_at, backfills from legacy status, and drops status. |
| src/main/resources/db/migration/h2/V3__sealed_state.sql | H2 equivalent of the V3 sealed-state migration. |
| src/main/java/io/papermc/patchroulette/service/PatchService.java | Reworks lifecycle transitions to use PatchState and codec-based persistence. |
| src/main/java/io/papermc/patchroulette/repository/PatchRepository.java | Switches repository to PatchEntity and adds state-type filtering method. |
| src/main/java/io/papermc/patchroulette/repository/PatchEntity.java | New JPA entity representing persisted patch rows with state_type discriminator. |
| src/main/java/io/papermc/patchroulette/repository/PatchCodec.java | Central conversion between domain Patch and persistence PatchEntity. |
| src/main/java/io/papermc/patchroulette/model/Status.java | Removes legacy mutable status enum. |
| src/main/java/io/papermc/patchroulette/model/StateType.java | Adds enum discriminator for persisted state. |
| src/main/java/io/papermc/patchroulette/model/PatchState.java | Adds sealed lifecycle state model carrying state-specific data. |
| src/main/java/io/papermc/patchroulette/model/Patch.java | Converts Patch into a domain record with state + lastUpdated. |
| src/main/java/io/papermc/patchroulette/controller/ApiController.java | Updates API DTO mapping and stats aggregation to use PatchState. |
Suppressed comments (2)
src/main/resources/db/migration/postgresql/V3__sealed_state.sql:16
- The domain model now assumes certain columns are present for each state (e.g., WIP requires responsible_user + started_at; DONE requires responsible_user + duration), but the DB schema still allows invalid combinations. Adding CHECK constraints here would prevent corrupted rows that would otherwise crash PatchCodec's requireNonNull calls at runtime.
ALTER TABLE patch ALTER COLUMN state_type SET NOT NULL;
ALTER TABLE patch DROP COLUMN status;
src/main/resources/db/migration/h2/V3__sealed_state.sql:16
- The domain model now assumes certain columns are present for each state (e.g., WIP requires responsible_user + started_at; DONE requires responsible_user + duration), but the DB schema still allows invalid combinations. Adding CHECK constraints here would prevent corrupted rows that would otherwise crash PatchCodec's requireNonNull calls at runtime.
ALTER TABLE patch ALTER COLUMN state_type SET NOT NULL;
ALTER TABLE patch DROP COLUMN status;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| END; | ||
|
|
||
| -- Work start time was recorded in last_updated when work began. | ||
| UPDATE patch SET started_at = last_updated WHERE status = 1; |
| END; | ||
|
|
||
| -- Work start time was recorded in last_updated when work began. | ||
| UPDATE patch SET started_at = last_updated WHERE status = 1; |
| final Patch patch = this.loadPatch(patchId); | ||
| if (!(patch.state() instanceof PatchState.InProgress) | ||
| && !(patch.state() instanceof PatchState.Completed)) { | ||
| throw new IllegalStateException("Patch " + patchId + " is not WIP"); |
Comment on lines
+95
to
+101
| case PatchState.InProgress w -> { | ||
| if (!w.responsibleUser().equals(user)) { | ||
| throw new IllegalStateException( | ||
| "User " + user + " is not responsible for patch " + patchId); | ||
| } | ||
| yield new PatchState.Completed(user, Duration.between(w.startedAt(), Instant.now())); | ||
| } |
Comment on lines
+16
to
+35
| record Available() implements PatchState { | ||
| @Override | ||
| public String label() { | ||
| return "AVAILABLE"; | ||
| } | ||
| } | ||
|
|
||
| record InProgress(String responsibleUser, Instant startedAt) implements PatchState { | ||
| @Override | ||
| public String label() { | ||
| return "WIP"; | ||
| } | ||
| } | ||
|
|
||
| record Completed(String responsibleUser, Duration duration) implements PatchState { | ||
| @Override | ||
| public String label() { | ||
| return "DONE"; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace the mutable status enum + nullable columns with a sealed
PatchState (Available / InProgress / Completed) as the domain model.
Each state carries only the data valid in that state, so invalid
combinations are unrepresentable: transitions are exhaustive switches
(adding a state breaks compilation), and the previous requireNonNull /
null-guard logic disappears.
repository.PatchEntity (state_type discriminator + columns), with
PatchCodec as the single conversion point
column and drop it; responsible_user/duration/last_updated are reused
as-is, preserving existing data
removing the two-map invariant
Stack created with GitHub Stacks CLI • Give Feedback 💬