GH-835 Add permission-based access to warps #856
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| package com.eternalcode.core.configuration.implementation; | ||
|
|
||
| import com.eternalcode.core.configuration.ReloadableConfig; | ||
| import com.eternalcode.core.feature.warp.WarpConfigEntry; | ||
| import com.eternalcode.core.injector.annotations.component.ConfigurationFile; | ||
| import net.dzikoysk.cdn.entity.Description; | ||
| import net.dzikoysk.cdn.source.Resource; | ||
| import net.dzikoysk.cdn.source.Source; | ||
|
|
||
| import java.io.File; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| @ConfigurationFile | ||
| public class WarpsConfiguration implements ReloadableConfig { | ||
|
There was a problem hiding this comment. More like WarpsLocation, configuration is in config |
||
|
|
||
| @Description({"# Warps configuration", "# These are warp locations, for your own safety, please don't touch it."}) | ||
| public Map<String, WarpConfigEntry> warps = new HashMap<>(); | ||
|
Comment on lines
+17
to
+18
There was a problem hiding this comment. 🛠️ Refactor suggestion Improve encapsulation and naming of the While the field is appropriately typed and described, there are several improvements that can be made:
Here's a suggested refactor: @Description({"# Warps configuration", "# These are warp locations, for your own safety, please don't modify directly."})
private final Map<String, WarpConfigEntry> warpsLocation = new HashMap<>();
public Map<String, WarpConfigEntry> getWarpsLocation() {
return Collections.unmodifiableMap(warpsLocation);
}
public void setWarp(String key, WarpConfigEntry value) {
warpsLocation.put(key, value);
}
public void removeWarp(String key) {
warpsLocation.remove(key);
}This approach maintains functionality while providing better control over how the warps are accessed and modified. |
||
|
|
||
| @Override | ||
| public Resource resource(File folder) { | ||
| return Source.of(folder, "warps.yml"); | ||
| } | ||
|
|
||
| } | ||
|
Comment on lines
+1
to
+25
There was a problem hiding this comment. 🛠️ Refactor suggestion Consider adding utility methods and validation logic. While the current implementation is functional, consider the following enhancements to improve the robustness and usability of the
Here are some suggested method signatures: public boolean isValidWarp(WarpConfigEntry entry) {
// Implement validation logic
}
public void mergeConfiguration(Map<String, WarpConfigEntry> newWarps) {
// Implement merge logic
}
public boolean hasWarp(String warpName) {
return warps.containsKey(warpName);
}These additions would enhance the functionality and make the class more robust and easier to use in other parts of the system. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| package com.eternalcode.core.feature.warp; | ||
|
|
||
| import com.eternalcode.commons.bukkit.position.Position; | ||
| import net.dzikoysk.cdn.entity.Contextual; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| @Contextual | ||
| public class WarpConfigEntry { | ||
|
There was a problem hiding this comment. What is the difference between |
||
| public Position position; | ||
| public List<String> permissions; | ||
|
There was a problem hiding this comment. 🛠️ Refactor suggestion Consider using a Set for permissions and add validation The current implementation uses a
Consider the following improvements:
Here's an example of how you could implement these changes: import java.util.HashSet;
import java.util.Set;
import java.util.stream.Collectors;
public class WarpConfigEntry {
private final Position position;
private final Set<String> permissions;
public WarpConfigEntry(Position position, List<String> permissions) {
this.position = position;
this.permissions = normalizePermissions(permissions);
}
private Set<String> normalizePermissions(List<String> permissions) {
if (permissions == null) {
throw new IllegalArgumentException("Permissions list cannot be null");
}
return permissions.stream()
.filter(p -> p != null && !p.trim().isEmpty())
.map(String::toLowerCase)
.collect(Collectors.toCollection(HashSet::new));
}
// ... getters and other methods
}This implementation ensures that the permissions are stored efficiently, normalized, and validated.
Comment on lines
+10
to
+11
There was a problem hiding this comment. 🛠️ Refactor suggestion Enhance encapsulation and optimize data structures. The current implementation with public fields violates encapsulation principles and might lead to inconsistent object state. Consider the following improvements:
Example refactoring: private final Position position;
private final Set<String> permissions;
public Position getPosition() {
return position;
}
public Set<String> getPermissions() {
return Collections.unmodifiableSet(permissions);
}This refactoring improves encapsulation, prevents duplicate permissions, and ensures immutability of the object state. |
||
|
|
||
| public WarpConfigEntry() { | ||
| } | ||
|
|
||
| public WarpConfigEntry(Position position, List<String> permissions) { | ||
| this.position = position; | ||
| this.permissions = permissions; | ||
| } | ||
|
Comment on lines
+13
to
+19
There was a problem hiding this comment. 🛠️ Refactor suggestion Improve constructor implementation and consider removing the default constructor. The current constructor implementation has potential issues:
Consider the following improvements:
Example refactoring: public WarpConfigEntry(Position position, Set<String> permissions) {
this.position = Objects.requireNonNull(position, "Position cannot be null");
this.permissions = Collections.unmodifiableSet(new HashSet<>(
Objects.requireNonNull(permissions, "Permissions cannot be null")
));
}This refactoring ensures that the object is always in a valid state and prevents external modification of the permissions set. |
||
| } | ||
|
Comment on lines
+8
to
+20
There was a problem hiding this comment. 🛠️ Refactor suggestion Consider enhancing encapsulation and adding documentation The
Here's an example of how you could refactor the class: /**
* Represents a configuration entry for a warp.
*/
@Contextual
public class WarpConfigEntry {
private final Position position;
private final List<String> permissions;
/**
* Constructs a new WarpConfigEntry.
*
* @param position The position of the warp.
* @param permissions The list of permissions required to access the warp.
*/
public WarpConfigEntry(Position position, List<String> permissions) {
this.position = position;
this.permissions = List.copyOf(permissions); // Create an immutable copy
}
public Position getPosition() {
return position;
}
public List<String> getPermissions() {
return List.copyOf(permissions); // Return an immutable copy
}
}This refactored version provides better encapsulation, immutability, and documentation.
Comment on lines
+1
to
+20
There was a problem hiding this comment. 💡 Codebase verification Clarify the responsibilities and naming conventions of Warp-related classes. The current structure shows potential naming inconsistencies and unclear relationships between
This will help in streamlining the warp system's architecture and making the design decisions more transparent. 🔗 Analysis chainClarify the purpose and relationship of WarpConfigEntry in the warp system. The purpose and usage of
This information will help in understanding the design decisions and potentially identifying opportunities for system-wide improvements. To help clarify the relationship between 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Find usages and related classes of WarpConfigEntry
echo "Searching for WarpConfigEntry usages:"
rg --type java -n "WarpConfigEntry"
echo "\nSearching for other Warp-related classes:"
rg --type java -l "class.*Warp"
echo "\nSearching for WarpImpl class definition:"
rg --type java -n "class WarpImpl"
Length of output: 4339 Script: #!/bin/bash
# Description: Find interactions between WarpImpl and WarpConfigEntry
echo "Searching for WarpImpl usages of WarpConfigEntry:"
rg --type java "WarpConfigEntry" eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpImpl.java
echo "\nSearching for methods in WarpConfigRepository that use WarpConfigEntry:"
rg --type java "WarpConfigEntry" eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpConfigRepository.java
Length of output: 1071 Script: #!/bin/bash
# Description: Retrieve the definition of the Warp interface
echo "Fetching Warp interface definition:"
rg --type java "interface Warp" eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/
echo "\nFetching all implementations of the Warp interface:"
rg --type java "implements Warp" eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/
echo "\nFinding usages of WarpImpl in the codebase:"
rg --type java "WarpImpl"
Length of output: 1883 |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,61 +1,126 @@ | ||||||||||||||||||||||||||
| package com.eternalcode.core.feature.warp; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import com.eternalcode.commons.bukkit.position.PositionAdapter; | ||||||||||||||||||||||||||
| import com.eternalcode.core.configuration.ConfigurationManager; | ||||||||||||||||||||||||||
| import com.eternalcode.core.configuration.implementation.LocationsConfiguration; | ||||||||||||||||||||||||||
| import com.eternalcode.core.configuration.implementation.WarpsConfiguration; | ||||||||||||||||||||||||||
| import com.eternalcode.core.injector.annotations.Inject; | ||||||||||||||||||||||||||
| import com.eternalcode.core.injector.annotations.component.Service; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import java.util.ArrayList; | ||||||||||||||||||||||||||
| import java.util.HashMap; | ||||||||||||||||||||||||||
| import java.util.List; | ||||||||||||||||||||||||||
| import java.util.Map; | ||||||||||||||||||||||||||
| import java.util.Optional; | ||||||||||||||||||||||||||
| import java.util.concurrent.CompletableFuture; | ||||||||||||||||||||||||||
| import java.util.function.Consumer; | ||||||||||||||||||||||||||
| import java.util.stream.Collectors; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Service | ||||||||||||||||||||||||||
P1otrulla marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
| class WarpConfigRepository implements WarpRepository { | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private final LocationsConfiguration locationsConfiguration; | ||||||||||||||||||||||||||
| private final WarpsConfiguration warpsConfiguration; | ||||||||||||||||||||||||||
| private final ConfigurationManager configurationManager; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Inject | ||||||||||||||||||||||||||
| WarpConfigRepository( | ||||||||||||||||||||||||||
| ConfigurationManager configurationManager, | ||||||||||||||||||||||||||
| LocationsConfiguration locationsConfiguration, | ||||||||||||||||||||||||||
| WarpsConfiguration warpsConfiguration | ||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||
| this.locationsConfiguration = locationsConfiguration; | ||||||||||||||||||||||||||
| this.configurationManager = configurationManager; | ||||||||||||||||||||||||||
| this.warpsConfiguration = warpsConfiguration; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| this.migrateWarps(); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||
| public void addWarp(Warp warp) { | ||||||||||||||||||||||||||
| WarpConfigEntry warpConfigEntry = new WarpConfigEntry(PositionAdapter.convert(warp.getLocation()), warp.getPermissions()); | ||||||||||||||||||||||||||
| this.edit(warps -> warps.put( | ||||||||||||||||||||||||||
| warp.getName(), | ||||||||||||||||||||||||||
| warpConfigEntry | ||||||||||||||||||||||||||
| )); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||
| public void removeWarp(String warp) { | ||||||||||||||||||||||||||
| this.edit(warps -> warps.remove(warp)); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||
| public void addPermissions(String warp, String... permissions) { | ||||||||||||||||||||||||||
| this.edit(warps -> { | ||||||||||||||||||||||||||
| WarpConfigEntry warpConfigEntry = warps.get(warp); | ||||||||||||||||||||||||||
| if (warpConfigEntry == null) { | ||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| List<String> newPermissions = new ArrayList<>(warpConfigEntry.permissions); | ||||||||||||||||||||||||||
| newPermissions.addAll(List.of(permissions)); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| warpConfigEntry.permissions = newPermissions; | ||||||||||||||||||||||||||
|
Comment on lines
+61
to
+64
There was a problem hiding this comment. Prevent possible In the Consider initializing the permissions list when it's if (warpConfigEntry == null) {
return;
}
+if (warpConfigEntry.permissions == null) {
+ warpConfigEntry.permissions = new ArrayList<>();
+}
List<String> newPermissions = new ArrayList<>(warpConfigEntry.permissions);
newPermissions.addAll(List.of(permissions));
warpConfigEntry.permissions = newPermissions;📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+53
to
+66
There was a problem hiding this comment. 🛠️ Refactor suggestion Refactor Suggestion: Reduce Code Duplication in Permission Methods The You could implement a private method that handles permission updates, parameterized by the action to perform (add or remove): private void updatePermissions(String warp, Consumer<List<String>> permissionUpdater) {
this.edit(warps -> {
WarpConfigEntry warpConfigEntry = warps.get(warp);
if (warpConfigEntry == null) {
return;
}
List<String> newPermissions = new ArrayList<>(warpConfigEntry.permissions);
permissionUpdater.accept(newPermissions);
warpConfigEntry.permissions = newPermissions;
});
}Then, refactor @Override
public void addPermissions(String warp, String... permissions) {
this.updatePermissions(warp, perms -> perms.addAll(List.of(permissions)));
}
@Override
public void removePermission(String warp, String permission) {
this.updatePermissions(warp, perms -> perms.remove(permission));
}Also applies to: 68-82 |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||
| public void removePermission(String warp, String permission) { | ||||||||||||||||||||||||||
| this.edit(warps -> { | ||||||||||||||||||||||||||
| WarpConfigEntry warpConfigEntry = warps.get(warp); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (warpConfigEntry == null) { | ||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| List<String> newPermissions = new ArrayList<>(warpConfigEntry.permissions); | ||||||||||||||||||||||||||
| newPermissions.remove(permission); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| warpConfigEntry.permissions = newPermissions; | ||||||||||||||||||||||||||
|
Comment on lines
+77
to
+80
There was a problem hiding this comment. Handle In the Initialize the permissions list if it's if (warpConfigEntry == null) {
return;
}
+if (warpConfigEntry.permissions == null) {
+ warpConfigEntry.permissions = new ArrayList<>();
+}
List<String> newPermissions = new ArrayList<>(warpConfigEntry.permissions);
newPermissions.remove(permission);
warpConfigEntry.permissions = newPermissions;📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private void edit(Consumer<Map<String, WarpConfigEntry>> editor) { | ||||||||||||||||||||||||||
| Map<String, WarpConfigEntry> warps = new HashMap<>(this.warpsConfiguration.warps); | ||||||||||||||||||||||||||
| editor.accept(warps); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| warps.forEach((key, value) -> System.out.println(key + ": " + value)); | ||||||||||||||||||||||||||
|
There was a problem hiding this comment. is it needed here There was a problem hiding this comment. 🛠️ Refactor suggestion Replace Using Apply this change to use a logger: +import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+private static final Logger logger = LoggerFactory.getLogger(WarpConfigRepository.class);
...
-warps.forEach((key, value) -> System.out.println(key + ": " + value));
+warps.forEach((key, value) -> logger.debug("{}: {}", key, value));
|
||||||||||||||||||||||||||
| this.warpsConfiguration.warps.putAll(warps); | ||||||||||||||||||||||||||
| this.configurationManager.save(this.warpsConfiguration); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||
| public CompletableFuture<Optional<Warp>> getWarp(String name) { | ||||||||||||||||||||||||||
| return CompletableFuture.completedFuture(Optional.of(this.warpsConfiguration.warps.get(name)) | ||||||||||||||||||||||||||
| .map(warpConfigEntry -> new WarpImpl(name, warpConfigEntry.position, warpConfigEntry.permissions))); | ||||||||||||||||||||||||||
|
Comment on lines
+95
to
+96
There was a problem hiding this comment. Prevent In the Here's the suggested fix: -return CompletableFuture.completedFuture(Optional.of(this.warpsConfiguration.warps.get(name))
+return CompletableFuture.completedFuture(Optional.ofNullable(this.warpsConfiguration.warps.get(name))
.map(warpConfigEntry -> new WarpImpl(name, warpConfigEntry.position, warpConfigEntry.permissions)));📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||
| public CompletableFuture<List<Warp>> getWarps() { | ||||||||||||||||||||||||||
| return CompletableFuture.completedFuture(this.warpsConfiguration.warps.entrySet().stream() | ||||||||||||||||||||||||||
| .map(warpConfigEntry -> { | ||||||||||||||||||||||||||
| WarpConfigEntry warpContextual = warpConfigEntry.getValue(); | ||||||||||||||||||||||||||
| return new WarpImpl(warpConfigEntry.getKey(), warpContextual.position, warpContextual.permissions); | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| .collect(Collectors.toList())); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private void migrateWarps() { | ||||||||||||||||||||||||||
| if (this.locationsConfiguration.warps.isEmpty()) { | ||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+110
to
+112
There was a problem hiding this comment. Handle potential In the Add a null check before proceeding: if (this.locationsConfiguration.warps == null || this.locationsConfiguration.warps.isEmpty()) {
return;
}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| this.edit(warps -> warps.putAll(this.locationsConfiguration.warps | ||||||||||||||||||||||||||
| .entrySet() | ||||||||||||||||||||||||||
| .stream() | ||||||||||||||||||||||||||
| .collect(Collectors.toMap(Map.Entry::getKey, entry -> | ||||||||||||||||||||||||||
| new WarpConfigEntry(entry.getValue(), new ArrayList<>())) | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| )); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| this.locationsConfiguration.warps.clear(); | ||||||||||||||||||||||||||
| this.configurationManager.save(this.locationsConfiguration); | ||||||||||||||||||||||||||
| this.configurationManager.save(this.warpsConfiguration); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,15 +15,16 @@ | |
| import dev.triumphteam.gui.builder.item.ItemBuilder; | ||
| import dev.triumphteam.gui.guis.Gui; | ||
| import dev.triumphteam.gui.guis.GuiItem; | ||
| import net.kyori.adventure.text.Component; | ||
| import net.kyori.adventure.text.minimessage.MiniMessage; | ||
| import org.bukkit.Material; | ||
| import org.bukkit.Server; | ||
| import org.bukkit.entity.Player; | ||
|
|
||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Optional; | ||
|
|
||
| @Service | ||
| public class WarpInventory { | ||
|
|
||
|
|
@@ -80,7 +81,6 @@ private Gui createInventory(Language language) { | |
| } | ||
| } | ||
|
|
||
| Gui gui = Gui.gui() | ||
| .title(this.miniMessage.deserialize(warpSection.title())) | ||
| .rows(rowsCount) | ||
|
|
@@ -198,13 +198,11 @@ public void openInventory(Player player, Language language) { | |
| } | ||
|
|
||
| public void addWarp(Warp warp) { | ||
| if (!this.warpManager.warpExists(warp.getName())) { | ||
| return; | ||
| } | ||
|
|
||
| for (Language language : this.translationManager.getAvailableLanguages()) { | ||
| AbstractTranslation translation = (AbstractTranslation) this.translationManager.getMessages(language); | ||
| Translation.WarpSection.WarpInventorySection warpSection = translation.warp().warpInventory(); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this method from public API