cloud icon indicating copy to clipboard operation
cloud copied to clipboard

Command pipeline does not use the right argument parser when multiple commands are used

Open bergerkiller opened this issue 3 years ago • 2 comments

When using a custom argument parser, it looks like parameters configured using annotations do not get used. What I do:

  1. Register the annotation so that it is translated to a single parser parameter (unique key + bool true as value)
  2. Register a factory for the argument parser mapped to type, where parser parameters are read
  3. Use the value read using parameters in the argument parser

I have a custom parser annotation, parameter and argument parser declared:

Parser annotation and parameter
    /**
     * Declares that the command will require write access to the saved train.
     * If the sender has no such access, declines the argument.
     */
    @Target(ElementType.PARAMETER)
    @Retention(RetentionPolicy.RUNTIME)
    public @interface SavedTrainRequiresAccess {
        public static final ParserParameter<Boolean> PARAM = new ParserParameter<Boolean>(
                "savedtrain.requiresaccess", TypeToken.get(Boolean.class));
    }

    public class SavedTrainPropertiesParser implements ArgumentParser<CommandSender, SavedTrainProperties> {
        private final boolean mustHaveAccess;

        public SavedTrainPropertiesParser(boolean mustHaveAccess) {
            this.mustHaveAccess = mustHaveAccess;
        }

        public boolean isMustHaveAccess() {
            return this.mustHaveAccess;
        }

        @Override
        public ArgumentParseResult<SavedTrainProperties> parse(CommandContext<CommandSender> commandContext, Queue<String> inputQueue) {
            commandContext.getSender().sendMessage("I am parsing with mustHaveAccess=" + mustHaveAccess);
            inputQueue.remove();
            return ArgumentParseResult.success(SavedTrainProperties.none() /*not shown*/);
        }
    }

Which are registered as follows:

manager.getParserRegistry().registerAnnotationMapper(SavedTrainRequiresAccess.class, (a, typeToken) -> {
    return ParserParameters.single(SavedTrainRequiresAccess.PARAM, Boolean.TRUE);
});

manager.getParserRegistry().registerParserSupplier(TypeToken.get(SavedTrainProperties.class), (parameters) -> {
    boolean mustHaveAccess = parameters.get(SavedTrainRequiresAccess.PARAM, Boolean.FALSE);
    System.out.println("MustHaveAccess: " + mustHaveAccess);
    return new SavedTrainPropertiesParser(mustHaveAccess);
});

When then declaring a command using the annotations method:

@CommandMethod("savedtrain <savedtrainname> rename <newsavedtrainname>")
private void commandRename(
        final CommandSender sender,
        final @SavedTrainRequiresAccess @Argument("savedtrainname") SavedTrainProperties savedTrain,
        final @Argument("newsavedtrainname") String newSavedTrainName
) {
    sender.sendMessage("Command executed!");
}

Note that there are several commands like this, some without @SavedTrainRequiresAccess annotated!

Other commands
    @CommandMethod("savedtrain <savedtrainname> info")
    @CommandDescription("Shows detailed information about a saved train")
    private void commandShowInfo(
            final CommandSender sender,
            final @Argument("savedtrainname") SavedTrainProperties savedTrain
    ) {
        sender.sendMessage("Other command info");
    }

    @CommandMethod("savedtrain <savedtrainname> export")
    @CommandDescription("Exports detailed information about a saved train to a network location")
    private void commandShowInfo(
            final CommandSender sender,
            final @Argument("savedtrainname") SavedTrainProperties savedTrain
    ) {
        sender.sendMessage("Other command export");
    }

On startup I see many messages flow by with 'MustHaveAccess: false' and then once 'MustHaveAccess: true', as expected, because I have several commands registered that parse SavedTrainProperties as arguments. Only one has this access annotation, so, so far so good.

The problem is when it comes time to parse user-provided input. I find that the mustHaveAccess flag is suddenly false. That is, I read for /savedtrain name rename newname:

I am parsing with mustHaveAccess=false

Further testing showed that it is using a parser from an entirely different command, one that uses different parameters. This is wrong.


I can use registerCommandPostProcessor, check whether the individual argument uses the SavedTrainPropertiesParser, and if so, perform post processing on it. In that situation the SavedTrainPropertiesParser isMustHaveAccess() returns true, as expected.

Sample code
manager.registerCommandPostProcessor(context -> {
    for (CommandArgument<CommandSender, ?> arg : context.getCommand().getArguments())
    {
        if (arg.getParser() instanceof SavedTrainPropertiesParser
                && ((SavedTrainPropertiesParser) arg.getParser()).isMustHaveAccess())
        {
            // This indeed gets printed!
            context.getCommandContext().getSender().sendMessage("isMustHaveAccess=true!");
        }
    }
});

My guess is that during parsing it has some internal cache purely mapped to type, which it uses instead of the command-declared parser. Or, maybe the other commands get matched first and that parser is used, even though there is another command that matches more perfectly.

bergerkiller avatar Nov 16 '20 23:11 bergerkiller

This is because nodes of the same type at the same level are merged into a single node. This is due to the fact that you cannot have multiple variable nodes as the children of any parent node, so these are all considered to be the same. I can't think of any good way to work around this, as they'll all use the same parser. I guess the best approach is to use command meta for this.

Citymonstret avatar Jan 13 '21 12:01 Citymonstret

The way Ive fixed that for myself, is using a command preprocessor to analyze the command. It puts the state information of whether mustHaveAccess is true/false, which the parser then successfully picks up.

Probably, if the parser can have access to the full command information, then this can also be done that way. Parser parameters become a bit unneeded then.

This all works because in the command object itself, it still as the original argument information (not merged)

bergerkiller avatar Jan 16 '21 20:01 bergerkiller