First attempt at refactoring commands. by Gnuxie · Pull Request #403 · matrix-org/mjolnir

Gnuxie · 2022-11-01T15:00:43Z

The idea is that commands can be used in other contexts than just from matrix, e.g. the mjolnir appservice web api that is given to the widget.

We also begin organizing commands into tables that can be automatically created from a set of features that are enabled in mjolnir's config (e.g. "synapse admin").

The only commands to have been changes so far is the ban/unban commands. Though the parser logic in that command is horrible.

This doesn't attempt to change the way we parse commands from Matrix, but I do plan to change that too, unsure of whether that will be in this PR though.

The PR is a draft because I want initial comments on the approach/design before i commit and rewrite more of the existing command handler.

Yoric

Yeah, sounds like a sound plan.

Yoric · 2022-11-03T12:43:42Z

src/commands/MatrixInterfaceCommand.ts

+    result: Awaited<ExecutorReturnType>) => Promise<void>;
+
+/**
+ * Note, matrix interface command can be multi step ie ask for confirmation.


Nit: Generally speaking, documentation for a class should start with a description of what the class does, before any note. Here and for other classes.

Yoric · 2022-11-03T12:44:49Z

src/commands/CommandHandler.ts

        } else if (parts[1] === 'make' && parts[2] === 'admin' && parts.length > 3) {
            return await execMakeRoomAdminCommand(roomId, event, mjolnir, parts);
        } else {
+            const command = commandTable.findAMatchingCommand(parts.slice(1));


So the idea is to apply it gradually, by progressively moving commands from handleCommand? Sounds like a good idea.

Yoric · 2022-11-03T12:45:09Z

src/commands/CommandHandler.ts

        return await mjolnir.client.sendMessage(roomId, reply);
    }
 }
+


Yoric · 2022-11-03T12:45:35Z

src/models/PolicyList.ts

 */

-import { extractRequestError, LogService, MatrixClient, UserID } from "matrix-bot-sdk";
+import { extractRequestError, LogService, MatrixClient, RoomCreateOptions, UserID } from "matrix-bot-sdk";


Is that change related?

src/commands/ApplicationCommand.ts

Yoric · 2022-11-10T07:29:10Z

src/Mjolnir.ts

                await client.sendReadReceipt(roomId, event['event_id']);
-                return handleCommand(roomId, event, this);
+
+                return handleCommand(roomId, event, this, this.matrixCommandTable);


Nit: Shouldn't handleCommand be a method of matrixCommandTable?

Yep, but can't be until we have moved all the commands to the new table

src/commands/ApplicationCommand.ts

Yoric · 2022-11-10T07:37:04Z

src/commands/MatrixInterfaceCommand.ts

+    mjolnir: Mjolnir,
+    roomId: string,
+    event: any,
+    parts: string[]) => Promise<Parameters<ExecutorType>>;


I'm not entirely certain that parts is the right type for these. It makes it impossible to parse things that contains whitespaces. Or perhaps we'll want to add a special case for "any series of words between quotes" and turn them into a single part?

Yep, this was mostly for compatibility. I'm not sure how to design the parsing properly yet but I do want this to be replaced.

What i do know though is that I am thinking of something where everything is read from a stream but we can just declare the accepting types for each argument + provide a parse function for specific arguments if they don't have common types (common meaning like a matrix room or mxid).

Yoric · 2022-11-10T07:41:43Z

src/commands/MatrixInterfaceCommand.ts

+ * This is used by mjolnirs.
+ */
+export class MatrixCommandTable {
+    public readonly features: ApplicationFeature[];


Is this the list of active features?

Gnuxie

So, if you neglected to tell me if I was crazy before, how about now?

Gnuxie · 2022-11-23T17:58:26Z

src/commands/Validation.ts

+/**
+ * Why do we need a Result Monad for the parser signiture.
+ * I (Gnuxie) don't like monadic error handling, simply because
+ * I'm a strong believer in failing early, yes i may be misinformed.
+ * The only reason we don't use an exception in this case is because
+ * these are NOT to be used nilly willy and thrown out of context
+ * from an unrelated place. The Monad ensures locality (in terms of call chain)
+ * to the user interface by being infuriating to deal with.
+ * It also does look different to an exception
+ * to a naive programmer. Ideally though, if the world had adopted
+ * condition based error handling i would simply create a condition
+ * type for validation errors that can be translated/overidden by
+ * the command handler, and it wouldn't have to look like this.
+ * It's important to remember the errors we are reporting are to do with user input,
+ * we're trying to tell the user they did something wrong and what that is.
+ * This is something completely different to a normal exception,
+ * where we are saying to ourselves that our assumptions in our code about
+ * the thing we're doing are completely wrong. The user never
+ * should see these as there is nothing they can do about it.
+ *
+ * OK, it would be too annoying even for me to have a real Monad.
+ * So this is dumb as hell, no worries though
+ * 
+ * OK I'm beginning to regret my decision.
+ * 
+ * TODO: Can we make ValidationResult include ValidationError
+ */


Tell me what you think

Gnuxie · 2022-11-23T17:59:15Z

src/commands/UnbanBanCommand.ts

-        return null;
+    if (list.isErr()) {
+        return ValidationResult.Err(list.err);
+    } else if (!ruleType) {
+        return ValidationResult.Err(
+            ValidationError.makeValidationError('uknown rule type', "Please specify the type as either 'user', 'room', or 'server'")
+        );
+    } else if (!entity) {
+        return ValidationResult.Err(
+            ValidationError.makeValidationError('no entity', "No entity was able to be parsed from this command")
+        );
+    } else if (mjolnir.config.commands.confirmWildcardBan && /[*?]/.test(entity) && !force) {
+        return ValidationResult.Err(
+            ValidationError.makeValidationError("wildcard required", "Wildcard bans require an additional `--force` argument to confirm")
+        );


And here as an example

Yoric · 2022-11-24T07:11:34Z

src/commands/ApplicationCommand.ts

+ * Used by ApplicationCommands as required feature flags they depend on to function.
+ */
+export interface ApplicationFeature {
+    name: string,


Doc: Is this a human-readable name? A unique key? If the latter, possibly rename to key.

Yoric · 2022-11-24T07:12:07Z

src/commands/ApplicationCommand.ts

+ */
+const APPLICATION_FEATURES = new Map<string/*feature name*/, ApplicationFeature>();
+
+export function defineApplicationFeature(feature: ApplicationFeature): void {


Nit: Doc needed

Yoric · 2022-11-28T10:22:01Z

src/commands/Validation.ts

+ * type for validation errors that can be translated/overidden by
+ * the command handler, and it wouldn't have to look like this.
+ * It's important to remember the errors we are reporting are to do with user input,
+ * we're trying to tell the user they did something wrong and what that is.


Well, generally speaking, there are several kinds of errors and error-handling:

Something in my code is broken. In JS, that's generally represented by a TypeError (even if it's not a type error, sigh) or SyntaxError (except SyntaxError is also used when parsing JSON, sigh).

Something in my environment is broken. In JS, that's generally other cases of Error.

Something in my input is broken. There is nothing built-in in JS, but that could be handled by creating a new variant of Error.

While I'm ok with using monads in the right setting and there are ways to use Monads in JS with some syntactic support, by encoding them as a generator and its caller. However, introducing a different paradigm in the middle of an application feels a bit cumbersome, so I'd like to avoid it if possible.

On the other hand, I don't see anything that looks like monadic binding, so this isn't really a monad, more a data structure to represent good parse result/bad parse result, right? That feels ok to me.

What does it bring wrt a new variant of Error, though? Better type-checking?

so this isn't really a monad, more a data structure to represent good parse result/bad parse result, right? That feels ok to me.

Yep

What does it bring wrt a new variant of Error, though? Better type-checking?

Because as I said, so it doesn't get misused. The validation errors are a description of something that the user has done wrong, for the user to see.

Gnuxie requested review from Yoric and jesopo November 1, 2022 15:00

Yoric reviewed Nov 10, 2022

View reviewed changes

Gnuxie marked this pull request as ready for review November 11, 2022 11:33

Gnuxie added 3 commits November 23, 2022 17:42

First stage of commands refactor.

953b065

ValidationInterface

cddfe58

Use Ban/Unban command as an example.

3fa1a43

Gnuxie force-pushed the gnuxie/commands branch from 3e2e505 to 3fa1a43 Compare November 23, 2022 17:57

Gnuxie commented Nov 23, 2022

View reviewed changes

Gnuxie requested a review from Yoric November 23, 2022 18:01

Yoric reviewed Nov 28, 2022

View reviewed changes

H-Shay requested a review from a team as a code owner January 21, 2025 19:18

Conversation

Gnuxie commented Nov 1, 2022

Uh oh!

Yoric left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gnuxie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants