Repository navigation
Harden the three utilities shared with Deportation, and rename the group - #1
Merged
Merged
Conversation
plugin-infrastructure lifted DurationParserUtil, MessageContainer and
SchemaMigrator from sibling plugins. Deportation had already forked and fixed
all three for its 2.0.0 release, so the library shipped the pre-fix versions of
code that exists elsewhere in a better state. This brings the fixes upstream, so
adopting the library is a deletion for a consumer rather than a regression.
Each class had one theme.
DurationParserUtil is fed straight from command input, so every rejection path
must be an IllegalArgumentException and no input may produce a silently wrong
duration. Three ways it could:
- toLowerCase() used the default locale. Under a Turkish default, "MIN"
lower-cases to "mın" (dotless i), matching no unit key, so a valid duration
was rejected outright. Now Locale.ROOT.
- Long.parseLong was unguarded, so a long enough digit run threw a
NumberFormatException out of the caller's command handler.
- totalSeconds += amount * multiplier was plain long arithmetic, so a large
amount wrapped around and yielded a plausible-looking but completely wrong
duration. Now Math.addExact / Math.multiplyExact, converted to IAE.
It also gains parseAllowingPermanent, returning Optional.empty() for "no
expiry": three distinct outcomes (permanent / a real duration / invalid) so
callers stop reaching for a -1 sentinel that a parse failure can masquerade as.
The keywords are a parameter rather than a constant — the three-outcome contract
is the reusable part, the English words are the calling plugin's UX.
MessageContainer had a markup-injection hole. Callers substitute runtime values
into templates, and runtime values are routinely attacker-controlled: text
authored by one player, persisted, and re-rendered to another is the standard
shape (a punishment reason, a shop description, a mail subject). Rendered with
MiniMessage's Placeholder.parsed, a stored <click:run_command:'/op evil'> runs
that command as whoever clicks it. Stored privilege escalation, from one wrong
resolver at one call site.
The fix makes the safe path the only path: value(name, text) wraps
Placeholder.unparsed, markup(name, component) takes structured data that is
never re-parsed, and deserializeRaw is gone — "render this arbitrary string as
MiniMessage" is the same hole with extra steps. A call site that stays inside
this API never imports Placeholder, so reintroducing Placeholder.parsed shows up
in review as a new, unexplained import.
Two behavioural fixes came with it: load() now drops keys absent from the file
(it merged, so a deleted message kept being served until restart — merge belongs
at the ConfigurationNode level), and it no longer throws, since one list of
non-scalars used to fail an entire reload rather than costing one string.
SchemaMigrator needed to survive its unhappy path, which for a migrator is the
path that matters — it runs once, against real data:
- No rollback. A step that failed part-way left uncommitted work on the
connection, which the pool then handed to the next borrower.
- Statements were sent as one batch, so files needed allowMultiQueries=true in
the operator's JDBC arguments and a failure in the fifth statement was
reported against the whole file. They are now split client-side, quote- and
comment-aware so a ';' inside a literal is not a delimiter, and executed one
at a time.
- A file containing no statements recorded its version anyway, so a migration
emptied by a bad merge left the schema permanently missing the change with
nothing to notice it by.
- migrate() threw IOException as well as SQLException, so a plugin disabling
itself on a failed migration needed two catch blocks.
Docs record the invariants rather than just the API: name the version table per
plugin (two plugins sharing 'schema_version' read each other's version — one
refuses to start, the other re-applies migrations that were never its own),
write migrations to be re-runnable on engines without transactional DDL, and
note that the default statement splitter lexes MySQL/MariaDB rather than
standard SQL, which is the one dialect concern the compiler does not prompt for.
AGENTS.md's "No tests yet" stopped being true two commits after it was written.
Finally, the Maven group and every package move from net.democracrycraft — a
misspelling of DemocracyCraft — to com.minecraftcitiesnetwork. Worth doing now
because the library has no consumers yet; once one depends on it this becomes a
breaking change for everyone at once. Isolated in its own commit so the
substantive diff stays readable. The maven.democracycraft.net repository host is
spelled correctly and is untouched.
Tests go from 47 to 99. Two are worth calling out because they pin what the
dependencies actually do rather than what was assumed: MiniMessage's default
instance is lenient and does not raise on a malformed tag (so the
ParsingException fallback is a documented backstop, not a live path), and the
rollback test runs against a single-connection DataSource, since H2's
JdbcDataSource opens a fresh connection per call and would have let that test
pass with the rollback deleted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
plugin-infrastructure lifted DurationParserUtil, MessageContainer and SchemaMigrator from sibling plugins. Deportation had already forked and fixed all three for its 2.0.0 release, so the library shipped the pre-fix versions of code that exists elsewhere in a better state. This brings the fixes upstream, so adopting the library is a deletion for a consumer rather than a regression.
Each class had one theme.
DurationParserUtil is fed straight from command input, so every rejection path must be an IllegalArgumentException and no input may produce a silently wrong duration. Three ways it could:
It also gains parseAllowingPermanent, returning Optional.empty() for "no expiry": three distinct outcomes (permanent / a real duration / invalid) so callers stop reaching for a -1 sentinel that a parse failure can masquerade as. The keywords are a parameter rather than a constant — the three-outcome contract is the reusable part, the English words are the calling plugin's UX.
MessageContainer had a markup-injection hole. Callers substitute runtime values into templates, and runtime values are routinely attacker-controlled: text authored by one player, persisted, and re-rendered to another is the standard shape (a punishment reason, a shop description, a mail subject). Rendered with MiniMessage's Placeholder.parsed, a stored <click:run_command:'/op evil'> runs that command as whoever clicks it. Stored privilege escalation, from one wrong resolver at one call site.
The fix makes the safe path the only path: value(name, text) wraps Placeholder.unparsed, markup(name, component) takes structured data that is never re-parsed, and deserializeRaw is gone — "render this arbitrary string as MiniMessage" is the same hole with extra steps. A call site that stays inside this API never imports Placeholder, so reintroducing Placeholder.parsed shows up in review as a new, unexplained import.
Two behavioural fixes came with it: load() now drops keys absent from the file (it merged, so a deleted message kept being served until restart — merge belongs at the ConfigurationNode level), and it no longer throws, since one list of non-scalars used to fail an entire reload rather than costing one string.
SchemaMigrator needed to survive its unhappy path, which for a migrator is the path that matters — it runs once, against real data:
Docs record the invariants rather than just the API: name the version table per plugin (two plugins sharing 'schema_version' read each other's version — one refuses to start, the other re-applies migrations that were never its own), write migrations to be re-runnable on engines without transactional DDL, and note that the default statement splitter lexes MySQL/MariaDB rather than standard SQL, which is the one dialect concern the compiler does not prompt for. AGENTS.md's "No tests yet" stopped being true two commits after it was written.
Finally, the Maven group and every package move from net.democracrycraft — a misspelling of DemocracyCraft — to com.minecraftcitiesnetwork. Worth doing now because the library has no consumers yet; once one depends on it this becomes a breaking change for everyone at once. Isolated in its own commit so the substantive diff stays readable. The maven.democracycraft.net repository host is spelled correctly and is untouched.
Tests go from 47 to 99. Two are worth calling out because they pin what the dependencies actually do rather than what was assumed: MiniMessage's default instance is lenient and does not raise on a malformed tag (so the ParsingException fallback is a documented backstop, not a live path), and the rollback test runs against a single-connection DataSource, since H2's JdbcDataSource opens a fresh connection per call and would have let that test pass with the rollback deleted.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com