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

Map part of the entity to Json column without the need of Owned entity container. #30094

Closed
dsidedp opened this issue Jan 19, 2023 · 10 comments
Closed
Labels
closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. customer-reported

Comments

@dsidedp
Copy link

dsidedp commented Jan 19, 2023

Allow to map part of the entity (i.e. multiple properties) to a single Json column without the need of wrapping them into Owned entity.

Given class

public class RangeItem
{
    public int Quantity { get; set; }
    public int From { get; set; }
    public int To { get; set; }
    <...>
}

configuration could look like

builder.Entity<RangeItem>.Property(x => x.Quantity).ToJson("Details").HasJsonPropertyName("Quantity");
builder.Entity<RangeItem>.Property(x => x.From).ToJson("Details").HasJsonPropertyName("From");
builder.Entity<RangeItem>.Property(x => x.To).ToJson("Details").HasJsonPropertyName("To");

In case of class hierarchy (TPH) reuse of the same Json column should be allowed as well.
This will require something similar to JSON column sharing.

Thanks.

@roji
Copy link
Member

roji commented Jan 19, 2023

@dsidedp what is your reason for asking for this? What is problematic for you in using an owned entity?

@dsidedp
Copy link
Author

dsidedp commented Jan 19, 2023

TL;DR;
To some degree the reasoning could be the same as why many-to-many is now allowed without explicit join entity.
It was not a technical problem to create join entity, but without it - model looks cleaner and linking mechanics is hidden under the hood.

The real life case is the need to map wide (but not deep) class hierarchy to a table.

public class BaseItem
{
    public int Id { get; set; }
    <...>
}

public class RangeItem: BaseItem
{
    public int Quantity { get; set; }
    public int From { get; set; }
    public int To { get; set; }
}

public class MiscItem: BaseItem
{
    public string Name{ get; set; }
    public int Price { get; set; }
}
<other 200 subtypes of BaseItem with different properties>

By default, all columns in derived classes will have own DB column and I was looking for a way to avoid that.

I do understand that there are ways to solve the problem, like

  1. Having number of generic string/datetime/decimal/int columns and reuse them
  2. Use value converters;
  3. Wrap all properties in derived classes into Owned entity and map it JSON column (but as far as I understand this is not possible due to Json column sharing issue

But all of them have some drawback that looks unnecessary. Examples per case above:

  1. Any external app using same table not know which generic columns supposed to be used to build full object
  2. Not all conversions possible; Performance implications; Possible querying issues; Some scenarios are not currently possible due to '..properties are contained within the same hierarchy. All properties on an entity type must be mapped to unique different columns.'
  3. Require to wrap all properties to a owned container;

This feature will allow devs to avoid of all that - have a better db model, cleaner app model and provide all EF goodies like querying, updates, change tracking.

Ability to map to JSON with Owned entity gives almost all that (after json sharing will be allowed), but the need to have Owned entity container seems like unnecessary requirement and something that can be managed by EF, like join entity for many-to-many.

@roji
Copy link
Member

roji commented Jan 19, 2023

I'm not sure I understand... Your original proposal above still has a specific number of properties on RangeItem; with the current owned entity model you simply need to move these to a type ("Details") and make it owned by RangeItem... In other words, if we implemented your proposal, how exactly would that help with the "wide class hierarchy" problem? Could you provide a bit more context/code to make that clearer?

@dsidedp
Copy link
Author

dsidedp commented Jan 19, 2023

The main difference would be how app classes looks like.

Right now, I have to do this:

public class RangeItem: BaseItem
{
    public RangeData Data { get; set;}
}

public class RangeData 
{
    public int Quantity { get; set; }
    public int From { get; set; }
    public int To { get; set; }
}

public class MiscItem: BaseItem
{
    public MiscData Data { get; set; }
}

public class MiscData
{
    public string Name{ get; set; }
    public int Price { get; set; }
}

Configuration supposed to look like this

builder.Entity<BaseItem>(b => {.....}};

builder.Entity<RangeItem>(b =>
{
    b.OwnsOne(x => x.Data).ToJson("Data");
});

builder.Entity<MiscItem>(b =>
{
    b.OwnsOne(x => x.Data).ToJson("Data");
});

For each additional descendent of BaseItem I have to declare new class for it's data (like RangeData or MiscData) with the single purpose to be able to use them in OwnsOne method during the configuration.

Having 'data' classes will have side effect, most notable are :

  • When these classes are serialized they won't be plain, but have a complex object inside of them.
  • When query needs to be made, the Data.Quantity needs to be referenced instead of juts Quantity.

This proposal should allow classes to be plain and have configuration like this:

public class RangeItem: BaseItem
{
    public int Quantity { get; set; }
    public int From { get; set; }
    public int To { get; set; }
}

public class MiscItem: BaseItem
{
    public string Name{ get; set; }
    public int Price { get; set; }
}

builder.Entity<BaseItem>(b => {.....}};

builder.Entity<RangeItem>(b =>
{
    b.Property(x => x.Quantity ).ToJson("Data").HasJsonPropertyName("Quantity");
    b.Property(x => x.From).ToJson("Data").HasJsonPropertyName("From");
    b.Property(x => x.To).ToJson("Data").HasJsonPropertyName("To");
});

builder.Entity<MiscItem>(b =>
{
    b.Property(x => x.Name).ToJson("Data").HasJsonPropertyName("Name");
    b.Property(x => x.Price).ToJson("Data").HasJsonPropertyName("Price");
});

Ofc, each additional descendant will require explicit mapping as well, but the main benefit is we do not have to have all those data classes.

I think many-to-many without explicit join class is the best analogy.

For many-to-many we use to have class structure like this:

public class Book {
    public int Id { get; set; }
    public List<AuthorBook> AuthorBook{ get; set;}
}

public class Author {
    public int Id { get; set; }
    public List<Book> AuthorBook {get;set;}
}

public class AuthorBook {
    public int BookId {get;set;}
    public int AuthorId {get;set;
}

Now, we do not need to declare join entity AuthorBook with two ID properties, EF is doing this for us.

This AuthorBook is basically an analogue for Owned entity, and this proposal is about providing an option to map properties to Json without the need to group them into a new class.

IMHO, the motivation and benefits of this proposal are really close to those for many-to-many without join entity.

@ajcvickers
Copy link
Member

@dsidedp Can you post both the desired shape of your entity types and the corresponding SQL table schema that they will map to so we can be absolutely sure what it is you want to achieve?

@dsidedp
Copy link
Author

dsidedp commented Feb 2, 2023

For class structure like this

public class BaseItem
{
    public int Id { get; set; }
    public string UsualProperty { get; set; }
    public string? BaseClassString { get; set; }
    public bool BaseClassBool { get; set; }
}

public class RangeItem : BaseItem
{
    public int Quantity { get; set; }
    public int From { get; set; }
    public int To { get; set; }
}

public class MiscItem : BaseItem
{
    public string Name { get; set; }
    public int Price { get; set; }
}

Proposed feature should allow DbContext configuration like this:

builder.Entity<BaseItem>(b => 
{
    b.Property(x => x.BaseClassString).ToJson("Details").HasJsonPropertyName("JsonStringProperty");
    b.Property(x => x.BaseClassBool).ToJson("OtherDetails").HasJsonPropertyName("JsonBoolProperty");
});

builder.Entity<RangeItem>(b =>
{
    b.Property(x => x.Quantity).ToJson("Details").HasJsonPropertyName("Quantity");
    b.Property(x => x.From).ToJson("Details").HasJsonPropertyName("From");
    b.Property(x => x.To).ToJson("OtherDetails").HasJsonPropertyName("To");
});

builder.Entity<MiscItem>(b =>
{
    b.Property(x => x.Name).ToJson("Details").HasJsonPropertyName("Name");
    b.Property(x => x.Price).ToJson("OtherDetails").HasJsonPropertyName("Price");
});

Migration supposed to create table like this:

CREATE TABLE [dbo].[Items](
	[Id] [int] IDENTITY(1,1) NOT NULL,
        [Discriminator] [nvarchar](max) NOT NULL,
        [UsualProperty] [nvarchar](max) NOT NULL,
	[Details] [nvarchar](max) NULL,
	[OtherDetails] [nvarchar](max) NULL,
)

In the App, query like this

var mi = _dbContect.MiscItems.First();

supposed to produce SQL query like this:

SELECT TOP(1) [f].[Id], [f].[Discriminator], [f].[UsualProperty], 
   JSON_VALUE([f].[Details],'$.JsonStringProperty') AS [BaseClassString],
   Cast(JSON_VALUE([f].[OtherDetails],'$.JsonBoolProperty') as bit) AS [BaseClassBool],
   JSON_VALUE([f].[Details],'$.Name') AS [Name],
   Cast(JSON_VALUE([f].[OtherDetails],'$.Price') as int) AS [Price],
FROM [Items] AS [f]
WHERE Discriminator = 'MiscItem'

Updates and queries for 'json mapped' properties supposed to be supported as well. This is the one of the benefits in comparison to ValueConverters.

Optionally, it will be great if it will be possible to provide JsonPath as part of the property configuration. This might provide even more benefits, so allowing something like snippet below would be great as well.

b.Property(x => x.BaseClassString).ToJson("OtherDetails").HasJsonProperty("$.JsonObject.JsonStringProperty"); 

Hope it help.

@roji
Copy link
Member

roji commented Feb 2, 2023

This bears some resemblance to entity splitting, i.e. it configures some properties of your entity to be mapped to a specific JSON property in a specific JSON document (like entity splitting maps some properties to a different table).

However, ultimately this would require quite a big effort on the EF side just in order to allow users to not specify a CLR type; the value proposition seems really quite low compared to the big cost this would require. This is also the 1st time someone has expressed the desire to avoid these types.

Also, the current way is consistent with how .NET JSON serializers work in general: if you want to serialize .NET objects to a JSON document, then your object graph must correspond to the JSON document structure, with a .NET type being required for each object on the JSON side. I'm not aware of a System.Text.Json mechanism which allows you to specify on some .NET property that it would actually be serialized to some arbitrary JSON property on an arbitrary nested JSON object. So I'm not sure why EF would add support for this either.

IMHO, the motivation and benefits of this proposal are really close to those for many-to-many without join entity

I'm not sure that's a good analogy. M2M without join entity significantly simplifies actual LINQ queries - it's not just about removing a CLR type from the model. This isn't really the case here: having to explicitly specify the JSON document property in the query is trivial and doesn't add any complexity, compared to how M2M queries look like with an explicit join entity. So once again, while I agree there's some similarity here, M2M without a join entity seems to deliver far more value/simplification compared to this.

@dsidedp
Copy link
Author

dsidedp commented Feb 3, 2023

How about look at this from a different perspective.

EF can threat configuration from examples above as instructions how to build implicit Owned entity and then use it internally.

For example, configuration below

builder.Entity<BaseItem>(b => 
{
    b.Property(x => x.BaseClassString).ToJson("Details").HasJsonPropertyName("JsonStringProperty");
    b.Property(x => x.BaseClassBool).ToJson("OtherDetails").HasJsonPropertyName("JsonBoolProperty");
});

builder.Entity<RangeItem>(b =>
{
    b.Property(x => x.Quantity).ToJson("Details").HasJsonPropertyName("Quantity");
    b.Property(x => x.From).ToJson("Details").HasJsonPropertyName("From");
    b.Property(x => x.To).ToJson("OtherDetails").HasJsonPropertyName("To");
});

can be processed by EF (for RangeItem in this example) as :

  1. Dynamically create classless like this
public class ImplicitOwnedWrapper1
{
    public string? JsonStringProperty { get; set; }
    public int Quantity { get; set; }
    public int From { get; set; }
}

public class ImplicitOwnedWrapper2
{
    public bool JsonBoolProperty { get; set; }
    public int To { get; set; }
}
  1. Apply configuration for using these classes using existing Owned entity pattern
builder.Entity<RangeItem>(b =>
{
    b.OwnsOne("shadow property 1").ToJson("Details");
    b.OwnsOne("shadow property 2").ToJson("OtherDetails");
});
  1. Unwrap/properly translate calls from actual class properties to properties in implicit types.
    Basicaly, EF should understand that call to RangeItem.BaseClassString is actually call to Ef.Property<ImplicitOwnedWrapper1>(RangeItem, "shadow property 1").JsonStringProperty

In this case,

  • Item 2 is already implemented,
  • Functionality needed for item 1 is definitely not hard and probably already exist (proxy classes for lazy loading supposed to have some dynamic class building functionality near by).
  • Satisfies you concerns regarding System.Text.Json since now there is an object to map to full Json document.

Item 3 is the missing part and unfortunately I do not familiar with EF internals enough to assess complexity.

Does this looks better?

P.S.
M2M analogy works better from consumer POV. For consumers EF is kind of black box, so what EF consumer sees is that there was a need to have a 'unnecessary' class and now there is not, the same is here. With that being said I totally understand your point that M2M bring much more benefits than this one could bring, as well as the fact that you have to find balance between cost and benefits.

@ajcvickers
Copy link
Member

@dsidedp We discussed this and even if there is some value here, which we are not convinced of, the cost of implementation and maintenance is too high for what that value is. So this isn't something we plan to support in EF Core.

@dsidedp
Copy link
Author

dsidedp commented Feb 3, 2023

Ok, and thanks for looking at this!

@dsidedp dsidedp closed this as not planned Won't fix, can't repro, duplicate, stale Feb 3, 2023
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Feb 3, 2023
@ajcvickers ajcvickers added closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. and removed type-enhancement labels Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. customer-reported
Projects
None yet
Development

No branches or pull requests

3 participants