Skip to content

Remove dependencies on providers and support unknown ones #26

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

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

la-we
Copy link
Contributor

@la-we la-we commented Oct 10, 2023

Hey Jon,

thanks a lot for your awesome library, i been using it for a couple of years in my projects!

With your latest version 7, the changes to the way DesignTimeServices could be handed into the Compare-Method broke compatibility with Firebird .NET provider and probably others as only three Providers (SQLite, SQL-Server & Postgres) are known to your library.

This Pull-Request changes the implementation of the factory to be a little more eager in trying to create an appropriate DatabaseModelFactory and also removes dependencies on the other providers as this is imo unncessary ballast for your library.

Feel free to leave your comments, glad to hear your feedback!

@la-we la-we force-pushed the support-unknown-providers branch from 2087408 to fe02fae Compare October 10, 2023 05:43
@JonPSmith
Copy link
Owner

Hi @lweberprb,

Just to be clear, are you saying that this will automatically find the correct scaffolding service based on your DbContext? That sounds very useful. I see that you still have a "Your database provider isn't supported" exception so does this suggest it might not work?

@la-we
Copy link
Contributor Author

la-we commented Oct 11, 2023

It will indeed try to resolve the appropriate DatabaseModelFactory based on your DbContext instance. The exception is there if we are unable to call the contructor of the DatabaseModelFactory implementation of the provider. With this PR it is possible to construct all implementations that offer a default constructor or a constructor that accepts a scaffolding logger.

@la-we
Copy link
Contributor Author

la-we commented Oct 11, 2023

But you are right, the exception message might be misleading in this case. It would probably be better to throw something in the means of "Unable to construct DatabaseModelFactory of provider X, due to unknown contructor paramter combination" or something like this.

@JonPSmith
Copy link
Owner

Thanks @lweberprb, your code is a much better of my idea of handing other EF Core database types (see issue #22). Nice trick to load the assembly - super cool.

Now my problem is that my 'dev' branch contains code for issue #21 and #25 (version 6.0.2) and holds the the code ready for .NET 8. The simplest way to do this is to merge your PR to 'main', merge to 'dev' and release this feature in .NET 8 . The downside is .NET 8 isn't out until November 14, 2023. I can create a prerelease NuGet of this library using 8.0.0-rc.1, but you don't get the final .NET 8 version until end of November.

Any comments?

@la-we
Copy link
Contributor Author

la-we commented Oct 12, 2023

That's abolutely fine with me! Glad to hear you like the addition to the project!

I'll be using a self-build version of your library in the meantime, i.e. until .NET 8 is released, so no worries there :)

@JonPSmith JonPSmith merged commit be6222f into JonPSmith:master Oct 12, 2023
JonPSmith added a commit that referenced this pull request Oct 12, 2023
@bgrauer-atacom
Copy link

Hi all

Jumping in here since I had the same challenges as @lweberprb and the intention (but not yet the time) to come up with an issue.

Initially i had the Idea to pull IDatabaseModelFactory from the design service context according to the Documentation. Unfortunately, this is not working anymore (documentation issue dotnet/EntityFramework.Docs#4454 is placed).

After that, I was looking how EfCore is resolving design services and found DesignTimeServicesBuilder. I made an example how to use the builder here.

The builder has come challenges:

All in all the solution of @lweberprb is probably the more straightforward. In any case:

  • Would it not be more future proof to look for IDatabaseModelFactory implementations instead the base class?
  • Maybe use the design service stack like described here and create instances with ActivatorUtilities.CreateInstance?

Since the merge request is now merged: @JonPSmith I let the descision up to you if you want to follow up this in a separate issue.

@JonPSmith
Copy link
Owner

Hi @bgrauer-atacom,

Thanks for letting me know the the creation of a IDatabaseModelFactory for a specific database has changed in .NET 8. All my tests using SqlServer failed because the SqlServerDatabaseModelFactory has changed (now needs the IRelationalTypeMappingSource as the third parameter), but

But this clearly shows that if I want to this library to that supports any database that EF Core can access, then I have a common approach. Problem is that each database can have different parameters in its IDatabaseModelFactory. For instance the MySqlDatabaseModelFactory has a third parameter of type IMySqlOptions.

So, is it seem impossible to make this library that will work all database types.

Any comments from @bgrauer-atacom and @lweberprb??

JonPSmith added a commit that referenced this pull request Oct 12, 2023
JonPSmith added a commit that referenced this pull request Oct 12, 2023
@la-we
Copy link
Contributor Author

la-we commented Oct 13, 2023

Hmm, changes in the providers IDatabaseModelFactory are to be expected. Maybe the CompareWithEfDb method should accept a IDatabaseModelFactory instance? The Caller is therefore responsible for creating the instance and the library can just use it.

Could be a new overload that skips the libraries internal resolver methods for creating a particular instance.

Another idea could be, to resolve the constructors paramaters by reflection also. I.e. collect the necessary types and resolve these via the DbContext IServiceProvider. Might be worth a try although this might get rather complex for such a simple task.

Personally, I'd go for the first option. It is the simplest and least error prone approach to use any IDatabaseModelFactory. Nevertheless i will explore the possibilities to automate this via reflection and remove usages of explicit Activator.CreateInstance calls.

@bgrauer-atacom
Copy link

So, is it seem impossible to make this library that will work all database types.

From my understanding this should be possible when the library interacts with the design libraries like the EFCore CLI tools do.

Lets take Scaffolding as an example / inspiration:

  1. When executing: dotnet ef dbcontext scaffold initially a lot magic happens . For us relevant:
    • Project path / assembly path is evaluated
    • Startup project path / startup project assembly is evaluated
    • Provider is read from arguments
  2. The CLI tool gets the design assembly from the target project and invokes scaffolding. At that point the design service stack is created , built and used.

Putting all together:

// This happens in the test project
var dbContextBuilder = new DbContextOptionsBuilder<MyDbContext>();
dbContextBuilder.UseSqlite("Data Source=:memory:");
var dbContext = new MyDbContext(dbContextBuilder.Options);

// TODO: Maybe implement handler actions for logging purposes?
// HINT: The classes are only available when including the design package like this:
// https://learn.microsoft.com/en-us/ef/core/cli/services#referencing-microsoftentityframeworkcoredesign
var reporter = new OperationReporter(new OperationReportHandler());

// REVIEW: According to my understanding the assembly is where the DBContext is
var assembly = typeof(MyDbContext).Assembly;

// REVIEW: This is not the same behaviour as in the CLI but we don't know the real startup project. Using the test project
// assembly as stratup assembly hand enables the possibility to define design services inside of the test project for test purposes.
var startupAssembly = Assembly.GetEntryAssembly() ?? assembly;

// HINT: The classes are only available when including the design package like this:
// https://learn.microsoft.com/en-us/ef/core/cli/services#referencing-microsoftentityframeworkcoredesign
var builder = new DesignTimeServicesBuilder(assembly, startupAssembly, reporter, Array.Empty<string>());
var serviceProvider = builder.Build(dbContext);

var modelFactory = serviceProvider.GetRequiredService<IDatabaseModelFactory>();

If you don't want to undo #4, you could also put the service stack together using reflection:

/ / This happens in the test project
var dbContextBuilder = new DbContextOptionsBuilder<MyDbContext>();
dbContextBuilder.UseSqlite("Data Source=:memory:");
var dbContext = new MyDbContext(dbContextBuilder.Options);

// REVIEW: According to my understanding the assembly is where the DBContext is
var assembly = typeof(MyDbContext).Assembly;

// REVIEW: This is not the same behaviour as in the CLI but we don't know the real startup project. Using the test project
// assembly as stratup assembly hand enables the possibility to define design services inside of the test project for test purposes.
var startupAssembly = Assembly.GetEntryAssembly() ?? assembly;

try
{
    // REVIEW: In my case, the assembly was loaded correct. Unsure if this works in all cases.
    var designAssembly = Assembly.Load("Microsoft.EntityFrameworkCore.Design");

    var reportHandlerType = designAssembly.GetType("Microsoft.EntityFrameworkCore.Design.OperationReportHandler")
        ?? throw new InvalidOperationException("Unable to create an 'OperationReportHandler' instance. Are you using a supported EntityFrameworkCore version?");
    // TODO: Maybe implement handler actions for logging purposes?
    var reportHandler = Activator.CreateInstance(reportHandlerType, null, null, null, null);


    var reporterType = designAssembly.GetType("Microsoft.EntityFrameworkCore.Design.Internal.OperationReporter")
        ?? throw new InvalidOperationException("Unable to create an 'OperationReporter' instance. Are you using a supported EntityFrameworkCore version?");
    var reporter = Activator.CreateInstance(reporterType, reportHandler);


    var serviceBuilderType = designAssembly.GetType($"Microsoft.EntityFrameworkCore.Design.Internal.DesignTimeServicesBuilder")
        ?? throw new InvalidOperationException("Unable to create an 'DesignTimeServicesBuilder' instance. Are you using a supported EntityFrameworkCore version?");
    var serviceBuilder = Activator.CreateInstance(serviceBuilderType, assembly, startupAssembly, reporter, Array.Empty<string>());

    var serviceProvider = (IServiceProvider?) serviceBuilderType.GetMethods()
        .Where(x => {
            if (x.Name != "Build")
            {
                return false;
            }
            var pars = x.GetParameters();
            if(pars.Length != 1)
            {
                return false;
            }
            return pars[0].ParameterType.Name == "DbContext";
        })
        .FirstOrDefault()
        ?.Invoke(serviceBuilder, new object[] { dbContext })
        ?? throw new InvalidOperationException("Unable to bild design time service provider. Are you using a supported EntityFrameworkCore version?"); ;

    var modelFactory = serviceProvider.GetRequiredService<IDatabaseModelFactory>();
}
catch (FileNotFoundException ex)
{
    throw new InvalidOperationException($"Your startup project '{startupAssembly.GetName()}' doesn't reference " +
        "Microsoft.EntityFrameworkCore.Design. This package is required for the SchemaCompare to work. " +
        "Ensure your startup project is correct, install the package, and try again.", ex);
}

@JonPSmith
Copy link
Owner

Hi @bgrauer-atacom,

Thanks again for another way to get the scaffolding that I hadn't thought of. So, just to be clear you have looked at the program used in the dotnet ef dbcontext scaffold command and pulled out the code inside to get the model factory. Therefore, this should work for any EF Core database provider. It that correct?

I tried your code on my unit tests and it worked. I tried both of your code approaches and I thought that the second version makes it easier to create a new version when the next .NET is released every year. (I'm retired now but I want to make sure my libraries are available in the latest .NET releases and help people who ask about how to use my libraries).

I need a bit more work on this, but I will release a prerelease NuGet of this lib if you check it out (PS. The code in the 'dev' branch at the moment.

Thanks again.

PS. Could you send me a link to the dotnet ef dbcontext scaffold program as using internal code can change.

@JonPSmith
Copy link
Owner

Hi guys,

I have released EfCore.SchemaCompare 8.0.0-rc2-0002 for people to try this.

@bgrauer-atacom, I tried the shorter version with full Microsoft.EntityFrameworkCore.Design but my tests shows that the user must have the Microsoft.EntityFrameworkCore.Design in the caller's app. Therefore I went for the longer version, which allows me to update the Microsoft.EntityFrameworkCore.Design without having hand-change the .csprog file.

I'm now asking to try this out and let me know if is a problem.

@JonPSmith
Copy link
Owner

PS. The code in in the main branch now

@bgrauer-atacom
Copy link

Hi @JonPSmith
Sorry for my really bad response time.

First of all: Thanks for the changes. I really like your decision to use full reflection. (You are still referencing Microsoft.EntityFrameworkCore.Design. I think this is not necessary anymore.)

Regarding dotnet ef dbcontext scaffold:

(Disclaimer: This is what i found out by "reading code". There might be some wrong assumptions due to the complexity.)

Testing

Unfortunately, i am unable to test the newest Nuget version (8.0.0), since a database provider i use (Firebird) in my current projects does not yet support .NET 8 and EF 8.
I took the repo and rolled back to .NET/EF 7 for tests and had no breaking issues related to loading design services.

One minor thing: When the design DLLs are missing and the library is executed from a unit test (Visual Studio 17.8.1) i get following Exception-Message:

Your startup project 'testhost, Version=15.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' doesn't reference Microsoft.EntityFrameworkCore.Design. This package is required for the SchemaCompare to work. Ensure your startup project is correct, install the package, and try again.

It seems, that the entry assembly does not provide much helpful information unless someone does this inside of the test project.

@JonPSmith
Copy link
Owner

Hi @bgrauer-atacom,

Thanks for the extra info, but I'm not sure in your Testing section. I found that having the Microsoft.EntityFrameworkCore.Design of the EfCore.SchemaCompare didn't work on its own, and the caller has to have the Microsoft.EntityFrameworkCore.Design too. Is that what you see too?

@bgrauer-atacom
Copy link

Yes. The package Microsoft.EntityFrameworkCore.Design must somehow end up in the build output. Otherwise we would not be able to load the assembly.

This happens either by removing

<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>

inside of the library project or by referencing the library inside of the test project.

I prefer the current solution since this is more near to the behavior of the CLI and gives the user more freedom which design library he wants to use.

Maybe there is a better solution but i am not an msbuild or Nuget expert.


Another thing I i experienced while looking thru Ef libraries is, that they do not pin the version of the EF libraries:

This is probably a good thing to avoid dependency conflicts?

@JonPSmith
Copy link
Owner

Good, we are in sync on this. I will keep with the current arrangement.

Another thing...

I define the NuGet by version as each NET release often add new features / changes. EF Core NET 8 has new features and some subtle changes to existing methods.

@bgrauer-atacom
Copy link

I define the NuGet by version as each NET release often add new features / changes. EF Core NET 8 has new features and some subtle changes to existing methods.

Nevermind. I was mixing up Nuget references and project references. My intention was not the backwards compatibility, i wanted to avoid version conflicts inside of a mayor release but this is given by your nuget package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants