Added .get, renamed .getAll as .getRaw, added new .getAll method

[bevacqua]

As pointed out by @Bargs in #7301, the .get method was missing from the server-side settings config module. I took the liberty of also changing .getAll into .getRaw since it returns the entire manifest objects for the settings (includes descriptions, user-provided, and default values), and added a new .getAll implementation where an object of configuration key/value pairs is provided.

Note: .getAll wasn't being used anywhere anyways.

[Bargs]

What would you say to adding a cache for these settings? One thing that was unexpected/frustrating when I first tried to use this API was that the get methods returned promises. If the entire app is going through this API to make changes, it shouldn't be a problem knowing when to invalidate the cache for a given key.

[bevacqua]

Sounds good but also would need a hook to learn about changes made directly against the index?

[Bargs]

We should probably have some tests for these higher level methods.

[Bargs]

@bevacqua do we still have code that directly accesses the config in the .kibana index? Or are you thinking of user's modifying it themselves outside of Kibana?

[bevacqua]

I meant users. I was told to trust no one.

[Bargs]

Haha ok. I think there's a case to be made for users modifying Kibana internal data at their own risk, but it's not a huge deal, I'm ok with leaving it as is if we wanna play it safe.

In that case the code looks good, but I would like to see some tests that exercise those getters at least.

Also one other note, since I am using getAll in the feature/ingest branch, we'll need to keep that in mind depending on which PR gets merged first. It'll probably be yours and you won't have to worry about it, but in the off chance feature/ingest gets in first we'll need to update that usage in this PR.

[bevacqua]

I'd say block your issue on this one and I'll add tests so we can merge it before you do yours. Can you x-ref them?

[Bargs]

Yeah left a note on my PR. Like I said, very unlikely that one will be ready first anyways.

[bevacqua]

@Bargs

• Added a metric ton of tests
• Updated your API usage in master (via fba84993)
• Added .removeMany method
• Plus the original .get, .getRaw, and .getAll shuffling mentioned in OP

[bevacqua]

jenkins, test it

[cjcenizal]

I think these tests can be simplified by creating a separate "returns a promise" test that makes this kind of assertion instead of repeating it within each test. This will make each test's description more accurate, make the tests more readable since they will be testing "just one thing", and the tests themselves will be deflated a bit.

describe('#setMany()', () => {
    it('returns a promise', () => {
      const { uiSettings } = instantiate();
      const result = uiSettings.setMany({});
      expect(typeof result.then).to.be('function');
    });

    it('updates a single value in one operation', () => {
      /* ... */
    });

    it('updates several values in one operation', () => {
      /* ... */
    });
  });

[bevacqua]

Totally. Split out assertions into separate tests.

[Bargs]

jenkins, test it

[bevacqua]

@Bargs 🏓 (Ping)

[Bargs]

Do we know what promise implementation is returned? If so, expect.js's constructor assertion could be used instead of duck typing:

expect(result).to.be.a(Promise);

[Bargs]

One thing that's not obvious to me from the tests or the documentation is the difference between getRaw and getUserProvided or why I should prefer one over the other. It seems like getUserProvided doesn't merge the config in ES with the defaults, but if that were the case I don't see why the results are returned with nested objects with userValue keys. If getUserProvided only returns user values, couldn't it just return a flat object?

[Bargs]

Just a suggestion: expect.js's eql method only checks for loose equality. For strict and deep equality checks I usually like to do expect(_.isEqual(result, expected)).to.be.ok(); This might be especially important since some of the tests are dealing with null values.

[Bargs]

@bevacqua just a few minor questions/comments

[bevacqua]

@Bargs .getUserProvided() is leveraged by the REST API to respond with what's changed. The defaults aren't resent to the client as they aren't going to change unless the app gets restarted. That function uses the ES model. The .getDefaults() method uses the same model. .getRaw() also uses the same model, merging both user configuration in ES and defaults. Then there's .getAll(), which takes the output from .getRaw() and turns it into key value pairs as you mentioned.

The non-flat methods aren't flat because the metadata is used on the client-side, and to maintain the format for easy merging

[bevacqua]

Oh, wow, I didn't expect .eql to be that loose.

[Bargs]

Yeah, it sucks... that's why I'd still like to switch to Chai.

[bevacqua]

@Bargs Added comments, used .isEqual

[bevacqua]

jenkins, test it

[Bargs]

jenkins, test it

[Bargs]

Pending tests passing, LGTM

[bevacqua]

jenkins, test it

[bevacqua]

jenkins, test this

[cjcenizal]

LGTM