Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[uiSettings] make service request based #12243

Merged
merged 19 commits into from
Jun 12, 2017

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Jun 8, 2017

Closes #12047

Summary:

  • removes server.uiSettings() (throws error with instructions)
  • Adds request.getUiSettingsService() which returns a unique instance of UiSettingsService per request
  • Methods on UiSettingsService no longer require a request object
  • Adds server.uiSettingsServiceFactory(options) which can be used to create an instance of UiSettingsService with a custom callCluster() function

@spalger spalger changed the title Ui settings/request based Make uiSettings service request based Jun 8, 2017
@spalger spalger force-pushed the ui-settings/request-based branch from ddc49d2 to 8ddd0cf Compare June 8, 2017 22:31
@spalger spalger force-pushed the ui-settings/request-based branch from a688112 to c4e0b35 Compare June 8, 2017 22:38
@spalger spalger force-pushed the ui-settings/request-based branch from c4e0b35 to b74e71e Compare June 8, 2017 22:44
@spalger
Copy link
Contributor Author

spalger commented Jun 8, 2017

@kjbekkelund @azasypkin in case you looked at this before I added the tests, this is ready for review now.

@spalger spalger changed the title Make uiSettings service request based [uiSettings] make service request based Jun 9, 2017
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @spalger, sorry for the delay! I've reviewed everything except for the ui_settings tests - will finish reviewing them on Monday.

I've left several questions (mostly related to the changed behaviour) and just a few nits.

* Create an instance of UiSettingsService that will use the
* passed `callCluster` function to communicate with elasticsearch
*
* @param {Function} callCluster should accept a method name as a string and a params object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: doesn't look like that it really describes the arguments this function expects. Could you please fix?

By the way I've noticed that Kibana's Style Guide doesn't set JSDoc rules, that IMO we need to set at some point. It would be easier to auto-generate API docs eventually (at least for "public" API, that is available to plugins) and keep the same style across the teams.

* @param {Function} callCluster should accept a method name as a string and a params object
* @return {UiSettingsService}
*/
export function uiSettingsServiceFactory(server, options = {}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: since we have default value for options that means it's optional, but is callCluster function really optional?

As a side note, I usually try to avoid property-bag-as-the-only-function-argument. From my experience developers who will be changing this class later on will most likely add new argument to that catch-all bag without even thinking what the role new argument has (required or just optional). Same thing for users of this class/function, it's difficult to say what combination of options is considered valid and what is not just by looking at the function signature :)

Copy link
Contributor Author

@spalger spalger Jun 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note, I usually try to avoid property-bag-as-the-only-function-argument.

Yeah, I generally do my best to keep function arguments to a one/two values, but when there are more, or some/all of them are optional, I move to "named args" for a couple reasons:

  1. doesn't lead to multi-line argument lists
  2. makes it easier to specify a couple args and not all of them
  3. makes it really easy to merge arguments/provide defaults in wrappers
  4. I find it really easy to read when the options value is destructured as the first statement in the code
  5. generally leads to using the same name for values in all uses/locations

}

export class UiSettingsService {
constructor(options = {}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: same question as I left in uiSettingsServiceFactory, are all these arguments really optional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ I think we should stay away from optionals like this as much as possible, and if we want to define one it should list all the key/value pairs with their defaults.

// If the ui settings status isn't green we shouldn't be attempting to get
// user settings, since we can't be sure that all the necessary conditions
// (e.g. elasticsearch being available) are met.
const readUiSettingsInterceptor = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional nit: maybe that's just me, but when function returns results of different types it's a potential bug IMHO. I used to return null for nullable return type if I don't have anything to return.

I know it's very arguable, but what do you think?

Copy link
Contributor Author

@spalger spalger Jun 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm theoretically a huge fan of never using undefined to represent nothing and using null instead (reserving undefined to mean "uninitialized" or similar).

In practice though I find that opinion pretty controversial and think it's somewhat impractical to require. Also, requiring interceptors to return null seems like an unexpected API, based on my experience with such things... In general I treat null and undefined as equivalent, which is why I use == null a lot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, got it, sounds like a team-wide agreement :)

}

async _read(options = {}) {
const interceptValue = await this._readInterceptor(options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: do we need await here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do it we want _readInterceptor to support async logic, which it might need to do if the interceptor implemented some sort of auth check, or read values from a different cluster. IMO, using async for "extensions" when possible just make the extensions more usable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting point, thanks!

}

const adminCluster = server.plugins.elasticsearch.getCluster('admin');
const uiSettingsServices = uiSettingsServiceFactory(server, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: uiSettingsServices ----> uiSettingsService.

const adminCluster = server.plugins.elasticsearch.getCluster('admin');
const uiSettingsServices = uiSettingsServiceFactory(server, {
readInterceptor,
callCluster(...args) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: wondering if callAdminCluster would make the intention clearer? But if admin cluster here is really an obvious thing for other developers then it's fine as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose not to include admin in the name because it's really not a concern of the UiSettingsService what type of cluster it's calling. UiSettingsService just cares that it can talk to elasticsearch.

ignore401Errors = false
} = options;

const isIgnorableError = error => (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 Great that you got rid of that confusing "catch-with-filter" from Bluebird!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+++

};

const resp = await this._callCluster('get', clientParams, callOptions);
return resp._source;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: original code was returning resp._source || {}. Was it just legacy and now we can be sure that resp._source is always an object?

Copy link
Contributor Author

@spalger spalger Jun 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, the original code was using a single handler for the "ignored error" and "successful response" branches, now we branch on that condition and this is only for successful responses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, I missed that.

}

async setMany(changes) {
await this._write(changes);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: original code (for this and few other methods) was always (if promise is resolved) returning an empty object for some reason via .then(() => ({}));. We don't need this behaviour now?

Copy link
Contributor Author

@spalger spalger Jun 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, I verified nothing was consuming the resolved value of the "write" methods on this class so I just removed them, especially since they were useless.


server.decorate('server', 'uiSettings', () => {
throw new Error(`
server.uiSettings has been removed, see https://github.com/elastic/kibana/pull/12243.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ❤️ this

ignore401Errors = false
} = options;

const isIgnorableError = error => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+++


async _read(options = {}) {
const interceptValue = await this._readInterceptor(options);
if (interceptValue != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually prefer specific checks, but I'm okey with != null in JS (with a typesystem it should be more specific). I just read it as "I expect x to be defined"

(in JS some people set a value to null to mean not defined, while others do undefined — until we are more consistent on that, I think != null makes sense)

@kimjoar
Copy link
Contributor

kimjoar commented Jun 9, 2017

Baaaaah, I was commenting on some of Aleh's comments above. But stupid Github UI is stupid.

Copy link
Contributor

@kimjoar kimjoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code itself LGTM.

In general though I'm not really a fan of adding things to req. It feels like the Array.prototype.myThing days of JavaScript. I'm okey with this change for now though, then we can explore ways in the new platform to make this "cleaner" (moving away from a "third-party owned" req to our own req with a clearly defined API might be enough from my perspective).

}

export class UiSettingsService {
constructor(options = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ I think we should stay away from optionals like this as much as possible, and if we want to define one it should list all the key/value pairs with their defaults.

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I finished :) Everything looks good except for few things and many questions (mostly for my understanding).

describe('overview', function () {
it('has expected api surface', function () {
const { uiSettings } = setup();
expect(typeof uiSettings.get).to.equal('function');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it's possible to get rid of typeof with expect(uiSettings.get).to.be.a('function'); here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hold over from the previous tests, but happy to fix and get better error messages.

assertPromise(promise);

try {
await promise;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Hmm, it looks like you should throw an exception right at the end of this function to make sure we really get a rejected promise, otherwise it just masks the error in the code that is supposed to return rejected promise, but returns a resolved one instead.


it('throws if the first error is not a request', async () => {
const { uiSettings } = setup();
await assertRejection(uiSettings.get(null), 'hapi.Request');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Heh, it's the thing I've just mentioned above in assertRejection: your code doesn't throw anymore (assertRequest no longer exists), but the test is still green.

import expect from 'expect.js';

export function assertPromise(promise) {
if (!promise || typeof promise.then !== 'function') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: why don't you like expect(promise).to.be.a(Promise); right inside the tests? Honestly, it feels more natural to use kind of built-in Promise assertion (especially since we don't have that nasty Bluebird there anymore).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, if expect(promise).to.be.a(Promise); is good for you then it seems we may not need this file at all(assertRejection is only used in one test that is broken and redundant, see my note below).


callCluster.assertUpdateQuery = doc => {
sinon.assert.calledOnce(callCluster);
expect(callCluster.firstCall.args).to.eql([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: since it's called only once you can leverage sinon assert here as well (a bit easier to read):

sinon.assert.calledWithExactly(callCluster, 'update', {
  index,
  type,
  id,
  body: { doc }
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I figured calledWithExactly did a === strict equality check... works for me

const currentEsDocValue = merge({}, esDocSource);

const callCluster = sinon.spy(async (method, params) => {
expect(params)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Do you guys have team agreement on how sophisticated stubs should be? I usually try to make them as dumb as possible without much (any) logic/state inside them. So, in this case, I'd rather write something like this (assuming we don't need the state here):

const callCluster = sinon.stub();

callCluster.withArgs('get', sinon.match({
  index: sinon.match.string,
  type: sinon.match.string,
  id: sinon.match.string
})).returns({ _source: merge({}, currentEsDocValue) });

callCluster.withArgs('update', sinon.match({
  index: sinon.match.string,
  type: sinon.match.string,
  id: sinon.match.string,
  body: sinon.match.string,
  doc: sinon.match.string
})).returns({});

And the fact that tests are still green with the snippet above makes me wonder whether we need the "state" here at all (eg. merge(currentEsDocValue, params.body.doc);) or maybe we don't have enough checks. How does it affect tests?

Copy link
Contributor Author

@spalger spalger Jun 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method would work fine without the state, so I'll remove it, but do you know if the sinon.stub() that has multiple withArgs() branches will throw if calls don't match the stubbed arg tests? Also, I didn't realize you could do what you did with sinon.match()... I'm all for less stub logic but think that implementing a function inside of sinon.spy() can be a bit easier to read and understand (especially since I don't understand sinon inside an out)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but do you know if the sinon.stub() that has multiple withArgs() branches will throw if calls don't match the stubbed arg tests?

Off the top of my head, it won't throw and just return undefined. That should work okay in most of the cases since we usually test what we expect to see (e.g. verify that expected methods have been called with the right args, correct number of times, in the right order etc.) and not what "the thing" does internally.

Also, I didn't realize you could do that you did with sinon.match()...

Yeah, it's a pretty powerful thing that validates args and their types in one go.

I'm all for less stub logic but think that implementing a function inside of sinon.spy() can be a bit easier to read and understand (especially since I don't understand sinon inside an out)

I'm fine with it, whatever you like more

describe('enabled', () => {
it('registers a handler for kbnServer.ready()', () => {
const { readyPromise } = setup();
sinon.assert.calledOnce(readyPromise.then);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional nit: maybe it will be safer to add sinon.assert.calledWithExactly(readyPromise.then, sinon.match.func); too, to make sure that code adds resolve callback.



const states = chance.shuffle(['red', 'green', 'yellow']);
states.forEach(state => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: state => { ----> (state) => {

_default_: { id: 'string', params: {} }
};

Object.keys(mapping).forEach(function (dataType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional nit: maybe use arrow function here, just for the consistency?

const result = await uiSettings.getAll();
const defaults = getDefaultSettings();
const expectation = {};
Object.keys(defaults).forEach(key => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: key => { ----> (key) => { here and in all other test cases below.

@elastic elastic deleted a comment from azasypkin Jun 12, 2017
@spalger spalger merged commit 65d6b5d into elastic:master Jun 12, 2017
@spalger spalger deleted the ui-settings/request-based branch June 12, 2017 21:16
spalger added a commit to spalger/kibana that referenced this pull request Jun 28, 2017
* [server/uiSettings] make uiSettings service request based

* [server/uiSettings] disambiguate UiSettings/Service

* [server/uiSettings] link to PR in removal error

* [server/uiSettings] await _read before hydrating

* [server/uiSettings] focus tests, remove server integration

* [server/uiSettings] add tests for readInterceptor() arg

* [server/uiSettings] add server integration tests

* [server/uiExports] fix replaceInjectedVars tests

* [server/uiSettings] convert all methods to use async/await

* [uiSettings/serviceFactory] fix doc block

* [uiSettings/service] fix doc block

* [uiSettings/tests/callClusterStub] stop tracking state needlessly

* [uiSettings/tests] remove invalid tests and pointless promise helpers

* [uiSettings/forRequest] fix typo

* [uiSettings/tests] remove mixture of arrow and function expressions

* [uiSettings/tests/callClusterStub] leverage sinon.calledWithExactly

* [uiSettings/mixin/tests] add exception for eslint import/no-duplicates

* [uiSettings/mixin/tests] wrap single args in parens

(cherry picked from commit 65d6b5d)
spalger added a commit that referenced this pull request Jun 28, 2017
* [uiSettings] make service request based (#12243)

* [server/uiSettings] make uiSettings service request based

* [server/uiSettings] disambiguate UiSettings/Service

* [server/uiSettings] link to PR in removal error

* [server/uiSettings] await _read before hydrating

* [server/uiSettings] focus tests, remove server integration

* [server/uiSettings] add tests for readInterceptor() arg

* [server/uiSettings] add server integration tests

* [server/uiExports] fix replaceInjectedVars tests

* [server/uiSettings] convert all methods to use async/await

* [uiSettings/serviceFactory] fix doc block

* [uiSettings/service] fix doc block

* [uiSettings/tests/callClusterStub] stop tracking state needlessly

* [uiSettings/tests] remove invalid tests and pointless promise helpers

* [uiSettings/forRequest] fix typo

* [uiSettings/tests] remove mixture of arrow and function expressions

* [uiSettings/tests/callClusterStub] leverage sinon.calledWithExactly

* [uiSettings/mixin/tests] add exception for eslint import/no-duplicates

* [uiSettings/mixin/tests] wrap single args in parens

(cherry picked from commit 65d6b5d)

* [uiSettings] support defining settings with uiExports (#12250)

* [uiSettings] add `uiSettingDefaults` export type

* [uiSettings/uiExportsConsumer] ensure that there are not conflicts

* use `sinon.assert.calledWithExactly()`

* describe the UiExportsConsumer class

* make uiExport consumers filtering intention more clear

* fix typo

* fix typos

* add note about why getDefaults() is a function

(cherry picked from commit 7bec60c)
@jimgoodwin jimgoodwin added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:fix labels Jul 25, 2017
@havidarou
Copy link

Hi, I have updated from 5.5.2 to 5.6.0 and my server.uiSettings() no longer works.

I've read this thread but can't understand what is the new way of doing this.

Thank you in advance.

@spalger
Copy link
Contributor Author

spalger commented Sep 15, 2017

@havidarou if you have a Hapi request object then you can do it like this. If you don't have a request then you'll need to do something like this

The second example is a little more complicated, but hopefully you are working with a request. If you aren't please give me some more information about what you're doing. A code example on github would be excelent 😄

@havidarou
Copy link

Hi @spalger

I don't have a request, so I would use the second approach.

Anyway, I ended up doing an elasticsearch request...though I plan to test this eventually.

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.0.0-rc1 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants