feat: Support runtime server addition for Velocity proxy #118
Conversation
RobotHanzo
changed the title
RobotHanzo
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds support for dynamically registering servers at runtime and automatically applying appropriate resource packs to them based on configuration. The implementation introduces a new event listener for server registration events and refactors the resource pack registration logic to support both initial loading and dynamic server addition.
Changes:
- Added
ServerRegistrationListenerto handle runtime server registration events - Refactored resource pack registration logic into reusable methods (
getPackConfigs,processResourcePackConfig,createResourcePack,registerResourcePackForServer) - Updated build configuration to set Java 17 compatibility
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| velocity/src/main/java/com/convallyria/forcepack/velocity/listener/ServerRegistrationListener.java | New event listener that registers resource packs for dynamically added servers matching group configurations |
| velocity/src/main/java/com/convallyria/forcepack/velocity/ForcePackVelocity.java | Refactored resource pack registration logic into smaller, reusable methods to support both initial load and dynamic server addition; extracted common logic into helper methods |
| velocity/build.gradle.kts | Reorganized task configuration and added Java 17 compatibility settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Subscribe | ||
| public void onServerRegistered(ServerRegisteredEvent event) { | ||
| RegisteredServer registeredServer = event.registeredServer(); | ||
| String serverName = registeredServer.getServerInfo().getName(); | ||
| VelocityConfig groups = plugin.getConfig().getConfig("groups"); | ||
|
|
||
| if (groups == null) return; | ||
|
|
||
| // Check if we already have packs for this server | ||
| boolean hasPack = plugin.getResourcePacks().stream() | ||
| .anyMatch(pack -> pack.getServer().equals(serverName)); | ||
|
|
||
| if (hasPack) return; | ||
|
|
||
| for (String groupName : groups.getKeys()) { | ||
| VelocityConfig groupConfig = groups.getConfig(groupName); | ||
| List<String> servers = groupConfig.getStringList("servers"); | ||
| boolean exact = groupConfig.getBoolean("exact-match"); | ||
|
|
||
| boolean matches = exact ? | ||
| servers.contains(serverName) : | ||
| servers.stream().anyMatch(serverName::contains); | ||
|
|
||
| if (matches) { | ||
| plugin.log("New server %s matches group %s, adding resource packs...", serverName, groupName); | ||
|
|
||
| final Map<String, VelocityConfig> configs = plugin.getPackConfigs(groupConfig, groupName); | ||
|
|
||
| configs.forEach((id, config) -> { | ||
| if (config == null) return; | ||
| plugin.registerResourcePackForServer(config, id, groupName, "group", plugin.getConfig().getBoolean("verify-resource-packs"), serverName, null); | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The ServerRegistrationListener only checks the "groups" configuration but not the "servers" configuration. According to the PR description, dynamic servers should be applied resource packs if they qualify for "groups/servers configs", but this implementation only handles groups. If a dynamically added server should also match entries in the "servers" section of the config, that logic is missing.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
I think it failed to do that
fixed
Co-authored-by: Copilot <[email protected]>
Co-authored-by: RobotHanzo <[email protected]>
Co-authored-by: RobotHanzo <[email protected]>
Support servers config in ServerRegistrationListener
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private void handleRegisterForServer(VelocityConfig resourcePack, String name, String id, String typeName, String url, @Nullable String hash, boolean verifyPacks, String targetServerName, @Nullable Player player) { | ||
| VelocityResourcePack pack = createResourcePack(resourcePack, name, id, typeName, url, hash, verifyPacks, targetServerName, name, player); | ||
| if (pack != null) { | ||
| resourcePacks.add(pack); |
There was a problem hiding this comment.
The resourcePacks HashSet is not thread-safe, but it can be modified concurrently from both the main initialization thread and asynchronously from the ServerRegisteredEvent listener. This could lead to race conditions, data corruption, or ConcurrentModificationException. Consider using a thread-safe collection like ConcurrentHashMap.newKeySet() or Collections.synchronizedSet() to wrap the HashSet, or ensure all modifications happen on a single thread using proper synchronization.
|
|
||
| public void addResourcePack(String serverName, VelocityResourcePack pack) { | ||
| resourcePacks.add(pack); | ||
| log("Added resource pack for server %s (%s)", serverName, pack.getUUID().toString()); | ||
| } |
There was a problem hiding this comment.
This method is defined but never called anywhere in the codebase. It appears to be dead code that should either be removed or properly integrated if it was intended to replace the direct resourcePacks.add() calls in handleRegisterForServer().
| public void addResourcePack(String serverName, VelocityResourcePack pack) { | |
| resourcePacks.add(pack); | |
| log("Added resource pack for server %s (%s)", serverName, pack.getUUID().toString()); | |
| } |
This PR allows dynamically-added servers to be applied resource pack if they qualify for the groups/servers configs