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

Scaffold ServerVersion() call #960

Closed
lauxjpn opened this issue Nov 22, 2019 · 3 comments · Fixed by #961
Closed

Scaffold ServerVersion() call #960

lauxjpn opened this issue Nov 22, 2019 · 3 comments · Fixed by #961
Assignees
Milestone

Comments

@lauxjpn
Copy link
Collaborator

lauxjpn commented Nov 22, 2019

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
Copy link

roji commented Nov 23, 2019

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
Copy link
Collaborator Author

lauxjpn commented Nov 23, 2019

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
Copy link

We need to flow the model (or just its annotations) into the provider scaffolding method. Tracked by dotnet/efcore#10487

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

Successfully merging a pull request may close this issue.

3 participants