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

CosmosDb Collection Properties and Owned - Blocking Issues #14701

Closed
JohnGalt1717 opened this issue Feb 14, 2019 · 16 comments
Closed

CosmosDb Collection Properties and Owned - Blocking Issues #14701

JohnGalt1717 opened this issue Feb 14, 2019 · 16 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@JohnGalt1717
Copy link

While I realize that this isn't fully enabled and CosmosDb provider is very much a work in progress I'm not confident that the current solution here: 8172 really addresses the issue because Owned classes are exclusive from what I can tell and you need to be able to create collections and lists.

So in a document store it is entirely normal to do this:

public class Contact {
       public ICollection<string> SomeList {get; set;}
       public ICollection<Relationship> Relationships {get; set;
}

public class Relationshp  {
      public RelationshipTypes Type {get; set;}
      public ShortContact OtherContact {get; set;}
  1. In this case EVERYTHING above is stored in the Contact document including a list of ['somevalue1', 'somevalue2'] for the string collection AND the array.
  2. ShortContact is SHARED all over the place. Any time I have a reference that would otherwise be a JOIN in a relational database to the Contact table I use ShortContact that contains a minimum set of properties that describe the contact but not everything in the document.
  3. ShortContact needs to be updated every time the actual contact is updated because the properties like Name in ShortContact need to be updated with the new values.

So the issues are.

  1. It is my understanding (from the error telling me so) that if you put [Owned] on ShortContact and use Fluent on Relationship .HasOneOwned(t => t.OtherContact) as an example that you can't do the same anywhere else in your model because it's now exclusive.
  2. There is no handling for ICollection as far as I can tell. Nor IDictionary.
  3. Owned Collections don't seem to be properly supported because EnsureCreated still tries to create DbContext collection in your database even though the actual Entities all have ToContainer() specified and create their own containers (we use Database level throughput)
  4. Migrations, while not changing the model itself, are vital because you need to be able to update the database with triggers that update things like all ShortContact in the database with new values based on what is being updated/deleted from the root entity. Migrations don't have to track the model and update it, but they do need to create an empty migration that allows you to add linq commands to update data based on model changes AND update triggers etc. and then gen that into an executable script against your database to be run as part of your ci/cd.

These issues (minus #4) completely defeat the NoSQL/DocumentDb model and make a non-starter for the CosmosDb provider if they're not addressed before release of Ef Core 3.0. (i.e. it's essentially useless for any complex project that is properly using CosmosDb)

While I see references to some of these things in other closed tickets they're all dismissed. They shouldn't be because the CosmosDb provider can't be used in any real-world scenario until such time as these things are properly implemented as intended in the NoSQL world and generate a model that makes sense.

To me, all sub collections should be assumed to be sub collections of the parent document unless there is a DbSet in the DbContext or they are explicitly referenced in the modelBuilder somewhere as entities.

And primitive type lists and dictionaries need to be supported as first class citizens.

Thanks.

@ajcvickers
Copy link
Member

@divega @AndriySvyryd to follow up.

@AndriySvyryd
Copy link
Member

  1. It is my understanding (from the error telling me so) that if you put [Owned] on ShortContact and use Fluent on Relationship .HasOneOwned(t => t.OtherContact) as an example that you can't do the same anywhere else in your model because it's now exclusive.

I don't understand the issue that you are experiencing. Can you provide the model that you want to configure and the exception that EF throws?

  1. There is no handling for ICollection as far as I can tell. Nor IDictionary.

I filed #14762

  1. Owned Collections don't seem to be properly supported because EnsureCreated still tries to create DbContext collection in your database even though the actual Entities all have ToContainer() specified and create their own containers (we use Database level throughput)

Could you provide a model that has this issue?

  1. Migrations, while not changing the model itself, are vital because you need to be able to update the database with triggers that update things like all ShortContact in the database with new values based on what is being updated/deleted from the root entity. Migrations don't have to track the model and update it, but they do need to create an empty migration that allows you to add linq commands to update data based on model changes AND update triggers etc. and then gen that into an executable script against your database to be run as part of your ci/cd.

EF Core doesn't support data migration in general (except some very limited cases with seed data), implementing something as flexible as you are describing is currently out of scope.

@JohnGalt1717
Copy link
Author

Hi @AndriySvyryd

  1. It's really any case where a class is owned by multiple parents.

So if you create a "ShortContact" and you use it on a Relationship which causes it to be owned by the relationship AND you use it on say Account which has a Contact on it, which is also owned based on the methodology being used, EF Core freaks out and says that it's already owned and dies.

The primary issue with this and #3 is that Owned doesn't really solve the problem of NoSQL. You will, because you have no joins, have reference objects that are used everywhere you would have had a join in the database and they will be exactly the same everywhere VERY likely. In this scenario EF Core falls over. (which is also why it starts generating stuff in a collection under the name of the context.

#4 Sure EF Core does. You can easily create a blank migration right now and execute up/downs with data any time you want with migrationBuilder.Sql. This is largely what's needed for CosmosDb to start.

However, if done properly there would be a concept of a "Reference" that would be an object per #1 and 2 above and when that reference was created, it would automatically create cosmosdb server side triggers that update the values in the reference everywhere they are in the database automatically when any of the fields changed in the parent document. AND EF Core would automatically use these triggers without having the specify them. That would be how I would see migrations working best. The rest is just SQL commands or upsert/delete documents.

Of course at a minimum EF Core needs to support enumerating the triggers that will run on a given collection. As it stands right now you basically can't use triggers because there is no way to specify them and CosmosDb doesn't run triggers unless explicitly told to.

This is all basic stuff in Cosmos that EF Core has to support or you'll have a useless provider that doesn't work in any real-world scenario. References are a fact of life in Document Databases so Triggers are also not-optional at the very least and then embedded lists of the same type as other embedded lists are also non-optional.

The only part that is slightly optional is Migrations and automatic creation of triggers to clean up references. And even that, at least it should allow creation of those migrations and then execution of them manually.

@AndriySvyryd
Copy link
Member

@JohnGalt1717 EF Core already supports what you describe in 1 and 3, however you might be running into a bug. Unfortunately unless you provide a repro project there is no practical way for us to find it and fix it.

@JohnGalt1717
Copy link
Author

It's really simple to repro:

Class1 {
public Subclass SubObj {get; set;}
}

Class 2 {
public Subclass SubObj {get; set;}
}

[Owned]
public class Subclass {
……..
}

Write .HasOwned for Class1 and Class2 for their SubObj.

Volia, will crash on EnsureCreated.

@AndriySvyryd
Copy link
Member

@JohnGalt1717 It works fine for me. What version of Microsoft.EntityFrameworkCore.Cosmos are you using?

@JohnGalt1717
Copy link
Author

Preview 2

@AndriySvyryd
Copy link
Member

You mean 3.0.0-preview.19074.3 ? It works, see https://github.com/AndriySvyryd/CosmosRepro

I was able to repro 3 and filed #14841

@ajcvickers
Copy link
Member

@divega @smitpatel to follow up.

@JohnGalt1717
Copy link
Author

All: Maybe I'm missing something. Need an explicit sample that uses the same class in multiple parents with owned that shows this standard reference methodology for NoSQL.

Also need to know how to force cosmos to execute triggers in EF Core otherwise that's a show stopper for obvious reasons.

@AndriySvyryd
Copy link
Member

All: Maybe I'm missing something. Need an explicit sample that uses the same class in multiple parents with owned that shows this standard reference methodology for NoSQL.

@JohnGalt1717 https://github.com/AndriySvyryd/CosmosRepro is exactly this

@smitpatel
Copy link
Member

smitpatel commented Mar 1, 2019

So if you create a "ShortContact" and you use it on a Relationship which causes it to be owned by the relationship AND you use it on say Account which has a Contact on it, which is also owned based on the methodology being used, EF Core freaks out and says that it's already owned and dies.

Duplicate of #9630

@smitpatel
Copy link
Member

ShortContact needs to be updated every time the actual contact is updated because the properties like Name in ShortContact need to be updated with the new values.

EF Core does not support triggers, you can define them and run them yourself for now as a work-around. While triggers are useful, at the same time, requiring to update multiple documents like that goes against principal of data embedding inside a document. Because you update Contact, you need to update all the relationships which has short contacts. Ideally, your document which has relationship with other document should not store anything which requires frequent updates. In most cases that comes down to the Id/Pk of the related document. Which once you create the document, you would not change. Lack of join on document database is cons of them (and then there pros to counter certain limitations). You need to figure out what to do on client side in order to get-around the lack of join. Having minified version of document which requires frequent updates is certainly not the ideal way.

@ajcvickers
Copy link
Member

@divega Can this be closed now, or is there anything else that needs following up on?

@JohnGalt1717
Copy link
Author

@ajcvickers My issue is that because CosmosDB doesn't support enforcing triggers by default, if EF Core is released without the ability to define triggers per collection, this is a major issue for any significant deployment.

Further the assertion that my method of providing reference data in documents that have associations because of no joins is non-standard is completely incorrect. Mongo, Raven, and many other NoSQL DocumentDbs actually explicitly state that this is how you do it. The even explicitly state that you then should update those associated information whenever the originating document's values are updated.

Thus the entire premise of what and how the cosmosdb provider for EF Core is being written on is incorrect and going to result, as I started this thread with, in a useless provider that can't be used in the real world.

(And that's assuming that you can have multiple owners of the same child class, which you couldn't the last time I checked in Preview 2)

@ajcvickers
Copy link
Member

Notes from triage: Added "escape hatches" to the list in #12086 (@AndriySvyryd feel free to move to the correct section and check the box if this is fully done) so that it is easy to drop down to the Cosmos SDK and do things directly there if EF Core does not (yet) have any appropriate abstraction.

@ajcvickers ajcvickers added the closed-no-further-action The issue is closed and no further action is planned. label Mar 29, 2019
@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

4 participants