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.

Sign up for GitHub

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

Jump to bottom

Cosmos provider builds/uses wrong model when keys have value converters #19638

Closed
ajcvickers opened this issue Jan 19, 2020 · 4 comments · Fixed by #19644
Closed

Cosmos provider builds/uses wrong model when keys have value converters #19638

ajcvickers opened this issue Jan 19, 2020 · 4 comments · Fixed by #19644
Labels
area-cosmos closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone
5.0.0

Comments

@ajcvickers
Copy link
Member

ajcvickers commented Jan 19, 2020 •
edited
Loading

See KeysWithConvertersCosmosTest.

The same tests with the SQL Server provider build a model where the entity types look like this:

  EntityType: BytesStructKeyOptionalDependent
    Properties: 
      Id (BytesStructKey) Required PK AfterSave:Throw
      PrincipalId (Nullable<BytesStructKey>) FK Index
    Navigations: 
      Principal (BytesStructKeyPrincipal) ToPrincipal BytesStructKeyPrincipal Inverse: OptionalDependents
    Keys: 
      Id PK
    Foreign keys: 
      BytesStructKeyOptionalDependent {'PrincipalId'} -> BytesStructKeyPrincipal {'Id'} ToDependent: OptionalDependents ToPrincipal: Principal
    Indexes: 
      PrincipalId
  EntityType: BytesStructKeyPrincipal
    Properties: 
      Id (BytesStructKey) Required PK AfterSave:Throw
      Foo (string)
    Navigations: 
      OptionalDependents (ICollection<BytesStructKeyOptionalDependent>) Collection ToDependent BytesStructKeyOptionalDependent Inverse: Principal
      RequiredDependents (ICollection<BytesStructKeyRequiredDependent>) Collection ToDependent BytesStructKeyRequiredDependent Inverse: Principal
    Keys: 
      Id PK
  EntityType: BytesStructKeyRequiredDependent
    Properties: 
      Id (BytesStructKey) Required PK AfterSave:Throw
      PrincipalId (BytesStructKey) Required FK Index
    Navigations: 
      Principal (BytesStructKeyPrincipal) ToPrincipal BytesStructKeyPrincipal Inverse: RequiredDependents
    Keys: 
      Id PK
    Foreign keys: 
      BytesStructKeyRequiredDependent {'PrincipalId'} -> BytesStructKeyPrincipal {'Id'} ToDependent: RequiredDependents ToPrincipal: Principal
    Indexes: 
      PrincipalId

The Cosmos model either has an erroneous string AK property on each entity type:

Edit: The model is as expected: https://docs.microsoft.com/en-us/ef/core/providers/cosmos/?tabs=dotnet-core-cli#working-with-disconnected-entities

  EntityType: BytesStructKeyOptionalDependent
    Properties: 
      Id (BytesStructKey) Required PK AfterSave:Throw
      Discriminator (no field, string) Shadow Required
      PrincipalId (Nullable<BytesStructKey>) FK Index
      __jObject (no field, JObject) Shadow BeforeSave:Ignore AfterSave:Ignore ValueGenerated.OnAddOrUpdate
      id (no field, string) Shadow Required AlternateKey AfterSave:Throw
    Navigations: 
      Principal (BytesStructKeyPrincipal) ToPrincipal BytesStructKeyPrincipal Inverse: OptionalDependents
    Keys: 
      Id PK
      id
    Foreign keys: 
      BytesStructKeyOptionalDependent {'PrincipalId'} -> BytesStructKeyPrincipal {'Id'} ToDependent: OptionalDependents ToPrincipal: Principal
    Indexes: 
      PrincipalId
  EntityType: BytesStructKeyPrincipal
    Properties: 
      Id (BytesStructKey) Required PK AfterSave:Throw
      Discriminator (no field, string) Shadow Required
      Foo (string)
      __jObject (no field, JObject) Shadow BeforeSave:Ignore AfterSave:Ignore ValueGenerated.OnAddOrUpdate
      id (no field, string) Shadow Required AlternateKey AfterSave:Throw
    Navigations: 
      OptionalDependents (ICollection<BytesStructKeyOptionalDependent>) Collection ToDependent BytesStructKeyOptionalDependent Inverse: Principal
      RequiredDependents (ICollection<BytesStructKeyRequiredDependent>) Collection ToDependent BytesStructKeyRequiredDependent Inverse: Principal
    Keys: 
      Id PK
      id
  EntityType: BytesStructKeyRequiredDependent
    Properties: 
      Id (BytesStructKey) Required PK AfterSave:Throw
      Discriminator (no field, string) Shadow Required
      PrincipalId (BytesStructKey) Required FK Index
      __jObject (no field, JObject) Shadow BeforeSave:Ignore AfterSave:Ignore ValueGenerated.OnAddOrUpdate
      id (no field, string) Shadow Required AlternateKey AfterSave:Throw
    Navigations: 
      Principal (BytesStructKeyPrincipal) ToPrincipal BytesStructKeyPrincipal Inverse: RequiredDependents
    Keys: 
      Id PK
      id
    Foreign keys: 
      BytesStructKeyRequiredDependent {'PrincipalId'} -> BytesStructKeyPrincipal {'Id'} ToDependent: RequiredDependents ToPrincipal: Principal
    Indexes: 
      PrincipalId

Or something is wrong about how it is being used:

Microsoft.EntityFrameworkCore.Cosmos.KeysWithConvertersCosmosTest.Can_insert_and_read_back_with_struct_key_and_optional_dependents

System.InvalidOperationException : The instance of entity type 'IntStructKeyPrincipal' cannot be tracked because another instance with the key value '{id: IntStructKeyPrincipal|Microsoft.EntityFrameworkCore.KeysWithConvertersTestBase`1+IntStructKey[Microsoft.EntityFrameworkCore.Cosmos.KeysWithConvertersCosmosTest+KeysWithConvertersCosmosFixture]}' is already being tracked. When attaching existing entities, ensure that only one entity instance with a given key value is attached.
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.IdentityMap`1.ThrowIdentityConflict(InternalEntityEntry entry) in C:\aspnet\EntityFrameworkCore\src\EFCore\ChangeTracking\Internal\IdentityMap.cs:line 258
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.IdentityMap`1.Add(TKey key, InternalEntityEntry entry, Boolean updateDuplicate) in C:\aspnet\EntityFrameworkCore\src\EFCore\ChangeTracking\Internal\IdentityMap.cs:line 278
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.IdentityMap`1.Add(TKey key, InternalEntityEntry entry) in C:\aspnet\EntityFrameworkCore\src\EFCore\ChangeTracking\Internal\IdentityMap.cs:line 230
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.NullableKeyIdentityMap`1.Add(InternalEntityEntry entry) in C:\aspnet\EntityFrameworkCore\src\EFCore\ChangeTracking\Internal\NullableKeyIdentityMap.cs:line 59
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.StartTracking(InternalEntityEntry entry) in C:\aspnet\EntityFrameworkCore\src\EFCore\ChangeTracking\Internal\StateManager.cs:line 574
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.SetEntityState(EntityState oldState, EntityState newState, Boolean acceptChanges, Boolean modifyProperties) in C:\aspnet\EntityFrameworkCore\src\EFCore\ChangeTracking\Internal\InternalEntityEntry.cs:line 296
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.SetEntityState(EntityState entityState, Boolean acceptChanges, Boolean modifyProperties, Nullable`1 forceStateWhenUnknownKey) in C:\aspnet\EntityFrameworkCore\src\EFCore\ChangeTracking\Internal\InternalEntityEntry.cs:line 142
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityGraphAttacher.PaintAction(EntityEntryGraphNode`1 node) in C:\aspnet\EntityFrameworkCore\src\EFCore\ChangeTracking\Internal\EntityGraphAttacher.cs:line 94
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityEntryGraphIterator.TraverseGraph[TState](EntityEntryGraphNode`1 node, Func`2 handleNode) in C:\aspnet\EntityFrameworkCore\src\EFCore\ChangeTracking\Internal\EntityEntryGraphIterator.cs:line 39
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityGraphAttacher.AttachGraph(InternalEntityEntry rootEntry, EntityState targetState, EntityState storeGeneratedWithKeySetTargetState, Boolean forceStateWhenUnknownKey) in C:\aspnet\EntityFrameworkCore\src\EFCore\ChangeTracking\Internal\EntityGraphAttacher.cs:line 50
   at Microsoft.EntityFrameworkCore.DbContext.SetEntityState(InternalEntityEntry entry, EntityState entityState) in C:\aspnet\EntityFrameworkCore\src\EFCore\DbContext.cs:line 800
   at Microsoft.EntityFrameworkCore.DbContext.SetEntityStates(IEnumerable`1 entities, EntityState entityState) in C:\aspnet\EntityFrameworkCore\src\EFCore\DbContext.cs:line 1372
   at Microsoft.EntityFrameworkCore.DbContext.AddRange(IEnumerable`1 entities) in C:\aspnet\EntityFrameworkCore\src\EFCore\DbContext.cs:line 1386
   at Microsoft.EntityFrameworkCore.DbContext.AddRange(Object[] entities) in C:\aspnet\EntityFrameworkCore\src\EFCore\DbContext.cs:line 1244
   at Microsoft.EntityFrameworkCore.Internal.InternalDbSet`1.AddRange(TEntity[] entities) in C:\aspnet\EntityFrameworkCore\src\EFCore\Internal\InternalDbSet.cs:line 220
   at Microsoft.EntityFrameworkCore.KeysWithConvertersTestBase`1.InsertOptionalGraph[TPrincipal,TDependent]() in C:\aspnet\EntityFrameworkCore\test\EFCore.Specification.Tests\KeysWithConvertersTestBase.cs:line 1540
   at Microsoft.EntityFrameworkCore.KeysWithConvertersTestBase`1.Can_insert_and_read_back_with_struct_key_and_optional_dependents() in C:\aspnet\EntityFrameworkCore\test\EFCore.Specification.Tests\KeysWithConvertersTestBase.cs:line 27
ajcvickers added a commit that referenced this issue Jan 19, 2020
@ajcvickers
Robust ordering on key properties
f0e5dd4 
Fixes #13507

See also #19638 and #19641

Basic logic is:
* If model type implements some sort of IComparable, then use it
* Otherwise, if provider type implement some sore of IComparable, then use it
* Otherwise fail in model validation

Also added new test suite specifically for different types of keys with and without conversions.
@ajcvickers ajcvickers mentioned this issue Jan 19, 2020
Closed
@AndriySvyryd
Copy link
Member

This is by design see https://docs.microsoft.com/en-us/ef/core/providers/cosmos/?tabs=dotnet-core-cli#working-with-disconnected-entities
Related: #15289

@ajcvickers
Copy link
Member Author

ajcvickers commented Jan 19, 2020 •
edited
Loading

@AndriySvyryd The entities here are being Added and then saved, so the value should get generated.

By default EF Core generates the value by concatenating the discriminator and the primary key values, using '|' as a delimiter.

How do key values get turned into strings?

@AndriySvyryd
Copy link
Member

@ajcvickers https://github.com/dotnet/efcore/blob/0d76bbf45a42148924b413ef8f37bf49c1ce10d3/src/EFCore.Cosmos/ValueGeneration/Internal/IdValueGenerator.cs

ajcvickers added a commit that referenced this issue Jan 20, 2020
@ajcvickers
Robust ordering on key properties
45fee53 
Fixes #13507
Fixes #19638

See also #19641

Basic logic is:
* If model type implements some sort of IComparable, then use it
* Otherwise, if provider type implement some sore of IComparable, then use it
* Otherwise fail in model validation

Also added new test suite specifically for different types of keys with and without conversions.
@ajcvickers ajcvickers mentioned this issue Jan 20, 2020
Merged
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jan 20, 2020
@ajcvickers ajcvickers self-assigned this Jan 20, 2020
@ajcvickers ajcvickers added this to the 5.0.0 milestone Jan 20, 2020
@ajcvickers
Copy link
Member Author

Fixed issue by using provider values to generate the strings. These are much more likely to generate sane values when converted to strings.

ajcvickers added a commit that referenced this issue Jan 26, 2020
@ajcvickers
Robust ordering on key properties
f03be5d 
Fixes #13507
Fixes #19638

See also #19641

Basic logic is:
* If model type implements some sort of IComparable, then use it
* Otherwise, if provider type implement some sore of IComparable, then use it
* Otherwise fail in model validation

Also added new test suite specifically for different types of keys with and without conversions.
ajcvickers added a commit that referenced this issue Jan 26, 2020
@ajcvickers
Robust ordering on key properties
a355454 
Fixes #13507
Fixes #19638

See also #19641

Basic logic is:
* If model type implements some sort of IComparable, then use it
* Otherwise, if provider type implement some sore of IComparable, then use it
* Otherwise fail in model validation

Also added new test suite specifically for different types of keys with and without conversions.
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview1 Mar 13, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview1, 5.0.0 Nov 7, 2020
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-cosmos closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Milestone
5.0.0
Development

Successfully merging a pull request may close this issue.

Robust ordering on key properties dotnet/efcore
Robust ordering on key properties dotnet/efcore
2 participants
@ajcvickers @AndriySvyryd