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

[v8] DatabaseBuilder has useful methods set to internal #3866

Closed
aaronpowell opened this issue Dec 13, 2018 · 25 comments
Closed

[v8] DatabaseBuilder has useful methods set to internal #3866

aaronpowell opened this issue Dec 13, 2018 · 25 comments
Assignees

Comments

@aaronpowell
Copy link
Contributor

@aaronpowell aaronpowell commented Dec 13, 2018

In v7 I was relying on the DatabaseSchemaHelper class to expose some functionality to create the database in Chauffeur, in particular, this method: https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Core/Persistence/DatabaseSchemaHelper.cs#L113

The DatabaseSchemaHelper has been removed in v8 and replaced with DatabaseBuilder (or so it seems).

Unfortunately, the method I need, CreateSchemaAndData is marked as internal, along with its return type.

Because of this, it's pretty much impossible (without some dodgy reflection) Chauffeur is unable to bootstrap an Umbraco database in a simple manner. The only workaround I'm able to find is to pull in the InstallStep<T> for the database install, which feels really ugly and also won't give me the control I need in the process.

aaronpowell added a commit to aaronpowell/Umbraco-CMS that referenced this issue Dec 13, 2018
@zpqrtbnk zpqrtbnk self-assigned this Dec 13, 2018
@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Dec 13, 2018

These methods could be public really... it's only the CreateSchemaAndData one?

Out of curiosity... so Chauffeur manages the complete install? Thus bypassing the Umbraco installer? Do you also manage upgrades (ie would you also need UpgradeSchemaAndData)?

And then I am wondering, if you really want to take control of database install, maybe its the DatabaseSchemaCreator class that should be public. DatabaseBuilder seems to do too many install-related things (checking for db...).

@aaronpowell

This comment has been minimized.

Copy link
Contributor Author

@aaronpowell aaronpowell commented Dec 13, 2018

Created a PR with what I think would be best to make public.

I think there's value in the upgrade also being public, it futureproof's the usage.

Chauffeur doesn't run the full install, it just needs to be able to create the database. The scenario is that you have cloned from a git repo to join a project underway, and you can create the db locally using a pre-set connection string.

@aaronpowell

This comment has been minimized.

Copy link
Contributor Author

@aaronpowell aaronpowell commented Dec 13, 2018

But if there's a better class to use than DatabaseBuilder then I'd go with that. I was just following how the web installer works.

@aaronpowell

This comment has been minimized.

Copy link
Contributor Author

@aaronpowell aaronpowell commented Dec 13, 2018

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Dec 17, 2018

Can you get something from DI there? Assuming you can inject the IScopeProvider, ILogger and IUmbracoDatabaseFactory, the following code should get your a database with schema and data:

databaseFactory.Configure(connectionString, Constants.DbProviderNames.SqlCe);

using (var scope = scopeProvider.CreateScope())
{
    var creator = new DatabaseSchemaCreator(scope.Database, logger);
    creator.InitializeDatabaseSchema();
    scope.Complete();
}

We would then "just" make DatabaseSchemaCreator and its InitializeDatabaseSchema (maybe renamed to CreateSchemaWithData or something) public. Thoughts?

@aaronpowell

This comment has been minimized.

Copy link
Contributor Author

@aaronpowell aaronpowell commented Dec 17, 2018

The reason I was looking into the DatabaseBuilder was because that is what the install steps seem to rely on, and it'd mean I don't have to manage scope or anything (I'd prefer to do as little of that as possible!).

Let me dig into DatabseSchemaCreator and get back to you.

aaronpowell added a commit to aaronpowell/Umbraco-CMS that referenced this issue Dec 18, 2018
Making the DatabaseSchemaCreator type public so it can be used to create the schema without relying on the installer steps.
Also moved the OrderedTables property to a ReadOnlyCollection so you can't change the types that the db uses (which you could do if it was a List<T>
@aaronpowell

This comment has been minimized.

Copy link
Contributor Author

@aaronpowell aaronpowell commented Dec 18, 2018

Looks like the DatabaseSchemaCreator would do the trick, so I've created an alternative PR that does that.

I'd expect you'd want to use either #3867 or #3899, not both, but they are just a compare/contrast approach.

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Dec 18, 2018

Only the DatabaseBuilder would give you the upgrade, though. And that all probably can be made public. Might rename a few things for consistency though. Let's see.

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Dec 18, 2018

Reviewing now - trying to reduce the number of things we expose and might want to refactor at some point - do you need methods from the #region Configure ConnectionString to be public? Or is it ok to keep them all internal?

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Dec 18, 2018

Same question with ValidateSchema() - this method does weird things, and reports weird data structures, that I really would rather keep internal so we can refactor them. Let me know if we could keep them internal for now.

@aaronpowell

This comment has been minimized.

Copy link
Contributor Author

@aaronpowell aaronpowell commented Dec 18, 2018

From a Chauffeur stand point then I don't really need to be able to save the connection string, I don't expose the ability to edit that (it's a bit tricky due to the way Chauffeur hosts Umbraco).

ValidateSchema could be useful, I'd potentially use it from a test standpoint.

Being able to run the upgrade would also be good, Chauffeur supports running migrations which I'd assume the upgrade in DatabaseBuilder does.

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Dec 18, 2018

ok - going to merge + do a few changes, and then feel free to complain ;-) or ask for more.

zpqrtbnk added a commit that referenced this issue Dec 18, 2018
zpqrtbnk added a commit that referenced this issue Dec 18, 2018
@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Dec 18, 2018

Have merged. Keeping this issue open while you review.

@aaronpowell

This comment has been minimized.

Copy link
Contributor Author

@aaronpowell aaronpowell commented Dec 18, 2018

@aaronpowell

This comment has been minimized.

Copy link
Contributor Author

@aaronpowell aaronpowell commented Dec 18, 2018

Bugger, looks like a build hasn't been published to MyGet for a week ☹.

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

@nul800sebastiaan nul800sebastiaan commented Dec 19, 2018

☹ Looks like the unit tests are failing:

2018-12-19T12:52:05.9413785Z Failed   Create_Cached_Plugin_File
2018-12-19T12:52:05.9414099Z Error Message:
2018-12-19T12:52:05.9414794Z  TearDown : System.IO.DirectoryNotFoundException : Could not find a part of the path 'D:\a\1\s\build.tmp\tests\TypeLoader'.
@aaronpowell

This comment has been minimized.

Copy link
Contributor Author

@aaronpowell aaronpowell commented Dec 19, 2018

That's a bit of a bugger ☹️

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Dec 20, 2018

On it now

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Dec 20, 2018

Fixed, should be good tonight.

@aaronpowell

This comment has been minimized.

Copy link
Contributor Author

@aaronpowell aaronpowell commented Jan 8, 2019

I've created a hack to work around the problem in #3935 while we're working out what could be a proper solution to that.

In doing so I have come across another problem, that DatabaseBuilder.CreateSchemaAndData isn't actually going to work.

The problem is that on this line:

if (string.IsNullOrEmpty(_globalSettings.ConfigurationStatus) && !hasInstalledVersion)

Because the ConfigurationStatus has a value the if check means it skips trying to install and instead you go to the upgrade route.

I'd like to reopen #3899 and merge it, so you can use the DatabaseSchemaCreator directly.

aaronpowell added a commit to aaronpowell/Chauffeur that referenced this issue Jan 8, 2019
@aaronpowell

This comment has been minimized.

Copy link
Contributor Author

@aaronpowell aaronpowell commented Feb 6, 2019

@zpqrtbnk Just wanted to revisit this as I'd like to pick up working on Chauffeur for v8

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Feb 7, 2019

The public DatabaseSchemaCreator class now has a public InitializeDatabaseSchema() method that is meant to be used for this (as the database builder is doing more, unnecessary things). You end up doing something alike:

private void InitializeDatabase(IUmbracoDatabase database)
{
    database.BeginTransaction();
    var creator = new DatabaseSchemaCreator(database, _logger);
    creator.InitializeDatabaseSchema();
    database.CompleteTransaction();
}

Now that requires an IUmbracoDatabase - discussing this on #3935

@aaronpowell

This comment has been minimized.

Copy link
Contributor Author

@aaronpowell aaronpowell commented Feb 25, 2019

Dumb question on using IUmbracoDatabase, I found that that isn't in the container, so how do you get an instance of it?

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Feb 25, 2019

The normal way, which we use everywhere, is via a scope:

using (var scope = scopeProvider.CreateScope())
{
  var database = scope.Database;
  // do stuff
  scope.Complete();
}

where scopeProvider is an IScopeProvider that can be injected.

Then, there is an IUmbracoDatabaseFactory which can be injected too and can be used to create an IUmbracoDatabase directly - but without scoping, transaction, anything and it really really must be disposed after being used.

@aaronpowell

This comment has been minimized.

Copy link
Contributor Author

@aaronpowell aaronpowell commented Feb 25, 2019

Ah cool, didn't know that that was the best way to get it, was thinking I'd have to use the IUmbracoDatabaseFactory or something.

All sorted now!

aaronpowell added a commit to aaronpowell/Chauffeur that referenced this issue Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.