Adding a parameter is a breaking change, not a bugfix

DPDmancul commented 1 month ago

Describe the bug

From version 4.0.8 to 4.0.9 an (optional) parameter was added to the ValueObjectAtribute:

git diff 4.0.8...4.0.9 src/Vogen.SharedTypes/ValueObjectAttribute.cs

diff --git a/src/Vogen.SharedTypes/ValueObjectAttribute.cs b/src/Vogen.SharedTypes/ValueObjectAttribute.cs
index e8b59a1f..9cbc8172 100644
--- a/src/Vogen.SharedTypes/ValueObjectAttribute.cs
+++ b/src/Vogen.SharedTypes/ValueObjectAttribute.cs
@@ -29,7 +29,8 @@ public class ValueObjectAttribute<T> : ValueObjectAttribute
             ParsableForStrings parsableForStrings = ParsableForStrings.Unspecified,
             ParsableForPrimitives parsableForPrimitives = ParsableForPrimitives.Unspecified,
             TryFromGeneration tryFromGeneration = TryFromGeneration.Unspecified,
-            IsInitializedMethodGeneration isInitializedMethodGeneration = IsInitializedMethodGeneration.Unspecified) : base(
+            IsInitializedMethodGeneration isInitializedMethodGeneration = IsInitializedMethodGeneration.Unspecified,
+            PrimitiveEqualityGeneration primitiveEqualityGeneration = PrimitiveEqualityGeneration.Unspecified) : base(
             typeof(T),
             conversions,
             throws,
@@ -43,7 +44,8 @@ public class ValueObjectAttribute<T> : ValueObjectAttribute
             parsableForStrings,
             parsableForPrimitives,
             tryFromGeneration,
-            isInitializedMethodGeneration)
+            isInitializedMethodGeneration,
+            primitiveEqualityGeneration)
         {
         }
     }
@@ -73,7 +75,8 @@ public class ValueObjectAttribute : Attribute
             ParsableForStrings parsableForStrings = ParsableForStrings.Unspecified,
             ParsableForPrimitives parsableForPrimitives = ParsableForPrimitives.Unspecified,
             TryFromGeneration tryFromGeneration = TryFromGeneration.Unspecified,
-            IsInitializedMethodGeneration isInitializedMethodGeneration = IsInitializedMethodGeneration.Unspecified)
+            IsInitializedMethodGeneration isInitializedMethodGeneration = IsInitializedMethodGeneration.Unspecified,
+            PrimitiveEqualityGeneration primitiveEqualityGeneration = PrimitiveEqualityGeneration.Unspecified)
         {
         }
     }

Unfortunately this is not a bugfix, since if we go back from 4.0.9 to 4.0.8 the code breaks (for the missing parameter).

We could be tempted to consider it a minor change since the parameter is optional, but it breaks binary compatibility, hence it is indeed a major.

For more information see:

There is a way to avoid the breaking change and make a minor: instead of adding an optional parameter you have to add an overload with the new optional parameter (see https://stackoverflow.com/a/44104446/3070398):

// new attribute constructor
public ValueObjectAttribute(
    Type? underlyingType = null!,
    // ...
    IsInitializedMethodGeneration isInitializedMethodGeneration = IsInitializedMethodGeneration.Unspecified,
    PrimitiveEqualityGeneration primitiveEqualityGeneration = PrimitiveEqualityGeneration.Unspecified)
{ }

// old attribute constructor 
public ValueObjectAttribute(
    Type? underlyingtype = null!,
    // ...
    IsInitializedMethodGeneration isInitializedMethodGeneration = IsInitializedMethodGeneration.Unspecified) : this(
    underlyingtype,
    // ...
    isInitializedMethodGeneration)
{ }

Steps to reproduce

Make a package referencing Vogen 4.0.8, in which we define a value object
In a project reference the package above and Vogen 4.0.9
Call GetCustomAttribute on the defined value object type

For a more general scenario see: https://github.com/DPDmancul/cs-get-attribute-ctor-bug-mwe

Expected behaviour

No error is expected, but an exception of missing ValueObjectAttribute constructor is thrown

SteveDunn commented 4 weeks ago

Hi @DPDmancul - thanks for the bug report - sorry you've had trouble. Can I ask why you're reflecting over the value objects? It's nice to know what users do that for so that we can put additional code generation in order to avoid reflection. Also, can I ask why you went back? There was an issue with 4.0.9 which was fixed in 4.0.10 Thanks again, Steve

DPDmancul commented 4 weeks ago

I was no manually getting the attribute, but serializing to json Newtonsoft gets all the attributes of a type (hence also ValueObjectAttribute), maybe looking for some attributes configuring how to serialize. I didn't went back: 4.0.9 was released only yesterday (I got the bug two days ago, but needed some time to inspect, make the MWE and report), moreover I cannot update my dependencies every day, especially in published packages.

P.S. there is a case where i need reflection, but this was not broken since I only check if the type has the attribute (and so the constructor is not called): to automatically add the EF converter. In facts the source code generated converter needs EF, but I don't want to make my domain depend on EF (which is infrastructure). So I don't opt in for the generation to the converter but rather on model creation scan all entity types tagged with the ValueObjectAttribute and add a dynamically generated converter on the fly.

SteveDunn commented 4 weeks ago

I see, thank you @DPDmancul . You might be interested in the new stuff in the last couple of releases. Vogen now generates EFCore converters for types in other assemblies. I've documented it here (any feedback or contributions very welcome on the documentation):

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

I'll bear your use-case in mind re. breaking changes by adding default parameters. Apologies again. I can retrofit the constructor if you gets you out of a hole, or I can close this issue now and promise never to do it again! :) What would you prefer?

DPDmancul commented 4 weeks ago

Vogen now generates EFCore converters for types in other assemblies.

Good news! I still prefer my solution for the only reason with it I cannot forget to add a converter (in facts I never manually add a converter, but loop through entity types and automatically add the required converters) ;)

I can retrofit the constructor if you gets you out of a hole, or I can close this issue now and promise never to do it again! :) What would you prefer?

I think the best is to leave this as it is (I will update all at the latest version for this time), and for future releases to take in mind that adding a parameter is a breaking change (I my self didn't thought that was true also for optional parameters as you can see in the issue to dotnet), so to make it only a minor it is required to add a constructor overload. Thanks

SteveDunn / Vogen