GH-930 Msg placeholders & Async placeholder for future by P1otrulla · Pull Request #1203 · EternalCodeTeam/EternalCore

P1otrulla · 2025-10-12T11:27:01Z

Tested with love for OpenSource 💕

https://medal.tv/pl/games/minecraft/clips/lhPGLpi6DGCYMFpSd?invite=cr-MSxmazgsNDI2MDIzOTA4&

* Register new placeholders for `msg_toggle` and `socialspy_status` in `MsgPlaceholderSetup`. * Extend `MsgMessages` with a `Placeholders` interface and implement language-specific placeholder formats in `ENMsgMessages` and `PLMsgMessages`.

* Add caching logic for `msg_toggle` state in `MsgPlaceholderSetup`. * Introduce new formatted placeholders for `msg_toggle` and `socialspy_status`. * Extend `MsgMessages.Placeholders` to support a loading state with translations.

…erAPI` * Adjust cache duration in `MsgPlaceholderSetup` from 200ms to 5s. * Rename `msg_toggle` placeholders to `msg_status` for consistency. * Extend `ENPlaceholders` and `PLPlaceholders` with `OkaeriConfig`. * Add `PlaceholderAPI` dependency in `runServer` task configuration.

* Replace existing cache in `MsgPlaceholderSetup` with `AsyncPlaceholderCached`. * Introduce `AsyncPlaceholderCacheRegistry` for managing placeholder caches. * Add `AsyncPlaceholderCacheController` to handle cache invalidation on player quit. * Simplify placeholder logic and remove redundant caching methods.

…derCacheRegistry` * Replace individual `stateCache` with dynamically registered cache in the `AsyncPlaceholderCacheRegistry`. * Simplify placeholder setup logic and adjust `MsgToggleService` to utilize the registry for cache management.

…lidation * Add `Subscribe` annotations to invalidate cache on `EternalReloadEvent` and `EternalShutdownEvent`.

* Rename `onEternalDisable` to `onDisable` and `onReload`.

gemini-code-assist

Code Review

This pull request introduces a new asynchronous caching mechanism for placeholders and integrates it with the messaging feature. The changes include a new AsyncPlaceholderCacheRegistry and AsyncPlaceholderCached for managing async data loading, new placeholders for message and social spy status, and refactoring MsgToggleServiceImpl to use the new caching system. The overall implementation is good, but I've found a few areas for improvement. There's a race condition in AsyncPlaceholderCached that could lead to multiple concurrent loads for the same data, some code duplication in MsgPlaceholderSetup when defining similar placeholders, and a minor inconsistency in a comment in one of the message files. I've left specific comments with suggestions on how to address these points.

...alcore-core/src/main/java/com/eternalcode/core/placeholder/cache/AsyncPlaceholderCached.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/MsgPlaceholderSetup.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/messages/ENMsgMessages.java

...nalcore-core/src/main/java/com/eternalcode/core/feature/msg/toggle/MsgToggleServiceImpl.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/MsgPlaceholderSetup.java

...alcore-core/src/main/java/com/eternalcode/core/placeholder/cache/AsyncPlaceholderCached.java

...core/src/main/java/com/eternalcode/core/placeholder/cache/AsyncPlaceholderCacheRegistry.java

...nalcore-core/src/main/java/com/eternalcode/core/feature/msg/toggle/MsgToggleServiceImpl.java

noyzys

Jezeli cos niezrozumialego smialo pytac lub dawac wlasne propozycje zmian ;).
Wszystko mozna podkrecic i jezeli jest jakis problem nie sugerowac sie moimi zmianami bo zawsze jest jakies ala w trakcie review

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/MsgPlaceholderSetup.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/messages/PLMsgMessages.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/messages/ENMsgMessages.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/MsgPlaceholderSetup.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/messages/ENMsgMessages.java

Rollczi

1 komentarz na razie bo może być dużo do przerobienia

...nalcore-core/src/main/java/com/eternalcode/core/feature/msg/toggle/MsgToggleServiceImpl.java

Rollczi · 2026-02-21T15:12:17Z

nic się nie dzieje to zrobiłem commita

# Conflicts: # eternalcore-core/src/main/java/com/eternalcode/core/bridge/BridgeManager.java # eternalcore-core/src/main/java/com/eternalcode/core/bridge/placeholderapi/PlaceholderAPIPlaceholder.java # eternalcore-core/src/main/java/com/eternalcode/core/placeholder/PlaceholderBukkitRegistryImpl.java # eternalcore-core/src/main/java/com/eternalcode/core/placeholder/PlaceholderRaw.java # eternalcore-core/src/main/java/com/eternalcode/core/placeholder/PlaceholderRegistry.java # eternalcore-core/src/main/java/com/eternalcode/core/placeholder/PlaceholderReplacer.java # eternalcore-plugin/build.gradle.kts

CitralFlo

-- Polish below --
For me using watcher is a little counterintuitive.
We are using cache in PlaceholderAsync, so maybe moving it to the PlaceholderSetup.class would be a better option.

My idea is creating a interface or abstract that needs to be implemented in PlaceholderSetup for AsyncPlaceholder. Instead of putting the watcher in Repo and then using Repo in setup class. I would suggest using predefined standards with cache (required) in class method for refresh and method used in Service that would update / call update for specific player. Additional fallback option that would display some text before accessing value in db ex. "Loading..." it would not be a problem if the player will get answer from server in next message - when placeholder is ready.

PL:
Tak jak patrze to i tak opiera sie na cache.
Może jakiś standard do tego cache i wołanie do placeholderów z update z serwisu, dodatkowo jakis fallback jakby nie bylo. Raczej nie problem jak sie wyswietli "Loading..." a potem poprawna wartosc w następnej wiadomości.
Tylko wiesz np jako abstract albo interfejs dla PlaceholderAsync. Że wymaga tego placeholder i wszystkie warunki do trzymania tych danych są spełnione
napisze to w review.

CitralFlo · 2026-02-24T23:07:43Z

eternalcore-core/src/main/java/com/eternalcode/core/placeholder/Placeholder.java

+    }
+
+    static <T> Placeholder async(
+        String target,


I would rename target to sth like "placeholderName"

Jakubk15 · 2026-03-07T10:03:21Z

/gemini review

Jakubk15 · 2026-03-07T09:57:48Z

...core/src/main/java/com/eternalcode/core/bridge/placeholderapi/PlaceholderAPIPlaceholder.java

 import org.bukkit.entity.Player;

-public class PlaceholderApiReplacer implements Placeholder {
+public class PlaceholderAPIPlaceholder implements Placeholder {


Rename to PlaceholderApiPlaceholder, or rename other domain classes to have PlaceholderAPI instead of the Api

Jakubk15 · 2026-03-07T09:58:27Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/messages/ENMsgMessages.java

    Notice socialSpyEnable = Notice.chat("<color:#9d6eef>► <white>SocialSpy has been {STATE}<white>!");
    Notice socialSpyDisable = Notice.chat("<red>► <white>SocialSpy has been {STATE}<white>!");

+    @Comment("# Formatowanie placeholderów")


This is EN translation, so we use English comments

Suggested change

@Comment("# Formatowanie placeholderów")

@Comment("# Placeholders formatting")

Jakubk15 · 2026-03-07T10:01:01Z

...lcore-core/src/main/java/com/eternalcode/core/placeholder/watcher/PlaceholderWatcherKey.java

+public record PlaceholderWatcherKey<T>(String name, Class<T> type) {
+
+    public static <T> PlaceholderWatcherKey<T> of(String name, Class<T> type) {
+        return new PlaceholderWatcherKey<>(name, type);
+    }


So now we have 2 ways to construct this class, through the static of method or through the constructor. Shouldn't we restrict the constructor?

Jakubk15 · 2026-03-07T10:02:10Z

eternalcore-core/src/main/java/com/eternalcode/core/placeholder/PlaceholderAsync.java

+    @Override
+    public String provideValue(Player targetPlayer) {
+        CompletableFuture<T> future = cache.get(targetPlayer.getUniqueId());
+        if (future.isDone() && !future.isCompletedExceptionally()) {


Implement better exception handling

gemini-code-assist

Code Review

This pull request significantly refactors the placeholder system to support asynchronous placeholders, aiming to improve performance for database or network lookups and introducing NamedPlaceholder and PlaceholderWatcher. However, the current PlaceholderAPI integration is vulnerable to placeholder injection because it processes the entire message string, potentially including unsanitized user input. Furthermore, the use of a non-deterministic collection for storing placeholders can lead to unpredictable resolution orders, exacerbating injection vulnerabilities and causing inconsistent behavior. Beyond security, the review also identified a performance regression in MsgToggleServiceImpl, opportunities to improve loading state handling in PlaceholderAsync for better user feedback, potential duplicate data fetching, and a minor code simplification in PlaceholderBukkitRegistryImpl.

gemini-code-assist · 2026-03-07T10:10:23Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/MsgPlaceholderSetup.java

+        registry.register(Placeholder.async(
+            "msg_status",
+            MSG_STATE,
+            player -> msgToggleRepository.getPrivateChatState(player),
+            state -> state.name().toLowerCase()
+        ));
+
+        registry.register(Placeholder.async(
+            "msg_status_formatted",
+            MSG_STATE,
+            player -> msgToggleRepository.getPrivateChatState(player),
+            state -> state == MsgState.ENABLED
+                ? translation.msg().placeholders().msgEnabled()
+                : translation.msg().placeholders().msgDisabled()
+            )
+        );


You are registering two asynchronous placeholders, msg_status and msg_status_formatted, which both depend on the same underlying data from msgToggleRepository.getPrivateChatState(player). However, each call to Placeholder.async() creates a new PlaceholderAsync instance with its own separate cache and loading function. This will lead to duplicate database calls when both placeholders are requested for a player for the first time.

To fix this, the data loading logic should be shared. A possible solution is to adjust the async placeholder API to allow sharing a data source or cache for a given PlaceholderWatcherKey.

The best approach is to fix the root cause in the async placeholder design to avoid redundant data fetching for related placeholders.

gemini-code-assist · 2026-03-07T10:10:23Z

...nalcore-core/src/main/java/com/eternalcode/core/feature/msg/toggle/MsgToggleServiceImpl.java

    @Override
-    public CompletableFuture<MsgState> getState(UUID playerUniqueId) {
-        if (this.cachedToggleStates.containsKey(playerUniqueId)) {
-            return CompletableFuture.completedFuture(this.cachedToggleStates.get(playerUniqueId));
-        }
-
-        return this.msgToggleRepository.getPrivateChatState(playerUniqueId);
+    public CompletableFuture<MsgState> getState(UUID player) {
+        return watcher.track(player, msgToggleRepository.getPrivateChatState(player));
    }

    @Override
-    public CompletableFuture<Void> setState(UUID playerUniqueId, MsgState state) {
-        this.cachedToggleStates.put(playerUniqueId, state);
-
-        return this.msgToggleRepository.setPrivateChatState(playerUniqueId, state)
-            .exceptionally(throwable -> {
-                this.cachedToggleStates.remove(playerUniqueId);
-                return null;
-            });
+    public CompletableFuture<Void> setState(UUID player, MsgState state) {
+        return watcher.track(player, msgToggleRepository.setPrivateChatState(player, state))
+            .thenApply(unused -> null);
    }


The previous implementation of this service used a ConcurrentHashMap (cachedToggleStates) to cache the toggle states of players, avoiding repeated database lookups. This refactoring has removed the cache, meaning that every call to getState() will now result in a database query via msgToggleRepository.getPrivateChatState(). This is a significant performance regression, especially if getState() is called frequently (e.g., by toggleState).

The new PlaceholderWatcher system updates placeholder caches, but it does not provide a caching layer for the service itself. It's recommended to reintroduce a caching mechanism in this service, similar to the previous implementation. The PlaceholderWatcher can then be used to update or invalidate this service-level cache as well.

gemini-code-assist · 2026-03-07T10:10:23Z

eternalcore-core/src/main/java/com/eternalcode/core/placeholder/PlaceholderAsync.java

+            return mapper.apply(future.join());
+        }
+
+        return "null";


When the CompletableFuture for the placeholder value is not yet complete, this method returns the hardcoded string "null". This will be displayed directly to the user, which is not a good user experience. It would be better to display a loading message (e.g., "Loading...").

The message configuration files already define a loading string. Consider making this loading message configurable, perhaps by passing it as an argument to the Placeholder.async() factory method.

Suggested change

return "null";

return "<yellow>Loading...";

gemini-code-assist · 2026-03-07T10:10:23Z

...lcore-core/src/main/java/com/eternalcode/core/placeholder/PlaceholderBukkitRegistryImpl.java

    private final Server server;
-    private final Set<Placeholder> replacerPlayers = new HashSet<>();
-    private final Map<String, PlaceholderRaw> rawPlaceholders = new HashMap<>();
+    private final Set<Placeholder> placeholders = new HashSet<>();


The placeholders set in PlaceholderBukkitRegistryImpl is a HashSet, which does not guarantee iteration order. Since some placeholders (like PlaceholderAPIPlaceholder) can resolve placeholders within the results of other placeholders (double expansion), the final output of format() depends on the non-deterministic iteration order of the set. This can lead to inconsistent behavior and potential security bypasses if an attacker relies on a specific order to trigger or hide a placeholder injection. Using a LinkedHashSet or another ordered collection would ensure a consistent and predictable resolution order.

gemini-code-assist · 2026-03-07T10:10:23Z

...lcore-core/src/main/java/com/eternalcode/core/placeholder/PlaceholderBukkitRegistryImpl.java

+            if (placeholders != null) {
+                for (PlaceholderAsync<?> placeholder : placeholders) {
+                    ((PlaceholderAsync<T>) placeholder).update(player, value);
+                }
+            }


The if (placeholders != null) check is redundant. The asyncPlaceholders.getOrDefault(key, Collections.emptySet()) call on the preceding line will always return a non-null Set, either the one from the map or an empty set. You can safely remove this if statement and its braces.

for (PlaceholderAsync<?> placeholder : placeholders) { ((PlaceholderAsync<T>) placeholder).update(player, value); }

P1otrulla added 8 commits October 12, 2025 12:20

Enhance AsyncPlaceholderCacheController with event-based cache inva…

a8c6659

…lidation * Add `Subscribe` annotations to invalidate cache on `EternalReloadEvent` and `EternalShutdownEvent`.

Refactor AsyncPlaceholderCacheController event methods

2633ef6

* Rename `onEternalDisable` to `onDisable` and `onReload`.

the bomb was defused xd

dc6f963

P1otrulla requested a review from a team as a code owner October 12, 2025 11:27

P1otrulla linked an issue Oct 12, 2025 that may be closed by this pull request

Add a placeholder for the /msgtoggle command. i.e: %eternalcore_msgtoggle% #930

Open

P1otrulla requested review from CitralFlo, Jakubk15, igoyek, imDMK and vLuckyyy October 12, 2025 11:27

gemini-code-assist bot reviewed Oct 12, 2025

View reviewed changes

P1otrulla added 2 commits October 12, 2025 13:30

Follow gemini comment

aa360dd

Follow gemini comment

a128154

P1otrulla changed the title ~~Msg placeholders & Async placeholder for future~~ GH-930 Msg placeholders & Async placeholder for future Oct 12, 2025

P1otrulla requested a review from noyzys October 12, 2025 12:12