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

Allow model building outside of OnModelCreating #11738

Closed
jagalves opened this issue Apr 19, 2018 · 39 comments
Closed

Allow model building outside of OnModelCreating #11738

jagalves opened this issue Apr 19, 2018 · 39 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. regression type-enhancement
Milestone

Comments

@jagalves
Copy link

With EFCore 2, in order to have all mappings explicit, I create the model with an empty ConventionSet.

This scenario causes an exception with EFCore 2.1 (both preview1 and preview2)

Don't know if this is an actual bug or just something that I'm required to add as a convention in order to have a 100% by code mapping. Maybe there's some documentation that I can't find?

System.ArgumentNullException : Value cannot be null.
Parameter name: relationalTypeMapping
Microsoft.EntityFrameworkCore.Utilities.Check.NotNull[T](T value, String parameterName)
Microsoft.EntityFrameworkCore.Storage.Internal.TypeMappedRelationalParameter..ctor(String invariantName, String name, RelationalTypeMapping relationalTypeMapping, Nullable`1 nullable)
Microsoft.EntityFrameworkCore.Storage.Internal.RelationalParameterBuilder.AddParameter(String invariantName, String name, IProperty property)
Microsoft.EntityFrameworkCore.Storage.RelationalCommandBuilderExtensions.AddParameter(IRelationalCommandBuilder commandBuilder, String invariantName, String name, IProperty property)
Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.CreateStoreCommand()
Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.Execute(IRelationalConnection connection)
Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.Execute(DbContext _, ValueTuple`2 parameters)
Microsoft.EntityFrameworkCore.Storage.Internal.NoopExecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 verifySucceeded)
Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.Execute(IEnumerable`1 commandBatches, IRelationalConnection connection)
Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(IReadOnlyList`1 entriesToSave)
Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(Boolean acceptAllChangesOnSuccess)
Microsoft.EntityFrameworkCore.DbContext.SaveChanges(Boolean acceptAllChangesOnSuccess)
TestProject.UnitTest1.Test1() in UnitTest1.cs

Steps to reproduce

Test code attached. Switch between EFCore 2 and 2.1 preview in order to see the different behavior.

TestProject.zip

Further technical details

EF Core version: 2.1 preview 2
Database Provider: Sqlite
Operating system: window 10 1803 17134.1 (and previous)
IDE: at the moment 15.7 preview 4

@ajcvickers
Copy link
Contributor

@jagalves When using an empty convention set the model must still be constructed in a valid way--that is, with all required configuration including everything that is normally done by convention. In this case, there is a convention that creates type mappings, so if you don't run it, then you'll need to manually configure the type mapping for every property.

In general, it's hard to be sure everything is configured correctly and that the model is valid if you don't let any conventions run. We don't have any documentation on this because it is extremely advanced and not something we recommend people to do.

@jagalves
Copy link
Author

Well, this scenario worked fine in 2.0... I have a full complex model over which I have full control on how the mapping is defined and how my domain classes and my database tables are created/related. I'm trying to avoid cases where I generate migrations and then I have to go back and add overrides (like in my example userBuilder->Ignore(x=>x.NotMapped)) to avoid having columns and tables created by accident. This scenario is even worse if you use some other tool to do the migrations, by the way.

In other words, I'm trying to use what EF has to offer me, not override what it's trying to guess on my behalf. Is it possible to do that with the example code I provided?

@ajcvickers
Copy link
Contributor

@jagalves By convention, EF asks the provider for the most appropriate type mapping and annotates the property with that type mapping. If you're not happy with this convention, then you will need to create the type mappings explicitly and annotate them on the model. If you are okay with the type mapper "guessing" the type mapping, then you could ask the type mapper explicitly for each mapping and annotate the model from that...but then you would be doing exactly what the convention does. So I suppose this all depends on the definition of "guess".

@jagalves
Copy link
Author

If you're not happy with this convention,

It's not about being "happy". My scenario worked before and now it won't work anymore so I reported it because it could very well be a bug or something the team overlooked.

In my particular test case, is there something I can do to make it work? Like adding just that convention manually or removing the ones that would add "NotMapped" or any referenced classes to the model automatically. The type mapping convention seems to be required because explicitly setting the type with HasColumnType still fails.

If you believe the ticket is invalid or an unreasonable request you can close it.

@ajcvickers
Copy link
Contributor

@jagalves The issue is still open because it hasn't been triaged yet.

There are no conventions that will add types marked with NotMapped to the model. If you are seeing this, then please file an issue with full details so that we can investigate.

Column types are not the same as type mappings. Here is the code that in the convention:

foreach (var property in modelBuilder.Metadata.GetEntityTypes().SelectMany(e => e.GetDeclaredProperties()))
{
    property.Builder.HasAnnotation(
        CoreAnnotationNames.TypeMapping,
        _typeMappingSource.FindMapping(property),
        ConfigurationSource.Convention);
}

This is what you will need to do manually if not using the convention.

You can do things with conventions to create a set with just the ones you want, but this requires using internal code since conventions are not public yet, and still requires an advanced understanding of the EF Core model to ensure that you are building a valid model without using certain conventions.

@jagalves
Copy link
Author

@ajcvickers

There are no conventions that will add types marked with NotMapped to the model. If you are seeing this, then please file an issue with full details so that we can investigate.

I called it that to be obvious to whoever reads the code that I don't want it to be mapped if I didn't do it explicitly. If you change the code to keep the default conventions (override OnModelCreating and call GetModel from there with the provided modelbuilder), the test will fail in the assert that checks that it's null. Same thing for properties with a class type: they will throw because I didn't set the primary key. And then I have to go to the mapping add them to a list of the ones I'm ignoring. I had a considerable list before I found out clearing ConventionSets was possible.

I'll try tomorrow your suggestion. Too late to think in my timezone. Thanks.

@ajcvickers
Copy link
Contributor

Notes from triage:

  • General consensus is that building a model and getting it "right" without any conventions is currently extremely difficult and not likely to be a pit of success
  • Putting this on the backlog to consider what to do about this once conventions are made public (tracked by Allow applications to create model building conventions (aka public conventions) #214)
  • There are at least two kinds of conventions--those that detect and impact the structure of the model, and those that ensure other parts of the model are then configured appropriately--for example, type mappings. At a high level, removing some of the first kind is not unreasonable, while removing some of the second kind is likely to cause issues. Therefore, we might want to:
    • Explicitly enable removing/configuring of only certain conventions
    • Provide an "empty" convention set that still runs the second kind of convention without the first
    • Look into mechanisms to finalize/validate a model after it has been built manually in some way.

@ajcvickers ajcvickers changed the title Empty ConventionSet causes ArgumentNullException Parameter name: relationalTypeMapping Allow model building with some conventions disabled Apr 23, 2018
@ajcvickers ajcvickers added this to the Backlog milestone Apr 23, 2018
@sadjadbp
Copy link

I'm hitting this error with something like explained in #4410, so we need a GetCompiled method that uses

var conventionSet = SqlServerConventionSetBuilder.Build();
var modelBuilder = new ModelBuilder(conventionSet);

And now this project works with 2.0, but gives me this error when I upgrade to 2.1

@luigi68
Copy link

luigi68 commented Sep 11, 2018

Hi, I have exactly the same issue posted by @sadjadbp when calling the DbContext.SaveChanges
using EFCore 2.1.2. Is it Something that is going to be fixed or is something that I am doing wrong in my code?

Thanks in advance
Luigi

@merrycoder
Copy link

Same for me as for @sadjadbp - blocks me from upgrading to 2.1.

@almazsr
Copy link

almazsr commented Sep 12, 2018

We met the same problem, when tried to create IModel through ModelBuilder. By default EF Core caches IModel, thus all DbContext's use one instance of it. If you need a new specific instance of IModel per each DbContext creation, create custom IModelSource to prevent caching of IModel:

    public class CreatePerCallModelSource : ModelSource
    {
        public CreatePerCallModelSource(ModelSourceDependencies dependencies) : base(dependencies)
        {
        }

        public override IModel GetModel(DbContext context, IConventionSetBuilder conventionSetBuilder, IModelValidator validator)
        {
            return CreateModel(context, conventionSetBuilder, validator);
        }
    }

Then just replace the default ModelSource with created one to create a new DbContext with DbContextOptionsBuilder:
dbContextOptionsBuilder.ReplaceService<IModelSource, CreatePerCallModelSource>();

@luigi68
Copy link

luigi68 commented Sep 12, 2018

We met the same problem, when tried to create IModel through ModelBuilder. By default EF Core caches IModel, thus all DbContext's use one instance of it. If you need a new specific instance of IModel per each DbContext creation, create custom IModelSource to prevent caching of IModel:

    public class CreatePerCallModelSource : ModelSource
    {
        public CreatePerCallModelSource(ModelSourceDependencies dependencies) : base(dependencies)
        {
        }

        public override IModel GetModel(DbContext context, IConventionSetBuilder conventionSetBuilder, IModelValidator validator)
        {
            return CreateModel(context, conventionSetBuilder, validator);
        }
    }

Then just replace the default ModelSource with created one to create a new DbContext with DbContextOptionsBuilder:
dbContextOptionsBuilder.ReplaceService<IModelSource, CreatePerCallModelSource>();

I tried without luck... honestly i believe problem is somewhere else.

@AAltemus
Copy link

I'm hitting this error with something like explained in #4410, so we need a GetCompiled method that uses

var conventionSet = SqlServerConventionSetBuilder.Build();
var modelBuilder = new ModelBuilder(conventionSet);

And now this project works with 2.0, but gives me this error when I upgrade to 2.1

I'm experiencing the same. Tried in the latest 2.2 as well. Has anyone figured out a fix to this?

@ghost
Copy link

ghost commented Sep 18, 2018

I think I have the same problem when calling DbContext.SaveChanges
using EFCore 2.1.3. Any solution?

Microsoft.EntityFrameworkCore.Utilities.Check.NotNull[T](T value, String parameterName)
Microsoft.EntityFrameworkCore.Storage.Internal.TypeMappedRelationalParameter..ctor(String invariantName, String name, RelationalTypeMapping relationalTypeMapping, Nullable1 nullable) Microsoft.EntityFrameworkCore.Storage.Internal.RelationalParameterBuilder.AddParameter(String invariantName, String name, IProperty property) Microsoft.EntityFrameworkCore.Storage.RelationalCommandBuilderExtensions.AddParameter(IRelationalCommandBuilder commandBuilder, String invariantName, String name, IProperty property) Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.CreateStoreCommand() Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.Execute(IRelationalConnection connection) Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.Execute(DbContext _, ValueTuple2 parameters)
Microsoft.EntityFrameworkCore.Storage.Internal.NoopExecutionStrategy.Execute[TState,TResult](TState state, Func3 operation, Func3 verifySucceeded)
Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.Execute(IEnumerable1 commandBatches, IRelationalConnection connection) Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(IReadOnlyList1 entriesToSave)
Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(Boolean acceptAllChangesOnSuccess)
Microsoft.EntityFrameworkCore.DbContext.SaveChanges(Boolean acceptAllChangesOnSuccess)
unoGIS.SpatialDrivers.DbSpatialDrivers.DbInternalContext.SaveChanges()

@sabo3000
Copy link

same issue. please fix it as soon as possible!

@alekrist
Copy link

Having the same issue and this blocks us from upgrading to v2.1. Please fix this soon.

@Joganoth
Copy link

same issue for me, please fix it asap!

@ajcvickers
Copy link
Contributor

ajcvickers commented Sep 18, 2018

@sabo3000 @alekrist @Joganoth @mfernandezruiz @AAltemus @luigi68 @almazsr This issue is about the behavior of building the model using an explicitly empty convention set. Can you either:

  • Confirm that this is the case for the issues you are seeing and provide some details as to why you are using an empty convention set
  • Or file a new issue with a runnable project/solution that demonstrates the behavior you are seeing when using the normal conventions.

@ghost
Copy link

ghost commented Sep 18, 2018

This code runs until version 2.1 and with the 2.1.3 fails

using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Metadata.Conventions;
using System.ComponentModel.DataAnnotations;
using System.ComponentModel.DataAnnotations.Schema;

namespace EFCoreTest
{
    class Program
    {
        static void Main(string[] args)
        {
            var conventions = SqliteConventionSetBuilder.Build();
            var modelBuilder = new ModelBuilder(conventions);
            modelBuilder.Entity<User>();
            var model = modelBuilder.Model;

            var optionsBuilder = new DbContextOptionsBuilder();
            optionsBuilder.UseSqlite("Data Source=Test.db");
            optionsBuilder.UseModel(model);

            using (var context = new MyContext(optionsBuilder.Options))
            {
                context.Database.EnsureCreated();

                var user = new User()
                {
                    Name = "Adam"
                };

                context.Add(user);
                context.SaveChanges();
            }
        }
    }

    public class MyContext : DbContext
    {
        public MyContext(DbContextOptions options) : base(options)
        {
        }
    }

    [Table("Users")]
    public class User
    {
        [Key]
        public int Id { get; set; }
        public string Name { get; set; }
    }
}

@luigi68
Copy link

luigi68 commented Sep 18, 2018

@sabo3000 @alekrist @Joganoth @mfernandezruiz @AAltemus @luigi68 @almazsr This issue is about the behavior of building the model using an explicitly empty convention set. Can you either:

  • Confirm that this is the case for the issues you are seeing and provide some details as to why you are using an empty convention set
  • Or file a new issue with a runnable project/solution that demonstrates the behavior you are seeing when using the normal conventions.

About me I think I am not using empty convention set.
The following code virtually represent my actual implementation:

var connection = System.Data.SqlClient.SqlClientFactory.Instance.CreateConnection(...);
var conventionSet = SqlServerConventionSetBuilder.Build();
var modelBuilder = new ModelBuilder(conventionSet);

modelBuilder.Entity("ADDRESS").HasKey( )... // example...
...

var optionsBuilder = new DbContextOptionsBuilder();
optionsBuilder.UseModel(modelBuilder.Model);
optionsBuilder.UseSqlServer(connection);
...
var dbContext = new DbContext(optionsBuilder.Options)
...

Please let me know what i am doing wrong..
Thanks in advance

@VitaliiTsilnyk
Copy link

VitaliiTsilnyk commented Sep 18, 2018

@mfernandezruiz, @luigi68 You can use a standard way of creating a model by overriding the OnModelCreating method in your DbContext: https://docs.microsoft.com/en-us/ef/core/modeling/
Looks like you don't need the actual functionality this issue addresses. I don't see any convention changes in your examples.

@luigi68
Copy link

luigi68 commented Sep 18, 2018

@mfernandezruiz: I need to create model before creating context. And by the way this approach works fine with Entity Framework.

@VitaliiTsilnyk
Copy link

@luigi68 May I ask why? You can just define your model inside your DbContext.

@merrycoder
Copy link

We are more or less using the same code as @mfernandezruiz below. No matter if we use empty convention, or conventions built with SqliteConventionSetBuilder or SqlServerConventionSetBuilder, the code fails with the indicated error. We are not using database migrations, in case this makes a difference.

This code runs until version 2.1 and with the 2.1.3 fails

using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Metadata.Conventions;
using System.ComponentModel.DataAnnotations;
using System.ComponentModel.DataAnnotations.Schema;

namespace EFCoreTest
{
    class Program
    {
        static void Main(string[] args)
        {
            var conventions = SqliteConventionSetBuilder.Build();
            var modelBuilder = new ModelBuilder(conventions);
            modelBuilder.Entity<User>();
            var model = modelBuilder.Model;

            var optionsBuilder = new DbContextOptionsBuilder();
            optionsBuilder.UseSqlite("Data Source=Test.db");
            optionsBuilder.UseModel(model);

            using (var context = new MyContext(optionsBuilder.Options))
            {
                context.Database.EnsureCreated();

                var user = new User()
                {
                    Name = "Adam"
                };

                context.Add(user);
                context.SaveChanges();
            }
        }
    }

    public class MyContext : DbContext
    {
        public MyContext(DbContextOptions options) : base(options)
        {
        }
    }

    [Table("Users")]
    public class User
    {
        [Key]
        public int Id { get; set; }
        public string Name { get; set; }
    }
}

@ghost
Copy link

ghost commented Sep 19, 2018

@neris : My example is only for reproduce the problem. My application needs to create data models at run time and also needs to handle several data models at the same time, so overwriting 'OnModelCreating' does not help

@ajcvickers
Copy link
Contributor

Thanks for the additional info; using conventions but outside of OnModelCreating like this looks like a more common scenario than was originally reported. We will discuss what we can do.

A couple of random things to note from the discussion:

  • Context pooling is not required to get model caching; model caching always happens anyway.
  • Model caching can be controlled using IModelCacheKeyFactory

@ajcvickers
Copy link
Contributor

Triage: we will look into adding a Finalize/Build or similar method to the model that will make it possible to build the model externally from the context but still have all the conventions execute.

@ajcvickers ajcvickers added this to the 2.2.0 milestone Sep 20, 2018
@ajcvickers ajcvickers self-assigned this Sep 20, 2018
@ajcvickers ajcvickers changed the title Allow model building with some conventions disabled Allow model building outside of OnModelCreating Sep 20, 2018
ajcvickers added a commit that referenced this issue Sep 30, 2018
… the context

Fixes #11738

Naming to be discussed.

This runs the model-based conventions, but not model validation. Also moved setting or product version annotation to any use of the ModelBuilder and then removed from places in the tests that this would cause problems when the version changes.
@ajcvickers ajcvickers added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed needs-design labels Sep 30, 2018
@AndriySvyryd
Copy link
Member

AndriySvyryd commented Oct 1, 2018

@almazsr If you need to rebuild the model based on some external information you can implement IModelCacheKeyFactory so that it's only cached when appropriate, see Alternating models with the same DbContext

ajcvickers added a commit that referenced this issue Oct 2, 2018
… the context

Fixes #11738

Naming to be discussed.

This runs the model-based conventions and model validation. Also moved setting or product version annotation to any use of the ModelBuilder and then removed from places in the tests that this would cause problems when the version changes.
ajcvickers added a commit that referenced this issue Oct 2, 2018
… the context

Fixes #11738

Naming to be discussed.

This runs the model-based conventions and model validation. Also moved setting or product version annotation to any use of the ModelBuilder and then removed from places in the tests that this would cause problems when the version changes.
ajcvickers added a commit that referenced this issue Oct 2, 2018
… the context

Fixes #11738

Naming to be discussed.

This runs the model-based conventions and model validation. Also moved setting or product version annotation to any use of the ModelBuilder and then removed from places in the tests that this would cause problems when the version changes.
ajcvickers added a commit that referenced this issue Oct 2, 2018
… the context

Fixes #11738

Naming to be discussed.

This runs the model-based conventions and model validation. Also moved setting or product version annotation to any use of the ModelBuilder and then removed from places in the tests that this would cause problems when the version changes.
ajcvickers added a commit that referenced this issue Oct 2, 2018
… the context

Fixes #11738

Naming to be discussed.

This runs the model-based conventions and model validation. Also moved setting or product version annotation to any use of the ModelBuilder and then removed from places in the tests that this would cause problems when the version changes.
ajcvickers added a commit that referenced this issue Oct 3, 2018
… the context

Fixes #11738

Naming to be discussed.

This runs the model-based conventions and model validation. Also moved setting or product version annotation to any use of the ModelBuilder and then removed from places in the tests that this would cause problems when the version changes.
@roji
Copy link
Member

roji commented Oct 18, 2018

Note: breaking change for provider tests

@smitpatel
Copy link
Contributor

Also broke compat tests.

roji added a commit to npgsql/efcore.pg that referenced this issue Oct 18, 2018
@stefan-landgraf-is24
Copy link

@ajcvickers Thanks for supporting our case. Maybe i misunderstood something, but I still get the same error with EFCore 2.2 and the new method FinalizeModel():

var mb = new ModelBuilder(new ConventionSet(){});
 foreach (var assembly in modelAssemblies)
 {
      mb.AddConfigurationsFromAssembly(assembly);
 }
mb.FinalizeModel();

var optionsBuilder = new DbContextOptionsBuilder<EFDataContext>()
   .UseModel(mb.Model)

leads to this exception when saving changes:

System.ArgumentNullException : Value cannot be null.
Parameter name: relationalTypeMapping
   at Microsoft.EntityFrameworkCore.Utilities.Check.NotNull[T](T value, String parameterName)
   at Microsoft.EntityFrameworkCore.Storage.Internal.TypeMappedRelationalParameter..ctor(String invariantName, String name, RelationalTypeMapping relationalTypeMapping, Nullable`1 nullable)

@ajcvickers
Copy link
Contributor

@stefan-landgraf-is24 If you use an empty convention set, then you'll still need to make sure that everything that needs to be done in the model actually gets done. This includes setting appropriate attributes for type mappings.

The change that was made for 2.2 allows building of a model using conventions to work outside of OnModelCreating.

@stefan-landgraf-is24
Copy link

stefan-landgraf-is24 commented Jan 4, 2019

@ajcvickers How can I find out what has to be done, if the error message gives no hint at all?

System.ArgumentNullException : Value cannot be null. Parameter name: relationalTypeMapping

@ajcvickers
Copy link
Contributor

@stefan-landgraf-is24 The message could be better, but it's telling you that type mappings do not exist for the properties in the model. See this comment above for details: #11738 (comment)

In terms of what has to be done, the easiest by far to get right is to use a model builder with conventions in the normal way. If you really need there to be no conventions, then it's going to be tough to understand and replicate everything that the conventions are doing. I certainly would not attempt it, and I know EF Core pretty well. Some of the potential future work discussed above could make this easier, but it's not high on the priority list right now.

@AndriySvyryd
Copy link
Member

@stefan-landgraf-is24 You can use SqlServerConventionSetBuilder.Build()

@ajcvickers ajcvickers modified the milestones: 2.2.0-preview3, 2.2.0 Nov 11, 2019
@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
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. regression type-enhancement
Projects
None yet
Development

No branches or pull requests