SteveDunn / Vogen

A semi-opinionated library which is a source generator and a code analyser. It Source generates Value Objects
Apache License 2.0
888 stars 46 forks source link

Enables the generation of converters in other projects #541

Closed MithrilMan closed 5 months ago

MithrilMan commented 1 year ago

Describe the feature

Following clean architecture you usually split domain from infrastructure. Persistence implementation lives in the infrastructure project so any EntityFramework/Dapper converter must be generated there. This mean that it's not viable to use type converter in the value object definition (Domain project) because I don't want to add persistence dependency on that project.

Would be useful to have a source generator that generates converters for generated value objects. I could imagine an API like this (less verbose maybe...)

[ValueObjectEFConverter<CustomerId>]
[ValueObjectEFConverter<OrderId>]
public partial class EFConverters {
}

That would generate a converter for CustomerId and OrderId

this could be implemented more generically with a ValueObjectConverter where you pass the converters you want to be generated (e.g. using the enum you already have).

SteveDunn commented 11 months ago

A great suggestion. I too use onion/clean architecture. I enforce constraints using the fantastic NsDepCop (https://github.com/realvizu/NsDepCop) which inspects namespace references for violations. Your suggested way should also be possible to implement. I'll take a look when time permits, hopefully not too far away

MithrilMan commented 11 months ago

Nice to know about your interest in implementing the feature. An example of a source generator that achieve a similar API is the Json serializer built into net6/7/8 itself https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/source-generation?pivots=dotnet-8-0

The implementation is open-sourced here: https://github.com/dotnet/runtime/tree/c08faf9216976a14f06a11373fbd3aec7671bf7a/src/libraries/System.Text.Json/gen

I haven't spent time on source generators deeply (and I'm out of time to do that anytime soon), so I can't implement a PR myself, but I'll be willing to test it out once it's ready to be tested.

xamir82 commented 5 months ago

@SteveDunn Hey Steve, you must be busy, so sorry to tag you! But any updates on this one? I just started using Vogen and this is the first problem I encountered, pretty annoying...!

SteveDunn commented 5 months ago

Hi @xamir82 - I'm taking a look now, although it'll be a fairly big change, so might not arrive for a while

SteveDunn commented 5 months ago

There are a few issues with implementing this:

  1. It is difficult for source generators to share state between projects
  2. Inner classes can't be implemented in other projects. Changing the converters to be 'outer' classes has it's complications as they depend on private fields and constructors. For instance, they all use the __Deserialize([primitive] value) method, which is private. This method just calls the private construct, so could be inlined, but then the private constructor (CustomerId([primitive] value) would have to made public. Granted, the parameterless constructor is public, but there are analyzers to spot usages of this, plus, having it public is critical for other scenarios
  3. There is no way around decorating the value objects with attributes. For instance, for System.Text.Json (STJ), we need an attribute place on the type itself, so even if the converters were specified in another project, we'd still need a reference to STJ for the attribute. The same is true for Newtonsoft.Json. This may not be a problem as STJ is part of newer frameworks.

With this in mind, rather than loosening the constraints of value objects, I feel it would be better to use another mechanism to prohibit the use of infrastructure code from the domain layer. NSDepCop is ideal for this, and it's something I use. Not only does it mean that you don't have to have separate projects (and using circular dependencies as a safety net), it means you have finer grained control. Here, you prohibit MyApp.Domain.* to anything Microsoft.EntityFrameworkCore*, but allow MyApp.Domain.Types access. In addition to, or an alternative to, NSDepCop, you could use the 'banned api' analyzer.

I wanted to close this ticket, but I'll leave it open to see if there's any approaches I've missed.

MithrilMan commented 5 months ago

I'm the OP, after I posted the issue I created my own value objcet classes with their own serializers. Unluckily it's a closed source codebase I can't publish but I can give some inputs:

Let's start by stating that my solution is a bit different in design, I create my ValueObject starting from an aggregate "owner" (most of the time in my scenario these value objects are typed Identities), so I do something like this

[OwnsEntityId<long>("DeviceEventId")]
public class DeviceEvent : Entity<DeviceEventId>, IAggregateRoot

and following this snippet my source generator creates a scalar value object with a long value, named DeviceEventId In my example the aggregate implements Entity where T is a value object used as the aggregate.Id property

Beside this, since the issue was about the converters, my solution is having implemented another source generator that uses attributes placed on a static partial class like this:

[EntityIdConverter<ApplicationAggregate.ApplicationId>]
[EntityIdConverter<ApplicationAggregate.PermissionId>]
[EntityIdConverter<CustomerAggregate.CustomerId>]
[EntityIdConverter<RoleAggregate.RoleId>]
[EntityIdConverter<RoleAggregate.RolePermissionId>]
[EntityIdConverter<UserAggregate.UserDeniedPermissionId>]
[EntityIdConverter<UserAggregate.UserExplicitPermissionId>]
[EntityIdConverter<UserAggregate.UserId>]
[EntityIdConverter<UserAggregate.UserRoleId>]
[EntityIdConverterConfiguration(HasAddConvertersMethod = true)]
static partial class EntityIdConverters
{
}

and that produces a method for each entity I want to convert (in my case I was only interested in EF converters. image

Where the generated code driven by EntityIdConverterAttribute is for example something like this

using System;
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;

namespace WAY.Services.Auth.Infrastructure.Persistence.Converters;

#nullable enable
public static partial class EntityIdConverters {
   public class PermissionIdConverter : ValueConverter<WAY.Services.Auth.Domain.Aggregates.ApplicationAggregate.PermissionId?, long>
   {
      public PermissionIdConverter() : base(
         id => id == null ? default : id.Value,
         value => Convert(value))
      { }

      public static WAY.Services.Auth.Domain.Aggregates.ApplicationAggregate.PermissionId? Convert(long value)
         => WAY.Services.Auth.Domain.Aggregates.ApplicationAggregate.PermissionId.TryCreate(value).MatchFirst(
               id => id,
               error => throw new InvalidOperationException($"{error.Code}: {error.Description}")
         );
   }
}
#nullable disable

I also implemented a convention for EntityIdConverterConfigurationAttribute that I can use to generate an additional method that extends entity framework ModelConfigurationBuilder to inject my converters

using System;
using Microsoft.EntityFrameworkCore;
namespace WAY.Services.Auth.Infrastructure.Persistence.Converters;

public static partial class EntityIdConverters {
   public static void AddConverters(ModelConfigurationBuilder builder){
      builder.Properties<WAY.Services.Auth.Domain.Aggregates.ApplicationAggregate.ApplicationId>().HaveConversion<ApplicationIdConverter>();
      builder.Properties<WAY.Services.Auth.Domain.Aggregates.ApplicationAggregate.PermissionId>().HaveConversion<PermissionIdConverter>();
      builder.Properties<WAY.Services.Auth.Domain.Aggregates.CustomerAggregate.CustomerId>().HaveConversion<CustomerIdConverter>();
      builder.Properties<WAY.Services.Auth.Domain.Aggregates.RoleAggregate.RoleId>().HaveConversion<RoleIdConverter>();
      builder.Properties<WAY.Services.Auth.Domain.Aggregates.RoleAggregate.RolePermissionId>().HaveConversion<RolePermissionIdConverter>();
      builder.Properties<WAY.Services.Auth.Domain.Aggregates.UserAggregate.UserDeniedPermissionId>().HaveConversion<UserDeniedPermissionIdConverter>();
      builder.Properties<WAY.Services.Auth.Domain.Aggregates.UserAggregate.UserExplicitPermissionId>().HaveConversion<UserExplicitPermissionIdConverter>();
      builder.Properties<WAY.Services.Auth.Domain.Aggregates.UserAggregate.UserId>().HaveConversion<UserIdConverter>();
      builder.Properties<WAY.Services.Auth.Domain.Aggregates.UserAggregate.UserRoleId>().HaveConversion<UserRoleIdConverter>();
   }
}

And so in Entity Framework I've just to call that method like

 protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder)
 {
    base.ConfigureConventions(configurationBuilder);

   // ...

    EntityIdConverters.AddConverters(configurationBuilder);
 }

This solves my problem: value object are created in the Domain project and this converter class is in the Infrastructure layer.

SteveDunn commented 5 months ago

Thanks for the detailed description on how you do it @MithrilMan . One of the things that Vogen did was to throw exceptions in the ValueConverter if the value was null or invalid. But a few users said that they would like to have an uninitialized value object if it cannot be deserialized. This went against the primary goal of Vogen to not allow uninitialized instances. To facilitate this I added a private __Deserialize() method, meaning the converter can create uninitialized instances, but nothing else can. Having the converters as anything other than inner classes would mean exposing this mechanism, which is not something we'd want to do.

But thanks again, and I'm glad you can now use the goodness of value objects in the domain and infrastructure where it should be! 👍

aradalvand commented 5 months ago

This went against the primary goal of Vogen to not allow uninitialized instances. To facilitate this I added a private __Deserialize() method, meaning the converter can create uninitialized instances, but nothing else can. Having the converters as anything other than inner classes would mean exposing this mechanism, which is not something we'd want to do.

@SteveDunn Hey Steve, to quickly chime in: That's not the case. This sort of scenario was precisely why [UnsafeAccessor] was introduced (see the proposal).

So, you don't have to make any private member public in order to implement this functionality; you can take advantage of this new .NET 8 feature to source-generate a converter that accesses any private member of the value object from the outside with no overhead.

Correct me if I'm wrong, but please consider reopening the issue.

SteveDunn commented 5 months ago

This went against the primary goal of Vogen to not allow uninitialized instances. To facilitate this I added a private __Deserialize() method, meaning the converter can create uninitialized instances, but nothing else can. Having the converters as anything other than inner classes would mean exposing this mechanism, which is not something we'd want to do.

@SteveDunn Hey Steve, to quickly chime in: That's not the case. This sort of scenario was precisely why [UnsafeAccessor] was introduced (see the proposal).

So, you don't have to make the constructor public in order to implement this; you can take advantage this new .NET 8 feature to source-generate a converter that calls the private constructor of the value object from the outside with no overhead.

Correct me if I'm wrong, but please consider reopening the issue.

Thank you @aradalvand - that looks really interesting! I'll take a look at that right now and update this thread shortly.

SteveDunn commented 5 months ago

I should have a build ready over the weekend. The [UnsafeAccessor] stuff is really great for source generators!

SteveDunn commented 5 months ago

This was released in 4.0.9 - thanks for the input!