SteveDunn / Vogen

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

New [EfCoreConverter] doesn't seems to work #625

Closed CheloXL closed 3 months ago

CheloXL commented 3 months ago

Describe the bug

Hi Steve, I'm trying to use the new option to generate EfConverters on a different library, followed the steps, and got... nothing.

Steps to reproduce

I have one project with all my VOs. I removed from "conversions" the Conversions.EfCoreValueConverter. Created a new project, added a public partial class and decorated it with [EfCoreConverter<....>] (and of course, added Vogen to the packages)... And nothing happens. Rider shows the "partial" keyword as not used, and nothing gets generated.

Expected behaviour

Something has to be generated. Am I using this feature in the wrong way?

SteveDunn commented 3 months ago

Hi @CheloXL - thanks for the bug report. Are you targetting .NET 8?

SteveDunn commented 3 months ago

I think I've found the problem. The EfCoreConverter<> attributes are not being picked up unless there's one ValueObject<> attribute in the project!

SteveDunn commented 3 months ago

I'm doing a fix now, but a workaround is put a dummy value object in the project, e.g. [ValueObject] public partial class _;

CheloXL commented 3 months ago

Perfect. Thanks.. yes, I found that placing the partial class in the same project did work. One thing while you are at this.. can you remove the "public" keyword from the partial class? I want to have that class internal and only access it through an extension method that does all the registrations.

CheloXL commented 3 months ago

On another note... and this is outside the scope of this issue but would be a nice-to-have feature "included" within this one: If you can add a static method like the following one that auto-adds/removes registrations based on the attributes used to decorate the class, that would be awesome, as I can register all VOs at once. (VogenEfCoreConverters is the class that both holds the method and have the EfCoreConverter attributes).

public static ModelConfigurationBuilder AddVogenConversions(this ModelConfigurationBuilder configurationBuilder)
{
    configurationBuilder.Properties<InstallationId>().HaveConversion<VogenEfCoreConverters.InstallationIdEfCoreValueConverter, VogenEfCoreConverters.InstallationIdEfCoreValueComparer>();
    configurationBuilder.Properties<CountryId>().HaveConversion<VogenEfCoreConverters.CountryIdEfCoreValueConverter, VogenEfCoreConverters.CountryIdEfCoreValueComparer>();
    .... more configurations based on the attributes...
    return configurationBuilder;
}
CheloXL commented 3 months ago

Another issue I just found... if I don't name the class "EfCoreConverters", the project doesn't compile. Seems that you have the class name hardcoded in some places (because the partial class is correctly created, but then you reference a class named as noted instead of using the name of the class the user defined).

SteveDunn commented 3 months ago

can you remove the "public" keyword from the partial class?

I've changed this so that it takes on the accessibility of the declaring class, e.g. internal partial class Converters generates all other partial implementations as internal. Same for public.

Each inner class that represents the converter is public. I suppose we could limit the accessibility to that so that they're not accessible from outside of the Converters class.

SteveDunn commented 3 months ago

Re. ModelConfigurationBuilder. I'm taking a look at that now. I'm not familiar with it, so I browsed the documentation. I can see this:

    /// <summary>
    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
    ///     any release. You should only use it directly in your code with extreme caution and knowing that
    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
    /// </summary>

I understand that it might be useful for you, but I'm curious to see if it's commonly used by the efcore community. I'm wary of having to change the generator to keep track with changes in efcore...

SteveDunn commented 3 months ago

I've got an initial implementation of this working. It adds an extension method like this:

  internal static class EfCoreConverters__Ext
  {
      internal static global::Microsoft.EntityFrameworkCore.ModelConfigurationBuilder RegisterAllInEfCoreConverters(this global::Microsoft.EntityFrameworkCore.ModelConfigurationBuilder configurationBuilder)
      {
        configurationBuilder.Properties<Domain.Id>().HaveConversion<UsingTypesGeneratedInTheSameProject.EfCoreConverters.IdEfCoreValueConverter, UsingTypesGeneratedInTheSameProject.EfCoreConverters.IdEfCoreValueComparer>();
configurationBuilder.Properties<Domain.Name>().HaveConversion<UsingTypesGeneratedInTheSameProject.EfCoreConverters.NameEfCoreValueConverter, UsingTypesGeneratedInTheSameProject.EfCoreConverters.NameEfCoreValueComparer>();
configurationBuilder.Properties<Domain.Age>().HaveConversion<UsingTypesGeneratedInTheSameProject.EfCoreConverters.AgeEfCoreValueConverter, UsingTypesGeneratedInTheSameProject.EfCoreConverters.AgeEfCoreValueComparer>();
configurationBuilder.Properties<Domain.Department>().HaveConversion<UsingTypesGeneratedInTheSameProject.EfCoreConverters.DepartmentEfCoreValueConverter, UsingTypesGeneratedInTheSameProject.EfCoreConverters.DepartmentEfCoreValueComparer>();
configurationBuilder.Properties<Domain.HireDate>().HaveConversion<UsingTypesGeneratedInTheSameProject.EfCoreConverters.HireDateEfCoreValueConverter, UsingTypesGeneratedInTheSameProject.EfCoreConverters.HireDateEfCoreValueComparer>();

        return configurationBuilder; 
      }
  }

The naming convention is RegisterAllIn[NameOfConverterTarget], e.g. RegisterAllInEfCoreConverters for the declaration

[EfCoreConverter<Id>]
[EfCoreConverter<Name>]
[EfCoreConverter<Age>]
[EfCoreConverter<Department>]
[EfCoreConverter<HireDate>]
internal sealed partial class EfCoreConverters;

I should be able to release this tomorrow.

CheloXL commented 3 months ago

I understand that it might be useful for you, but I'm curious to see if it's commonly used by the efcore community. I'm wary of having to change the generator to keep track with changes in efcore...

ModelConfigurationBuilder is what you get on the protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder) in a DbContext. Not sure from where you got that documentation, but I have:

// <summary>
///     Provides a simple API surface for setting defaults and configuring conventions before they run.
/// </summary>

ConfigureConventions is where I usually do global configuration in EfCore.

SteveDunn commented 3 months ago

This is where I found the documentation (via SourceLink). I can see it's a slightly older version though, so it might've changed:

image
CheloXL commented 3 months ago

Ahh.. but those comments refer to those specifics methods/properties (ctor, ModelConfiguration)... Basically, all of the ones decorated with [EntityFrameworkInternal]. The class itself and the public methods are used to let you configure the DbContext. See ConfigureConventions

SteveDunn commented 3 months ago

Ah, OK, thanks. Anyway, closing this one as it is released in 4.0.10 (publishing now). Thanks again for your help!

Hona commented 2 months ago

@SteveDunn I think this should be reopened, when I create a marker class like so:

[EfCoreConverter<Domain.GameId>]
internal partial class VogenEfCoreConverters;

it fails, as it can't find 'EfCoreConverters'

However, when I make the marker class that name it all works...

[EfCoreConverter<Domain.GameId>]
internal partial class EfCoreConverters;

Let me know if you need a minimal repro repo :)

Tested with <PackageReference Include="Vogen" Version="4.0.12" />

SteveDunn commented 2 months ago

Hi @Hona - many thanks for the bug report. Apologies for this. I found a few places where this name was hardcoded. I've fixed that and I'm prepping a release now. Should be an hour or so. Thanks again!

Hona commented 2 months ago

Appreciate that @SteveDunn

It's low priority, but love your work

SteveDunn commented 2 months ago

This is in 4.0.15 - will be on NuGet shortly