EF Core Support Extension #2

[Aragas]

This addresses the following issue:

• EF Core's ChangeTracker tries to get the hash code from a non initialized Vogen type it created itself.

My suggestion as a fix is to introduce a custom ValueComparer, just like we have already EfCoreValueConverter

    // value based
    public class EfCoreValueComparer : global::Microsoft.EntityFrameworkCore.ChangeTracking.ValueComparer<VOTYPE>
    {
        // Not sure what the best approach would be
        public EfCoreValueComparer() : base((left, right) => left == right, instance => instance._isInitialized ? instance._value.GetHashCode() : 0) { }
        public EfCoreValueComparer() : base((left, right) => left == right, instance => instance._value != null ? instance._value.GetHashCode() : 0) { }
    }
    // reference based
    public class EfCoreValueComparer : global::Microsoft.EntityFrameworkCore.ChangeTracking.ValueComparer<VOTYPE>
    {
        // Not sure what the best approach would be
        public EfCoreValueComparer() : base(CreateDefaultEqualsExpression(), instance => instance._isInitialized ? instance._value.GetHashCode() : 0) { }
        public EfCoreValueComparer() : base(CreateDefaultEqualsExpression(), instance => instance._value != null ? instance._value.GetHashCode() : 0) { }
    }

And as an extension of #512, here's the new extension method:

public static class VogenExtensions
{
    public static PropertyBuilder<VOTYPE> HasVogenConversion(this PropertyBuilder<VOTYPE> propertyBuilder) =>
        propertyBuilder.HasConversion<VOTYPE.EfCoreValueConverter, VOTYPE.EfCoreValueComparer>();
}

[CheloXL]

I stumbled with this issue today. All my entities have a correct ID, but when EF needs to create a shadow property (for example, a hidden ID in an owned type), VoGen throws as the created struct is not initialized. This doesn't happen with class-based VO (as EF compares with null). Anyways, having both options (class/reference types) is not a bad idea, just in case.

[CheloXL]

@SteveDunn Even if you end up implementing this or not, I think that in the same vein as ToString, GetHashCode should never throw (even on uninitialized values). Same for any overridden method from Object.

[SteveDunn]

Thanks for the suggestion @Aragas . I'm not that familiar with EFCore. In your example above, aside from the extension method, is that the entirety of what needs to be generated? As mentioned in #512 , we might need a new Vogen.EFCore NuGet package for these extension methods as Vogen can't generate them, and they can't reasonably be added as a dependency of the main Vogen pacakge.

@CheloXL - ToString doesn't throw (it emits [UNINITIALIZED]). but GetHashCode does, as there's no reasonably default value to return. Would @Aragas 's suggestion above solve the problem that you currently have?

Again, apologies, I not too familiar with EFCore.

[SteveDunn]

For the example above, I think this is the correct way:

public EfCoreValueComparer() : base((left, right) => left == right, instance => instance._isInitialized ? instance._value.GetHashCode() : 0) { }

This would be the same for both value and reference types.

[CheloXL]

GetHashCode should never throw. If the VO is not initialized, the common sensible value should be 0. That's independent of anyone using EF or not.

Regarding EF core: While you do not need a custom comparer for each converter you register, usually it is a good practice to have one. If you don't provide one, EF will use an internal one that basically uses Equals() and GetHashCode() (that's why it throws now).

Even if you fix the GetHashCode method, I'm not sure how that will work, as the Equals method between two uninitialized values returns false (and please note that for domain/business I think that's ok). But maybe for the EF value comparer, two uninitialized values should be equals.

If you fix the GetHashCode, I could check if there are any differences using a custom value comparer/default (as right now I'm working on a project where the GetHashCode throws).

[SteveDunn]

Thanks @CheloXL , I'm doing a build now. I'm still wary of relaxing GetHashCode as the primary goal of Vogen is to ensure that there are never any instances of uninitialised/default value objects. In my opinion, GetHashCode is used for a purpose. But if there's ever an uninitialised value object, then it doesn't have any purpose whatsoever.

Of course, I'd like to make it possible for EFCore users to use it. It seems like the suggestions above will fix the hascode/comparison issue.

If not, maybe GetHashCode could be relaxed if EFCore is specified in the ValueObjectAttribute...?

[Aragas]

@CheloXL you can use the converter example in the meantime from here #511 (comment)

[Aragas]

Thanks @CheloXL , I'm doing a build now. I'm still wary of relaxing GetHashCode as the primary goal of Vogen is to ensure that there are never any instances of uninitialised/default value objects. In my opinion, GetHashCode is used for a purpose. But if there's ever an uninitialised value object, then it doesn't have any purpose whatsoever.

Of course, I'd like to make it possible for EFCore users to use it. It seems like the suggestions above will fix the hascode/comparison issue.

If not, maybe GetHashCode could be relaxed if EFCore is specified in the ValueObjectAttribute...?

In my case EF Core is not the primary usecase for Vogen, so I believe GetHasCode is correct with it's check, since we are able to have a workaround by defining a custom converter and comparer

[CheloXL]

@SteveDunn The problem with uninitalized values/GetHashCode is usually related to structs. And my concern is that the framework in general uses uninitialized values for comparison with default values, and never expects that GetHashCode throws. And since we do not have control over those values created by the framework, is that I'm not fond of that behavior.

If you don't want to change the actual behavior, I propose two options:

1. A configuration setting that if the value is uninitialized makes GetHashCode returns 0.
2. Let me override/change the generated GetHashCode.

What do you think? Is that ok for you?

[Aragas]

@SteveDunn I believe the right comparison would actually be
(!left._isInitialized && !right._isInitialized) || (left._isInitialized && right._isInitialized && left.Equals(right))

[SteveDunn]

@SteveDunn The problem with uninitalized values/GetHashCode is usually related to structs. And my concern is that the framework in general uses uninitialized values for comparison with default values, and never expects that GetHashCode throws. And since we do not have control over those values created by the framework, is that I'm not fond of that behavior.

If you don't want to change the actual behavior, I propose two options:

1. A configuration setting that if the value is uninitialized makes GetHashCode returns 0.
2. Let me override/change the generated GetHashCode.

What do you think? Is that ok for you?

Yes, that sounds reasonable @CheloXL . I'll add this to next next version

[SteveDunn]

@SteveDunn I believe the right comparison would actually be (!left._isInitialized && !right._isInitialized) || (left._isInitialized && right._isInitialized && left.Equals(right))

This is now in the just-published beta package: https://www.nuget.org/packages/Vogen/3.0.23-beta.1

Would you both mind having a look to see if it looks OK? And maybe suggest some tests for the 'ConsumerTests' project? 🙏

[CheloXL]

Preliminary tests don't work. Ef is throwing a null reference exception inside a compiled lamba so I can't debug what's happening, but after reviwing some of the code, I suspect the problem is in the ValueComparer (If I don't register the ValueComparer along with the ValueConverter, I get the NotInitialized exception, but that's ok as that's the problem this tries to solve).

The code (!left._isInitialized && !right._isInitialized) || (left._isInitialized && right._isInitialized && left.Equals(right)), instance => instance._isInitialized ? instance._value.GetHashCode() : 0) works fine if the VO is a struct, but throws if the value is a class.

I believe you should check if the VO is a struct or a class and add additional null checks where needed. In my code, some VO are structs and some are classes, that's why I'm getting one error or the other.

[SteveDunn]

Thanks @CheloXL - I'll get it to generate different code based on value/reference type

[Aragas]

Oh right, I forgot to add ReferenceEqual(value, null) to the check!

[SteveDunn]

Thanks @CheloXL - just doing a build a now. Would you be able to suggest any example tests or examples, e.g. something similar to what we already have here: https://github.com/SteveDunn/Vogen/blob/main/samples/Vogen.Examples/SerializationAndConversion/EFCore/EfCoreExamples.cs

The next beta build should be there later today or tomorrow.

[SteveDunn]

@CheloXL - @Aragas The 2nd beta is there now on NuGet: https://www.nuget.org/packages/Vogen/3.0.23-beta.2

[CheloXL]

@SteveDunn It still doesn't work. It's missing a null check. I manually implemented a comparer for the VO that's currently failing. I'm attaching the code below. Please note that I had to expose _isInitialized and I'm using Value since this is a test and I don't have access to the internals, but you should not have to have problems in the generated code using the private fields (as you actually do).

public class InputDeviceIdEfCoreValueComparer : ValueComparer<InputDeviceId>
{
	public InputDeviceIdEfCoreValueComparer() : base((left, right) => DoEquals(left, right), instance => DoGetHashCode(instance)) { }

	private static bool DoEquals(InputDeviceId? left, InputDeviceId? right)
	{
		if (left is null)
		{
			return right is null;
		}

		if (right is null)
		{
			return false;
		}

		if (ReferenceEquals(left, right))
		{
			return true;
		}

		if (!left.IsInitialized && !right.IsInitialized)
		{
			return true;
		}

		return left.IsInitialized && right.IsInitialized && left.Value.Equals(right.Value);
	}

	private static int DoGetHashCode(InputDeviceId instance)
	{
		return instance.IsInitialized ? instance.GetHashCode() : 0;
	}
}

[SteveDunn]

Thanks @Aragas / @CheloXL for your help. This is now implemented in 3.0.23