Added settings manager #1645

mamaso · 2016-04-26T18:53:50Z

Addresses some of the comments in #1559

Highlighting relevant pieces:

Two relevant configuration settings:

enableConfigChanges: on/off for the whole dynamic settings piece, default off
lockDefinedSettings: on/off for locking config specified settings, default on.

Settings Manager

settings / cache / db management

Parse Server Constructor:

db settings pulled
cached settings updated if necessary
cached settings pushed to db
ParseServer.config.settingsInitialized promise to track state (code smell, but needs a more significant api change otherwise)

SettingsRouter:

All ops require master key
POST
- return error if enableConfigChanges is not true
- updates values & persists IFF current & update are different
- returns object with only values which were updated
GET
- returns all unlocked settings
- if a setting is undefined in server configuration, it will appear as null

middlewares.handleParseHeaders:

fetches database configuration & updates cache once per request IFF enableConfigChanges true

drew-gross · 2016-04-26T19:07:02Z

src/SettingsManager.js

+
+  function getSettingsCollection() {
+    let config = cache.apps.get(appId);
+    return database.getDatabaseConnection(appId, config.collectionPrefix).adaptiveCollection(settingsCollectionName);


We don't want to introduce new uses of collectionPrefix inside Parse Server. You should go through the database adapter to retrieve the collection, that will apply the prefix automatically, and also let you avoid referencing the appId in here, I think.

Can you give me a code snippet? Not sure how I can get access to the current dbController without the getDatabaseConnection code.

once you have a config it's just config.database.adapter. You can see the API in MongoStorageAdapter and MongoCollection although I'm changing a lot of stuff in there recently.

drew-gross · 2016-04-26T19:21:14Z

A few suggestions but generally this looks good. Also looks like there are a few failing tests to fix.

ghost · 2016-04-27T17:47:26Z

@mamaso updated the pull request.

codecov-io · 2016-04-27T18:03:54Z

Current coverage is 92.21%

Merging #1645 into settings-endpoint will increase coverage by -0.58%

4 files in src were modified. more
- Misses +3
- Partials -1
- Hits +14
9 files (not in diff) in src/authDataManager were modified. more
- Misses +11
- Hits +13
15 files (not in diff) in src/Routers were modified. more
- Misses +35
- Hits +95
10 files (not in diff) in src/LiveQuery were modified. more
- Misses +7
- Hits +44
8 files (not in diff) in src/Controllers were modified. more
- Misses +1
- Hits +72
3 files (not in diff) in ...apters/Storage/Mongo were modified. more
- Misses +1
- Hits +28
2 files (not in diff) in src/Adapters/Logger were modified. more
- Misses +7
- Hits +2
2 files (not in diff) in src/Adapters/Files were modified. more
- Misses +4
- Hits +12
2 files (not in diff) in src/Adapters were modified. more
- Misses +3
14 files (not in diff) in src were modified. more
- Misses +17
- Partials -9
- Hits +183

@@           settings-endpoint      #1645   diff @@
===================================================
  Files                     87         88      +1   
  Lines                   5455       6098    +643   
  Methods                    0       1077   +1077   
  Branches                1005       1212    +207   
===================================================
+ Hits                    5063       5623    +560   
- Misses                   382        475     +93   
+ Partials                  10          0     -10

Powered by Codecov. Last updated by 4462145...430a61f

drew-gross · 2016-04-27T18:46:21Z

spec/helper.js

@@ -43,9 +43,9 @@ var defaultConfiguration = {
 };

 // Set up a default API server for testing with default configuration.
-var api = new ParseServer(defaultConfiguration);
+global.parseServerObject = new ParseServer(defaultConfiguration);


Can you adjust these tests to not need global variables please? I'd suggest fetching everything you need for tests from the HTTP api rather than looking at the object itself. Then it's more of an end-to-end test as well.

Yeah, this is an ugly side effect of the pull/update/push promise behavior that happens in the constructor.

Adjusted so that setServerConfiguration returns the constructed parse server object instead of using a new global, is that a fair compromise?

The tests are split into two groups: testing ParseServer constructor behavior & testing the settings router e2e. Testing the constructor behavior benefits from having the constructed Parse Server object.

yeah that seems like a good solution.

drew-gross · 2016-04-27T19:09:05Z

Cool, just a couple more changes. We usually use () for anonymous functions that take no arguments instead of _, _ is actually a valid variable name in JavaScript so using _ could be a little more confusing and potentially abused later. No need to change it in this PR (unless you feel like it) but in future ones could you use that style?

mamaso · 2016-04-27T20:50:29Z

Yeah, that makes a lot of sense why one might avoid _, I'll add in that change as I'm making some anyway.

A question for you: do you have any ideas for how to make hidden/locked settings easier to manage? I considered going with a convention based approach where if a new setting was added to the cache that had a name containing 'manager' or 'controller' we would treat it as hidden. Ideally this settings piece would automatically work for new settings without any code changes.

drew-gross · 2016-04-27T21:17:18Z

I think the safest option for now would be to have a whitelist of settings that appear. That way we can avoid any unintentional effects, and as we see how people use this in practice and how the dashboard uses the settings, we can get a better feel for the problem and what might make sense for future default behaviour. I'm also beginning to wonder if it might actually be better to see if we can simplify the 2 additional settings into 1 or even 0 additional settings for now, so that we can what problems have been solved and what problems aren't yet solved before adding more complexity.

If you haven't looked at the /serverInfo endpoint yet, I'd suggest you check that out as well, it's used extensively by the dashboard and might be relevant here.

ghost · 2016-04-28T16:05:49Z

@mamaso updated the pull request.

ghost · 2016-04-28T16:11:51Z

@mamaso updated the pull request.

mamaso · 2016-04-28T22:02:52Z

The CI errors seem to be inconsistent timeout issues with various tests, don't think that there's anything significant there

blacha · 2016-04-28T23:38:54Z

spec/helper.js

-  server = app.listen(port);
+    // the configuration hasn't changed
+    if (configuration === currentConfiguration) {
+        return;H


Don't think that H should be there

Good catch, guess that slipped in but didn't cause an issue

mamaso · 2016-05-06T20:22:15Z

No problem, I can agree with that!

drew-gross · 2016-05-06T20:22:47Z

Pinging @davimacedo who is building Parse Server hosting so probably would be using this API extensively.

gfosco · 2016-05-07T21:45:40Z

Looks great! 👍

davimacedo · 2016-05-07T22:32:01Z

@drew-gross It's really a great work of @mamaso and for sure it will help Parse Hosting solutions and also future community contributions regarding ideas on parse-community/parse-dashboard#188

My unique suggestion for future contributions is to add more tests: for each setting we could update it and verify the expected behaviors.

drew-gross · 2016-05-09T17:58:38Z

src/cli/cli-definitions.js

+    help: "Sets the log level"
+  },
+  "enableConfigChanges": {
+    env: "ENABLE_CONFIG_CHANGES",


We've been discussing in #1715 what things to expose as env vars, as I think we want to use less env vars in the future. What do you think about making this not be an env var?

Also, what about defaulting to true? Then the dashboard can just work out of the box without needing to change any config?

Or even just removing this setting, and having all settings be editable unless defined in code/cli. The endpoint requires the master key anyway, so it wouldn't be a security issue, and you could always just define everything in code if you want to disable editing even for master key.

Only concern with removing the setting is that enabling config changes adds an extra db call per request. The setting is the way to provide a simple switch to opt out of that penalty.

I'm happy to default to true, whether that's by removing the setting or otherwise.

Ah good point. Lets leave the setting and have it default to enabled then, and also add to the docs that it exists primarily for optimization purposes. Parse philosophy is typically to prioritize better user experience over most other thing, and I think having the dashboard work out of the box for changing settings is better user experience. Also I think given that optimization is the purpose of this setting, we don't need the env var.

Where should I add the documentation on this? Create a wiki page?

ghost · 2016-05-09T20:46:49Z

@mamaso updated the pull request.

ghost · 2016-05-09T21:05:48Z

@mamaso updated the pull request.

ghost · 2016-05-13T20:12:16Z

@mamaso updated the pull request.

drew-gross · 2016-05-13T20:33:36Z

src/SettingsManager.js

+  logLevel: settings => configureLogger({ level: settings.logLevel }),
+  verbose: settings => {
+    settings.logLevel = 'silly';
+    configureLogger({ level: settings.logLevel });


Shouldn't this only be done when the setting for verbose is true?

yep, good catch

Not a fan of having both logLevel & verbose settings as the pair leads to weird interactions, but not sure how better to handle it

ghost · 2016-05-13T20:40:59Z

@mamaso updated the pull request.

drew-gross · 2016-05-13T20:42:52Z

Awesome! Will merge assuming tests pass. Docs can go in the README. Feel free to make that a separate PR.

mamaso · 2016-05-13T20:44:40Z

Just a heads up, this pr is against the ParsePlatform:settings-endpoint branch so there are likely some merge issues. Want me to point it at master instead?

drew-gross · 2016-05-13T21:00:38Z

I just merged it. We can do a new PR against master after rebasing.

faviomob · 2016-08-10T19:14:34Z

Guys, any progress here? I've deployed Parse Dashboard and still can't see AppSetting tab there =(

mamaso · 2016-08-10T19:47:59Z

Unfortunately my focus has shifted elsewhere and this didn't make it out of the feature branch :/ happy for someone else to pick it up if they like

mario · 2016-11-09T19:30:15Z

@mamaso what remains to be done, and where's the branch? :)

Cliffordwh · 2016-11-30T17:54:50Z

Any news on this?

Added settings manager

83151b7

ghost added the GH Review: review-needed label Apr 26, 2016

drew-gross reviewed Apr 26, 2016
View reviewed changes

drew-gross added GH Review: needs-revision and removed GH Review: review-needed labels Apr 26, 2016

Clarity improvements & test fixes for settings manager

bda6744

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Apr 27, 2016

drew-gross reviewed Apr 27, 2016
View reviewed changes

drew-gross added GH Review: accepted and removed GH Review: review-needed labels Apr 27, 2016

Updated cli-defs, features router, removed global test object

b4fea9e

Fixed cli definitions syntax error

f2f8c79

mamaso closed this Apr 28, 2016

mamaso reopened this Apr 28, 2016

blacha reviewed Apr 28, 2016
View reviewed changes

drew-gross reviewed May 9, 2016
View reviewed changes

drew-gross mentioned this pull request May 9, 2016

Adds settings endpoint to configure loglevel #1418

Closed

drew-gross added GH Review: needs-revision and removed GH Review: accepted labels May 9, 2016

Removed ENABLE_CONFIG_CHANGES env var & improved doc

6a428ec

ghost added GH Review: review-needed and removed GH Review: needs-revision labels May 9, 2016

Disable config changes for most tests

bf45772

test fixes

f6a8950

drew-gross reviewed May 13, 2016
View reviewed changes

Correct handling of verbose

92dd715

drew-gross added GH Review: accepted and removed GH Review: review-needed labels May 13, 2016

drew-gross merged commit 993ff20 into parse-community:settings-endpoint May 13, 2016

fantasy8-me mentioned this pull request Oct 19, 2016

Is App-Settings included in dashboard parse-community/parse-dashboard#558

Closed

Added settings manager #1645

Added settings manager #1645

Conversation

mamaso commented Apr 26, 2016

Two relevant configuration settings:

Settings Manager

Parse Server Constructor:

SettingsRouter:

middlewares.handleParseHeaders:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drew-gross Apr 26, 2016 • edited Loading

Choose a reason for hiding this comment

drew-gross commented Apr 26, 2016 • edited Loading

ghost commented Apr 27, 2016

codecov-io commented Apr 27, 2016 • edited Loading

Current coverage is 92.21%

drew-gross Apr 27, 2016 • edited Loading

Choose a reason for hiding this comment

mamaso Apr 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drew-gross commented Apr 27, 2016 • edited Loading

mamaso commented Apr 27, 2016

drew-gross commented Apr 27, 2016

ghost commented Apr 28, 2016

ghost commented Apr 28, 2016

mamaso commented Apr 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamaso commented May 6, 2016

drew-gross commented May 6, 2016

gfosco commented May 7, 2016

davimacedo commented May 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 9, 2016

ghost commented May 9, 2016

ghost commented May 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 13, 2016

drew-gross commented May 13, 2016

mamaso commented May 13, 2016

drew-gross commented May 13, 2016

faviomob commented Aug 10, 2016

mamaso commented Aug 10, 2016

mario commented Nov 9, 2016

Cliffordwh commented Nov 30, 2016

drew-gross Apr 26, 2016 •

edited

Loading

drew-gross commented Apr 26, 2016 •

edited

Loading

codecov-io commented Apr 27, 2016 •

edited

Loading

drew-gross Apr 27, 2016 •

edited

Loading

mamaso Apr 27, 2016 •

edited

Loading

drew-gross commented Apr 27, 2016 •

edited

Loading