Keep yessql.db as the fallback name for sqlite

[MikeAlhayek]

@hishamco @Skrypt

Fix #15154

[MikeAlhayek]

@hishamco note here I am using orchardcore.db as per suggestion from @sebastienros during meeting today.

[Skrypt]

I will leave that one for @sebastienros to decide 😄

[hishamco]

@hishamco note here I am using orchardcore.db as per suggestion from @sebastienros during meeting today.

Usually, I see the recording and then react to feedback suggestions, but you didn't mention that's the meeting suggestion feel free to close my PR

[MikeAlhayek]

The meeting suggestion to lower case the name. This PR provides the fix I described in the issue.

[hishamco]

There are a lot of changes in your PR that might not be necessary, I agree with @Skrypt that we might need some sort of logging or validation

Again while the IO check happened once I don't think it's a big issue

[MikeAlhayek]

@hishamco can you give me an example of an unnesasry change? This PR make renaming the file seemless without any breaking change. "Breaking change that we brought to main already".

I still don't think we should be doing the IO check if we don't have to. Validation should not be needed. We also don't validate if someone changes other appsettings like site name, or database prefix... if you have access to the appsettings and make direct changes, we should assume that you know what you are doing and not add validation for that.

[Skrypt]

Those are all valid arguments but in the end what we want is to make things easier for ITs that don't know that these settings may have a typo. If the app fails silently then who's going to go and debug the issue? You every time or an IT guy that will more likely look at the app logs to find what the issue might be?

Now, here we are introducing a new failing point and on @hishamco PR we are doing an I/O check ... it's all about compromise here. I'm not against each of these submitted PR's so stop wasting time and leave it for @sebastienros to decide 😉

[Skrypt]

To be simpler. Test it, Rename the appsetting databasename and see what the app logs or not.

[hishamco]

According to the documentation, this should fix the issue:

{
    "OrchardCore_Data_Sqlite": {
        "DatabaseName": "yessql.db",
        "UseConnectionPooling": false
    }
}

[MikeAlhayek]

@Skrypt if the file is missing, yessql will throw exception. That is enough for someone to know they screwed up the appsettings. I still don't believe we should have a check. In the framework if we check for everything IT does, we'll just bog down the framework.

Perhapse, we should just undo that PR that introduced the breaking change and keep the file called yessql.db and we don't need to worry about customizing the name :)

[hishamco]

I'm not sure what's the wrong with the above settings while its documented? we could close both PRs :)

[MikeAlhayek]

@hishamco the problem is that it is a breaking change that could be avoided.

[hishamco]

You can if you set the settings as I mentioned in #15157 (comment)

[Skrypt]

I just tried it and I get nothing in the logs. I just get a 404 page when debugging the website with VS Code. And I tried with this branch. I added "DatabaseName": "OrchardCore.db" in the appsettings.json file of my tenant and I renamed the database to OrchardCore2.db

Here is my log:

2024-01-24 13:46:05.8801|||14|Microsoft.Hosting.Lifetime|INFO|Now listening on: https://localhost:5001 
2024-01-24 13:46:05.8988|||14|Microsoft.Hosting.Lifetime|INFO|Now listening on: http://localhost:5000 
2024-01-24 13:46:05.8988||||Microsoft.Hosting.Lifetime|INFO|Application started. Press Ctrl+C to shut down. 
2024-01-24 13:46:05.8988||||Microsoft.Hosting.Lifetime|INFO|Hosting environment: Development 
2024-01-24 13:46:05.8988||||Microsoft.Hosting.Lifetime|INFO|Content root path: F:\Repositories\OrchardCore\src\OrchardCore.Cms.Web 

Don't assume, test it. 😠

[Skrypt]

And the worst part is that it created a new OrchardCore.db file in my tenant folder...

[MikeAlhayek]

@Skrypt if you tested this PR, then it assumes that your file is yessql.db. If the file is called anything else then it should be in the settings file.

In the settings file you can have
{
"DatabaseName": "jasmin.db"
}

and the file should be jasmin.db. If you don't specify anything then it would be yessql.db. if you create a new site, then the file will be called orchardcore.db and the same name will be found in the settings.

[Skrypt]

All my point is about making a typo on that DatabaseName since the beginning. I tested the use case and I'm reporting you the issue. You said it would log ;it doesn't and it creates a new database named as what I'm adding in the appsettings.json file of my tenant instead of just throwing at the right place before creating a new database that will try to initialize a new setup. The worst part is that it doesn't even get me to the setup page. I get a 404 ... that's not good ... we can do better.

Don't want to argue more on this.

[sebastienros]

get; set;

[sebastienros]

Please revert the PR that changed the database name. We'll put it back when the migration path is well-defined and the implementation is not brittle.