diff --git a/documents/docs/networking/commands/commands-deep-dive.md b/documents/docs/networking/commands/commands-deep-dive.md index 5a0178c..0540dcb 100644 --- a/documents/docs/networking/commands/commands-deep-dive.md +++ b/documents/docs/networking/commands/commands-deep-dive.md @@ -64,11 +64,31 @@ Concrete subclasses add command-specific fields, all set via constructor. Reques ### `CommandHandler` 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. +Handlers receive their dependencies (the `ResponseDispatcher`, registries and managers etc.) through constructor injection. +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` +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` +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` Class diagram of the CommandRouter diff --git a/documents/docs/networking/commands/guide-on-implementing-a-command.md b/documents/docs/networking/commands/guide-on-implementing-a-command.md index 037251f..e45b6fd 100644 --- a/documents/docs/networking/commands/guide-on-implementing-a-command.md +++ b/documents/docs/networking/commands/guide-on-implementing-a-command.md @@ -159,6 +159,9 @@ public class GreetHandler implements CommandHandler { **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). diff --git a/documents/images/docs/networking/commands/handler_check.png b/documents/images/docs/networking/commands/handler_check.png new file mode 100644 index 0000000..4824d15 Binary files /dev/null and b/documents/images/docs/networking/commands/handler_check.png differ diff --git a/documents/images/docs/networking/commands/handler_check.puml b/documents/images/docs/networking/commands/handler_check.puml new file mode 100644 index 0000000..8343c1b --- /dev/null +++ b/documents/images/docs/networking/commands/handler_check.puml @@ -0,0 +1,8 @@ +@startuml +skinparam backgroundColor transparent + +interface HandlerCheck { + + check(request: Request): Optional +} + +@enduml \ No newline at end of file diff --git a/documents/images/docs/networking/commands/handler_check_executor.png b/documents/images/docs/networking/commands/handler_check_executor.png new file mode 100644 index 0000000..aa3a444 Binary files /dev/null and b/documents/images/docs/networking/commands/handler_check_executor.png differ diff --git a/documents/images/docs/networking/commands/handler_check_executor.puml b/documents/images/docs/networking/commands/handler_check_executor.puml new file mode 100644 index 0000000..07dc033 --- /dev/null +++ b/documents/images/docs/networking/commands/handler_check_executor.puml @@ -0,0 +1,31 @@ +@startuml +skinparam backgroundColor transparent + +class CommandHandlerExecutor { + - responseDispatcher: ResponseDispatcher + + CommandHandlerExecutor(responseDispatcher: ResponseDispatcher) + + execute(handler: CommandHandler, request: Request): void +} + +interface HandlerCheck { + + check(request: Request): Optional +} + +abstract class CommandHandler { + - checks: List + # addCheck(check: HandlerCheck): void + + getChecks(): List + + 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 \ No newline at end of file diff --git a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/ServerApp.java b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/ServerApp.java index 1b5f62a..febc9a9 100644 --- a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/ServerApp.java +++ b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/ServerApp.java @@ -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(); } diff --git a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/checks/UserLoggedInCheck.java b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/checks/UserLoggedInCheck.java new file mode 100644 index 0000000..8bc305f --- /dev/null +++ b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/checks/UserLoggedInCheck.java @@ -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 check(Request request) { + Optional 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")); + } +} diff --git a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/check_nick/CheckUsernameHandler.java b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/check_nick/CheckUsernameHandler.java index c238ba1..95debb0 100644 --- a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/check_nick/CheckUsernameHandler.java +++ b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/check_nick/CheckUsernameHandler.java @@ -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 { +public class CheckUsernameHandler extends CommandHandler { private final ResponseDispatcher responseDispatcher; private final UserRegistry userRegistry; diff --git a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/login/LoginHandler.java b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/login/LoginHandler.java index 5d139f8..4215b3b 100644 --- a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/login/LoginHandler.java +++ b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/login/LoginHandler.java @@ -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 { +public class LoginHandler extends CommandHandler { private final ResponseDispatcher responseDispatcher; private final UserRegistry userRegistry; private final UserFactory userFactory; diff --git a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/logout/LogoutHandler.java b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/logout/LogoutHandler.java index c5ca9fd..4ef9f98 100644 --- a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/logout/LogoutHandler.java +++ b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/logout/LogoutHandler.java @@ -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 { +public class LogoutHandler extends CommandHandler { private final ResponseDispatcher responseDispatcher; private final UserRegistry userRegistry; diff --git a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/ping/PingHandler.java b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/ping/PingHandler.java index 1669cd8..aed42e5 100644 --- a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/ping/PingHandler.java +++ b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/app/commands/ping/PingHandler.java @@ -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 { +public class PingHandler extends CommandHandler { private final ResponseDispatcher responseDispatcher; /** diff --git a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/NetworkManager.java b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/NetworkManager.java index d9382a4..83a39d3 100644 --- a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/NetworkManager.java +++ b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/NetworkManager.java @@ -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) { diff --git a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/CommandHandler.java b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/CommandHandler.java index 0fdae3c..aa9c4da 100644 --- a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/CommandHandler.java +++ b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/CommandHandler.java @@ -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 { - void execute(T request); +/** + * Abstract base class for handling requests of a specific type. + * + * @param the type of request this handler processes, must extend {@link Request} + */ +public abstract class CommandHandler { + private final List checks = new ArrayList<>(); + + /** + * Adds a handler check to be performed before request execution. + * + *

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 getChecks() { + return Collections.unmodifiableList(checks); + } + + /** + * Executes the request. + * + *

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) {} } diff --git a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/CommandHandlerExecutor.java b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/CommandHandlerExecutor.java new file mode 100644 index 0000000..0de6c97 --- /dev/null +++ b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/CommandHandlerExecutor.java @@ -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. + * + *

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 handler, Request request) { + for (HandlerCheck check : handler.getChecks()) { + Optional result = check.check(request); + if (result.isPresent()) { + responseDispatcher.dispatch(result.get()); + return; + } + } + + handler.execute(request); + } +} diff --git a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/CommandRouter.java b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/CommandRouter.java index 269c6a4..5b177ed 100644 --- a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/CommandRouter.java +++ b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/CommandRouter.java @@ -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, 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. + * + *

This method establishes a type-safe mapping between a request class and its handler. + * + *

Only one handler can be registered per request type; subsequent registrations will + * overwrite previous ones. + * + * @param 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 void register(Class request, CommandHandler handler) { handlers.put(request, handler); } + /** + * Executes the appropriate handler for the given request. + * + *

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); } } diff --git a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/checks/HandlerCheck.java b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/checks/HandlerCheck.java new file mode 100644 index 0000000..5db599f --- /dev/null +++ b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/command/execution/checks/HandlerCheck.java @@ -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 check(Request request); +} diff --git a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/sessions/SessionManager.java b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/sessions/SessionManager.java index c0ffe55..569b828 100644 --- a/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/sessions/SessionManager.java +++ b/src/main/java/ch/unibas/dmi/dbis/cs108/casono/server/network/sessions/SessionManager.java @@ -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 { *

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);