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

Custom Interface Support for HotChocolate.Types.Analyzers Codegen with [DataLoader] Attribute #8023

Closed
kondelik opened this issue Feb 14, 2025 · 9 comments

Comments

@kondelik
Copy link
Contributor

kondelik commented Feb 14, 2025

Product

Hot Chocolate

Is your feature request related to a problem?

When using HotChocolate.Types.Analyzers codegen with [DataLoader] attribute, it would be beneficial to have a way to generate and register DataLoaders with custom interfaces.

Reason:

Currently, there is no way to use the code-generated IDataLoader<,> implementation in a Clean (Onion) Architecture.

edit: Well, this statement of mine is wrong. You have to write partial class to generated dataloader by hand, just something quick as internal sealed partial class BrandByIdDataLoader : IMyAwesomeDataLoader; and add it to DI with services.AddScoped<IMyAwesomeDataLoader>(s => s.GetRequiredService<BrandByIdDataLoader>());

Clean Architecture in a Nutshell:

  • MyAwesomeApp(.Presentation) -> MyAwesomeApp.Infrastructure (GreenDonut.Data.EntityFramework + HotChocolate.Types.Analyzers) -> MyAwesomeApp.Application (GreenDonut.Abstractions + MediatR) -> MyAwesomeApp.Domain

This is based on the schema from the blog post: Hot Chocolate 15

The problem is that the auto-generated interfaces are in MyAwesomeApp.Infrastructure, but the MediatR RequestHandler implementations are in MyAwesomeApp.Application, making them inaccessible.

Current workaround:

  • Add a partial DataLoader class to MyAwesomeApp.Infrastructure (partial to the source-generated one) marking it as implementing the custom interface.
  • Register it manually as follows:
    services.TryAddTransient( // Transient because IDataLoaderScope manages the scope of DataLoader
      (IServiceProvider sp) => sp
        // Found this in your source code, didnt test it.
        .GetRequiredService<IDataLoaderScope>()
        .GetDataLoader<ProductsByBrandDataLoader>()
        // or maybe `.GetDataLoader<IProductsByBrandDataLoader>()` and cast.
    );

The solution you'd like

My code

IMyAwesomeDataLoader interface in MyAwesomeApp.Application library:

public interface IMyAwesomeDataLoader : GreenDonut.IDataLoader<int, BrandDto>;

Implementation in MyAwesomeApp.Infrastructure library:

public static class MyAwesomeDataLoaders {

  [DataLoader
    Implements=typeof(IMyAwesomeDataLoader) // <------ Interface from a different library. Must inherit IDataLoader<,> (?)
  ] 
  public static async Task<Dictionary<int, ProductDto>> GetProductByIdAsync(
    IReadOnlyList<int> ids,
    PagingArguments pagingArgs,
    QueryContext<CallDto> queryContext,
    IDbContextFactory<MyAwesomeDbContext> context,
    CancellationToken cancellationToken)
  {
    return new Dictionary<int, ProductDto>();
  }
}

Generated code

Generated GreenDonutDataLoaderModule.735550c.g.cs

// <auto-generated/>
public sealed partial class ProductByIdDataLoader
    : global::GreenDonut.DataLoaderBase<int, global::MyAwesomeApp.Domain.DTO.ProductDto>
    // , IProductByIdDataLoader // <-------------------------------------------- I think I don't need the generated interface
    , global::MyAwesomeApp.Application.Common.DataLoaders.IMyAwesomeDataLoader // <---- because I have my custom interface
{
 (...)
}

Generated GreenDonutDataLoaderModule.735550c.g.cs (Registration with DI)

// <auto-generated/>
(...)
global::Microsoft.Extensions.DependencyInjection.DataLoaderServiceCollectionExtensions.AddDataLoader<
  global::MyAwesomeApp.Application.Common.DataLoaders.IMyAwesomeDataLoader, // <------- Register as my interface
  global::MyAwesomeApp.Infrastructure.DataLoaders.ProductsByBrandDataLoader // <-- no change
>(services);

Possible problems:

  • I think that usage of typeof in attribute is possible from C# version 7.3 (May 2018) but I am not sure. My googlefu is failing me, but AFAIK its possible from the same version as usage of nameof() in attribute.
  • This is not a solution for full DataLoader class (when not using [DataLoader] attribute, good old class FooDataLoader : BatchDataLoader<,>, IMyAwesomeDataLoader {}). That should be new feature request, because its more complicated - there can be more then one interface to register.
@glen-84
Copy link
Collaborator

glen-84 commented Feb 14, 2025

I currently handle this as follows:

  1. Turn off the generation of interfaces:

    [assembly: DataLoaderDefaults(GenerateInterfaces = false)]
  2. Create an interface in the application layer:

    public interface IRolesByUserIdDataLoader : IDataLoader<long, Role[]>;
  3. Implement the interface in the infrastructure layer using a partial class:

    public sealed partial class RolesByUserIdDataLoader : IRolesByUserIdDataLoader;

@kondelik
Copy link
Contributor Author

kondelik commented Feb 14, 2025

I currently handle this as follows:

  1. Turn off the generation of interfaces:
    [assembly: DataLoaderDefaults(GenerateInterfaces = false)]
  2. Create an interface in the application layer:
    public interface IRolesByUserIdDataLoader : IDataLoader<long, Role[]>;
  3. Implement the interface in the infrastructure layer using a partial class:
    public sealed partial class RolesByUserIdDataLoader : IRolesByUserIdDataLoader;

This is pretty much the same as the "Current workaround" I mentioned. You still have to register your DataLoaders in DI yourself, because the code-generated Add{DataLoaderModuleName}(this IServiceCollection services) will not register them as your IRolesByUserIdDataLoader interface, just as the concrete implementation (RolesByUserIdDataLoader) only.

@glen-84
Copy link
Collaborator

glen-84 commented Feb 14, 2025

Ah, right, I'm registering them using Scrutor.

Your suggestion seems reasonable, but we'll see what Michael says. 🙂

@kondelik
Copy link
Contributor Author

kondelik commented Feb 14, 2025

I'm registering them using Scrutor.

That is another thing I am concerned about because this may be a mistake.

Getting a DataLoader is more complex than just getting a scoped service; it manages its own scope. It's better to leave it to code generation.

@glen-84
Copy link
Collaborator

glen-84 commented Feb 14, 2025

Ya, this was an old side project, and a lot has changed since then.

As a temporary solution, does it work to call services.AddDataLoader<IMyAwesomeDataLoader, ProductsByBrandDataLoader>() yourself?

@michaelstaib
Copy link
Member

michaelstaib commented Feb 17, 2025

This is incorrect, in a clean architecture project you define the interfaces without any generator in your Application layer. You could event create your very own without any dependency on DataLoader in the application layer.

In the infrastructure sits the generator and it will produce a partial class. In there implement your interface to the partial class and wire up DI. You do not need to completely wire up DI just add your interface as an additional.

internal sealed partial class BrandByIdDataLoader : IBrandByIdDataLoader;

services.AddScoped<IBrandByIdDataLoader>(s => s.GetRequiredService<BrandByIdDataLoader>());

@kondelik
Copy link
Contributor Author

kondelik commented Feb 17, 2025

I am sorry, but why is this incorrect?

I have my IMyAwesomeDataLoader in Aplication layer (as you)

I have my generator in Infrastructure (Persistence) layer generating ProductByIdDataLoader (as you)

And I am asking if HotChocolate.Types.Analyzers can auto-implement (just by adding : IMyAwesomeInterface to generated code) them for me so I dont have to write partial for every single dataloader I have. Which is possible, just annoying. This is feature request, not bug report 😄

services.AddScoped(s => s.GetRequiredService());

Oh, ok, that ExecutionDataLoaderScope.cs looks scary, but if its ok to simply GetRequiredService<>() then this get much easier 👍

@michaelstaib
Copy link
Member

Currently, there is no way to use the code-generated IDataLoader<,> implementation in a Clean (Onion) Architecture.

this remark is incorrect ... i should have quoted ....

We have built the code generation with clean architecture in mind. in your infrastructure layer you need to configure the code gen to not generate interfaces for the DataLoader and then use the partials.

In general when we designed it we considered the interface on the attribute but found it inflexible and error prone. With a partial class you get lots more flexibility.

@kondelik
Copy link
Contributor Author

kondelik commented Feb 18, 2025

Fair enough 👍 Thank for your time.

This part helped me a lot:

services.AddScoped<IBrandByIdDataLoader>(s => s.GetRequiredService<BrandByIdDataLoader>());

Because I thought that you have to get your DataLoader via GetDataLoader<> extension method.

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

No branches or pull requests

3 participants