Merge branch 'feat/57-pre-execution-checks-for-commandhandler' into 'main'

Add pre-execution checks with CommandHandlerExecutor to CommandHandler Closes #57 See merge request cs108-fs26/Gruppe-13!86
2026-04-08 18:58:20 +02:00
parent b60374f7e5 3de20e3d6c
commit 795830cf02
18 changed files with 257 additions and 22 deletions
@@ -64,11 +64,31 @@ Concrete subclasses add command-specific fields, all set via constructor. Reques
 ### `CommandHandler<T extends Request>`
 <img src="../../../images/docs/networking/commands/command_handler.png" alt="Class diagram of the CommandHandler" />

-The `CommandHandler` is a single-method interface responsible for executing a typed request.
+The `CommandHandler` is an abstract base class responsible for executing a typed request.
 The handler contains the domain logic: reading from registries and managers, modifying state, and dispatching a response via the `ResponseDispatcher`.

 Handlers receive their dependencies (the `ResponseDispatcher`, registries and managers etc.) through constructor injection.
-This keeps them fully testable without the network layer.
+They can also register reusable pre-execution checks via `addCheck(...)`. These checks are stored on the handler and are evaluated before `execute(...)` runs.
+
+When a piece of trivial validation is shared by multiple handlers, it should be extracted into a dedicated `HandlerCheck` instead of being duplicated in each handler.
+
+### `HandlerCheck`
+<img src="../../../images/docs/networking/commands/handler_check.png" alt="Class diagram of HandlerCheck and CommandHandlerExecutor" />
+
+The `HandlerCheck` is a functional interface for reusable pre-execution validation.
+Its `check(...)` method receives the incoming `Request` and returns an empty `Optional` if the request may continue.
+If the check fails, it returns an `ErrorResponse` wrapped in the `Optional`, which will be dispatched to the client instead of calling the handler.
+
+This is the right place for small shared checks such as "is the user logged in?" or other simple preconditions that multiple handlers need.
+
+### `CommandHandlerExecutor`
+<img src="../../../images/docs/networking/commands/handler_check_executor.png" alt="Class diagram of HandlerCheck and CommandHandlerExecutor" />
+
+The `CommandHandlerExecutor` runs all checks registered on a handler before invoking `execute(...)`.
+If any `HandlerCheck` returns a response, the executor dispatches it immediately and aborts execution.
+Otherwise, the handler is executed normally.
+
+This keeps precondition handling separate from the actual domain logic inside the handler.

 ### `CommandRouter`
 <img src="../../../images/docs/networking/commands/command_router.png" alt="Class diagram of the CommandRouter" />
@@ -159,6 +159,9 @@ public class GreetHandler implements CommandHandler<GreetRequest> {
 **Rules for the Handler class:**

 - Declare all dependencies as `private final` fields, injected through the constructor.
+- Use `addCheck(...)` to attach pre-execution checks that should run before `execute(...)`.
+- If several handlers share the same trivial logic, such as verifying that the user is logged in, 
+  extract that logic into a separate `HandlerCheck` and reuse it instead of duplicating the code.
 - Always dispatch exactly one response per execution path. Every branch must end with a
  `responseDispatcher.dispatch(...)` call.
 - Use `ErrorResponse` for domain-level failures (e.g. entity not found, precondition not met).
@@ -0,0 +1,8 @@
+@startuml
+skinparam backgroundColor transparent
+
+interface HandlerCheck {
+    + check(request: Request): Optional<Response>
+}
+
+@enduml
@@ -0,0 +1,31 @@
+@startuml
+skinparam backgroundColor transparent
+
+class CommandHandlerExecutor {
+    - responseDispatcher: ResponseDispatcher
+    + CommandHandlerExecutor(responseDispatcher: ResponseDispatcher)
+    + execute(handler: CommandHandler<Request>, request: Request): void
+}
+
+interface HandlerCheck {
+    + check(request: Request): Optional<Response>
+}
+
+abstract class CommandHandler<T extends Request> {
+    - checks: List<HandlerCheck>
+    # addCheck(check: HandlerCheck): void
+    + getChecks(): List<HandlerCheck>
+    + execute(request: T): void
+}
+
+interface ResponseDispatcher
+class Request
+class Response
+
+CommandHandlerExecutor --> CommandHandler : executes
+CommandHandler "1" o-- "0..*" HandlerCheck : registered checks
+CommandHandlerExecutor --> HandlerCheck : evaluates
+HandlerCheck ..> Request : inspects
+HandlerCheck ..> Response : returns failure response
+CommandHandlerExecutor --> ResponseDispatcher : dispatches failures
+@enduml
@@ -15,6 +15,7 @@ import ch.unibas.dmi.dbis.cs108.casono.server.app.commands.ping.PingRequest;
 import ch.unibas.dmi.dbis.cs108.casono.server.domain.user.UserCleanupJob;
 import ch.unibas.dmi.dbis.cs108.casono.server.domain.user.UserRegistry;
 import ch.unibas.dmi.dbis.cs108.casono.server.network.NetworkManager;
+import ch.unibas.dmi.dbis.cs108.casono.server.network.command.execution.CommandHandlerExecutor;
 import ch.unibas.dmi.dbis.cs108.casono.server.network.command.execution.CommandRouter;
 import ch.unibas.dmi.dbis.cs108.casono.server.network.command.parsing.CommandParserDispatcher;
 import ch.unibas.dmi.dbis.cs108.casono.server.network.events.DisconnectEvent;
@@ -46,11 +47,12 @@ public class ServerApp {

        EventBus eventBus = new EventBus();
        CommandParserDispatcher dispatcher = new CommandParserDispatcher();
-        CommandRouter router = new CommandRouter();
+        SessionManager sessionManager = new SessionManager(eventBus, dispatcher);
+        ResponseDispatcher responseDispatcher = new ResponseDispatcher(sessionManager);
+        CommandHandlerExecutor handlerExecutor = new CommandHandlerExecutor(responseDispatcher);
+        CommandRouter router = new CommandRouter(handlerExecutor);

-        SessionManager sessionManager = new SessionManager(eventBus, dispatcher, router);
        eventBus.subscribe(DisconnectEvent.class, event -> sessionManager.onDisconnect(event));
-        NetworkManager networkManager = new NetworkManager(port, sessionManager);

        UserRegistry userRegistry = new UserRegistry();
        eventBus.subscribe(
@@ -71,10 +73,9 @@ public class ServerApp {
                SESSION_DISCONNECT_JOB_PERIOD,
                TimeUnit.SECONDS);

-        ResponseDispatcher responseDispatcher = new ResponseDispatcher(sessionManager);
-
        registerCommands(dispatcher, router, responseDispatcher, userRegistry);

+        NetworkManager networkManager = new NetworkManager(port, sessionManager, router);
        networkManager.start();
    }

@@ -0,0 +1,30 @@
+package ch.unibas.dmi.dbis.cs108.casono.server.app.checks;
+
+import ch.unibas.dmi.dbis.cs108.casono.server.domain.user.User;
+import ch.unibas.dmi.dbis.cs108.casono.server.domain.user.UserRegistry;
+import ch.unibas.dmi.dbis.cs108.casono.server.network.command.execution.checks.HandlerCheck;
+import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.request.Request;
+import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.response.ErrorResponse;
+import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.response.Response;
+import java.util.Optional;
+
+public class UserLoggedInCheck implements HandlerCheck {
+    private final UserRegistry userRegistry;
+
+    public UserLoggedInCheck(UserRegistry userRegistry) {
+        this.userRegistry = userRegistry;
+    }
+
+    @Override
+    public Optional<Response> check(Request request) {
+        Optional<User> user = userRegistry.getBySessionId(request.getSessionId());
+        if (user.isPresent()) {
+            return Optional.empty();
+        }
+        return Optional.of(
+                new ErrorResponse(
+                        request.getContext(),
+                        "USER_NOT_LOGGED_IN",
+                        "The execution of this command requires the user to be logged in"));
+    }
+}
@@ -7,7 +7,7 @@ import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.response.dispatch
 import java.util.Optional;

 /** Handles {@link CheckUsernameRequest}s to check whether a username is available. */
-public class CheckUsernameHandler implements CommandHandler<CheckUsernameRequest> {
+public class CheckUsernameHandler extends CommandHandler<CheckUsernameRequest> {
    private final ResponseDispatcher responseDispatcher;
    private final UserRegistry userRegistry;

@@ -13,7 +13,7 @@ import java.util.Optional;
 * Handles {@link LoginRequest}s to create a user for a session, if the session has not assigned one
 * already
 */
-public class LoginHandler implements CommandHandler<LoginRequest> {
+public class LoginHandler extends CommandHandler<LoginRequest> {
    private final ResponseDispatcher responseDispatcher;
    private final UserRegistry userRegistry;
    private final UserFactory userFactory;
@@ -7,7 +7,7 @@ import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.response.OkRespon
 import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.response.dispatcher.ResponseDispatcher;

 /** Handles {@link LogoutRequest} to logout connected user. */
-public class LogoutHandler implements CommandHandler<LogoutRequest> {
+public class LogoutHandler extends CommandHandler<LogoutRequest> {
    private final ResponseDispatcher responseDispatcher;
    private final UserRegistry userRegistry;

@@ -5,7 +5,7 @@ import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.response.OkRespon
 import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.response.dispatcher.ResponseDispatcher;

 /** Handler for {@link PingRequest}. */
-public class PingHandler implements CommandHandler<PingRequest> {
+public class PingHandler extends CommandHandler<PingRequest> {
    private final ResponseDispatcher responseDispatcher;

    /**
@@ -1,5 +1,6 @@
 package ch.unibas.dmi.dbis.cs108.casono.server.network;

+import ch.unibas.dmi.dbis.cs108.casono.server.network.command.execution.CommandRouter;
 import ch.unibas.dmi.dbis.cs108.casono.server.network.sessions.SessionManager;
 import ch.unibas.dmi.dbis.cs108.casono.server.network.transport.TcpTransport;
 import java.io.IOException;
@@ -15,6 +16,7 @@ public class NetworkManager implements Runnable {
    private Thread thread;
    private Boolean running;
    private SessionManager sessionManager;
+    private CommandRouter router;

    /**
     * Creates a new NetworkManager with the given port, session manager, and event bus.
@@ -22,12 +24,13 @@ public class NetworkManager implements Runnable {
     * @param port the port to listen on
     * @param sessionManager the session manager to use
     */
-    public NetworkManager(Integer port, SessionManager sessionManager) {
+    public NetworkManager(Integer port, SessionManager sessionManager, CommandRouter router) {
        this.port = port;
        this.logger = LogManager.getLogger(NetworkManager.class);
        this.thread = new Thread(this, "networkManager");
        this.running = true;
        this.sessionManager = sessionManager;
+        this.router = router;
    }

    /** Starts the internal thread to accept new connections. */
@@ -45,7 +48,7 @@ public class NetworkManager implements Runnable {

                logger.debug("Accepted connection from {}", clientSocket.getRemoteSocketAddress());

-                sessionManager.create(new TcpTransport(clientSocket));
+                sessionManager.create(new TcpTransport(clientSocket), router);
            }

        } catch (IOException e) {
@@ -1,7 +1,45 @@
 package ch.unibas.dmi.dbis.cs108.casono.server.network.command.execution;

+import ch.unibas.dmi.dbis.cs108.casono.server.network.command.execution.checks.HandlerCheck;
 import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.request.Request;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;

-public interface CommandHandler<T extends Request> {
-    void execute(T request);
+/**
+ * Abstract base class for handling requests of a specific type.
+ *
+ * @param <T> the type of request this handler processes, must extend {@link Request}
+ */
+public abstract class CommandHandler<T extends Request> {
+    private final List<HandlerCheck> checks = new ArrayList<>();
+
+    /**
+     * Adds a handler check to be performed before request execution.
+     *
+     * <p>The {@code execute} Method is only invoked, if all specified checks passed.
+     *
+     * @param check the {@link HandlerCheck} to add
+     */
+    protected final void addCheck(HandlerCheck check) {
+        checks.add(check);
+    }
+
+    /**
+     * Returns an unmodifiable list of all registered handler checks.
+     *
+     * @return an unmodifiable list of {@link HandlerCheck} objects
+     */
+    public final List<HandlerCheck> getChecks() {
+        return Collections.unmodifiableList(checks);
+    }
+
+    /**
+     * Executes the request.
+     *
+     * <p>Subclasses should override this method to implement their specific request handling logic.
+     *
+     * @param request the request to execute, of type T
+     */
+    public void execute(T request) {}
 }
@@ -0,0 +1,46 @@
+package ch.unibas.dmi.dbis.cs108.casono.server.network.command.execution;
+
+import ch.unibas.dmi.dbis.cs108.casono.server.network.command.execution.checks.HandlerCheck;
+import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.request.Request;
+import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.response.Response;
+import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.response.dispatcher.ResponseDispatcher;
+import java.util.Optional;
+
+/**
+ * Invokes execution of the {@link CommandHandler} if all defined pre-execution checks pass.
+ *
+ * <p>If any check fails, the response returned by the check is dispatched immediately and execution
+ * is aborted. Otherwise, the handler will invoke execution normally.
+ */
+public class CommandHandlerExecutor {
+    private final ResponseDispatcher responseDispatcher;
+
+    /**
+     * Creates a new CommandHandlerExecutor to execute pre-execution checks and invoke the {@link
+     * CommandHandler} if all checks pass.
+     *
+     * @param responseDispatcher to dispatch the response with if an check fails
+     */
+    public CommandHandlerExecutor(ResponseDispatcher responseDispatcher) {
+        this.responseDispatcher = responseDispatcher;
+    }
+
+    /**
+     * Executes a command handler if all defined pre-execution checks pass.
+     *
+     * @param handler the command handler containing checks and execution logic
+     * @param request the incoming request to validate and process
+     * @throws NullPointerException if handler or request is null
+     */
+    public void execute(CommandHandler<Request> handler, Request request) {
+        for (HandlerCheck check : handler.getChecks()) {
+            Optional<Response> result = check.check(request);
+            if (result.isPresent()) {
+                responseDispatcher.dispatch(result.get());
+                return;
+            }
+        }
+
+        handler.execute(request);
+    }
+}
@@ -4,13 +4,46 @@ import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.request.Request;
 import java.util.HashMap;
 import java.util.Map;

+/** Routes incoming requests to their corresponding {@link CommandHandler} implementations. */
 public class CommandRouter {
    private final Map<Class<? extends Request>, CommandHandler<?>> handlers = new HashMap<>();
+    private final CommandHandlerExecutor handlerExecutor;

+    /**
+     * Creates a new CommandRouter with the specified handler executor.
+     *
+     * @param commandHandlerExecutor to delegate execution of checks and invocation of {@link
+     *     CommandHandler} to
+     */
+    public CommandRouter(CommandHandlerExecutor commandHandlerExecutor) {
+        this.handlerExecutor = commandHandlerExecutor;
+    }
+
+    /**
+     * Registers a {@link CommandHandler} for a specific request type.
+     *
+     * <p>This method establishes a type-safe mapping between a request class and its handler.
+     *
+     * <p>Only one handler can be registered per request type; subsequent registrations will
+     * overwrite previous ones.
+     *
+     * @param <T> the type of request handled by the provided handler
+     * @param request the request class to associate with the given command handler
+     * @param handler the command handler for the given request type
+     */
    public <T extends Request> void register(Class<T> request, CommandHandler<T> handler) {
        handlers.put(request, handler);
    }

+    /**
+     * Executes the appropriate handler for the given request.
+     *
+     * <p>Looks up the handler registered for the request's class and executes it through the
+     * configured {@link CommandHandlerExecutor}.
+     *
+     * @param request the request to be executed
+     * @throws UnknownRequestException if no handler is registered for the request type
+     */
    // Safe, because during registration, it's ensured that the provided CommandHandler only
    // receives requests it can handle.
    @SuppressWarnings("unchecked")
@@ -23,6 +56,7 @@ public class CommandRouter {
            throw new UnknownRequestException(
                    "Unable to execute request " + requestName + ". Type unknown", requestName);
        }
-        handler.execute(request);
+
+        handlerExecutor.execute(handler, request);
    }
 }
@@ -0,0 +1,23 @@
+package ch.unibas.dmi.dbis.cs108.casono.server.network.command.execution.checks;
+
+import ch.unibas.dmi.dbis.cs108.casono.server.network.command.execution.CommandHandler;
+import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.request.Request;
+import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.response.ErrorResponse;
+import ch.unibas.dmi.dbis.cs108.casono.server.network.protocol.response.Response;
+import java.util.Optional;
+
+/**
+ * Functional interface used by pre-execution checks. Executed before the execute method of the
+ * {@link CommandHandler} is called
+ */
+@FunctionalInterface
+public interface HandlerCheck {
+    /**
+     * Checks that the requirements outlined by this HandlerCheck are fulfilled.
+     *
+     * @param request to execute the check with
+     * @return Optional containing no value if the check was successful. And a {@link ErrorResponse}
+     *     if the check failed, the response will be dispatched to the client.
+     */
+    Optional<Response> check(Request request);
+}
@@ -19,16 +19,13 @@ public class SessionManager {
    private final EventBus eventBus;
    private final Logger logger;
    private final CommandParserDispatcher dispatcher;
-    private final CommandRouter router;

    /** Constructs a new SessionManager. */
-    public SessionManager(
-            EventBus eventBus, CommandParserDispatcher dispatcher, CommandRouter router) {
+    public SessionManager(EventBus eventBus, CommandParserDispatcher dispatcher) {
        this.sessions = new ConcurrentHashMap<>();
        this.eventBus = eventBus;
        this.logger = LogManager.getLogger(SessionManager.class);
        this.dispatcher = dispatcher;
-        this.router = router;
    }

    /**
@@ -37,9 +34,10 @@ public class SessionManager {
     * <p>Will create both worker threads and start them.
     *
     * @param transport to create session from
+     * @param router the command router used by the created session
     * @return newly created session
     */
-    public Session create(TransportLayer transport) {
+    public Session create(TransportLayer transport, CommandRouter router) {
        Session session = new Session(transport, eventBus, dispatcher, router);
        SessionReader reader = new SessionReader(session, eventBus);
        SessionWriter writer = new SessionWriter(session);