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.

Sign up for GitHub

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

Jump to bottom

Cleaner options for ResolveWith and static classes/methods #5445

Open
glen-84 opened this issue Sep 29, 2022 · 10 comments · May be fixed by #7360
Open

Cleaner options for ResolveWith and static classes/methods #5445

glen-84 opened this issue Sep 29, 2022 · 10 comments · May be fixed by #7360
Assignees
@glen-84
Labels
🌶️ hot chocolate

Comments

@glen-84
Copy link
Collaborator

glen-84 commented Sep 29, 2022 •
edited
Loading

Is your feature request related to a problem?

I started with something like this (a nested resolver class):

public sealed class ArticleRelatedContentType : ObjectType<ArticleRelatedContent>
{
    protected override void Configure(IObjectTypeDescriptor<ArticleRelatedContent> descriptor)
    {
        descriptor.BindFieldsExplicitly();

        descriptor
            .Field("players")
            .ResolveWith<Resolvers>(_ => Resolvers.Players(default!, default!));
    }

    private class Resolvers
    {
        [UsePaging]
        [UseProjection]
        [UseFiltering]
        [UseSorting]
        public static IQueryable<Player> Players(
            [Service] PlayerService playerService,
            [Parent] ArticleRelatedContent relatedContent)
        {
            return playerService.GetPlayersByArticleId(relatedContent.Article.Id);
        }
    }
}

But the default! stuff bothers me. It seems like there should be some way to reference a method without a lambda. Maybe a Delegate?

I was also unable to make the class static, since the generic type argument cannot be static.

I'm now thinking of switching to something like this instead (just a method on the type class):

public sealed class ArticleRelatedContentType : ObjectType<ArticleRelatedContent>
{
    protected override void Configure(IObjectTypeDescriptor<ArticleRelatedContent> descriptor)
    {
        descriptor.BindFieldsExplicitly();

        descriptor
            .Field("players")
            .ResolveWith<ArticleRelatedContentType>(_ => Players(default!, default!));
    }

    [UsePaging]
    [UseProjection]
    [UseFiltering]
    [UseSorting]
    private static IQueryable<Player> Players(
        [Service] PlayerService playerService,
        [Parent] ArticleRelatedContent relatedContent)
    {
        return playerService.GetPlayersByArticleId(relatedContent.Article.Id);
    }
}

Again we have the lambda, and it also feels unnecessary to specify the current type in the generic.

The solution you'd like

Something simpler/cleaner, like:

   descriptor
        .Field("players")
        .ResolveWith(Players);

If ResolveWith took a Delegate, could that work?

(I know that Resolve can also be used, but it can get messy to put all of the code into the Configure method.)

Product

Hot Chocolate
@michaelstaib
Copy link
Member

Why do you not use the Resolve lambda?

@michaelstaib
Copy link
Member

OR do you mean minimal API like?

@michaelstaib
Copy link
Member

For static you could just use resolve btw

@michaelstaib
Copy link
Member

michaelstaib commented Sep 29, 2022 •
edited
Loading

How would think about this?

public sealed class ArticleRelatedContentType : ObjectType<ArticleRelatedContent>
{
    protected override void Configure(IObjectTypeDescriptor<ArticleRelatedContent> descriptor)
    {
        descriptor.BindFieldsExplicitly();

        descriptor
            .Field("players")
            .Resolve(
                [UsePaging]
                [UseProjection]
                [UseFiltering]
                [UseSorting]
                ([Service] PlayerService playerService,
                [Parent] ArticleRelatedContent relatedContent) 
                => playerService.GetPlayersByArticleId(relatedContent.Article.Id));
    }
}

you could in this case even do a static delegate.

@michaelstaib
Copy link
Member

CC @PascalSenn

@glen-84
Copy link
Collaborator Author

glen-84 commented Sep 29, 2022 •
edited
Loading

Why do you not use the Resolve lambda?

  1. As mentioned, if there are multiple/many resolvers that are not simple one-liners, it'll get messy having all the code in one class method.
  2. I don't think it supports DI ([Service]), the [Parent] attribute, etc., so that all has to be accessed via the context.

Trying to switch it now to something like:

    descriptor
        .Field("players")
        .Resolve((IResolverContext context) =>
        {
            var playerService = context.Service<PlayerService>();
            var relatedContent = context.Parent<ArticleRelatedContent>();

            return playerService.GetPlayersByArticleId(relatedContent.Article.Id);
        })
        .UsePaging()
        .UseProjection()
        .UseFiltering()
        .UseSorting();

And getting this for some reason:

Unable to infer the element type from the current resolver.

Maybe I'm doing something stupid.

How would think about this?

I feel like it looks much easier to read as a separate method. Resolve is okay for simple cases. With all these attributes it looks a bit crazy. 😄

I assume by your answer that it's not technically possible to take a Delegate. 😞

@michaelstaib
Copy link
Member

Can you specify some code ... like how you would want to write it?

@glen-84
Copy link
Collaborator Author

glen-84 commented Sep 29, 2022

I did, under The solution you'd like ...

   descriptor
        .Field("players")
        .ResolveWith(Players);

Where ResolveWith takes a Delegate ... can the engine invoke a delegate and figure out necessary parameters etc. with theDelegate.Method.GetParameters(), etc.?

I haven't written a ton of C#, so forgive me if this is a silly question.

@michaelstaib
Copy link
Member

its probably what I suggested ... the above, which also the minimal API uses compiles delegates...

I will have a look at it ... It only works since .NET 6. But I do not know... if it can infer if from the method.

@stale stale bot added ⌛ stale Nothing happened with this issue in quite a while and removed ⌛ stale Nothing happened with this issue in quite a while labels Jan 28, 2023
@ChilliCream ChilliCream deleted a comment from stale bot Jan 4, 2024
@dcby
Copy link

dcby commented Apr 24, 2024 •
edited
Loading

@glen-84, problem solves very easy with simple extension methods.

internal static class DescriptorExtensions
{
  public static IObjectFieldDescriptor Field(this IObjectTypeDescriptor descriptor, Delegate @delegate)
    => descriptor.Field(@delegate.Method);

  public static IObjectFieldDescriptor Field<TRuntimeType>(this IObjectTypeDescriptor<TRuntimeType> descriptor, Delegate @delegate)
    => descriptor.Field(@delegate.Method);

  public static IObjectFieldDescriptor ResolveWith(this IObjectFieldDescriptor descriptor, Delegate @delegate)
    => descriptor.ResolveWith(@delegate.Method);
}

Then

descriptor
  .Field(Resolvers.Players);

// -- or --

descriptor
  .Field("myname")
  .ResolveWith(Resolvers.Players);

@michaelstaib, does it matter in terms of performance if we provide MethodInfo instead of Expression<Func<TIn, TOut>>? How does it compile internally?

@glen-84 glen-84 self-assigned this Aug 12, 2024
@glen-84 glen-84 linked a pull request Aug 12, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@glen-84 glen-84

Labels
🌶️ hot chocolate
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Added ResolveWith overload that takes a Delegate ChilliCream/graphql-platform
4 participants
@glen-84 @michaelstaib @dcby @tobias-tengler