Keep yessql.db as the fallback name for sqllite #15155
Conversation
|
Please try the PR on your old database, it SHOULD work |
| Cache = SqliteCacheMode.Shared, | ||
| Pooling = sqliteOptions.UseConnectionPooling | ||
| }; | ||
| var dataSource = Path.Combine(databaseFolder, sqliteOptions.DatabaseName); | ||
| if (!File.Exists(dataSource)) |
There was a problem hiding this comment.
I don't this this is a good idea to do IO operation for this. I still like the approach I recommended in the issue better
There was a problem hiding this comment.
This runs on the first request to the tenant if I'm not wrong
There was a problem hiding this comment.
You want to store the database name in the shellsetting config file instead. The issue will still remain if people are not changing it.
There was a problem hiding this comment.
If value is null from shellsettings then return "yessql.db". That could work. But you are adding a new setting that was not necessary before. Is the intent to add that config to the shellsettings to make this dynamic too?
There was a problem hiding this comment.
It makes sense if the intent is to make this name configurable. I thought that the intent was to only change its default name. But yeah, to be honest I don't know what is the added value of making this name configurable from the shellSettings json file. Why would someone want to rename that file is the question?
It has always been a fixed value since the beginning but I'm not against the idea. Though, it will add complexity and it needs to be documented and it needs to throw if the DB is not found when someone puts in a random name in these shellshettings.
There was a problem hiding this comment.
We might you the tenant's name while it's safe, the idea at the beginning to rid-off the yessql
There was a problem hiding this comment.
@Skrypt I remember the configuration was done mainly for backward compatibility, so you can yessql.db from within the configuration
For that this PR might not necessary
There was a problem hiding this comment.
What I don't like about making the Databasename configurable from the appsettings.json file is that it adds a failure point that was not there before. If we do this then at least we need to throw/log ...
There was a problem hiding this comment.
I don't think we should care. If someone decided to screw up their appsettings file then their app won't work. Same as changing the tenant name or anything else like tablePrefix. I think we should use appsettings and assume that the file matches the name if one found otherwise yessql. I submitted a PR that fixes this issue #15157
|
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. |
Fixes #15154