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..ca5a137 100644 --- a/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java +++ b/src/main/java/com/ultikits/plugins/sidebar/config/SideBarConfig.java @@ -1,8 +1,11 @@ package com.ultikits.plugins.sidebar.config; +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; @@ -44,13 +47,13 @@ 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", "", "&7服务器时间", - "&f%server_time_hh:mm:ss%", + "&f%server_time_HH:mm:ss%", "", "&6play.example.com" ); @@ -64,4 +67,87 @@ 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%"; + + /** + * 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 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 + * 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 migrateLegacyDefaultLines() { + if (lines == null) { + return false; + } + boolean changed = false; + List migrated = new ArrayList<>(lines.size()); + for (String line : lines) { + String replacement = LEGACY_LINE_REPLACEMENTS.get(line); + if (replacement != null) { + migrated.add(replacement); + 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..da4634f 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,24 @@ public void init() { dataOperator = plugin.getDataOperator(SideBarPreference.class); bukkitPlugin = Bukkit.getPluginManager().getPlugin("UltiTools"); + // 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 legacy default line " + + "migration: " + e.getMessage()); + } + } + // Check PlaceholderAPI placeholderApiAvailable = Bukkit.getPluginManager().getPlugin("PlaceholderAPI") != null; if (!placeholderApiAvailable) { 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..4a468dd 100644 --- a/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java +++ b/src/test/java/com/ultikits/plugins/sidebar/config/SideBarConfigTest.java @@ -1,11 +1,32 @@ package com.ultikits.plugins.sidebar.config; +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.List; +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 { @@ -139,6 +160,300 @@ void emptyLines() { } } + // ============================ + // Default sidebar renderability + // ============================ + + @Nested + @DisplayName("Default Sidebar Renderability") + class DefaultSidebarRenderabilityTests { + + /** + * 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 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") + .replace("%server_online%", "12") + .replace("%server_max_players%", "100") + .replace("%player_world%", "world") + .replace("%player_ping%", "42"); + // 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 + @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(); + } + } + + @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 { + // 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)); + + assertThat(config.getLines()) + .as("an operator's own persisted line must survive init() untouched") + .contains("&aOperator's own custom line"); + } + } + + /** + * 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); + }); + } + + // ============================ + // Legacy %world_name% default line migration (issue #13, CR-01) + // ============================ + + @Nested + @DisplayName("Legacy Default Line Migration") + class LegacyDefaultLineMigration { + + @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("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.migrateLegacyDefaultLines(); + 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.migrateLegacyDefaultLines(); + + 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%")); + + SideBarConfig config = new SideBarConfig(); + config.init(mockPlugin); + + assertThat(config.migrateLegacyDefaultLines()).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.migrateLegacyDefaultLines(); + 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.migrateLegacyDefaultLines(); + + 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"); + } + } + /** * Create a real SideBarConfig instance. * The no-arg constructor calls super("config/sidebar.yml") which only stores the path