Feature/move server settings to database #542
Conversation
| get("", SettingsController.serverSettings) | ||
| patch("", SettingsController.modifyServerSettings) |
There was a problem hiding this comment.
the URL seems to make sense as is but I'm thinking that it might become confusing to API users?
There was a problem hiding this comment.
probably not possible since it would change an existing endpoint (on the other hand the frontend could quickly be updated), but would it make sense to rename "settings" to "server" and then add a "settings" sub endpoint?
at least I wouldn't consider "about" and "check-update" to be settings
e.g.:
- server
- settings
- about
- check-update
There was a problem hiding this comment.
Not a good idea, unnecessary breaking changes are pointless. There might be other private Tachidesk forks and clients we don't know about.
| object SettingsTable : IntIdTable() { | ||
| val key = varchar("key", 256) | ||
| val value = varchar("value", 4096) | ||
| val requiresRestart = bool("requires_restart").default(false) | ||
| } |
There was a problem hiding this comment.
It might be a better idea to use java.util.prefs.Preferences as storage backend instead?
- Using a database storage allows to easily make a portable Tachidesk installation (like on a usb stick)
- Using database will make you lose settings when the database gets corrupted or data is deleted
| ip = PartialSetting(value = config.ip), | ||
| port = PartialSetting(value = config.port), | ||
| socksProxyEnabled = PartialSetting(value = config.socksProxyEnabled), | ||
| socksProxyHost = PartialSetting(value = config.socksProxyHost), | ||
| socksProxyPort = PartialSetting(value = config.socksProxyPort), | ||
| webUIEnabled = PartialSetting(value = config.webUIEnabled), | ||
| webUIFlavor = PartialSetting(value = config.webUIFlavor), | ||
| initialOpenInBrowserEnabled = PartialSetting(value = config.initialOpenInBrowserEnabled), | ||
| webUIInterface = PartialSetting(value = config.webUIInterface), | ||
| electronPath = PartialSetting(value = config.electronPath), | ||
| downloadAsCbz = PartialSetting(value = config.downloadAsCbz), | ||
| downloadsPath = PartialSetting(value = config.downloadsPath), | ||
| maxParallelUpdateRequests = PartialSetting(value = config.maxParallelUpdateRequests), | ||
| basicAuthEnabled = PartialSetting(value = config.basicAuthEnabled), | ||
| basicAuthUsername = PartialSetting(value = config.basicAuthUsername), | ||
| basicAuthPassword = PartialSetting(value = config.basicAuthPassword), | ||
| debugLogsEnabled = PartialSetting(value = config.debugLogsEnabled), | ||
| systemTrayEnabled = PartialSetting(value = config.systemTrayEnabled) |
There was a problem hiding this comment.
This list of values are repeated 8 times across the code base (2 times before + 6 times now), this is too much duplication.
Some concerns
- We are cluttering the current package too much, put all settings stuff into a new sub-package
- the new setting module(s) seem too big and complected, I would split it apart heavily
- We should completely eradicate all
com.typesafe.configbased code and only use a HOCON parser to read the config file (also there is an android config somewhere that needs to be dealt with) as we are building our own config engine now - With this design we have multiple sources of truth: conf file and the database conf storage, this looks like an anti-pattern to me. No its not actually one source of truth and then an override, from user's view the conf file is the source of truth and from app's view it's the database. To fix this we might need to perform surgery and remove the conf file entirely or better than that make it the actual settings storage hence the source of truth which would fix the issue completely(also changing the format to toml or something along the way to make writing the conf file from software easy and accessible)
- As more rant about this I believe that the settings storage should be in plain text and accessible to the user, this is why we like technologies like csv, LaTeX, markdown more than their alternative binary forms. So we should either keep HOCON or switch to some other human readable format.
There was a problem hiding this comment.
the idea would be to completely remove the old config file and only use the settings from the database.
the settings would then only be changeable via the UI (and via a launcher, in case that is still planned?)
regarding the code duplication, this is so that there is a partial data class of the settings, so that via the endpoint a single setting can be changed, without having to send all settings
not sure if this can be done in a better way, haven't found anything besides this
so regarding the backend of the settings, I understand you want to keep a config file?
if java.util.prefs.Preferences should be used, if I understand it correctly, we have to implement that ourself, since by default it will be saved in the registry
There was a problem hiding this comment.
the idea would be to completely remove the old config file and only use the settings from the database.
the settings would then only be changeable via the UI (and via a launcher, in case that is still planned?)
I don't really like that idea because we have a lot of headless (docker, NAS, etc) users that prefer the sys admin conventional way, we also service home users, we really want to make things easier and more simple stupid for them (this has been the driving force behind most of my decisions) but we shouldn't turn our back to those lovely power users.
regarding the code duplication, this is so that there is a partial data class of the settings, so that via the endpoint a single setting can be changed, without having to send all settings
not sure if this can be done in a better way, haven't found anything besides this
@Syer10 any ideas?
I could try to come up with something when I have the time.
so regarding the backend of the settings, I understand you want to keep a config file?
Yes, as explained before.
if java.util.prefs.Preferences should be used, if I understand it correctly, we have to implement that ourself, since by default it will be saved in the registry
About that one I was just throwing the an idea, now that I think about it it might not be the best idea?
- Java prefs are something that is kept hidden away and it's not an industry standard (besides in android)
- Keeping the config in cleartext and accessible is almost a must, even though one of our regulars on discord likes to play with the database file but that would be annoying to anyone else.
| Settings.init() | ||
|
|
||
| Settings.setup() | ||
|
|
||
| Settings.update(Settings.fromConfig(serverConfig)) |
There was a problem hiding this comment.
This seems too complicated, we need one entry point, the steps here are a blackbox to ServerSetup
| Settings.put(property.name, setting) | ||
| } | ||
|
|
||
| inline operator fun <R, reified SETTING> getValue(thisRef: R, property: KProperty<*>): SETTING { |
There was a problem hiding this comment.
Not everyone is familiar with KProperty, maybe some comments with reference to external docs would be helpful
|
new approach #545 |
No description provided.