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

Cosmos: Allow PK with just the partition key and using id as the partition key #21396

Closed
Paul-Beliavskis opened this issue Jun 24, 2020 · 3 comments · Fixed by #21405
Closed
Labels
area-cosmos 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

@Paul-Beliavskis
Copy link

When setting HasNoDiscriminator on the dbContext entity and the partitionkey and primary key are the same there is an error thrown when I try to add an entity to the context.

Steps to reproduce

My db context looks like this:

    public class UserDbContext : DbContext
    {
        public UserDbContext(DbContextOptions<UserDbContext> options)
            : base(options)
        {
        }

        public DbSet<User> Users { get; set; }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.HasDefaultContainer("Users");
            modelBuilder.Entity<User>()
                        .HasPartitionKey(u => u.UserId)
                        .HasNoDiscriminator();
        }
    }

Notice the HasNoDiscriminator is added and the partition key is the primary key.

Then when I add to this DBContext like so:
var newAdmin = await _userDbContext.Users.AddAsync(admin, cancellationToken);

I get an exception with the following stack trace:

   at System.Text.StringBuilder.Remove(Int32 startIndex, Int32 length)
   at Microsoft.EntityFrameworkCore.Cosmos.ValueGeneration.Internal.IdValueGenerator.NextValue(EntityEntry entry)
   at Microsoft.EntityFrameworkCore.ValueGeneration.ValueGenerator.NextValueAsync(EntityEntry entry, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.ValueGeneration.ValueGenerator.NextAsync(EntityEntry entry, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.ValueGenerationManager.<GenerateAsync>d__8.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.<SetEntityStateAsync>d__24.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityGraphAttacher.<PaintActionAsync>d__5.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityEntryGraphIterator.<TraverseGraphAsync>d__1`1.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.DbContext.<AddAsync>d__71`1.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Threading.Tasks.ValueTask`1.get_Result()
   at System.Runtime.CompilerServices.ValueTaskAwaiter`1.GetResult()
   at Panviva.Services.Configuration.Application.Handlers.RequestHandlers.CommandHandlers.PostOrganizationHandler.<Handle>d__8.MoveNext() in C:\Panviva\io-panviva-services-configuration\Panviva.Services.Configuration\Panviva.Services.Configuration.Application\Handlers\RequestHandlers\CommandHandlers\PostOrganizationHandler.cs:line 125
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at MediatR.Pipeline.RequestExceptionProcessorBehavior`2.<Handle>d__2.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at MediatR.Pipeline.RequestExceptionProcessorBehavior`2.<Handle>d__2.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at MediatR.Pipeline.RequestExceptionActionProcessorBehavior`2.<Handle>d__2.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at MediatR.Pipeline.RequestExceptionActionProcessorBehavior`2.<Handle>d__2.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at MediatR.Pipeline.RequestPostProcessorBehavior`2.<Handle>d__2.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at MediatR.Pipeline.RequestPreProcessorBehavior`2.<Handle>d__2.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at MediatR.Pipeline.RequestExceptionProcessorBehavior`2.<Handle>d__2.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at MediatR.Pipeline.RequestExceptionProcessorBehavior`2.<Handle>d__2.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at MediatR.Pipeline.RequestExceptionActionProcessorBehavior`2.<Handle>d__2.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at MediatR.Pipeline.RequestExceptionActionProcessorBehavior`2.<Handle>d__2.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at MediatR.Pipeline.RequestPostProcessorBehavior`2.<Handle>d__2.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at MediatR.Pipeline.RequestPreProcessorBehavior`2.<Handle>d__2.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at MediatR.Pipeline.RequestPreProcessorBehavior`2.<Handle>d__2.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at Panviva.Services.Configuration.Controllers.OrganizationsController.<PostOrganization>d__10.MoveNext() in C:\Panviva\io-panviva-services-configuration\Panviva.Services.Configuration\Panviva.Services.Configuration\Controllers\OrganizationsController.cs:line 207

After looking at the code in https://github.com/dotnet/efcore/blob/master/src/EFCore.Cosmos/ValueGeneration/Internal/IdValueGenerator.cs
I believe I have found the issue. I will create a pr for it.

Further technical details

EF Core version: 3.1.5
Database provider: (e.g. Microsoft.EntityFrameworkCore.CosmosDb)
Target framework: 3.1(e.g. .NET Core 3.0)
Operating system: Windows 10 Enterprise
IDE: Visual Studio 2019 16.6

@AndriySvyryd
Copy link
Member

This isn't a supported scenario. If the only property in the primary key is the partition key then the id value will always be the same as the partition key value meaning there will be only one item per partition resulting in very bad perf. You'd be better off not configuring the partition key at all.

@AndriySvyryd AndriySvyryd changed the title Ef Core 3.1.5 throwing an exception when using the cosmosDB provider and setting HasNoDiscriminator on the dbContext. Cosmos: throw when PK is only the partition key Jun 24, 2020
@AndriySvyryd AndriySvyryd self-assigned this Jun 24, 2020
@Paul-Beliavskis
Copy link
Author

Paul-Beliavskis commented Jun 24, 2020

This isn't a supported scenario. If the only property in the primary key is the partition key then the id value will always be the same as the partition key value meaning there will be only one item per partition resulting in very bad perf. You'd be better off not configuring the partition key at all.

But what if most of our transactions are simply looking up user objects by id. It was my understanding that cosmos would be able to go directly to that partition and get that object instead of searching through every partition if the id I am using to search for is the partition key. The documentation also states that the partition key should be as unique as possible:
https://docs.microsoft.com/en-us/azure/cosmos-db/partitioning-overview

Did I mis-interpret the doc?

Also in the case where you say it is not supported, why throw an exception that doesn't tell you anything about the fact that this scenario is not supported? The exception gets thrown line 69 in https://github.com/dotnet/efcore/blob/master/src/EFCore.Cosmos/ValueGeneration/Internal/IdValueGenerator.cs
When it tries to remove the pipe on the end of the string.

It's also quite interesting that when I set the partition key but do not set the HasNoDiscriminator then the id field has the value of the Discriminator which means many items end up having the same Id which doesn't make sense to me.

I propose either throwing an appropriate exception or just allowing the id field to contain the partition key.

@AndriySvyryd AndriySvyryd changed the title Cosmos: throw when PK is only the partition key Cosmos: Allow PK with just the partition key and using id as the partition key Jun 24, 2020
@AndriySvyryd
Copy link
Member

Did I mis-interpret the doc?

You read it correctly. It was updated since the last time I read it, we'll change our recommendation to match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-cosmos 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.

3 participants