GH-137 Refactor project

[Jakubk15]

This pull request introduces significant refactoring and modernization to the ParcelLockers plugin, focusing on codebase simplification, improved configuration and notification handling, and enhanced plugin initialization. The changes streamline service and manager creation, replace legacy handlers with newer implementations, and update build and workflow configurations for better compatibility and efficiency.

Codebase Refactoring & Service Architecture

• Replaced legacy managers (ConfigurationManager, NotificationAnnouncer, etc.) with new service-based architecture (ConfigService, NoticeService, etc.), and updated command, GUI, and controller classes to use these new services for cleaner dependency management.
• Removed cache classes and direct cache updates from repository initialization, simplifying repository usage and improving separation of concerns.
• Replaced old argument and handler implementations in command registration with new handler classes (InvalidUsageHandlerImpl, MissingPermissionsHandlerImpl) and updated message handling to use MessageConfig and NoticeService through the Multification library.

Plugin Initialization & Shutdown Improvements

• Refactored plugin startup and shutdown logic to use new manager and service classes, improved error handling on database connection failure, and removed legacy server/software checks.
• Updated GUI initialization to use TriumphGui and new GuiManager, and replaced legacy GUI classes with new ones for lockers and main interfaces.

Build & Workflow Modernization

• Updated GitHub Actions workflow to use JDK 21, simplified Gradle build steps, and standardized artifact naming.
• Improved Gradle configuration for parallelism and caching, and enabled configuration cache for faster builds.
• Updated .gitattributes to enforce LF line endings for shell scripts and removed unnecessary rules.

New Features & Utilities

• Added a new ParcelLockersLibraryLoader class to support dynamic Maven library loading for Paper plugins, enabling better dependency management.
• Added a batch script deletePluginConfigs.bat for easier cleanup of plugin configuration files during development.

[noyzys]

Krótki, ale myślę znaczący review wobec głównej logiki architektury wobec mojej opinii.

-> UserServiceImpl można rozbić na osobne komponenty, izolując odpowiednie funkcjonalności. Aktualnie klasa łączy cachowanie, logikę biznesową i komunikację z repository. Najlepiej wydzielić operacje cache do osobnej klasy, która będzie odpowiadać wyłącznie za zarządzanie cache, podczas gdy reszta logiki pozostanie w serwisie.

• Proponuje np. UserIdentity, UserIndex - cache

-> Standardowa kolekcja HashMap nie jest thread-safe, co przy operacjach asynchronicznych z użyciem CF (CompletableFuture) może powodować race conditions i potencjalne awarie. Brak synchronizacji przy dostępie do kolekcji Map stanowi poważne zagrożenie. Rozwiązaniem jest użycie ConcurrentHashMap oraz odpowiedniego zarządzania stanem w logice.

-> Krytyczny Błąd Logiczny w Metodzie getOrCreate():
Metoda zwraca użytkownika znalezionego po nazwie (byName), nawet jeśli UUID jest różne. Taka sytuacja może powodować poważne problemy z integralnością danych - nowe UUID może zostać zignorowane, a dwa różne UUID mogą mieć tę samą nazwę, co prowadzi do niespójności schematu danych.

Według osobistych i dobrych praktyk, zalecałbym unikanie takich metod lub ich gruntowną refaktoryzację z uwzględnieniem:

• Walidacji spójności UUID i nazwy użytkownika
• Mechanizmów obsługi konfliktów
• Jawnych wyjątków dla niejednoznacznych sytuacji
• Inwalidacja cache

Rozważyć implementację wzorca tzw. Identity Map dla zarządzania cache, dzięki temu uzyskujemy:

• zapobieganie duplikacji instancji klas w cache,
• gwarancja że każda encja jest reprezentowana przez jedną instancję,
• lekko podbuduje wydajność i spójności danych w flow.

Czyli tak naprawdę dzięki zmianom uzyskujemy:

• Separację i izolację cache do osobnego komponentu (UserIdentity, UserIndex)
• Izolujemy odpowiedzialność - czyli: CACHE vs LOGIKA BIZNESOWA vs REPOSITORY
• Dostajemy wstęp do czystej architektury i obiecanego refactoringu

[thread-safe]

• Naprawienie problemu z kolekcją HashMap w środowisku asynchronicznym

• Dostrzeganie i świadomość wystąpienia RCE (Race Conditions) w Cfach.

• Eliminacja niebezpiecznej logiki i metody w serwisie

• Wprowadzenie poprawnej integralności ze schematami danych

• Wprowadzamy wzorzec Identity Map - trochę będziemy architektami

• Podnosimy overall system quality - projekt jest na dobrej drodze

[noyzys]

Wstępnie wrzuciłbym taką dawkę implementacji walidacji:

• Nie musi to być logowanie w String#format
• Wcięcia z IDE żeby było widać co dany fragment robi
• Na samym dole use case co do logiki w serwisach tylko trzeba było by zaorać większość

public record ValidationResult(
    boolean isValid, 
    String errorMessage
) {

    public static ValidationResult valid() {
        return new ValidationResult(true, null);
    }
    
    public static ValidationResult invalid(String message) {
        return new ValidationResult(false, message);
    }
}

interface LockerValidationService {
    
    ValidationResult validateCreateParameters(UUID uniqueId, String name, Position position);
    
    ValidationResult validateNoConflicts(UUID uniqueId, Position position, 
                                       Optional<Locker> existingByUUID, 
                                       Optional<Locker> existingByPosition);
}

interface ConflictDetectionStrategy {
    
    ValidationResult detectConflicts(UUID uniqueId, Position position,
                                   Optional<Locker> existingByUUID,
                                   Optional<Locker> existingByPosition);
}

@FunctionalInterface
interface ValidationRule<T> {

    ValidationResult validate(T value);
    
    default ValidationRule<T> and(ValidationRule<T> other) {
        return value -> {
            
            ValidationResult result = this.validate(value);
            return result.isValid() ? other.validate(value) : result;
        };
    }
}

// Impl: 
public final class LockerValidator implements LockerValidationService {

    private final List<ValidationRule<CreateLockerRequest>> creationRules;
    private final ConflictDetectionStrategy conflictStrategy;
    
    public LockerValidator() {
        this.creationRules = List.of(
            this::validateUUID,
            this::validateName,
            this::validatePosition);
        
        this.conflictStrategy = this::defaultConflictDetection;
    }
    
    public LockerValidator(List<ValidationRule<CreateLockerRequest>> creationRules,
                                   ConflictDetectionStrategy conflictStrategy) {
        this.creationRules = creationRules;
        this.conflictStrategy = conflictStrategy;
    }

    private record CreateLockerRequest(
        UUID uniqueId, 
        String name,
        Position position) { 
    }

    @Override
    public ValidationResult validateCreateParameters(UUID uniqueId, String name, Position position) {
        CreateLockerRequest request = new CreateLockerRequest(uniqueId, name, position);
        
        return creationRules.stream()
            .reduce(ValidationRule::and)
            .map(rule -> rule.validate(request))
            .orElse(ValidationResult.valid());
    }
    
    @Override
    public ValidationResult validateNoConflicts(UUID uniqueId, Position position,
                                              Optional<Locker> existingByUUID,
                                              Optional<Locker> existingByPosition) {
        return conflictStrategy.detectConflicts(uniqueId, position, existingByUUID, existingByPosition);
    }
    
    private ValidationResult validateUUID(CreateLockerRequest request) {
        return request.uniqueId() != null ? 
            ValidationResult.valid() : 
            ValidationResult.invalid("UUID cannot be null");
    }
    
    private ValidationResult validateName(CreateLockerRequest request) {
        return request.name() != null && !request.name().trim().isEmpty() ?
            ValidationResult.valid() :
            ValidationResult.invalid("Name cannot be empty");
    }
    
    private ValidationResult validatePosition(CreateLockerRequest request) {
        return request.position() != null ?
            ValidationResult.valid() :
            ValidationResult.invalid("Position cannot be null");
    }

    private ValidationResult defaultConflictDetection(UUID uniqueId, Position position,
                                                    Optional<Locker> existingByUUID,
                                                    Optional<Locker> existingByPosition) {
        return existingByUUID.flatMap(byUUID -> existingByPosition.map(byPosition -> validateBothExist(uniqueId, position, byUUID, byPosition)))
                .orElseGet(() -> existingByUUID
                .map(locker -> validateUUIDConflict(uniqueId, position, locker))
                .orElseGet(() -> existingByPosition
                    .map(locker -> validatePositionConflict(uniqueId, position, locker))
                    .orElse(ValidationResult.valid())));
    }
    
    private ValidationResult validateBothExist(UUID uniqueId, Position position, 
                                             Locker byUUID, Locker byPosition) {
        if (!byUUID.equals(byPosition)) {
            return ValidationResult.invalid(String.format("Conflicting lockers found. UUID %s vs Position %s", 
                uniqueId, position));
        }

        return ValidationResult.valid();
    }
    
    private ValidationResult validateUUIDConflict(UUID uniqueId, Position position, Locker locker) {
        if (!locker.position().equals(position)) {
            return ValidationResult.invalid(String.format("Locker with UUID %s exists at different position: %s", 
                uniqueId, locker.position()));
        }

        return ValidationResult.valid();
    }
    
    private ValidationResult validatePositionConflict(UUID uniqueId, Position position, Locker locker) {
        if (!locker.uuid().equals(uniqueId)) {
            return ValidationResult.invalid(String.format("Locker at position %s has different UUID: %s", 
                position, locker.uuid()));
        }

        return ValidationResult.valid();
    }  
}

// Wiadomo referencja -> 
private final LockerValidationService lockerValidator

// Pełny use case walidacji
public CompletableFuture<Locker> create(UUID uniqueId, String name, Position position) {
    return CompletableFuture.supplyAsync(() -> {
        
        ValidationResult validation = lockerValidator.validateCreateParameters(uniqueId, name, position);
        if (!validation.isValid()) {
            throw new IllegalArgumentException(validation.errorMessage());
        }

        Optional<Locker> existingByUUID = lockerCache.getByUUID(uniqueId);
        Optional<Locker> existingByPosition = lockerCache.getByPosition(position);
        
        ValidationResult conflictCheck = lockerValidator.validateNoConflicts(
            uniqueId, position, existingByUUID, existingByPosition);
        
        if (!conflictCheck.isValid()) {
            throw new IllegalStateException(conflictCheck.errorMessage()); // rzucamy wyjątek właściwy z cause
        }

        Locker locker = new Locker(uniqueId, name, position);
        lockerCache.put(locker);
        
        return lockerRepository.save(locker)
            .thenApply(savedLocker -> {
                // moze byc logowanie że np. stworzono locker czy coś takiego
                return savedLocker;
            });
    }).thenCompose(Function.identity());
}

[Jakubk15]

/gemini review

[Copilot]

Pull Request Overview

This PR introduces significant refactoring and modernization to the ParcelLockers plugin, focusing on service-based architecture improvements, enhanced GUI management, and configuration system modernization. The changes streamline the codebase by removing cache-based implementations in favor of service managers and replace legacy components with modern alternatives.

• Replaced legacy managers with new service-based architecture using ConfigService, NoticeService, and manager classes
• Modernized GUI system with new manager classes and improved placeholder handling
• Updated configuration system from CDN to OkaeriConfig with new ConfigItem serialization
• Removed cache classes and direct cache operations in favor of service-layer caching

Reviewed Changes

Copilot reviewed 112 out of 114 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
Test files	Updated to use new service architecture and removed cache dependencies
Configuration	Migrated from CDN to OkaeriConfig with new ConfigItem implementation
GUI implementation	Replaced legacy GUI classes with new service-based implementation
Repository classes	Renamed methods from find/remove to fetch/delete and removed cache integration
Manager classes	Converted concrete classes to interfaces with implementation classes
Service classes	Added new service layer with caching using Caffeine

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[gemini-code-assist]

Code Review

This is an impressive and extensive refactoring of the ParcelLockers plugin. The move to a service-based architecture, the adoption of modern tools like okaeri-configs and Paper's library loader, and the overall code cleanup have significantly improved the codebase's quality, maintainability, and performance. The new structure is much cleaner and more robust.

I have a few minor suggestions for further improvement. I also noticed that Sentry integration for error reporting was removed. Was this intentional? It's a significant change to how errors are tracked, and it might be worth reconsidering or documenting the reason for its removal.

Overall, excellent work on modernizing the plugin!