Make Sqlite database name configurable

[MikeAlhayek]

@sebastienros based on the last discussion we had about this change.

This PR sets the default database to orchardcore.db for new sites. Existing site that were created in the past using yessql.db will still work as before using yessql.db. This all happens seamlessly and does not introduce any breaking change.

/cc @hishamco

[hishamco]

This is similar to #15157

[MikeAlhayek]

It is with the minor changes requested by @sebastienros

[Skrypt]

Does it do the same behavior than your previous PR when you change the DatabaseName to a different one and that the DB is not found?

[MikeAlhayek]

@Skrypt yes. if you want to modify the DatabaseName in the appsettings, you'll need to also ensure it matches the name of the file. There is no need for you to change anything manually and everything works seamlessly including backward. But, if you have permission to manually modify the DatabaseName or the appsettings manually, you'll also have to carry that same change on the filename to ensure you use the same datafile. Also with this approach, you can change the database name to anything you want like jasmin.db

[Skrypt]

Yeah, I understand that. But my question was: does it create a new database when someone does a typo in that DatabaseName? Did you handle that use case by throwing an exception like I asked or is it something that @sebastienros said that was not necessary to be done?

[MikeAlhayek]

no we are not throwing an exception if the user decided to have a typo where the filename does not match the settings. I think @sebastienros was okay not to check if the file exists on every startup when we reviewed the 2 PRs. But, I am not 100% positive since we decided to revert the breaking change meanwhile.

[Skrypt]

Thanks for for answer. I don't agree with this.

It leaves the app in a state that it is hard to diagnose what is happening. Working in a team of multiple people here and knowing that this could happen that someone pushes an update to the settings on the repository and getting a 404 without any logs is the kind of things that wouldn't make my life easier.

Really hard to understand.

[Skrypt]

sebastienros/yessql#530

[MikeAlhayek]

@Skrypt yes that would be the responsibility of Yessql not OC.

IMO, I do not think this is something we should worry about. It is the same behavior as if the user decided to rename the file today, they will experience the same behavior. From OC stand point, the user should be super careful when updating the settings files or database documents manually. We'll assume if they are making these kind of changes, they know what they are doing. In other words, if you mess up the JSON object in your database your app will blow up. The same logic we follow with all of the settings including protection keys and other appsettings or the tenants.json.

[Skrypt]

We just had an issue today where our blob storage got emptied. If we would have use SQLite then it would have returned a 404 page for all our client websites. It's not about a dev making the change that is my concern. It's about the fact that it can now be changed. Which means a bot could try and change this... and it would take some time for an IT guy to find what is the issue without any logs.

Hopefully, we don't have security holes in OC "cough cough".

I just don't agree with you on saying... that's the responsibility of this guy that makes the change... what if he dies? 😄 🤡

[MikeAlhayek]

what if he dies?

They he would not have broken the site by manually logging into the file system and modifying it. lol

changing appsettings will result in app restart. so you'll immediately know you did something wrong. I am not against checking if the file exists or not. I just don't think this is a concert OC should worry about. I would like to see that in YesSql instead.

[Skrypt]

I don't remember all the details of what YesSQL does from memory. But I think it takes the connection string and creates a DB if it's not found. YesSQL will add only the document table to it because it needs it. Then, it won't initialize a setup request on Orchard Core. Leaving you with a 404 page without any logs. Suggestion would be to at least log when YesSQL creates a new database but I'm not even sure that it is possible.

That's the whole story of why me and "someone else" did not move forward with this change a while ago.

And you can see @sebastienros answer on the issue I just created on YesSQL repository about it.

so you'll immediately know you did something wrong.

Not when you are changing an appsettings.json file. It will pick it up on the next app restart. Which could happen a whole lot of time after.

[MikeAlhayek]

what do you want to happen if the the IT admin renamed the existing yessql.db file before he died? Or if your bot renamed it for you?

[Skrypt]

I want to have logs. If we can't have logs and we allow modifying the database name and that we can't find a solution with YesSQL then I would simply not allow renaming the database name. I feel like It's going into a full circle back again. I think you understand what I want; but we are going to get another round at argueing about this untill someone wins with the strongest arguement.

I exposed all the facts here. I don't understand why we would take this PR and merge it if it has never been accepted as a solution before.

Logs or a clear path where that database doesn't get created with only a "document" table inside of it and that we don't get to a 404 page when restarting the app pool. I did say that right?

[MikeAlhayek]

It's not about winning an argument here.

Others seem to want to be able to rename the database name. I don't mind having that flexibility as long as we do not introduce a breaking change.

Managing the database is. Yessql responsibility not OC.

Today, if you rename yessql.db to something else, you'll experience the same behavior and what this PR does. So there is no disadvantage presented by this PR.

Yessql already has a flexibility of allowing any name you want. Since that is a feature in yessql, I see no reason to not support it in OC.

[Skrypt]

Yeah, but today we expect having a yessql.db file in that folder. Not "any named" file. Somehow we never create any database with SQL Server maybe we should do the same with SQLite if that's even possible. This SQLite thing has always been an issue. YesSQL doesn't really manage DB's it just throws queries to it. So far, we never remove/create DB's with it unless it is SQLite.

https://www.connectionstrings.com/sqlite-net-provider/disable-create-database-behaviour/

[MikeAlhayek]

Yes but yessql does not care. Anyway, I had my peace with this argument. If more than one person disagree with this PR, feel free to close it. I see no disadvantage of margining it and giving the people that need it the option.

@sebastienros @Piedone please share your thought here. Do you support allowing renaming the yessql.db in OC or not.

[Skrypt]

Keep the PR opened. But it definetely needs also an upgrade to a fixed version of YesSQL if we ever merge it. Means, we need to find a way to make it work properly if there's any way. I'm going to keep throwing ideas on the YesSQL issue.

[Piedone]

What do you mean by "But it definetely needs also an upgrade to a fixed version of YesSQL if we ever merge it", Jasmin?

I think that we had put an immense amount of effort into this, with now 4 PRs, for something that nobody actually ever asked for. I'm OK with merging this (with the above tiny remark), but not with spending any more of my time on it and I suggest the same to you.

[Skrypt]

I'm moving forward to something else. I'm done wasting my time on this one. 😉
These 4 PR's should never have existed in the first place.

Even if @sebastienros said that he wanted this as a solution in the last meeting. I have the right to disagree.

But you all have the right to approve the PR and merge it too 😉