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

InMemory: Improve in-memory key generation #6872

Closed
ardalis opened this issue Oct 26, 2016 · 59 comments
Closed

InMemory: Improve in-memory key generation #6872

ardalis opened this issue Oct 26, 2016 · 59 comments
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@ardalis
Copy link

ardalis commented Oct 26, 2016

The Issue

For testing purposes, you should be able to delete, recreate, and reseed InMemory databases and the result should be the same for each test. Currently identity columns do not reset, so IDs increment with each test iteration.

Repro Steps

This test fails. It should pass.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.DependencyInjection;
using Xunit;

namespace EfCoreInMemory
{
    public class Item
    {
        public int Id { get; set; }
        public string Name { get; set; }
    }
    public class AppDbContext : DbContext
    {
        public DbSet<Item> Items { get; set; }

        public AppDbContext(DbContextOptions<AppDbContext> options ):base(options)
        {
        }
    }
    public class Tests
    {
        private static DbContextOptions<AppDbContext> CreateNewContextOptions()
        {
            // Create a fresh service provider, and therefore a fresh 
            // InMemory database instance.
            var serviceProvider = new ServiceCollection()
                .AddEntityFrameworkInMemoryDatabase()
                .BuildServiceProvider();

            // Create a new options instance telling the context to use an
            // InMemory database and the new service provider.
            var builder = new DbContextOptionsBuilder<AppDbContext>();
            builder.UseInMemoryDatabase()
                   .UseInternalServiceProvider(serviceProvider);

            return builder.Options;
        }

        [Fact]
        public void Test1()
        {
            // create a brand new dbContext
            var dbContext = new AppDbContext(CreateNewContextOptions());

            // add one item
            var item = new Item();
            dbContext.Items.Add(item);
            dbContext.SaveChanges();

            // ID should be 1
            Assert.Equal(1, item.Id);

            dbContext.Database.EnsureDeleted();

            Assert.False(dbContext.Items.Any());

            var item2 = new Item();
            dbContext.Items.Add(item2);
            dbContext.SaveChanges();

            // ID should STILL be 1
            Assert.Equal(1, item2.Id);

        }
    }
}

Further technical details

Project.json:

{
  "version": "1.0.0-*",
  "testRunner": "xunit",
  "dependencies": {
    "NETStandard.Library": "1.6.0",
    "xunit": "2.2.0-beta2-build3300",
    "dotnet-test-xunit": "2.2.0-preview2-build1029",
    "Microsoft.AspNetCore.Server.Kestrel": "1.0.0",
    "Microsoft.AspNetCore.TestHost": "1.0.0",
    "Microsoft.EntityFrameworkCore": "1.0.0",
    "Microsoft.EntityFrameworkCore.InMemory": "1.0.0"
  },
  "frameworks": {
    "netcoreapp1.0": {
      "imports": [
        "dotnet5.6"
      ],
      "dependencies": {
        "Microsoft.NETCore.App": {
          "type": "platform",
          "version": "1.0.0"
        }
      }
    }
  }
}

VS2015

@robertmclaws
Copy link

I don't know if I agree with this exact behavior, mostly because I'm having a hard time envisioning a unit test or operational scenario where you'd want to delete an existing item, then add a new item with the same ID.

HOWEVER, I do see the need for a command like dbContext.ResetIdTracking() that frees up any IDs not presently in the change tracker. You could then in this command at the end of each test function to reset the context for the next test.

@ardalis
Copy link
Author

ardalis commented Oct 26, 2016

The repel shows one test. The problem exhibits itself when you have many tests. Every test will end up with different IDs - there is now way to really reset the in memory db.

Steve Smith

On Oct 26, 2016, at 15:26, Robert McLaws (Microsoft MVP) notifications@github.com wrote:

I don't know if I agree with this exact behavior, mostly because I'm having a hard time envisioning a unit test or operational scenario where you'd want to delete an existing item, then add a new item with the same ID.

HOWEVER, I do see the need for a command like dbContext.ResetIdTracking() that frees up any IDs not presently in the change tracker. You could then in this command at the end of each test function to reset the context for the next test.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@ardalis
Copy link
Author

ardalis commented Oct 26, 2016

repro not repel

@ajcvickers
Copy link
Member

Note for triage: dupe of #4096.

@rowanmiller
Copy link
Contributor

The InMemory provider doesn't the Identity pattern you would get in a relational database. When a column is configured as ValueGeneratedOnAdd it uses values from a single generator for all tables. This would be similar to having all tables setup to use a single sequence in a relational database. This raises one of the important design principles of our InMemory provider, which is that we are not trying to emulate a relational database. This becomes more important as we start to light up non-relational providers.

If you want to have a database that acts like a relational database, but doesn't have the overhead of I/O, then we'd recommend using an In-Memory SQLite database - http://www.sqlite.org/inmemorydb.html.

We're going to update our testing docs to make the SQLite option more prominent.

@ardalis
Copy link
Author

ardalis commented Oct 31, 2016

Thanks, @rowanmiller

2 things I'd like to see that I think would be useful:

  • A way to 100% reset the in-memory database, to use in testing. I don't care whether it behaves like a relational database or not - for test purposes I want a way to reset its state completely.
  • A testing sample that shows not just how to test EF directly, but how to test an app (like an ASP.NET Core app) that is using EF, resetting the database after each test. The current guidance on how to do this is not trivial to achieve using ASP.NET Core's standard Startup.ConfigureServices code used to configure EF.

Thanks!

@rowanmiller rowanmiller reopened this Oct 31, 2016
@rowanmiller
Copy link
Contributor

@ardalis fair point on point 1, I'm re-opening this issue for us to re-discuss.

Regarding point 2, our docs show an example that tests a BlogService, which would be the same as testing an MVC controller etc. - https://docs.efproject.net/en/latest/miscellaneous/testing.html. For testing, you generally wouldn't be using Startup.cs to setup all your services, you'd be creating your test doubles of everything the controller depends on and passing them in. The example I linked to shows how to do that for EF, but you'd have to do it for the other dependencies too.

@robertmclaws
Copy link

I do want to point out that I made a suggestion on how to handle what @ardalis is asking for in point 1 with a command like dbContext.ResetIdTracking(), or something to that effect.

@ardalis
Copy link
Author

ardalis commented Oct 31, 2016

@rowanmiller You're right, unit testing an individual controller action works just like your BlogService sample. To clarify, I'm talking about integration testing using WebHostBuilder, as shown here: https://docs.asp.net/en/latest/testing/integration-testing.html. In this case, there's no direct access to the controller action. The web host is constructed for each test and the request is made such that it goes through the full MVC stack (routing, filters, etc). This is an awesome new feature of ASP.NET Core but getting the data reset for each test has proven much more difficult than I would like. To be fair, I haven't yet resorted to using SqlLite for this purpose, but my hope has been to avoid the need for this since it adds complexity to the process. Thanks.

@ardalis
Copy link
Author

ardalis commented Oct 31, 2016

...and regarding that link, you'll see it doesn't include any EF examples. That's because I couldn't figure a great way to include them such that they were reset between each test.

@ardalis
Copy link
Author

ardalis commented Oct 31, 2016

Here's a complete example showing how to (try to) configure an ASP.NET Core app using WebHostBuilder and InMemory for integration tests: https://github.com/ardalis/CleanArchitecture/blob/master/src/CleanArchitecture.Web/Startup.cs#L73-L104

The sample shown works, but starts to fall down as you add more entities and tests to it, as test data starts to step on other test data. One workaround I was able to use was hardcoding the IDs of test data, as suggested in another issue. However, any guidance or assistance with how to reset data for each integration test like this would be appreciated.

@rowanmiller
Copy link
Contributor

@ardalis what if you used a throw away instance of the InMemory database for each test? There is an overload of UseInMemoryDatabase that takes a string (the named instance to connect to).

@ardalis
Copy link
Author

ardalis commented Oct 31, 2016

I tried that but the numbers still incremented every time. I ended up working around it by hard-coding the initial ID. I had tried this:

        services.AddDbContext<AppDbContext>(options =>
            options.UseInMemoryDatabase(Guid.NewGuid().ToString()));

But even with that in place, and with disposing of the TestServer after every test, the ID numbers would continue incrementing between tests (which made it hard for me to construct API calls like /item/{id} when I couldn't count on a valid test ID to use).

Again this was fixed by specifying the ID in my test data method. A working version of what I have currently can be found here:
https://github.com/ardalis/ddd-guestbook/tree/Lab5Start

@rowanmiller
Copy link
Contributor

rowanmiller commented Nov 2, 2016

Note for triage: One idea we had in triage was replacing some services to make the value generator resettable, but it involves copying a lot of code because the value generator is cached - and can't be replaced. You have to re-implement InMemoryValueGeneratorSelector to keep a handle of the created generators (so that you can reset them) and then re-implement InMemoryIntegerValueGeneratorFactory and InMemoryIntegerValueGenerator to make them resettable. You have to copy the code, as the public surface doesn't give you the hooks you need.

@ajcvickers
Copy link
Member

@rowanmiller Does this work:

modelBuilder
    .Entity<Blog>()
    .Property(e => e.Id)
    .HasValueGenerator<InMemoryIntegerValueGenerator<int>>();

@ajcvickers
Copy link
Member

@ardalis @robertmclaws Here is some code that might work for you:

public static class DbContextExtensions
{
    public static void ResetValueGenerators(this DbContext context)
    {
        var cache = context.GetService<IValueGeneratorCache>();

        foreach (var keyProperty in context.Model.GetEntityTypes()
            .Select(e => e.FindPrimaryKey().Properties[0])
            .Where(p => p.ClrType == typeof(int)
                        && p.ValueGenerated == ValueGenerated.OnAdd))
        {
            var generator = (ResettableValueGenerator)cache.GetOrAdd(
                keyProperty,
                keyProperty.DeclaringEntityType,
                (p, e) => new ResettableValueGenerator());

            generator.Reset();
        }
    }
}

public class ResettableValueGenerator : ValueGenerator<int>
{
    private int _current;

    public override bool GeneratesTemporaryValues => false;

    public override int Next(EntityEntry entry)
        => Interlocked.Increment(ref _current);

    public void Reset() => _current = 0;
}

To use, call context.ResetValueGenerators(); before the context is used for the first time and any time that EnsureDeleted is called. For example:

using (var context = new BlogContext())
{
    context.ResetValueGenerators();
    context.Database.EnsureDeleted();

    context.Posts.Add(new Post {Title = "Open source FTW", Blog = new Blog {Title = "One Unicorn"}});
    context.SaveChanges();
}

No matter how many times I call this code, Blog.Id and Post.Id will always get the value 1.

The code works by finding every int primary key in the model and setting the cached value generator to a ResettableValueGenerator, or resetting that value generator if it has already been created. It can be easily adapted for other key types.

@divega divega added this to the Backlog milestone Nov 4, 2016
@divega
Copy link
Contributor

divega commented Nov 4, 2016

Moving to backlog to consider:

  1. Automatically resetting value generators when in-memory database is dropped (value generators would need to be associated with the database instead of the model)
  2. Add identity behavior on the in-memory database (and stop emulating it with client-side value generation)
  3. Add an explicit API to reset value generation in the product.

@rowanmiller rowanmiller changed the title Database.EnsureDeleted() should reset any/all ID identity columns for InMemoryDatabase InMemory: Ability to reset "identity" values (maybe automatically during Database.EnsureDeleted()) Nov 4, 2016
@brockallen
Copy link

FYI I ran into this today where the state of the in-mem database is preserved across each run of my unit tests (and each test creates a whole new DI system per test with a new ServiceCollection, etc.). I can't imagine I'd ever want to preserve the old data across each test, since unit tests don't run in a consistent order, and they might run in parallel.

This behavior seems like a strange choice by default if this is being positioned for unit testing. Why isn't the in-mem database just a singleton when added to DI?

As a workaround I'm using Guid.NewGuid() to set the database name on each run, but it feels uncomfortable to have to do so.

@ErikEJ
Copy link
Contributor

ErikEJ commented Dec 28, 2016

@brockallen The InMemory provider is not a relational database provider, current recommendation is to use SQLite (or maybe SQL Compact?) instead: https://docs.microsoft.com/en-us/ef/core/miscellaneous/testing/

@brockallen
Copy link

@ErikEJ I'm confused how that relates. I don't care what DB is used -- IMO, between tests it should start from scratch/empty.

@RichiCoder1
Copy link

RichiCoder1 commented Mar 10, 2017

bump
Ran into this ourselves. I'm also baffled that this is the default behavior. What is the intended use case of InMemory if not testing?

@spottedmahn
Copy link

@rowanmiller

We're going to update our testing docs to make the SQLite option more prominent.

I started with the SQLite user guide but quickly stopped once I learned schemas are not supported.

@TAGC
Copy link

TAGC commented Apr 14, 2019

I ran into this issue and was led down a rabbit hole that culminated in this thread. I agree with everyone else here - unit tests are supposed to be completely independent of each other and therefore able to run in any order without any change in test results. There absolutely should be a way to completely reset all EF Core state between unit tests for this reason, independent of what sort of database provider you're trying to simulate.

In my case, I'm trying to verify my application service logic by performing deep equality comparisons between entities. The persistence of primary key auto-generation state between unit tests is screwing up these comparisons when comparing the entity IDs:

 DeepEqual.Syntax.DeepEqualException : Comparison Failed: The following 3 differences were found.
	Actual[0].Id != Expected[0].Id (4 != 1)
	Actual[1].Id != Expected[1].Id (5 != 2)
	Actual[2].Id != Expected[2].Id (6 != 3)
Stack Trace:

@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview4 Apr 15, 2019
@JRBonnema
Copy link

I tried to test this issue using 3.0.0 preview5 but I ran into the issue described in #14906 (Could not load type 'System.Collections.Generic.IAsyncEnumerable'1' from assembly 'System.Interactive.Async, Version=4.0.0.0), so in my case testing was not possible.

@denmitchell
Copy link

@DanNSam, I like your workaround. I encountered a similar issue and ended up creating a MaxPlusOneValueGenerator that works with Entities that implement IHasIntegerId (a public int Id property).

@ajcvickers, is Microsoft still looking at possible a long-term solution for this?

@ajcvickers
Copy link
Member

@denmitchell As indicated by the Closed state and the closed-fixed label, this issue has been closed as fixed. As indicated by the milestone 3.0.0-preview4, it was first included in the EF Core 3.0 preview 4 release.

@eskensberg
Copy link

#6872 (comment) is great (thanks @ajcvickers), but... turns out that it does not work with tests running in parallel.

I've ended up fixing it by providing a custom IServiceProvider to UseInternalServiceProvider that declares IValueGeneratorCache as instance per test run.

There's an example building the custom IServiceProvider using Autofac. Autofac supports named scopes making it reasonably easy to set up:

public class Test : IDisposable
{
    static readonly Lazy<IContainer> container = new Lazy<IContainer>(() =>
    {
        var collection = new ServiceCollection()
            .AddEntityFrameworkInMemoryDatabase();

        // replacing IValueGeneratorCache lifetime from singleton to instance per test run
        var valueGeneratorCacheDescriptor = collection.First(x => x.ServiceType == typeof(IValueGeneratorCache));

        collection.Remove(valueGeneratorCacheDescriptor);

        var builder = new ContainerBuilder();
        builder.Populate(collection);

        builder
            .RegisterType(valueGeneratorCacheDescriptor.ImplementationType)
            .InstancePerMatchingLifetimeScope("testrun")
            .As(valueGeneratorCacheDescriptor.ServiceType);

        return builder.Build();
    });

    readonly ILifetimeScope scope;
    readonly DbContext dbContext;

    public Test()
    {
        scope = container.Value.BeginLifetimeScope("testrun");

        var serviceProvider = scope.Resolve<IServiceProvider>();

        var optionsBuilder = new DbContextOptionsBuilder<CoreDbContext>();
        optionsBuilder.UseInMemoryDatabase(Guid.NewGuid().ToString("D"));
        optionsBuilder.UseInternalServiceProvider(serviceProvider);

        dbContext = new DbContext(optionsBuilder.Options);
    }

    public void TestRun()
    {
        // ...
    }

    public void Dispose()
    {
        dbContext?.Dispose();
        scope?.Dispose();
    }
}

what would I go around using this snippet?
Do I have to wrap each test using it and run my logic inside the test?

@ajcvickers
Copy link
Member

@eskensberg Can you file a new issue describing the problem when running tests in parallel?

@wmcainsh
Copy link

wmcainsh commented Aug 8, 2019

@DanNSam, I like your workaround. I encountered a similar issue and ended up creating a MaxPlusOneValueGenerator that works with Entities that implement IHasIntegerId (a public int Id property).

For anyone experiencing the auto-increment issue who doesn't use a column called Id on every table, I slightly modified the solution provided by @denmitchell

    public override int Next(EntityEntry entry)
    {
        var context = entry.Context;
        var qry = generic.Invoke(context, null) as DbSet<TEntity>;

        var key1Name = entry.Metadata
                            .FindPrimaryKey()
                            .Properties
                            .First()
                            .Name;

        var currentMax = qry.Max(e =>
            (int)e.GetType()
                  .GetProperty(key1Name)
                  .GetValue(e));

        //return max plus one
        return currentMax + 1;
    }

@ardalis
Copy link
Author

ardalis commented Sep 27, 2019

This still is not working 100% for me when I try to reproduce the original behavior at the top of this issue. That is, trying to add an item, deleting the database, and then trying to add a separate new instance of the same entity. I get an InvalidOperationException saying the item is already being tracked:

image

The good news is that the new entity's ID is 1 as it should be. In my test I'm using the same dbContext before and after calling EnsureDeleted - maybe that's the unsupported scenario because the identitymap in the dbcontext still has a reference to the now-deleted entity from before I called EnsureDeleted.

Repro is here: https://github.com/ardalis/EFCoreFeatureTests/blob/ced23657c3c9257758d6734104fb3fc0f0562c25/EFCoreFeatureTests/UnitTest1.cs

@smitpatel
Copy link
Member

@ardalis - Filed #18103 because cause of that issue is different from value generation.

@aherrick
Copy link

aherrick commented Oct 3, 2019

Is this broken in EF Core 3? Not sure I'm seeing this working and having trouble resetting my in memory DB. I'm wondering if similar to here:

#18187

@RobK410
Copy link

RobK410 commented Oct 4, 2019

I see usage of TryAddSingleton in the bowels of the InMemory code. I think the problem as I can understand it, is that internally the in memory database functionality uses singletons. Are they not thread safe? Therefore, for parallel tests, or tests that execute multi-threaded, run risk of non-thread safe issues. Is this correct?

I found this to be the case when I wrote my own inmemory db abstraction for .NET Framework EF6

@AndriySvyryd
Copy link
Member

@vasont That's likely a duplicate of #17672

@ajcvickers ajcvickers modified the milestones: 3.0.0-preview4, 3.0.0 Nov 11, 2019
@saliksaly
Copy link

@ardalis @robertmclaws Here is some code that might work for you:

public static class DbContextExtensions
{
    public static void ResetValueGenerators(this DbContext context)
    {
        var cache = context.GetService<IValueGeneratorCache>();

        foreach (var keyProperty in context.Model.GetEntityTypes()
            .Select(e => e.FindPrimaryKey().Properties[0])
            .Where(p => p.ClrType == typeof(int)
                        && p.ValueGenerated == ValueGenerated.OnAdd))
        {
            var generator = (ResettableValueGenerator)cache.GetOrAdd(
                keyProperty,
                keyProperty.DeclaringEntityType,
                (p, e) => new ResettableValueGenerator());

            generator.Reset();
        }
    }
}

public class ResettableValueGenerator : ValueGenerator<int>
{
    private int _current;

    public override bool GeneratesTemporaryValues => false;

    public override int Next(EntityEntry entry)
        => Interlocked.Increment(ref _current);

    public void Reset() => _current = 0;
}

To use, call context.ResetValueGenerators(); before the context is used for the first time and any time that EnsureDeleted is called. For example:

using (var context = new BlogContext())
{
    context.ResetValueGenerators();
    context.Database.EnsureDeleted();

    context.Posts.Add(new Post {Title = "Open source FTW", Blog = new Blog {Title = "One Unicorn"}});
    context.SaveChanges();
}

No matter how many times I call this code, Blog.Id and Post.Id will always get the value 1.

The code works by finding every int primary key in the model and setting the cached value generator to a ResettableValueGenerator, or resetting that value generator if it has already been created. It can be easily adapted for other key types.

@ajcvickers, please - I have been using your solution to set id value generators for my in-emory tests with seed data. Now, in EF Core 3.1, it stoped working. It seems that ResettableValueGenerator.Next() is newer called.
Again, I get error: System.InvalidOperationException: 'The instance of entity type X cannot be tracked because another instance with the key value '{Id: 1}' is already being tracked.

Would not you know solution for EF Core 3.1?

Thanks

@ajcvickers
Copy link
Member

@saliksaly It should no longer be necessary to use a workaround here since the underlying issues were fixed in 3.0. If you're running into issues with in-memory keys, then please open a new issue and include a small, runnable project or complete code listing so that we can investigate.

@saliksaly
Copy link

@saliksaly It should no longer be necessary to use a workaround here since the underlying issues were fixed in 3.0. If you're running into issues with in-memory keys, then please open a new issue and include a small, runnable project or complete code listing so that we can investigate.

Thanks, here is the issue: #19854

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

No branches or pull requests