graphql-dotnet / graphql-dotnet

GraphQL for .NET
https://graphql-dotnet.github.io
MIT License
5.86k stars 921 forks source link

Support argument of Regex type by PatternMatchingVisitor #4037

Open gao-artur opened 2 months ago

gao-artur commented 2 months ago

Currently, the PatternMatchingVisitor only supports string argument

https://github.com/graphql-dotnet/graphql-dotnet/blob/22052e73d734d3f0b60b52a04897afb12656c55a/src/GraphQL/Utilities/Visitors/Custom/PatternMatchingVisitor.cs#L40-L41

It can be useful also to add support for Regex arguments

Field<string>("Hello")
    .Argument<StringGraphType>("arg", argument => argument.ApplyDirective("pattern", "regex", Patterns.AlphabeticalPattern()))
    .Resolve(ctx => ctx.GetArgument<string>("arg"));

public static partial class Patterns
{
    [GeneratedRegex("[A-Z]+")]
    public static partial Regex AlphabeticalPattern();
}
Shane32 commented 2 months ago

Sounds like a great idea!

Shane32 commented 2 months ago

You'd probably have to make a custom scalar to allow this however.

Shane32 commented 2 months ago

Which also means it's a breaking change unfortunately.

Shane32 commented 2 months ago

Maybe a ctor option to the directive to use the custom scalar so it's not breaking by default?

gao-artur commented 2 months ago

I don't think I understand. Why does it require a custom scalar? The ApplyDirective receives an object as an argument, so it can be anything. The only change is required in PatternMatchingVisitor to try cast the applied?.FindArgument("regex")?.Value to Regex in addition to string. Am I missing something?

Shane32 commented 2 months ago

Schema validation should fail to coerce the argument to StringGraphType (see https://github.com/graphql-dotnet/graphql-dotnet/blob/master/src/GraphQL/Types/Directives/Custom/PatternMatchingDirective.cs ). Unless we don't have schema validation for applied directives' arguments. In which case the schema printing code would crash when printing the schema within the ScalarGraphType.ToAst method since it won't know how to convert a RegEx class to an AST.

Shane32 commented 2 months ago

With a custom scalar, ParseLiteral can support strings, and ParseValue can support both strings and RegEx instances. The ToAST method can be overridden to provide a string representation of the RegEx instance so schema printing still works.

Shane32 commented 2 months ago

At least, without trying any code, that's my best guess as to what will happen.

gao-artur commented 2 months ago

You are right. The visitor works correctly, but schema printing fails. But I don't understand your proposal with a custom scalar...