Tenants does not provide deletion?

[pgy866]

Tenants does not provide deletion? I can only install the tenants, Delete in "\ App_Data \ Sites"

[barthamark]

I'd like to work on this one since we'll need it in our project soon. The goal here is to

• be able to delete a tenant from code
• create an API similar to the setup tenant (but this will reset or delete it)
• add a Delete button to the Dashboard.

So what do I need to take care of when deleting a tenant?

• I assume we need to delete the related folder from the file system from the App_Data/Sites folder.
• If the database is SQLite then it will be also taken care of.
• If it's not SQLite then delete the tables from the database.
• Remove Media using a related general service.
• Delete ShellSettings (if this is not deletion but reset then it can be skipped).

Any other thing or suggestions?

@agriffard you reset your Try Orchard Core tenants on every Monday, do you have any related custom solutions or suggestions about this issue?

Thank you!

[sebastienros]

I suggest that we create an event handler for tenants with Creating and Deleting methods. Then custom implementation that will delete whatever it specific to each module, or modular component (tables, file system, ...)

[barthamark]

It could be IShellEventHandler with Saving and Deleting placed in OrchardCore/OrchardCore.Abstraction projects so it would be accessible from anywhere. Also deleting Media, database etc. would be implemented in these handlers (placed in the related module) except deleting the shell settings from Tenants.json which would be in IShellsSettingsSources.Delete method and deleting the appsettings.json which would be in IShellConfigurationSources.Delete method.

The process could be executed in the IShellHost.RemoveShellAsync():

1. Deactivate shell.
2. Run Deleting event (so delete media, database, etc.).
3. Delete appsettings.json and configuration from tenants.json.
4. Delete tenant folder.

Sounds good?

[Piedone]

I think the deletion of basic common resources like the site folder in App_Data, DB content and Media folder (or container) should be done centrally and not left up to each module to clean up their own stuff. If we rely on every module author to write their own uninstall logic every time they use any of these then we'll end up with the case of Windows uninstallers, where a lot of junk can remain.

I'm not against modules having the ability to run uninstall logic (just as it is in 1.x) should this be necessary for whatever reason, but that being a necessity will lead to it being forgotten.

To handle the DB table deletion and if there is no other way at the moment to know which tables correspond to a given tenant (I think there is no good way to do this for tenants without a table prefix, apart from excluding the table prefixes of all other tenants) we could do e.g. either of the following:

• Make using table prefixes mandatory, the default (that can be changed by the user as today) being the technical tenant name. This way tables to be deleted can be fetched based on the prefix.
• Keep a list of table names in a system table, which is populated with each Create*IndexTable().

While these are not trivial either this is something that should be done once as opposed to every module author needing to be careful in the indefinite future. For existing tenants these can be simply omitted, as for those handful or production of live Orchard installations it shouldn't pose a problem.

[barthamark]

@sebastienros you also mentioned that making prefixes mandatory could also work. Should we implement this and use it when deleting a tenant?

[sebastienros]

We still need the uninstall event so we can remove a module's tables (not by default as we would lose data).

I agree with you about the fact that deleting a tenant should not be the responsibility of modules, but that modules should be able to do more things. I am fine with setting a default table prefix if none is specified, like for the default tenant. That will only work with new setups, and I assume everyone is fine with it. I would suggest to just make the prefix field mandatory when displayed, in the UI, because technically we don't require it. SQLite doesn't need prefixes to get the database deleted, just delete the file.

It doesn't solve how the tables will be deleted? From the admin or manually? How do we list the tables to delete?

[Piedone]

If you're deleting a tenant I don't see what the intention would be apart from loosing data on purpose. We could make programmatic services to wipe out all data and just let users delete ShellSettings (i.e. making a tenant inaccessible but not removing any data) but I see little use of that.

The question is indeed just interesting for DBs not using a single file (so everything apart from SQLite right now). To delete all tables (i.e. every data stored in the DB) for a tenant we need to know which tables correspond to a given tenant. As I've explained above since table prefixes allow this mandatory table name prefixes solves this problem. Then e.g. with SQL Server we can list all the table names (so we then construct DROP TABLE queries) like this:

SELECT TABLE_NAME 
FROM information_schema.tables 
WHERE TABLE_NAME LIKE 'TenantTablePrefix[_]%'

[Skrypt]

Nothing prevents you selecting a table without prefix with that query :

IF @tablePrefix != null
    SELECT TABLE_NAME 
    FROM information_schema.tables 
    WHERE TABLE_NAME LIKE @tablePrefix + '[_]%'
ELSE
    SELECT TABLE_NAME 
    FROM information_schema.tables 
    WHERE TABLE_NAME NOT LIKE @tablePrefix + '[_]%'

But that would also delete all other tables not related with Orchard Core not starting with a prefix. The only other way I can think of right now is that we keep a list of tables owned by each tenants if we want to support no prefixes.

Though, this SQL Query would work only with SQL Server so I'm thinking that this should maybe be more generic for each DB. It would require that we be able to list all tables that have been created through migrations for each tenants through code.

[sebastienros]

When I mentioned "without loosing data" I meant for disabling a module. Unrelated.

Deleting all prefixes tables seems scary, we might delete more things that are not related, and there is a risk of prefix collection (foo_ and foo_foo). Maybe a simpler approach would be to store the list of tables that were created during the migrations. We would assume that all tables have to be created like so, but that would be an easy bet. It could also be part of a special document handled by yessql directly because it provides the SchemaBuilder. Or an event for use to save the list of tables.

Also that would simplify database agnosticism as the only thing to do is call DROP.

[Piedone]

That's a good solution too.

[barthamark]

@sebastienros I assume a part of the development would be done in the YesSQL repository.

If we go with the event handler approach, an ISchemaBuilderEvents interface with a couple of events such as TableCreated would be fired inside the SchemaBuilder.cs class and in Orchard Core there would be a special document (e.g. TenantTables) which is updated every time the ISchemaBuilderEvents.TableCreated event is fired.

When deleting the tenant we would just need to iterate through the table names stored in the TenantTables document and drop them one-by-one including the Document and Identifiers tables at the end.

How does this solution sound?

[barthamark]

@sebastienros what's your opinion on this? I'll start implementing this in YesSQL if you think this is the right approach.

[sebastienros]

CQRS? Ok to add the list of commands that are executed by the schemabuilder inside a specific document of YesSQL, and we can then use it to list all the available tables, and then delete them.

[barthamark]

Not sure if I understand this. What commands to be executed by the schemabuilder inside a specific document? My idea is to store the table names in a document (so one single document dedicated to store table names) and when deleting a tenant this document would be fetched and the list of table names would be used. Is this what you meant?

[sebastienros]

What I mean is instead of storing the created tables, store all the commands (create table, add column, delete table, ...) that the schema builder executed. This way we can still compute all the existing tables, but also what columns are available, and so on.

[jeremycook]

I am all for storing or calculating a list of tables, but here's a thought on what might be a simpler approach and depending on how you interpret "consistent" here, may be a more consistent approach.

Table Prefix

What if the table prefix is required and restricted to [a-z][a-z0-9]* characters. So no "_" and the like. Then I presume that we wouldn't have to worry about prefixes like "foo_foo". Querying for tables that start with "foo_" would be reliable.

There is always the risk that a DBA or developer manually created a table with that prefix, but even in those cases I (personally) would still expect the tables to be deleted because I would have created those tables to go along with that site. If I wanted those tables to be treated independent of the site I would create a special schema (in SQL Server terms) or invent a prefix I'm not going to use when creating new sites. In the SQLite case all bets are off, and the database file is just going to be blown away. Blowing away tables with the site's prefix in SQL Server, etc. whether or not it is tracked by the CMS or YesSql is equivalent (in my mind) to deleting the SQLite database file.

Delete Warning

Moreover, the warning message when invoking the Delete action could warn folks that all tables in database "database-connection-string" that start with "foo_" will be deleted, and list those tables that will be deleted when prompting the use to confirm the action. In the SQLite case, the message would read a little different, warning that the database file will be deleted.

Confirming Delete Event

Building upon the delete message idea, if modules can get in on the delete events then it could be valuable to have a Confirming event that is called prior to displaying the warning message, subscribers could provide a message that is specific to their module that will be presented to the user as part of the warning message.

[jeremycook]

As much as I like the uniformity of requiring a prefix. I also like tables without prefixes when there will only be one tenant per database. So tracking generated tables seems best since it allows for the option to not prefix tables (and is the safest, of course).

[hishamco]

@barthamark if you are still interested on this one please start a PR, otherwise I will schedule this on the PRs that I will work on

[JoshTango]

I found another problem. I deleted a tenant by deleting the folder and removing it from the tenants.json but the tables were still in my sql server database with the prefix for my tenant. I assume I have to remove those tables as well? I think the above instructions only work for sqllite.

[JoshTango]

I assume we only have to delete the tables and no views or stored procs?

[barthamark]

@JoshTango deleting tables is definitely a goal here as well as Media items (which can be in App_Data or in Azure Media Storage as well). You can read about the ideas in the messages above.

[hishamco]

I read all the comments there can we agree on the following:

• Deleting a tenant SHOULD remove all the things added by OC EXCEPT database tables, this including:
  1. Deleting media items
  2. Deleting shell settings entry
  3. Deleting data protection keys
  4. Deleting appsetting.json
• Add event handlers for installing, uninstalling and leave the implemention to the module itself (here we don't worry about removing the tables, because the creator knows everything about his data
• Show a confirmation before deleting, to inform the user the list in point 1 will be deleted and we could allow him to decide if he/she want to delete/undelete some of the options

Let's make this simple and make the heavy lifting to the modules themselves

[JoshTango]

Sounds good and in addition to this you could also offer an export feature and see if they want the tenant exported into a zip file somewhere.

[hishamco]

We already have import/export feature but it will be nice if we can export tenants content as zip file, but there are some chanllenges, what if the data are HUGE?

Anyhow let us hear @sebastienros and @Piedone about this to make it happen ;)

[Piedone]

Keeping DB tables intact would make this pointless and confusing. I'm still on the opinion I've elaborated under https://github.com/OrchardCMS/OrchardCore/issues/1212#issuecomment-460261478, I wouldn't leave cleaning up after themselves completely up to each module.

[hishamco]

What if we extend the YesSQl to let the migration do two things: adding the tables as well as remove them, something similar to Up & Down in EF migration. This way the job will be easier to call the Down method for every modules

[hishamco]

What if we extend the YesSQl to let the migration do two things: adding the tables as well as remove them, something similar to Up & Down in EF migration. This way the job will be easier to call the Down method for every modules