"Restart Etherpad" in settings.json editor in admin does not remove deleted fields

[orblivion]

Describe the bug If I remove a field in the settings.json editing Admin panel and click "Save" followed by "Restart Etherpad", the removed field will not be reflected in the effective settings as seen in the site's behavior. New fields and changes to existing fields are reflected, however. Once the server is manually restarted (ctrl-c, re-run), the removed fields are reflected in the effective settings as expected.

To Reproduce

Steps to reproduce the behavior:

1. Open a pad and watch your console. You'll see log items, including INFO.
2. Go to /admin/settings
3. Change "loglevel" to "ERROR"
4. Click "Save" and "Restart Etherpad"
5. Reload pad and watch your console. You'll see no log items (assuming no errors).
6. Go back to /admin/settings
7. Comment out "loglevel"
8. Click "Save" and "Restart Etherpad"
9. Reload pad and watch your console. You'll still see no log items (assuming no errors).
10. Ctrl-C to quit Etherpad, and run it again.
11. Reload pad and watch your console. You'll again see log items, including INFO.

Expected behavior

When you uncomment "loginfo" and save and restart the server, it should behave according to its default value of "INFO".

Desktop (please complete the following information):

• Debian on QubesOS
• Firefox
• 68.11.0esr (64-bit)

[orblivion]

To be clear, the core of the error is in what is meant by "restart etherpad". There are two options:

#1 Kill the etherpad server, and start it again #2 Click the "Restart Etherpad" button in the admin

For #1, there is no error. The error comes when I do #2.

As you suggested, I tried editing settings.json using vim (I'm on Linux), but then clicked "restart etherpad" in the admin. (I didn't edit settings.json in the admin or click "Save Settings"). I found the same error. So, I'm pretty sure that the error is to do with the manner of restarting Etherpad rather than the manner of editing or saving settings.json.

[JohnMcLear]

@orblivion yeah this sounds about right, I haven't looked at it yet but I imagine logic is something like this

var foo = "bar";

then you remove foo from settings.json and when it reloads foo is still bar because we don't nullify values that aren't present.

The fix here would be to nuke settings with settings = {}; then repopulate. I imagine we just populate.

@orblivion can you look into this and put a patch in or would you prefer if a core team member did? Afaik it should be pretty easy to fix..

[orblivion]

Sure, I'll look into it.

[orblivion]

Not that easy to do a good fix. The way it's designed, the settings blob is the module itself. I.e. settings.maxAge is known as exports.maxAge within Settings.js:

https://github.com/ether/etherpad-lite/blob/develop/src/node/utils/Settings.js#L204

By your method I would have to nuke it with exports = {}. 2 problems:

• It would delete the methods exported by Settings.js, such as reloadSettings().
• It would nuke the only copy of the default value for each setting, such as seen in the above link. I would need those default values for the very problem I'm trying to solve here.

The solution I'm gonna go for is to make all of those default settings to be defined more like:

exports.defaults.maxAge = ...

Then, on every settings reload (including initial load of the site) I'll "nuke" the settings by writing all of the exports.default fields onto exports, before going on to whatever custom stuff is in settings.json and the credentials file.

Will definitely need careful review!