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

Add inheritance support for complex types #31250

Open
Tracked by #31238
AndriySvyryd opened this issue Jul 12, 2023 · 13 comments
Open
Tracked by #31238

Add inheritance support for complex types #31250

AndriySvyryd opened this issue Jul 12, 2023 · 13 comments

Comments

@AndriySvyryd
Copy link
Member

Only TPH would be supported

Related to #9630

@marchy
Copy link

marchy commented Oct 24, 2023

Please, please prioritize doing this.

We use type hierarchies all over our domain, specifically associative enums which in C# are (currently) best modelled via abstract records (or Kotlin's data classes equivalent).

Currently we have to add "access wrapper" properties around these objects and hide away the field-mapping implementation details in partial classes to try to keep the domain clean.

For example, if some entity can be associated to either a user or an anonymous user (ie: modelled in the following example through an Installation entity), we have to add discriminator columns and hide away the 'XOR'-like logic of referencing the respective columns ourselves.

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


public partial class SomeEntity {
	// ... other state of the entity
	
	// NOTE: complex type hierarchy
	public Identity Identity {
		get => IdentityType switch {
			nameof(Identity.Anonymous) => new Identity.Anonymous( InstallationID.Value, Platform.Value, Installation ),
			nameof(Identity.Authenticated) => new Identity.Authenticated( AccountID.Value, Account ),
			_ => throw new ArgumentOutOfRangeException( $"Unknown identity type '{IdentityType}' ),
		};
		init => {
			(string IdentityType, string? InstallationID, Installation? Installation, long? AccountID) persistedValue = value switch {
				Identity.Anonymous anonymousIdentity => {
					IdentityType: nameof(Identity.Anonymous),
					InstallationID: anonymousIdentity.InstallationID,
					Platform: anonymousIdentity.Platform,
					Installation: anonymousIdentity.Installation,
					AccountID: null,
					Account: null,
				},
				Identity.Authenticated authenticatedIdentity => {
					IdentityType: nameof(Identity.Authenticated),
					InstallationID: null,
					Platform: null,
					Installation: null,
					AccountID: authenticatedIdentity.AccountID,
					Account: authenticatedIdentity.Account,
				}
			};
			(IdentityType, InstallationID, Platform, Installation, AccountID, Account) = persisted value
		}
	}
}


// HACK: Hide DAL fields away from the domain model
/*DAL*/partial class SomeEntity {
	internal string IdentityType { get; private set; }
	public Platform? Platform { get; private set; } // compound foreign key to Installation
	public string? InstallationID { get; private set; } // compound foreign key to Installation
	public Installation? Installation { get; private set; } // NOTE: could be lazy-loaded
	public long? AccountID { get; private set; } // foreign key to Account
	public Account? Account { get; private set; } // NOTE: could be lazy-loaded
}

This SHOULD be exactly what complex types can handle, by simply mapping Identity as a complex type and specifying that it is a type hierarchy with a given discriminator column name (ie: IdentityType). EF should take care of the rest, as it would for a TPH mapping – without either the partial class or access wrapper being needed at all.

ie: in future EF version:

modelBuilder.Entity<SomeEntity>()
	.ComplexProperty( _ => _.Identity )
	.HasDiscriminator<string>( "IdentityType" )
public abstract record Identity {
	public record Anonymous( string InstallationID, Platform Platform, Installation? Installation ) : Identity;
	public record Authenticated( long AccountID, Account? Account ) : Identity;
}


public class SomeEntity {
	// ... other state of the entity
	
	// NOTE: complex type hierarchy
	public Identity Identity { get; private set; }
}

(presumably with some extra specifiers in the DB config if you don't want to prepend 'Identity_' in front of all the 3 complex type fields above, as would be the default: Identity_InstallationID, Identity_Platform, Identity_AccountID)

POTENTIALLY DEPENDENT ISSUE: #31376

@atrauzzi
Copy link

atrauzzi commented Dec 6, 2023

@roji -- I don't know if this is helpful at all, but I'll just put this here: https://gist.github.com/atrauzzi/dde4f5e92fb783cb6847ff2b1c2d6710

Seems relevant to this convo?

(Thoughts welcome, especially if EF8 makes any part of it obsolete.)

@atrauzzi
Copy link

Actually, I was just trying out the [ComplexType] functionality and decided to go back to my solution in the gist above. Not for a lack of wanting it to work, but I just think what I've built there is more versatile than what EF offers currently.

Very seriously, I'm totally open to EF adapting it into a new feature if you want...

@roji
Copy link
Member

roji commented Jan 14, 2024

@atrauzzi I haven't gone into your solution in-depth, but it seems like what you've done above handles polymorphism/inheritance completely outside of EF's model; as such, it's more like what EFCore.PG did to support JSON before the new ToJson() support in 8.0. Not going through EF's model means that a lot of things are impossible, since EF has no idea what information is actually being modelled inside your document.

In fact, if this is the kind of thing you're looking for, why build your own thing rather than just using System.Text.Json's built-in polymorphism feature? You should be able to easily set up a value converter that just does this; of course, at that point your can't query into the document, but that would be the case with you solution as well, it seems.

BTW we refer to these kinds of schemes - where EF isn't aware of the data inside the document - as "weakly-typed" JSON mapping (see #28871 which tracks these). These make sense mainly for scenarios where there really isn't any fixed data schema (i.e. the same JSON column can sometimes hold an array of Blogs, sometimes a dictionary with Customers...). We believe that actual fully dynamic scenarios are rare, difficult to work with, and EF may not be the right tool for the job. But that doesn't seem to be what you're looking for in this case - only the ability to model inheritance/polymorphism in a strongly-typed model.

In any case, we do intend to support discriminator-based inheritance for complex types mapped to JSON - but there's lots of competing priorities.

@atrauzzi
Copy link

atrauzzi commented Jan 15, 2024

@roji -- I actually am using System.Text.Json polymorphism overall, but there's a tiny bit extra on top. All of that plugs back into the out-of-the-box System.Text.Json polymorphism functionality after that. Trying to remember why I needed this, but I promise there was some DX or scenario that System.Text.Json polymorphic serialization wasn't handling well.

edit: I think it's because the parent has to define all of its children. So I couldn't cross assemblies without creating circular references.

another edit: This is probably a good time to mention again how features don't always release fully baked and it forces devs into what look like self-inflicted workarounds -- but are actually necessary to make a feature useful...

And yeah, you got it. I don't necessarily want completely unpredictable data. But if I have an IList<ISomething>, I want to be able to get back all the various subtypes of ISomething in there.

Is there a "medium-typed" or discriminator based JSON feature being tracked?

@roji
Copy link
Member

roji commented Jan 15, 2024

This is probably a good time to mention again how features don't always release fully baked and it forces devs into what look like self-inflicted workarounds -- but are actually necessary to make a feature useful

I get what you're saying; but this is where the "half-baked" idea sometimes goes a bit too far for me - various people have various needs, and everyone obviously considers the thing they're missing as vital, making the feature "half-baked". Specifically, saying that complex types as a feature "isn't useful" is because it doesn't yet support inheritance is a bit much; many people can use it who don't require inheritance (just as they've been using owned types for many years without inheritance being supported).

EF is a big, complicated project, with many intersecting features; it's simply not feasible to release something that supports the entire matrix of possibilities in day one. Nor do I think would that be desirable: if we were bent on making everything always work with everything, we'd end up wasting a lot of time on various intersections which nobody needs or cares about. So releasing a feature without the entire "matrix" is also a way to get it out there, and gather feedback on what people actually need (and how badly), rather than doing everything up-front always.

Just for fun, we discussed the theoretical possibility of supporting discriminator-based inheritance for complex types without JSON, i.e. when the complex type is just mapped to columns in the principal's table. That's of course technically possible, and not dissimilar to regular TPH mapping (some columns would just be unused depending on the type being represented). Would it be useful for us to spend on time on it? Probably not. Is there someone out there that needs it and would consider complex types incomplete without it? For sure.

Is there a "medium-typed" or discriminator based JSON feature being tracked?

What do you mean by this? Supporting mapping complex types to JSON is tracked by #31252, and allowing discriminator-based inheritance for that is tracked by this issue.

@atrauzzi
Copy link

...we'd end up wasting a lot of time on various intersections which nobody needs or cares about.

Yup, very fair, and I know. Although when it comes to things like polymorphic navigations and polymorphic JSON, there might be some blind spots. Waiting for people to tell you what they want isn't always the best because oftentimes, you just don't hear back -- or worse... people hack it up worse than what I showed you above and suffer in silence. 🥀

EF is a big, complicated project, with many intersecting features; it's simply not feasible to release something that supports the entire matrix of possibilities in day one.

And I think this is where I feel like things tend to get too defensive to service the plea "how can we possibly know anything?" 🤷‍♂️ .
I'm nowhere near as smart as you or other EF maintainers, but I can guarantee this would have been the first thing I would have called out on a design review. Believe me, nobody expects 100% of corner cases to be covered and that's not an argument I sense anyone making.

....Some applied intuition on how certain features will be used however would go a really long way. With release cycles that are so wide, missing what looks like a "corner", but is actually "a whole room" 😅 could mean something goes effectively unused for an entire release.

Happy to be part of the solution though, give me MVP and I'll start offering early design suggestions. I always manage to find the "most popular corner cases"! 😆


What do you mean by this? Supporting mapping complex types to JSON is tracked by #31252, and allowing discriminator-based inheritance for that is tracked by this issue.

Ah thank you, upvoted and subscribed! I might post a couple of questions or notes in there as well.

And to my earlier point, I wouldn't have even known what to search for to find that issue to upvote. Further reinforcing the inconsistency of "waiting for people to tell us with upvotes" for prioritization. Yes, you likely have other signals, but without the link you provided, I'd have fallen into neither demographic. I would not be surprised if the vast majority of EF Core users are in a similar situation.

@roji
Copy link
Member

roji commented Jan 15, 2024

And I think this is where I feel like things tend to get too defensive to service the plea "how can we possibly know anything?"

I don't think we ever really do that.

I can guarantee this would have been the first thing I would have called out on a design review.

To be clear, I don't think anyone doubted we'll do this - at least for me it's really a matter of when; implementing complex types was already enough work that we could deliver both JSON mappings and inheritance in the same release. That doesn't make something half-baked: lots of people are using complex types successfully; if you require inheritance, don't use it yet.

Having said all that, I do believe this is one of our higher priorities in general and I really hope we get it into 9!

@atrauzzi
Copy link

...if you require inheritance, don't use it yet.

I mean, I've facilitated it myself today. But hopefully it will be easy to switch over to any official flavour. 🙏

Having said all that, I do believe this is one of our higher priorities in general and I really hope we get it into 9!

I'll be there!! 😄

@atrauzzi
Copy link

@roji -- You mentioned that you don't follow the approach above.

When you define a parent type, System.Text.Json requires you to put [JsonPolymorphic] on it. No problem there really. To get the subtypes however, you have to somehow indicate them. So they have a [JsonDerivedType] attribute. The problem though is that the [JsonDerivedType] attribute has to go on the same type that has [JsonPolymorphic] on it. The parent/marker/base/whatever.

The problem with that approach is that this inadvertently implies that all the types have to be in the same assembly. If I define a type that is to "be a polymorph" in another assembly, configuring it would create a circular reference.

By doing what I've done above with the [JsonPolymorph] attribute, I've broken that apart and made it possible for polymorphs to register themselves and have their parent inferred through reflection. I do all this reflection up-front, rather than on-demand to ensure that the cost is only incurred at startup.

@roji
Copy link
Member

roji commented Jan 15, 2024

Cross-assembly hierarchies are explicitly addressed as a use-case for the contract model (docs) - that would seem to be the recommended way to do that (in any case, I'd go that way instead of rolling my own thing with a registry etc.).

But in any case, this issue isn't about proper use of System.Text.Json deserialization - and in any case, if it works for you and you're happy, by all means...

@atrauzzi
Copy link

I think that's effectively what I did, although I have the registry to optimize it a bit up front and so that I can manage the discriminators separately.

I think it's just worth calling out this use case for any future EF Core implementation. Be aware that people are going to want to be able to define parent and child types separately. Minimizing the amount of customization to accomplish this would be super nice.

@jakenuts
Copy link

...if you require inheritance, don't use it yet.

I mean, I've facilitated it myself today. But hopefully it will be easy to switch over to any official flavour. 🙏

Having said all that, I do believe this is one of our higher priorities in general and I really hope we get it into 9!

I'll be there!! 😄

Ooooo, care to share your solution? Just started considering use of JsonColumns and the ability to do polymorphic deserialization/querying is all that's holding me back.

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

6 participants