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

Allow mapping optional complex properties #31376

Open
Tracked by #31238
roji opened this issue Jul 30, 2023 · 18 comments
Open
Tracked by #31238

Allow mapping optional complex properties #31376

roji opened this issue Jul 30, 2023 · 18 comments

Comments

@roji
Copy link
Member

roji commented Jul 30, 2023

We currently always return a non-null instance for complex properties, including when all their columns are null. We may need to add an option to opt into optional complex types, where if all properties are null, null is returned instead.

See #9005 for the same thing with owned entity types.

@roji roji changed the title Allow mapping optional non-collection non-JSON complex types Allow mapping optional complex properties Jul 30, 2023
@ajcvickers ajcvickers added this to the Backlog milestone Aug 6, 2023
@marchy
Copy link

marchy commented Oct 23, 2023

Without this the use of complex types is extremely limited.

Since these are "identity-less" concepts by definition, they may often be un-set in the domain (ie: it's not a strong enough domain entity concept to warrant its own identifiable entity – high correlation to not always being set).

Please adopt nullability/optionality to both the JSON-backed and column-backed complex types in the future – until then back to good-old nullable string-column JSON mapping without any notion of complex types it is. 😔

@roji
Copy link
Member Author

roji commented Oct 23, 2023

@marchy please note that you can already use owned entity modeling as a sort of complex type modeling; that supports both optionality and JSON columns. The new complex type modeling being introduced in 8.0 is still new and does not support everything, but we plan to improve that.

@marchy
Copy link

marchy commented Oct 24, 2023

Thanks for the suggestion @roji, well familiar with the implications of Owned Types, as as you mention it has been around for years.

However they fail on the identity-less semantics of value objects (ie: multiple records cannot have the same Owned Entity values without conflicting) – the very reason complex types have been introduced.

Requiring value objects to be non-optional is highly arbitrary and extremely limiting.

🤞 Really looking forward to a big subsequent EF9 push on complex types to support optionality (this issue), type hierarchies and JSON mapping before they can become realistically feasible to adopt.

@marchy
Copy link

marchy commented Oct 24, 2023

PS: One of the scenarios this may enable as well is complex type hierarchies – where each type in the hierarchy has to be modelled as optional, as each sub-class complex type would essentially be mutually exclusive to the other sub-classes (ie: optional, XOR-like semantics, as any instance can only be of one sub-class type or another, but never multiple)

@roji
Copy link
Member Author

roji commented Oct 24, 2023

Requiring value objects to be non-optional is highly arbitrary and extremely limiting.

This wasn't a design decision or anything - we simply didn't have enough time to implement optional complex types for 8.0 (this is more complex than it looks).

One of the scenarios this may enable as well is #31250 (comment) – where each type in the hierarchy has to be modelled as optional [...]

Optionality and inheritance mapping are pretty much orthogonal, but we do plan to do both. Optional complex types will likely be prioritized much higher though.

@marchy
Copy link

marchy commented Oct 25, 2023

Thank you @roji.

Having been on EF since v4 (ie: the first code-first version circa 2009)... I do appreciate that anything to do with ORM's is extremely difficult and takes time to cover the many scenarios that need to be considered. 😄

Appreciate the insight on the thinking in priority.

The optionals support could potentially enable hand-rolling associative enums however – since they are essentially the same as TPC and as long as you can null out all other sub-class complex objects except the the one the instance conforms to. In theory you could achieve this without any official support for inheritance – so long as the framework doesn't get in the way by detecting the abstract base class and preventing the mapping in some way (a scenario to consider).

This is the specific scenario riddled throughout our domain (and indeed many domains):

public abstract record Identity {
	public record Anonymous( string InstallationID, Platform Platform, Installation? Installation ) : Identity;
	public record Authenticated( long AccountID, Account? Account ) : Identity;
}

The moment you can model these as complex types, you can add all sorts of variances throughout the domain – for example when purchasing a ticket for something, you might have variances that have different fields based on the ticket type etc. Same thing when choosing a payment type, or pickup/delivery option, or login provider etc.

If we could map the above based on a manual discriminator column (IdentityType:string/enum) and two nullable complex types (AnonymousIdentity? and AuthenticatedIdentity?) that would already be a win.

Thinking this:

public partial class SomeEntity {
	// ... other state of the entity
	
	// NOTE: complex type hierarchy
	public Identity Identity {
		get => IdentityType switch {
			nameof(Identity.Anonymous) => AnonymousIdentity!,
			nameof(Identity.Authenticated) => AuthenticatedIdentity!,
			_ => throw new ArgumentOutOfRangeException( $"Unknown identity type '{IdentityType}' ),
		};
		init => {
			// NOTE: XOR-like logic to set all sub-class complex types in the TPC hierarchy to null except for the one the instance conforms to
			(string IdentityType, AnonymousIdentity? AnonymousIdentity, AuthenticatedIdentity? AuthenticatedIdentity) persistedValue = value switch {
				Identity.Anonymous anonymousIdentity => {
					IdentityType: nameof(Identity.Anonymous),
					AnonymousIdentity: anonymousIdentity,
					AuthenticatedIdentity: null,
				},
				Identity.Authenticated authenticatedIdentity => {
					IdentityType: nameof(Identity.Authenticated),
					AnonymousIdentity: null,
					AuthenticatedIdentity: authenticatedIdentity,
				}
			};
			(IdentityType, AnonymousIdentity, AuthenticatedIdentity) = persisted value
		}
	}
}


// HACK: Hide DAL fields away from the domain model
/*DAL*/partial class SomeEntity {
	internal string IdentityType { get; private set; }
	public AnonymousIdentity? AnonymousIdentity { get; private set; }
	public AuthenticatedIdentity? AuthenticatedIdentity { get; private set; }
}

Having EF automatically link the two parts together in TPC-style would definitely be the end-game (maybe EFC 10!) 🚀

End-game: (no partial class hackiness needed)

public class SomeEntity {
	// ... other state of the entity
	
	// complex type hierarchy
	// NOTE: this is still not itself optional – despite its constituent parts of each TPC-style sub-class getting mapped to optional complex types under the cover
	public Identity Identity { get; private set; }
}

@roji
Copy link
Member Author

roji commented Oct 25, 2023

@marchy when we do get to complex type inheritance mapping (#31250), it will definitely not be composition-based as in your sample above, but rather inheritance-based (much like the current TPH for non-complex-types). You're free to implement what composition-based scheme you want (once we implement optional support), but that generally seems like quite a complicated way to go about things, pushing a lot of manual work on the query writer (e.g. needing to deal with the discriminator manually).

@alexmurari
Copy link
Contributor

alexmurari commented Oct 27, 2023

This is a must have for always valid domains.

E.g. a Phone value object can't be instantiated with null/empty Number value. The class protects its invariants. Nullability must be at the value object level (Phone?).

Here's an excelent article about when value objects should or should not be null: https://enterprisecraftsmanship.com/posts/nulls-in-value-objects/

@jscarle
Copy link

jscarle commented Oct 27, 2023

I completely agree with @alexmurari. Value Object validity dictates that its the Value Object itself that should be optional and nullable.

@aradalvand
Copy link

aradalvand commented Nov 15, 2023

I second that complex types are currently basically unusable in a good 50% of scenarios precisely because of this limitation. Hopefully this makes it to v9; should be considered a high-priority item IMO.

@marchy
Copy link

marchy commented Nov 15, 2023

Thanks @roji, that sounds ideal indeed – was just showing how the composition-based approach can let you model the associative enums scenario (ie: no shared state between different sub-classes) with just the support of optionals, rather than needing #31250 (where I did drop an example of how that simplify things even further).

Hope that helps prioritize.

@AndriySvyryd
Copy link
Member

We should set NRT annotations correctly to avoid warnings when configuring nullable complex types:

modelBuilder.Entity<MyEntity>().ComplexProperty(e => e.Complex).Property(c => c!.Details);

@ManeeshTripathi14
Copy link

I am working on a project where we have already designed our domain entity based on DDD, where we have nullable complex property (value object) because those are NOT mandatory by business requirement. i was facing one issue with OwnsOne, in case of OwnsOne ef is not able to detect changes and entity state does not change to modified if we update the value of complex property( update means here replacing the old with new and NOT modifying the property of complex property individually ) so now due to this limitation i can't use complex property.
Please allow NULL for complex property. so that we can desin domain entity without considering infrastructure concerns.
thanks

@oleg-varlamov
Copy link

@ManeeshTripathi14
I absolutely agree with you. In all previous projects we used NHibernate, which works very well with DDD. But in new projects, due to good JSON support, we decided to switch to EFCore. We have been waiting for about a year for the appearance of new Complex Properties, since OwnedEntity is a very strange implementation that brought more problems than benefits. And in the end, when Complex Properties came out, we were so disappointed :( It is almost necessary for Complex Properties to support NULL values.

@cjblomqvist
Copy link

Preferably, do not limit this to require att least one non nullable property like it is for owned. Rather, add a "hidden" nullable column (or bool or whatever) if needed.

@plppp2001

This comment was marked as resolved.

@roji

This comment was marked as resolved.

@plppp2001

This comment was marked as resolved.

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