From 20a78e9903880870c7d23221f2012e8d040f1d63 Mon Sep 17 00:00:00 2001 From: Ling Bao Date: Sun, 6 Sep 2026 23:19:15 +1000 Subject: [PATCH 1/7] test(13-08): pin the shipped sidebar defaults to what a fresh install can render MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a Default Sidebar Renderability nested test class asserting that every placeholder token found in the shipped default lines list is either resolved by this module itself (currently none are — the module's own SideBarService.parsePlaceholders() does nothing but delegate to PlaceholderAPI or return the text unchanged) or is a real, documented PlaceholderAPI placeholder requiring the external provider. The assertion enumerates every %token% found in the defaults rather than grepping for one known-bad string, so a sibling token added later is caught too. A second test proves the fix touches shipped defaults only — an operator-configured lines list (including one still carrying the broken token) passes through unchanged. RED, observed: defaultLinesContainNoTokenThatNothingResolves fails naming "world_name" as the unresolvable token. %world_name% is not a real PlaceholderAPI placeholder syntax at all — PlaceholderAPI's "World" expansion's actual world-name placeholder is %world_name_% (an explicit world argument is mandatory), and the bundled Player expansion's placeholder for "the world the current player is in" is %player_world%. A bare %world_name% resolves under neither, so it stays literal on screen regardless of whether PlaceholderAPI is installed. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv --- .../sidebar/config/SideBarConfigTest.java | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java b/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java index 969e576..35d95fd 100644 --- a/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java +++ b/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java @@ -2,8 +2,14 @@ import org.junit.jupiter.api.*; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import static org.assertj.core.api.Assertions.*; @@ -139,6 +145,86 @@ void emptyLines() { } } + // ============================ + // Default sidebar renderability + // ============================ + + @Nested + @DisplayName("Default Sidebar Renderability") + class DefaultSidebarRenderabilityTests { + + /** + * Placeholder tokens the shipped defaults are allowed to contain. Every one of these + * is a real PlaceholderAPI placeholder that resolves once PlaceholderAPI (and, for + * vault_eco_balance_formatted, Vault plus an economy provider) is installed -- + * "documented as requiring the external placeholder provider" is a legitimate category, + * distinct from a token that resolves nowhere no matter what is installed. + */ + private final Set KNOWN_RESOLVABLE_TOKENS = new HashSet<>(Arrays.asList( + "player_name", + "server_online", + "server_max_players", + "player_world", + "vault_eco_balance_formatted", + "player_ping" + )); + + private boolean isKnownResolvable(String token) { + if (KNOWN_RESOLVABLE_TOKENS.contains(token)) { + return true; + } + // PlaceholderAPI's Server expansion accepts an arbitrary SimpleDateFormat pattern + // as a dynamic suffix: %server_time_%. + return token.startsWith("server_time_"); + } + + private List extractTokens(List lines) { + Pattern tokenPattern = Pattern.compile("%([a-zA-Z0-9_:]+)%"); + List tokens = new ArrayList<>(); + for (String line : lines) { + Matcher matcher = tokenPattern.matcher(line); + while (matcher.find()) { + tokens.add(matcher.group(1)); + } + } + return tokens; + } + + @Test + @DisplayName("Default lines contain no token that nothing resolves") + void defaultLinesContainNoTokenThatNothingResolves() { + SideBarConfig config = createRealConfig(); + + List tokens = extractTokens(config.getLines()); + assertThat(tokens).isNotEmpty(); + + List unresolvableTokens = new ArrayList<>(); + for (String token : tokens) { + if (!isKnownResolvable(token)) { + unresolvableTokens.add(token); + } + } + + assertThat(unresolvableTokens) + .as("Every placeholder token in the shipped defaults must be a real, " + + "resolvable PlaceholderAPI placeholder -- not a token nothing provides") + .isEmpty(); + } + + @Test + @DisplayName("An operator-configured line is unaffected") + void anOperatorConfiguredLineIsUnaffected() { + SideBarConfig config = createRealConfig(); + List customLines = Arrays.asList( + "&aCustom Line 1", "%world_name%", "&bAnother line" + ); + + config.setLines(customLines); + + assertThat(config.getLines()).isEqualTo(customLines); + } + } + /** * Create a real SideBarConfig instance. * The no-arg constructor calls super("config/sidebar.yml") which only stores the path From 3205b685e85b830d6441f315c944175b3e8925c7 Mon Sep 17 00:00:00 2001 From: Ling Bao Date: Sun, 6 Sep 2026 23:20:12 +1000 Subject: [PATCH 2/7] fix(13-08): ship a default sidebar that renders without extra plugins Replaces the shipped default sidebar's %world_name% token with %player_world%. %world_name% is not valid PlaceholderAPI syntax at all: the "World" expansion's real world-name placeholder is %world_name_% (an explicit world argument is mandatory), and the bundled Player expansion's placeholder for "the world the current player is in" is %player_world%. The old token therefore stayed literal on every player's screen regardless of whether PlaceholderAPI was installed -- it was never a case of "needs an external plugin", it was simply not a placeholder anything recognizes. Every other token already shipped in the defaults (player_name, server_online, server_max_players, vault_eco_balance_formatted, player_ping, server_time_hh:mm:ss) is real, documented PlaceholderAPI syntax that legitimately requires the external provider to resolve -- left unchanged, per the "resolved by the module or documented as requiring the provider" rule; this module's own SideBarService.parsePlaceholders() resolves nothing itself. Closes UltiKits/UltiSideBar#13 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv --- .../java/com/ultikits/plugins/sidebar/config/SideBarConfig.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java b/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java index a94600e..53e989f 100644 --- a/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java +++ b/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java @@ -44,7 +44,7 @@ public class SideBarConfig extends AbstractConfigEntity { "&7欢迎, &f%player_name%", "", "&e在线人数: &f%server_online%/%server_max_players%", - "&e世界: &f%world_name%", + "&e世界: &f%player_world%", "", "&e金币: &f%vault_eco_balance_formatted%", "&ePing: &f%player_ping%ms", From 03f4139d7561d5e0b3f6727f5094e57fa84ce73a Mon Sep 17 00:00:00 2001 From: Ling Bao Date: Mon, 7 Sep 2026 01:41:58 +1000 Subject: [PATCH 3/7] test(13-08): prove the legacy %world_name% line survives upgrade without migration, and drive real init()/placeholder resolution CR-01: AbstractConfigEntity.init() only fills missing keys and never overwrites a persisted "lines" list, so every server that already ran this plugin keeps the old %world_name% default forever. Adds a RED test (SideBarConfig.migrateLegacyWorldNameDefaultLine() does not exist yet) that starts from a persisted sidebar.yml carrying the old default line plus an operator's custom line, and asserts only the stale line is rewritten -- both in memory and on disk -- while the custom line survives untouched, plus a no-op case once already migrated and a case proving a line that only mentions the old token inside other text is left alone. WR-01: rewrites defaultLinesContainNoTokenThatNothingResolves to route every default line through SideBarService's real parsePlaceholders()/PlaceholderAPI seam (stubbed to behave like a real installation: known placeholders substituted, unknown ones left literal) instead of checking token names against a hand-authored allow-list mirroring this same file's own defaults -- closing the self-consistent-but-wrong gap where a broken token and its allow-list entry could land in the same commit. This also widens the leftover-token regex to %[^%]+% (was [a-zA-Z0-9_:]+), incidentally resolving IN-01's narrow-token-shape concern. WR-02: rewrites anOperatorConfiguredLineIsUnaffected to drive a real init() against a temp sidebar.yml instead of round-tripping a bare Lombok setter/getter, which could not fail for any change to SideBarConfig's default-handling behavior. Adds mockPluginBackedBy(), a Mockito default-Answer double, because UltiToolsPlugin's getConfigFolder()/getConfigFile() are protected final and declared outside this test's package -- a normal when(mock.getConfigFolder()) does not compile from here. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv --- .../sidebar/config/SideBarConfigTest.java | 256 ++++++++++++++---- 1 file changed, 201 insertions(+), 55 deletions(-) diff --git a/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java b/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java index 35d95fd..a4c11e2 100644 --- a/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java +++ b/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java @@ -1,17 +1,32 @@ package com.ultikits.plugins.sidebar.config; -import org.junit.jupiter.api.*; +import com.ultikits.plugins.sidebar.UltiSideBarTestHelper; +import com.ultikits.plugins.sidebar.service.SideBarService; +import com.ultikits.ultitools.abstracts.UltiToolsPlugin; +import me.clip.placeholderapi.PlaceholderAPI; +import org.bukkit.configuration.file.YamlConfiguration; +import org.bukkit.entity.Player; +import org.junit.jupiter.api.*; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.Answers; +import org.mockito.MockedStatic; +import org.mockito.Mockito; + +import java.io.File; +import java.lang.reflect.Method; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.HashSet; import java.util.List; -import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; import static org.assertj.core.api.Assertions.*; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; @DisplayName("SideBarConfig Tests") class SideBarConfigTest { @@ -154,74 +169,205 @@ void emptyLines() { class DefaultSidebarRenderabilityTests { /** - * Placeholder tokens the shipped defaults are allowed to contain. Every one of these - * is a real PlaceholderAPI placeholder that resolves once PlaceholderAPI (and, for - * vault_eco_balance_formatted, Vault plus an economy provider) is installed -- - * "documented as requiring the external placeholder provider" is a legitimate category, - * distinct from a token that resolves nowhere no matter what is installed. + * Sentinel substitutions for the placeholders a real PlaceholderAPI installation (Player + * + Server expansions) resolves, sourced independently from PlaceholderAPI's own + * placeholder wiki rather than re-derived from the defaults under test in this file -- + * so a future commit cannot introduce a broken token and "fix" this test in the same + * edit by adding the same name to a local allow-list. + *

+ * {@code vault_eco_balance_formatted} is deliberately excluded (WR-03): it additionally + * requires Vault plus a registered economy provider, a materially larger install surface + * than "PlaceholderAPI is installed", so it is exempted below by name rather than + * silently substituted here. */ - private final Set KNOWN_RESOLVABLE_TOKENS = new HashSet<>(Arrays.asList( - "player_name", - "server_online", - "server_max_players", - "player_world", - "vault_eco_balance_formatted", - "player_ping" - )); - - private boolean isKnownResolvable(String token) { - if (KNOWN_RESOLVABLE_TOKENS.contains(token)) { - return true; - } - // PlaceholderAPI's Server expansion accepts an arbitrary SimpleDateFormat pattern - // as a dynamic suffix: %server_time_%. - return token.startsWith("server_time_"); - } - - private List extractTokens(List lines) { - Pattern tokenPattern = Pattern.compile("%([a-zA-Z0-9_:]+)%"); - List tokens = new ArrayList<>(); - for (String line : lines) { - Matcher matcher = tokenPattern.matcher(line); - while (matcher.find()) { - tokens.add(matcher.group(1)); + private static final String VAULT_DEPENDENT_TOKEN = "%vault_eco_balance_formatted%"; + + private String stubResolve(String text) { + String resolved = text + .replace("%player_name%", "Steve") + .replace("%server_online%", "12") + .replace("%server_max_players%", "100") + .replace("%player_world%", "world") + .replace("%player_ping%", "42"); + // PlaceholderAPI's Server expansion accepts an arbitrary SimpleDateFormat pattern as + // a dynamic suffix: %server_time_%. + return resolved.replaceAll("%server_time_[^%]+%", "12:00:00"); + } + + @Test + @DisplayName("Default lines contain no token that nothing resolves") + void defaultLinesContainNoTokenThatNothingResolves() throws Exception { + SideBarConfig config = createRealConfig(); + + // Route every default line through the module's own placeholder-resolution path + // (SideBarService.parsePlaceholders -> PlaceholderAPI.setPlaceholders) instead of + // checking token names against a hand-authored allow-list mirroring this same + // file's defaults -- that allow-list could never catch a broken token added + // alongside a matching allow-list entry in the same commit. The PlaceholderAPI seam + // is stubbed to behave the way a real installation does: a recognized placeholder is + // substituted, and one no registered expansion recognizes is left untouched in the + // output -- exactly the symptom the original issue (#13) reported. + SideBarService service = new SideBarService(); + UltiSideBarTestHelper.setField(service, "placeholderApiAvailable", true); + Player player = Mockito.mock(Player.class); + + Method parsePlaceholders = SideBarService.class + .getDeclaredMethod("parsePlaceholders", Player.class, String.class); + parsePlaceholders.setAccessible(true); + + try (MockedStatic placeholderApi = Mockito.mockStatic(PlaceholderAPI.class)) { + placeholderApi.when(() -> PlaceholderAPI.setPlaceholders(eq(player), anyString())) + .thenAnswer(invocation -> stubResolve(invocation.getArgument(1))); + + List unresolvedTokensRemaining = new ArrayList<>(); + Pattern leftoverTokenPattern = Pattern.compile("%[^%]+%"); + for (String line : config.getLines()) { + String rendered = (String) parsePlaceholders.invoke(service, player, line); + Matcher matcher = leftoverTokenPattern.matcher(rendered); + while (matcher.find()) { + String leftover = matcher.group(); + if (!VAULT_DEPENDENT_TOKEN.equals(leftover)) { + unresolvedTokensRemaining.add(leftover); + } + } } + + assertThat(unresolvedTokensRemaining) + .as("Every default line must render through the module's own placeholder " + + "path with no leftover token that nothing resolves") + .isEmpty(); } - return tokens; } @Test - @DisplayName("Default lines contain no token that nothing resolves") - void defaultLinesContainNoTokenThatNothingResolves() { + @DisplayName("An operator-configured line survives init() against a persisted file that also holds the legacy default") + void anOperatorConfiguredLineIsUnaffected(@TempDir Path tempDir) throws Exception { + // Drives the real init()-mediated persisted-file-vs-default precedence (CR-01) -- + // a bare setLines()/getLines() round-trip cannot fail for any change to + // SideBarConfig's default-handling behavior and proves nothing about upgrade safety. + File configFile = new File(tempDir.toFile(), "config/sidebar.yml"); + Files.createDirectories(configFile.getParentFile().toPath()); + YamlConfiguration persisted = new YamlConfiguration(); + persisted.set("lines", Arrays.asList( + "&e世界: &f%world_name%", + "&aOperator's own custom line" + )); + persisted.save(configFile); + SideBarConfig config = createRealConfig(); + config.init(mockPluginBackedBy(tempDir)); - List tokens = extractTokens(config.getLines()); - assertThat(tokens).isNotEmpty(); + assertThat(config.getLines()) + .as("an operator's own persisted line must survive init() untouched") + .contains("&aOperator's own custom line"); + } + } - List unresolvableTokens = new ArrayList<>(); - for (String token : tokens) { - if (!isKnownResolvable(token)) { - unresolvableTokens.add(token); - } + /** + * Builds an {@code UltiToolsPlugin} test double whose {@code getConfigFolder()}/ + * {@code getConfigFile(String)} resolve against {@code tempDir}. Those two methods are + * {@code protected final} on {@code UltiToolsPlugin}, declared outside this test's package, + * so a normal {@code Mockito.when(mock.getConfigFolder())...} does not even compile here -- + * this uses Mockito's {@code mock(Class, Answer)} default-answer form instead, which + * intercepts every method call by reflection ({@code invocation.getMethod()}) rather than by + * a source-level call to the (inaccessible) method. + */ + private static UltiToolsPlugin mockPluginBackedBy(Path tempDir) { + return Mockito.mock(UltiToolsPlugin.class, invocation -> { + String methodName = invocation.getMethod().getName(); + if ("getConfigFolder".equals(methodName)) { + return tempDir.toString(); } + if ("getConfigFile".equals(methodName)) { + String path = invocation.getArgument(0); + return new File(tempDir.toFile(), path); + } + return Answers.RETURNS_DEFAULTS.answer(invocation); + }); + } - assertThat(unresolvableTokens) - .as("Every placeholder token in the shipped defaults must be a real, " + - "resolvable PlaceholderAPI placeholder -- not a token nothing provides") - .isEmpty(); + // ============================ + // Legacy %world_name% default line migration (issue #13, CR-01) + // ============================ + + @Nested + @DisplayName("Legacy World-Name Default Line Migration") + class LegacyWorldNameLineMigration { + + @TempDir + Path tempDir; + + private UltiToolsPlugin mockPlugin; + + @BeforeEach + void setUp() { + mockPlugin = mockPluginBackedBy(tempDir); + } + + private File persistLines(List lines) throws Exception { + File configFile = new File(tempDir.toFile(), "config/sidebar.yml"); + Files.createDirectories(configFile.getParentFile().toPath()); + YamlConfiguration persisted = new YamlConfiguration(); + persisted.set("lines", lines); + persisted.save(configFile); + return configFile; } @Test - @DisplayName("An operator-configured line is unaffected") - void anOperatorConfiguredLineIsUnaffected() { - SideBarConfig config = createRealConfig(); - List customLines = Arrays.asList( - "&aCustom Line 1", "%world_name%", "&bAnother line" - ); + @DisplayName("Rewrites a persisted line byte-identical to the old %world_name% default; a custom line survives") + void rewritesLegacyLineButLeavesCustomLineUntouched() throws Exception { + File configFile = persistLines(Arrays.asList( + "&7欢迎, &f%player_name%", + "&e世界: &f%world_name%", + "&aOperator's own custom line" + )); + + SideBarConfig config = new SideBarConfig(); + config.init(mockPlugin); + + boolean rewritten = config.migrateLegacyWorldNameDefaultLine(); + assertThat(rewritten).isTrue(); + config.save(); + + assertThat(config.getLines()) + .as("the stale %world_name% line must be rewritten to the corrected default") + .contains("&e世界: &f%player_world%") + .doesNotContain("&e世界: &f%world_name%"); + assertThat(config.getLines()) + .as("an operator's own custom line must be left untouched") + .contains("&aOperator's own custom line"); + + YamlConfiguration onDisk = YamlConfiguration.loadConfiguration(configFile); + assertThat(onDisk.getStringList("lines")) + .as("the migration must be persisted back to disk") + .contains("&e世界: &f%player_world%", "&aOperator's own custom line") + .doesNotContain("&e世界: &f%world_name%"); + } + + @Test + @DisplayName("Does not touch a line that merely mentions %world_name% inside other text") + void doesNotTouchLineThatOnlyMentionsTheLegacyToken() throws Exception { + persistLines(Collections.singletonList("&7Custom: &f%world_name% (renamed by admin)")); + + SideBarConfig config = new SideBarConfig(); + config.init(mockPlugin); + + boolean rewritten = config.migrateLegacyWorldNameDefaultLine(); + + assertThat(rewritten).isFalse(); + assertThat(config.getLines()) + .containsExactly("&7Custom: &f%world_name% (renamed by admin)"); + } + + @Test + @DisplayName("Is a no-op once the persisted line already uses the corrected placeholder") + void noOpWhenAlreadyMigrated() throws Exception { + persistLines(Collections.singletonList("&e世界: &f%player_world%")); - config.setLines(customLines); + SideBarConfig config = new SideBarConfig(); + config.init(mockPlugin); - assertThat(config.getLines()).isEqualTo(customLines); + assertThat(config.migrateLegacyWorldNameDefaultLine()).isFalse(); } } From ed30860d15582f2be866eafb38f866c31b697ab1 Mon Sep 17 00:00:00 2001 From: Ling Bao Date: Mon, 7 Sep 2026 01:43:38 +1000 Subject: [PATCH 4/7] fix(13-08): migrate the persisted %world_name% default line so upgrades actually reach it CR-01: AbstractConfigEntity.init() only fills a @ConfigEntry key that is MISSING from the persisted file and never overwrites one that already exists, so the earlier %world_name% -> %player_world% default fix (3205b68) never reached any server that had already run this plugin -- exactly the population issue #13 was filed against. Adds SideBarConfig.migrateLegacyWorldNameDefaultLine(), a targeted, exact-match, one-time migration: it rewrites a persisted "lines" entry only when it is byte-identical to the OLD shipped default, leaving any operator customisation -- including a line that merely mentions %world_name% inside other text -- untouched. Wired into SideBarService.init() (called from UltiSideBar.registerSelf() and, via reload(), from reloadSelf()), which is the earliest module-owned hook that runs after AbstractConfigEntity.init() has already populated the config from disk; init() itself is final and cannot be overridden, and a ConfigChangeListener registered here would miss the very first load. Idempotent, so running again on every reload is harmless. Also applies IN-01: %server_time_hh:mm:ss% used a 12-hour pattern with no AM/PM marker; changed to %server_time_HH:mm:ss% (24-hour). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv --- .../plugins/sidebar/config/SideBarConfig.java | 53 ++++++++++++++++++- .../sidebar/service/SideBarService.java | 17 ++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java b/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java index 53e989f..41a9c2e 100644 --- a/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java +++ b/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java @@ -1,5 +1,6 @@ package com.ultikits.plugins.sidebar.config; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -50,7 +51,7 @@ public class SideBarConfig extends AbstractConfigEntity { "&ePing: &f%player_ping%ms", "", "&7服务器时间", - "&f%server_time_hh:mm:ss%", + "&f%server_time_HH:mm:ss%", "", "&6play.example.com" ); @@ -64,4 +65,54 @@ public class SideBarConfig extends AbstractConfigEntity { public SideBarConfig() { super("config/sidebar.yml"); } + + /** + * The pre-6.3.0 shipped default world-name line, which used the invalid PlaceholderAPI + * syntax {@code %world_name%} (UltiKits/UltiSideBar#13 -- the real "World" expansion + * placeholder, {@code %world_name_%}, requires an explicit world argument). + * {@code AbstractConfigEntity.init()} never overwrites a key that already exists on disk, + * so any server that has ever started this plugin keeps this exact string in its persisted + * {@code sidebar.yml} forever unless it is rewritten explicitly. + */ + private static final String LEGACY_WORLD_NAME_LINE = "&e世界: &f%world_name%"; + + /** + * The corrected default that replaces {@link #LEGACY_WORLD_NAME_LINE}, kept in sync by hand + * with the "lines" default above. + */ + private static final String CURRENT_WORLD_NAME_LINE = "&e世界: &f%player_world%"; + + /** + * One-time migration for a persisted {@code sidebar.yml} whose {@code lines} list still + * carries the old, invalid {@link #LEGACY_WORLD_NAME_LINE} default (issue #13). Rewrites + * only a list entry that is byte-identical to that old default -- any operator + * customisation, including a line that merely mentions {@code %world_name%} alongside other + * text, is left untouched. Idempotent: once migrated, no entry matches + * {@link #LEGACY_WORLD_NAME_LINE} any more, so a second call is a no-op. + *

+ * Must be called after {@code init(UltiToolsPlugin)} has populated {@link #lines} from + * disk. The caller is responsible for persisting the result with {@code save()} when this + * method returns {@code true} -- this method only updates the in-memory value. + * + * @return {@code true} if at least one line was rewritten, {@code false} otherwise + */ + public boolean migrateLegacyWorldNameDefaultLine() { + if (lines == null) { + return false; + } + boolean changed = false; + List migrated = new ArrayList<>(lines.size()); + for (String line : lines) { + if (LEGACY_WORLD_NAME_LINE.equals(line)) { + migrated.add(CURRENT_WORLD_NAME_LINE); + changed = true; + } else { + migrated.add(line); + } + } + if (changed) { + lines = migrated; + } + return changed; + } } diff --git a/src/main/java/com/ultikits/plugins/sidebar/service/SideBarService.java b/src/main/java/com/ultikits/plugins/sidebar/service/SideBarService.java index e4700d6..87760c2 100644 --- a/src/main/java/com/ultikits/plugins/sidebar/service/SideBarService.java +++ b/src/main/java/com/ultikits/plugins/sidebar/service/SideBarService.java @@ -15,6 +15,7 @@ import org.bukkit.scoreboard.*; import org.bukkit.scheduler.BukkitTask; +import java.io.IOException; import java.util.*; import java.util.concurrent.ConcurrentHashMap; @@ -59,6 +60,22 @@ public void init() { dataOperator = plugin.getDataOperator(SideBarPreference.class); bukkitPlugin = Bukkit.getPluginManager().getPlugin("UltiTools"); + // One-time migration (issue #13, CR-01): AbstractConfigEntity.init() -- which has + // already run by this point, via UltiToolsPlugin's constructor -- only fills keys that + // are MISSING from the persisted file and never overwrites an existing "lines" value, + // so a server that has ever started an older version of this plugin keeps the invalid + // %world_name% default forever without this explicit, exact-match rewrite. Runs again + // on every reload() (this method is also called from reload()), which is harmless: once + // migrated, the exact-match check finds nothing left to rewrite. + if (config.migrateLegacyWorldNameDefaultLine()) { + try { + config.save(); + } catch (IOException e) { + plugin.getLogger().warn("Failed to persist the sidebar.yml %world_name% " + + "placeholder migration: " + e.getMessage()); + } + } + // Check PlaceholderAPI placeholderApiAvailable = Bukkit.getPluginManager().getPlugin("PlaceholderAPI") != null; if (!placeholderApiAvailable) { From a378d4cc0350658a8b15a7095fbd90cb8500e7d9 Mon Sep 17 00:00:00 2001 From: Ling Bao Date: Mon, 7 Sep 2026 02:15:31 +1000 Subject: [PATCH 5/7] test(13-08): reject invalid server-time patterns in the renderability stub The stub's %server_time_% substitution accepted any nonempty suffix, even one no real SimpleDateFormat pattern can parse (e.g. "foo", since 'f' and 'o' are not SimpleDateFormat pattern letters and the constructor throws IllegalArgumentException). A shipped default with such a suffix would fail at runtime against a real PlaceholderAPI installation while defaultLinesContainNoTokenThatNothingResolves() still passed. The stub now attempts to construct SimpleDateFormat(suffix) and only substitutes a resolved value when that succeeds, leaving an invalid suffix as an unresolved token exactly like the real Server expansion would. A regression test pins both the reject and accept paths. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv --- .../sidebar/config/SideBarConfigTest.java | 40 +++++++++++++++++-- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java b/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java index a4c11e2..efdaae2 100644 --- a/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java +++ b/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java @@ -182,6 +182,8 @@ class DefaultSidebarRenderabilityTests { */ private static final String VAULT_DEPENDENT_TOKEN = "%vault_eco_balance_formatted%"; + private final Pattern serverTimeToken = Pattern.compile("%server_time_([^%]+)%"); + private String stubResolve(String text) { String resolved = text .replace("%player_name%", "Steve") @@ -189,9 +191,28 @@ private String stubResolve(String text) { .replace("%server_max_players%", "100") .replace("%player_world%", "world") .replace("%player_ping%", "42"); - // PlaceholderAPI's Server expansion accepts an arbitrary SimpleDateFormat pattern as - // a dynamic suffix: %server_time_%. - return resolved.replaceAll("%server_time_[^%]+%", "12:00:00"); + // PlaceholderAPI's Server expansion accepts a SimpleDateFormat pattern as a dynamic + // suffix: %server_time_%. A real installation only resolves it if + // the suffix is a legal SimpleDateFormat pattern -- an illegal pattern letter (e.g. + // %server_time_foo%, "f" is not a pattern letter) makes java.text.SimpleDateFormat's + // constructor throw IllegalArgumentException, so the real expansion cannot format it. + // Substituting every suffix unconditionally, as an earlier revision of this stub did, + // would let this test pass a shipped default that fails to render at runtime. + Matcher serverTimeMatcher = serverTimeToken.matcher(resolved); + StringBuffer buffer = new StringBuffer(); + while (serverTimeMatcher.find()) { + String suffix = serverTimeMatcher.group(1); + String replacement; + try { + new java.text.SimpleDateFormat(suffix).format(new java.util.Date()); + replacement = "12:00:00"; + } catch (IllegalArgumentException invalidPattern) { + replacement = serverTimeMatcher.group(); + } + serverTimeMatcher.appendReplacement(buffer, Matcher.quoteReplacement(replacement)); + } + serverTimeMatcher.appendTail(buffer); + return buffer.toString(); } @Test @@ -239,6 +260,19 @@ void defaultLinesContainNoTokenThatNothingResolves() throws Exception { } } + @Test + @DisplayName("The stub leaves an invalid server-time pattern unresolved, matching a real installation") + void stubResolveRejectsAnInvalidServerTimePattern() { + // Regression guard for the earlier permissive regex ("%server_time_[^%]+%" -> + // "12:00:00" unconditionally), which would let defaultLinesContainNoTokenThatNothingResolves() + // pass a shipped default containing an illegal SimpleDateFormat suffix -- java.text. + // SimpleDateFormat throws IllegalArgumentException on an illegal pattern letter such + // as 'f' or 'o', so a real PlaceholderAPI Server expansion cannot format + // "%server_time_foo%" either. The stub must mirror that failure, not paper over it. + assertThat(stubResolve("%server_time_foo%")).isEqualTo("%server_time_foo%"); + assertThat(stubResolve("&f%server_time_HH:mm:ss%")).isEqualTo("&f12:00:00"); + } + @Test @DisplayName("An operator-configured line survives init() against a persisted file that also holds the legacy default") void anOperatorConfiguredLineIsUnaffected(@TempDir Path tempDir) throws Exception { From 1694c8fd420c35e256df6e0d5816228b851e18f1 Mon Sep 17 00:00:00 2001 From: Ling Bao Date: Mon, 7 Sep 2026 02:39:44 +1000 Subject: [PATCH 6/7] test(13-08): prove the legacy 12-hour server-time line also survives upgrade PR #15 round-3 review (thread 3944542674): migrateLegacyWorldNameDefaultLine() only rewrites the %world_name% entry. AbstractConfigEntity.init() preserves the whole persisted "lines" list, so a server whose sidebar.yml still holds the byte-identical old shipped "&f%server_time_hh:mm:ss%" line keeps the ambiguous 12-hour time with no AM/PM marker forever, even though the shipped default was corrected to "&f%server_time_HH:mm:ss%". Two new RED tests under SideBarConfigTest$LegacyWorldNameLineMigration: - rewritesLegacyServerTimeLine: a persisted list containing only the legacy time line is not rewritten (asserts true, gets false). - rewritesBothLegacyDefaultsOnRealUpgrade: a real upgrade scenario with both legacy entries plus an operator's custom line -- the time line survives unmigrated while the world-name line and custom line behave correctly. Both fail against current behavior, confirming the defect the reviewer described. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv --- .../sidebar/config/SideBarConfigTest.java | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java b/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java index efdaae2..8c1b7ba 100644 --- a/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java +++ b/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java @@ -403,6 +403,55 @@ void noOpWhenAlreadyMigrated() throws Exception { assertThat(config.migrateLegacyWorldNameDefaultLine()).isFalse(); } + + @Test + @DisplayName("Also rewrites a persisted line byte-identical to the old 12-hour server-time default (PR #15 round-3 review)") + void rewritesLegacyServerTimeLine() throws Exception { + File configFile = persistLines(Collections.singletonList("&f%server_time_hh:mm:ss%")); + + SideBarConfig config = new SideBarConfig(); + config.init(mockPlugin); + + boolean rewritten = config.migrateLegacyWorldNameDefaultLine(); + assertThat(rewritten) + .as("a persisted server-time line using the ambiguous 12-hour pattern must be migrated too") + .isTrue(); + config.save(); + + assertThat(config.getLines()) + .contains("&f%server_time_HH:mm:ss%") + .doesNotContain("&f%server_time_hh:mm:ss%"); + + YamlConfiguration onDisk = YamlConfiguration.loadConfiguration(configFile); + assertThat(onDisk.getStringList("lines")) + .contains("&f%server_time_HH:mm:ss%") + .doesNotContain("&f%server_time_hh:mm:ss%"); + } + + @Test + @DisplayName("Rewrites both stale legacy defaults together on a real upgrade path, leaving the operator's custom line untouched") + void rewritesBothLegacyDefaultsOnRealUpgrade() throws Exception { + persistLines(Arrays.asList( + "&7欢迎, &f%player_name%", + "&e世界: &f%world_name%", + "&aOperator's own custom line", + "&f%server_time_hh:mm:ss%" + )); + + SideBarConfig config = new SideBarConfig(); + config.init(mockPlugin); + + boolean rewritten = config.migrateLegacyWorldNameDefaultLine(); + + assertThat(rewritten).isTrue(); + assertThat(config.getLines()) + .as("both stale legacy defaults must be corrected in the same pass") + .contains("&e世界: &f%player_world%", "&f%server_time_HH:mm:ss%") + .doesNotContain("&e世界: &f%world_name%", "&f%server_time_hh:mm:ss%"); + assertThat(config.getLines()) + .as("an operator's own custom line must survive untouched") + .contains("&aOperator's own custom line"); + } } /** From e6792ed712eecd70813b38d3df227f026cb525bf Mon Sep 17 00:00:00 2001 From: Ling Bao Date: Mon, 7 Sep 2026 02:41:38 +1000 Subject: [PATCH 7/7] fix(13-08): also migrate the persisted legacy 12-hour server-time line PR #15 round-3 review (thread 3944542674): the HH:mm:ss correction reached fresh installs only. AbstractConfigEntity.init() preserves a persisted "lines" list wholesale, and migrateLegacyWorldNameDefaultLine() rewrote only the byte-identical %world_name% entry, so an upgrading server kept the old shipped "&f%server_time_hh:mm:ss%" line -- an ambiguous 12-hour time with no AM/PM marker -- forever. Renamed the method to migrateLegacyDefaultLines() and generalised it to an exact-match lookup table (LEGACY_LINE_REPLACEMENTS) covering both tracked legacy defaults: the %world_name% world line and the hh:mm:ss server-time line. A future shipped-default correction extends the map, not the loop. Updated SideBarService.init()'s call site and comment to match. Verified: mvn -B verify -- Tests run: 124, Failures: 0, Errors: 0, Skipped: 0; BUILD SUCCESS; jacoco "All coverage checks have been met." Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv --- .../plugins/sidebar/config/SideBarConfig.java | 51 ++++++++++++++++--- .../sidebar/service/SideBarService.java | 22 ++++---- .../sidebar/config/SideBarConfigTest.java | 14 ++--- 3 files changed, 62 insertions(+), 25 deletions(-) diff --git a/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java b/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java index 41a9c2e..ca5a137 100644 --- a/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java +++ b/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java @@ -3,7 +3,9 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import com.ultikits.ultitools.abstracts.AbstractConfigEntity; import com.ultikits.ultitools.annotations.ConfigEntity; @@ -82,13 +84,45 @@ public SideBarConfig() { */ private static final String CURRENT_WORLD_NAME_LINE = "&e世界: &f%player_world%"; + /** + * The pre-6.3.0 shipped default server-time line, which used the ambiguous 12-hour pattern + * {@code hh:mm:ss} with no AM/PM marker (PR #15 round-3 review). Same persistence problem as + * {@link #LEGACY_WORLD_NAME_LINE}: {@code AbstractConfigEntity.init()} preserves this exact + * string in {@code sidebar.yml} on every server that has ever started an older version of + * this plugin, unless it is rewritten explicitly. + */ + private static final String LEGACY_SERVER_TIME_LINE = "&f%server_time_hh:mm:ss%"; + + /** + * The corrected default that replaces {@link #LEGACY_SERVER_TIME_LINE} with the unambiguous + * 24-hour pattern, kept in sync by hand with the "lines" default above. + */ + private static final String CURRENT_SERVER_TIME_LINE = "&f%server_time_HH:mm:ss%"; + + /** + * Every byte-identical legacy default line this plugin has ever shipped, mapped to its + * corrected replacement. Extend this map -- not the loop in + * {@link #migrateLegacyDefaultLines()} -- when a future shipped default needs the same + * exact-match migration treatment. + */ + private static final Map LEGACY_LINE_REPLACEMENTS; + + static { + Map replacements = new LinkedHashMap<>(); + replacements.put(LEGACY_WORLD_NAME_LINE, CURRENT_WORLD_NAME_LINE); + replacements.put(LEGACY_SERVER_TIME_LINE, CURRENT_SERVER_TIME_LINE); + LEGACY_LINE_REPLACEMENTS = Collections.unmodifiableMap(replacements); + } + /** * One-time migration for a persisted {@code sidebar.yml} whose {@code lines} list still - * carries the old, invalid {@link #LEGACY_WORLD_NAME_LINE} default (issue #13). Rewrites - * only a list entry that is byte-identical to that old default -- any operator - * customisation, including a line that merely mentions {@code %world_name%} alongside other - * text, is left untouched. Idempotent: once migrated, no entry matches - * {@link #LEGACY_WORLD_NAME_LINE} any more, so a second call is a no-op. + * carries one or more old, invalid shipped defaults tracked in + * {@link #LEGACY_LINE_REPLACEMENTS} (issue #13; PR #15 round-3 review extended this from the + * world-name line alone to also cover the 12-hour server-time line). Rewrites only a list + * entry that is byte-identical to a tracked legacy default -- any operator customisation, + * including a line that merely mentions a legacy token alongside other text, is left + * untouched. Idempotent: once migrated, no entry matches a tracked legacy default any more, + * so a second call is a no-op. *

* Must be called after {@code init(UltiToolsPlugin)} has populated {@link #lines} from * disk. The caller is responsible for persisting the result with {@code save()} when this @@ -96,15 +130,16 @@ public SideBarConfig() { * * @return {@code true} if at least one line was rewritten, {@code false} otherwise */ - public boolean migrateLegacyWorldNameDefaultLine() { + public boolean migrateLegacyDefaultLines() { if (lines == null) { return false; } boolean changed = false; List migrated = new ArrayList<>(lines.size()); for (String line : lines) { - if (LEGACY_WORLD_NAME_LINE.equals(line)) { - migrated.add(CURRENT_WORLD_NAME_LINE); + String replacement = LEGACY_LINE_REPLACEMENTS.get(line); + if (replacement != null) { + migrated.add(replacement); changed = true; } else { migrated.add(line); diff --git a/src/main/java/com/ultikits/plugins/sidebar/service/SideBarService.java b/src/main/java/com/ultikits/plugins/sidebar/service/SideBarService.java index 87760c2..da4634f 100644 --- a/src/main/java/com/ultikits/plugins/sidebar/service/SideBarService.java +++ b/src/main/java/com/ultikits/plugins/sidebar/service/SideBarService.java @@ -60,19 +60,21 @@ public void init() { dataOperator = plugin.getDataOperator(SideBarPreference.class); bukkitPlugin = Bukkit.getPluginManager().getPlugin("UltiTools"); - // One-time migration (issue #13, CR-01): AbstractConfigEntity.init() -- which has - // already run by this point, via UltiToolsPlugin's constructor -- only fills keys that - // are MISSING from the persisted file and never overwrites an existing "lines" value, - // so a server that has ever started an older version of this plugin keeps the invalid - // %world_name% default forever without this explicit, exact-match rewrite. Runs again - // on every reload() (this method is also called from reload()), which is harmless: once - // migrated, the exact-match check finds nothing left to rewrite. - if (config.migrateLegacyWorldNameDefaultLine()) { + // One-time migration (issue #13, CR-01; extended by PR #15 round-3 review to also cover + // the legacy 12-hour server-time line): AbstractConfigEntity.init() -- which has already + // run by this point, via UltiToolsPlugin's constructor -- only fills keys that are + // MISSING from the persisted file and never overwrites an existing "lines" value, so a + // server that has ever started an older version of this plugin keeps every stale shipped + // default (the invalid %world_name% line, the ambiguous %server_time_hh:mm:ss% line) + // forever without this explicit, exact-match rewrite. Runs again on every reload() (this + // method is also called from reload()), which is harmless: once migrated, the exact-match + // check finds nothing left to rewrite. + if (config.migrateLegacyDefaultLines()) { try { config.save(); } catch (IOException e) { - plugin.getLogger().warn("Failed to persist the sidebar.yml %world_name% " - + "placeholder migration: " + e.getMessage()); + plugin.getLogger().warn("Failed to persist the sidebar.yml legacy default line " + + "migration: " + e.getMessage()); } } diff --git a/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java b/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java index 8c1b7ba..4a468dd 100644 --- a/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java +++ b/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java @@ -325,8 +325,8 @@ private static UltiToolsPlugin mockPluginBackedBy(Path tempDir) { // ============================ @Nested - @DisplayName("Legacy World-Name Default Line Migration") - class LegacyWorldNameLineMigration { + @DisplayName("Legacy Default Line Migration") + class LegacyDefaultLineMigration { @TempDir Path tempDir; @@ -359,7 +359,7 @@ void rewritesLegacyLineButLeavesCustomLineUntouched() throws Exception { SideBarConfig config = new SideBarConfig(); config.init(mockPlugin); - boolean rewritten = config.migrateLegacyWorldNameDefaultLine(); + boolean rewritten = config.migrateLegacyDefaultLines(); assertThat(rewritten).isTrue(); config.save(); @@ -386,7 +386,7 @@ void doesNotTouchLineThatOnlyMentionsTheLegacyToken() throws Exception { SideBarConfig config = new SideBarConfig(); config.init(mockPlugin); - boolean rewritten = config.migrateLegacyWorldNameDefaultLine(); + boolean rewritten = config.migrateLegacyDefaultLines(); assertThat(rewritten).isFalse(); assertThat(config.getLines()) @@ -401,7 +401,7 @@ void noOpWhenAlreadyMigrated() throws Exception { SideBarConfig config = new SideBarConfig(); config.init(mockPlugin); - assertThat(config.migrateLegacyWorldNameDefaultLine()).isFalse(); + assertThat(config.migrateLegacyDefaultLines()).isFalse(); } @Test @@ -412,7 +412,7 @@ void rewritesLegacyServerTimeLine() throws Exception { SideBarConfig config = new SideBarConfig(); config.init(mockPlugin); - boolean rewritten = config.migrateLegacyWorldNameDefaultLine(); + boolean rewritten = config.migrateLegacyDefaultLines(); assertThat(rewritten) .as("a persisted server-time line using the ambiguous 12-hour pattern must be migrated too") .isTrue(); @@ -441,7 +441,7 @@ void rewritesBothLegacyDefaultsOnRealUpgrade() throws Exception { SideBarConfig config = new SideBarConfig(); config.init(mockPlugin); - boolean rewritten = config.migrateLegacyWorldNameDefaultLine(); + boolean rewritten = config.migrateLegacyDefaultLines(); assertThat(rewritten).isTrue(); assertThat(config.getLines())