search

PomeloFoundation / Pomelo.EntityFrameworkCore.MySql

Entity Framework Core provider for MySQL and MariaDB built on top of MySqlConnector
MIT License
2.71k stars 384 forks source link

Scaffold ServerVersion() call #960

Closed lauxjpn closed 5 years ago

lauxjpn commented 5 years ago

We currently do not scaffold the ServerVersion that is being used by the database server.

With the different implementations of MySQL compatible database servers out there and still heavy development on the feature side of MySQL, we are making use of server version specific statements in many parts of the Pomelo provider.

Though users should always specify the expected (or minimal supported) server version when setting up a DbContext, many user are not aware of that and therefore use the default server version, which is the latest one.

As we discussed on Wednesday, scaffolding the actual ServerVersion will help prevent this issue at least in those cases, where users assume that the scaffolded code is complete and does not need to be altered, which could lead to bugs later on, when Pomelo makes use of newly introduced MySQL features that are not yet supported by the older version of the underlying database used by the user.

This will be the first step towards making it mandatory to explicitly set the server version when setting up a DbContext for Pomelo.

It should be functional equivalent to the scrapped #932 PR.

/cc @ajcvickers, @roji

roji commented 5 years ago

Note that if you want singleton services to be affected by the version (e.g. TypeMappingSource, QuerySqlGenerator), then unless I'm mistaken a model annotation will not be sufficient - you're going to need something inheriting ISingletonOptions. This will cause a new service provider to be built for different, and a different instance of each singleton service to get instantiated. You can take a look at how Npgsql does this in NpgsqlOptions, which extends ISingletonOptions (or CosmosSingletonOptions in EF Core). @ajcvickers can confirm that I haven't said anything silly.

IIRC unfortunately we don't yet have a mechanism to scaffold db context options, which is how the version would be implemented. No time to find the tracking issue at the moment, but @bricelam can confirm.

lauxjpn commented 5 years ago

I think we implemented it in a similar way in #961, where we read the server version from the connection right before scaffolding, and then save it as an IMySqlOptions property (via our official DbContextBuilder extension method ServerVersion()) and call Initialize() as long as it has not already been setup for some reason.

We then use ProviderCodeGenerator.GenerateProviderOptions() to generate the code for the ServerVersion() extension method.

So we don't go the model annotation route that we tried in the scrapped #932 PR anymore. But we are making sure with this PR, that we do provide the same functionality we wanted to provide with #932, but this time just be reusing the already existing ServerVersion() extension method.

bricelam commented 5 years ago

We need to flow the model (or just its annotations) into the provider scaffolding method. Tracked by https://github.com/aspnet/EntityFrameworkCore/issues/10487