Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OverrideTypeReaderAttribute incorrectly applies to other parameters of the same type #936

Closed
glen3b opened this issue Jan 20, 2018 · 1 comment

Comments

@glen3b
Copy link

glen3b commented Jan 20, 2018

[glen@UltimateBuilder ~]$ dotnet --version
2.1.3
[glen@UltimateBuilder ~]$ dotnet --info
.NET Command Line Tools (2.1.3)

Product Information:
 Version:            2.1.3
 Commit SHA-1 hash:  a0ca411ca5

Runtime Environment:
 OS Name:     arch
 OS Version:  
 OS Platform: Linux
 RID:         linux-x64
 Base Path:   /opt/dotnet/sdk/2.1.3/

Microsoft .NET Core Shared Framework Host

  Version  : 2.0.4
  Build    : 7f262f453d8c8479b9af91d34c013b3aa05bc1ff

[glen@UltimateBuilder ~]$ 

Tested with Discord.Net AppVeyor build version 2.0.0-beta-00880 (for commit 05cd1ff).


Consider the following two classes, a type reader and a module (the type reader is not registered with the command service for any type - see the main method):

// For demonstration purposes; a real "validating" type reader would be more useful
public class NumericStringTypeReader : TypeReader
{
    public override Task<TypeReaderResult> ReadAsync(ICommandContext context, string input, IServiceProvider services)
    {
        if (int.TryParse(input, out int val))
        {
            return Task.FromResult(TypeReaderResult.FromSuccess(input));
        }
        return Task.FromResult(TypeReaderResult.FromError(CommandError.ParseFailed, "Could not parse stringy-integer argument."));
    }
}

public class TestCommandModule : ModuleBase
{
    [Command("stringintecho")]
    public Task StringIntEcho([OverrideTypeReader(typeof(NumericStringTypeReader))] string argument) => ReplyAsync("Message (stringIntEcho): " + argument);
    [Command("stringecho")]
    public Task StringEcho(string argument) => ReplyAsync("Message (stringEcho): " + argument);
}

Expected behavior:

  • The stringecho command echoes any argument given without error
  • Due to the presence of the overriding type reader, the stringintecho method only accepts and echoes parseable integers

Reality:

  • Both commands are restricted via the type reader (type reader's invocation evidenced by the distinct parse error message) to accepting only parseable integers
    (Note especially the first command invocation in the screenshot)
    message screenshot

Notes:
This issue does not occur if the declaration order of the commands is reversed. However, the order of invocation once the bot has started appears to be irrelevant.

The dependence on the order of declaration leads me to believe that the issue is in ModuleClassBuilder - BuildParameter's logic for the default type reader of a parameter, called while the module is built:

            if (builder.TypeReader == null)
            {
                var readers = service.GetTypeReaders(paramType);
                TypeReader reader = null;

                if (readers != null)
                    reader = readers.FirstOrDefault().Value;
                else
                    reader = service.GetDefaultTypeReader(paramType);

                builder.TypeReader = reader;
            }

GetTypeReaders returns an IDictionary<Type, TypeReader>, which appears to be mapping from Types of TypeReaders to their cached instances. This dictionary is populated in the GetTypeReader method, called earlier in our example by the BuildParameters invocation for our "stringintecho" command. "stringecho"'s parameter is of type string with no explicit type reader. Since "stringintecho" has already been built, however, readers is non-null and non-empty, containing a NumericStringTypeReader. However, the default type reader is desired. Since GetDefaultTypeReader already uses an internal dictionary for caching instances of the default (correct) type reader type, unless I'm missing something, this whole default case can be simplified to:

            if (builder.TypeReader == null)
            {
                builder.TypeReader = service.GetDefaultTypeReader(paramType);
            }

because if a type reader has not been specified, we don't want a random initialized type reader from the cache; we want the default for that type.

@Joe4evr
Copy link
Contributor

Joe4evr commented Jan 20, 2018

Sounds like a good solution. Want to make a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants