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

Azure Cosmos DB provider should default to implicit ownership #24803

Closed
JeremyLikness opened this issue Apr 29, 2021 · 8 comments · Fixed by #25283
Closed

Azure Cosmos DB provider should default to implicit ownership #24803

JeremyLikness opened this issue Apr 29, 2021 · 8 comments · Fixed by #25283
Labels
area-cosmos breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@JeremyLikness
Copy link
Member

JeremyLikness commented Apr 29, 2021

The typical case for document databases is to store a root entity and serialize its children, including single and collection navigations, as sub-properties rather than separately tracked entities. The EF Core provider for Azure Cosmos DB currently has two issues with this convention. First, complex entities in the graph are assumed to be independently tracked and therefore modeling the default behavior requires explicitly configuring ownership. For larger graphs this has a negative impact on maintainability. Second, complex entities shouldn't have to be tracked independently. They should be considered serializable types owned by the root entity that are serialized directly.

As a solution, I'd like to see:

  1. When I configure an entity for Cosmos DB as DbSet any complex types or collections in the graph are assumed to be owned by the parent entity. Let me opt out by configuration but don't force me to opt in.
  2. Instead of forcing complex types to be tracked as separate entities, treat them the same way as primitives, i.e. serialize and deserialize directly instead of tracking separately. This way their properties are still indexed and queryable.

Example from Cosmos DB getting started app:

namespace todo
{
    public class Family
    {
        [JsonPropertyName("id")]
        public string Id { get; set; }
        public string LastName { get; set; }
        public IList<Parent> Parents { get; set; }
        public IList<Child> Children { get; set; }
        public Address Address { get; set; }
        public bool IsRegistered { get; set; }
        public override string ToString()
        {
            return JsonSerializer.Serialize(this);
        }
    }

    public class Parent
    {
        public string FamilyName { get; set; }
        public string FirstName { get; set; }
    }

    public class Child
    {
        public string FamilyName { get; set; }
        public string FirstName { get; set; }
        public string Gender { get; set; }
        public int Grade { get; set; }
        public IList<Pet> Pets { get; set; }
    }

    public class Pet
    {
        public string GivenName { get; set; }
    }

    public class Address
    {
        public string State { get; set; }
        public string County { get; set; }
        public string City { get; set; }
    }
}

Currently I have to do this configuration, whereas with the SDK v4 it "just works":

builder.Entity<Family>()
    .HasPartitionKey(nameof(Family.LastName))
    .OwnsMany(f => f.Parents);

builder.Entity<Family>()
    .OwnsMany(f => f.Children)
        .OwnsMany(c => c.Pets);

builder.Entity<Family>()
    .OwnsOne(f => f.Address);

OwnsMany is fine at one level but gets complicated when there are multiple nested complex types. Using a value converter makes me lose the ability to query on the summary. It would be great if we could just configure the Family as a DbSet and only have to configure the partition key.

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Apr 29, 2021

Related - #6787 to allow configuring owned types without using deeply nested lambdas.

@ajcvickers ajcvickers added this to the Backlog milestone May 4, 2021
@ajcvickers ajcvickers modified the milestones: Backlog, 6.0.0 May 6, 2021
AndriySvyryd added a commit that referenced this issue Jun 15, 2021
AndriySvyryd added a commit that referenced this issue Jul 6, 2021
Allow to reconfigure STETs as regular entity types

Fixes #24803
AndriySvyryd added a commit that referenced this issue Jul 15, 2021
Allow to reconfigure STETs as regular entity types

Fixes #24803
AndriySvyryd added a commit that referenced this issue Jul 17, 2021
Allow to reconfigure STETs as regular entity types

Fixes #24803
AndriySvyryd added a commit that referenced this issue Jul 18, 2021
Allow to reconfigure STETs as regular entity types

Fixes #24803
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 19, 2021
@AndriySvyryd AndriySvyryd removed their assignment Jul 19, 2021
AndriySvyryd added a commit that referenced this issue Jul 20, 2021
Allow to reconfigure STETs as regular entity types
Rerun CosmosDiscriminatorConvention and BaseTypeDiscoveryConvention when ownership changes
Keep OwnedNavigationBuilder.DependentEntityType updated

Fixes #24803
@ghost
Copy link

ghost commented Jul 21, 2021

If I am reading the Merge correctly it only addresses issue 1.

Is Issue 2 being tracked separately?

Instead of forcing complex types to be tracked as separate entities, treat them the same way as primitives, i.e. serialize and deserialize directly instead of tracking separately. This way their properties are still indexed and queryable.

We are currently having to work around this by adding and serialising Guid keys on all child entities in complex types so that EF can track them properly so this second issue was of real interest to me and I would not like to see it get lost. Especially since it looked like it was going to get fixed in EFCore6.

@AndriySvyryd
Copy link
Member

@ACROV You are reading correctly. #17306 will allow value converters that output raw JSON, though it means that all complex types will become a black box for query.

You shouldn't need to create any keys for owned entities, EF Core creates shadow keys by convention that are not persisted, if there's an issue with them please file it separately.

@ajcvickers ajcvickers modified the milestones: 6.0.0-rc1, 6.0.0 Nov 8, 2021
@John-HLC
Copy link

John-HLC commented Nov 19, 2021

@AndriySvyryd, @ajcvickers, @JeremyLikness - Apologies in advance for my limited EF Core for Cosmos understanding.. if you can reach me half way...

You shouldn't need to create any keys for owned entities, EF Core creates shadow keys by convention that are not persisted, if there's an issue with them please file it separately.

The problem i think we ran across is that we do not keep a persistent DB context when using Blazor WASM (Client only) app.
Using the DBContext - we get all the data needed.. and use it disconnected from the DBContext.. then if we need to update data, we new up a new DBContext() and use the Update() command to update the entity. This I believe is recommended way to do things in Blazor WASM client only (not keeping the DbContext alive).

So in this case.. should we be adding an "Id" to every owned class going forward?
Or do you think we should be doing below as the recommended way to update Entities with owned content nested?

   var x= context.Add(this.SomeEntityWithNestedOwnedList);
   x.State = EntityState.Modified;

@John-HLC
Copy link

@ajcvickers - in this post you mentioned one could add a private CLR property to the owned entity. #19856

The best way to deal with this is to add a CLR property for the key (which can be private) to the owned entity type. The key values are then preserved while the entities are not being tracked, and re-attaching the entities works correctly. We should document this.

can you expand on this - what does it look like? We tried but did not work for us.

@AndriySvyryd
Copy link
Member

Or do you think we should be doing below as the recommended way to update Entities with owned content nested?

If that works, then yes. In most cases EF will generate the values for the shadow keys on owned entity types. So, unless something is failing you don't need to preserve them.

@John-HLC
Copy link

I know it's a small thing but my brain kind of hurts... when needing to update an entity
to code "Add"
and then on another line mark it as "Modified".

Makes me feel like I'm going off course on a work round, and from readability standpoint reading Add feels wrong when really updating (even if that's what its doing under the covers).

Would it be possible to introduce a new method on Microsoft.EntityFrameworkCore.Cosmos SDK DB Context... and name that method UpdateDisconnectedEntity() or something like that?

Or add the "Add" and "Mark as Modified" somewhere in documentation for Microsoft.EntityFrameworkCore.Cosmos for BlazorWASM.

@JeremyLikness - thoughts? since you work with Blazor WASM.

@AndriySvyryd
Copy link
Member

@John-HLC The workaround is needed because EF currently doesn't generate values when an entity is marked as Modified, #6999 tracks this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-cosmos breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants