Uses SavedObjects API in Courier Saved Object by tylersmalley · Pull Request #12407 · elastic/kibana

tylersmalley · 2017-06-19T20:34:13Z

No description provided.

tylersmalley · 2017-06-19T20:35:46Z

src/server/saved_objects/client/saved_objects_client.js

This change was made to maintain consistency with the fetch return format.

jbudz · 2017-06-19T20:54:11Z

Karma failures look legitimate

jbudz · 2017-06-19T21:00:40Z

src/ui/public/courier/saved_object/saved_object.js

thoughts on changing the 2000 to a constant to make it a little bit more clear? COLLECTION_TIME or similar.

2s is a noticeable wait, do you know if we can get away with a shorter time?

Left over from testing, should be 200.

jbudz · 2017-06-19T21:06:39Z

src/ui/public/courier/saved_object/saved_object.js

do we not need the onUpdate chain under this anymore?

I don't see anywhere where we were leveraging this behavior.

jbudz · 2017-06-19T21:20:45Z

src/server/saved_objects/routes/bulk_get.js

can you add a comment on the underscore, is this meant to be a private endpoint?

Will do - the underscore can actually be removed. I had it simply to not conflict with any saved object key names, but that won't be an issue when we move to ID's.

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

stacey-gammon · 2017-06-23T15:00:40Z

src/ui/public/saved_objects/saved_objects_client.js

    });
-  }
+
+  }, BATCH_INTERVAL, { leading: false })


should have a ; at the end because it's an assignment (my intellij gives me a little complaining highlight)

stacey-gammon · 2017-06-23T15:09:23Z

src/ui/public/saved_objects/saved_objects_client.js

+   * @param {integer} options.page - defaults to 1
+   * @param {integer} options.perPage - defaults to 20
+   * @param {array} option.fields - fields to be returned. Returns all unless defined
+   * @returns {promise}


Can you document what the promises will resolve or reject to for the functions in this file? I'm having a hard time figuring out when _type, _id _version and _attributes are used vs type, id, version and attributes. It looks like sometimes its either all _ or no underscore, but above there is a spot above that handles the response type from this call like this:

+ return { + _id: resp.id, + _type: resp.type, + _source: _.cloneDeep(resp._attributes), + found: resp._version ? true : false + };

And I'm wondering why id and type don't have underscores but version and attributes too. Having the comments would help me track that down easier (if only we had interfaces and typescript!)

Added documentation to each of the methods.

The Saved Object API itself returns { id, type, attributes, version }. When it's passed to the SavedObject, attributes were previously private and had an underscore. I removed that since it's apparent it's not private. The only remaining thing private to that object is _version.

The code you referenced from the Courier SavedObject is slightly confusing since it's translating SavedObject format to the Elasticsearch format. I choose that as opposed to updating every reference to _source, found, '_id`, etc.

If we are truly moving to the airbnb styleguide we shouldn't be using the underscore prefix to denote privacy - https://github.com/airbnb/javascript#naming--leading-underscore

But I guess that's not official, and I know it's still in use here and there, so I'm fine if you wish to leave as is as well.

stacey-gammon · 2017-06-23T15:18:22Z

src/ui/public/saved_objects/saved_objects_client.js

      .then(resp => get(resp, 'data'))
      .catch(resp => {
-        const respBody = resp.data || {};
+        const respBody = get(resp, 'data', {});


minor feedback (take it or leave it), but I think it'd be easier to read _.get than get because there is already a get function in this file

Actually, even better if we didn't use lodash at all in favor of native functionality (which I think is what we want to gravitate towards anyway). This is no different then

const respBody = resp.data || {};

right? for me, that is easier to mentally compute (though probably because I'm not very familliar with lodash).

also the line above could just be resp.data.

I guess the benefit of using the lodash get version is when you pass it long paths and anywhere along the path something is undefined, it won't crash and burn, but return the default value? If that's the only benefit, then doesn't seem necessary on a path only one attribute deep (unless it works if resp is also undefined?)

It is not identical: with the get it's using the default value if undefined, while with the || it's doing it if resp.data is falsy. As resp.data can't have any falsy values as "successful values" doing || is okey here. (I prefer get, though)

I updated the whole file to use _ instead of dereferencing the methods out to eliminate confusion.

spalger

Haven't gotten a chance to run this and verify the tests, but I have some early feedback.

spalger · 2017-06-23T20:07:55Z

src/server/saved_objects/client/saved_objects_client.js

+   * @param {string} type
+   * @param {object} body - { attributes: {}, id: myId }
+   * @param {object} options
+   * @param {boolean} options.overwrite - defaults to false


Pretty sure the correct JSDoc here would be:

* @param {object} [options={}] * @property {boolean} [options.overwrite=false]

spalger · 2017-06-23T20:10:04Z

src/server/saved_objects/client/saved_objects_client.js

+   * @returns {promise}
+  */
+  async create(type, body = {}, options = {}) {
+    const method = get(options, 'overwrite', false) ? 'index' : 'create';


no need for the overhead of get() here since options defaults to an empty object.

spalger · 2017-06-23T20:11:15Z

src/server/saved_objects/client/saved_objects_client.js


+  /**
+   * @param {object} options
+   * @param {string} options.type


LIke above, properties of an object param should be doc'd as @property {type} options.name

spalger · 2017-06-23T20:12:27Z

src/server/saved_objects/client/saved_objects_client.js

+   * @param {integer} options.version - ensures version matches that of persisted object
+   * @returns {promise}
+   */
+  async update(type, id, attributes, { version } = {}) {


{ version } = {} is one of many different ways the options are read in this file, I'd love if they were all done the same way, preferably like so:

async method(/* ... */, options = {}) { const { version: false } = options; // ... }

spalger · 2017-06-23T20:18:33Z

src/server/saved_objects/client/saved_objects_client.js

+   * @param {boolean} options.overwrite - defaults to false
+   * @returns {promise}
+  */
+  async create(type, body = {}, options = {}) {


The body argument here feels out of place. Before we had the concept of attributes, body was used to describe the the same thing, the key/value pairs that define the contents of a saved object. Now it seems that body represents the API request body (more or less), which isn't really relevant to the SavedObjectsClient. I think we should update this to use the same call signature as savedObjectsClient.update(type, id, attributes, options);

For create, do you think we should provide ID as a parameter or accept it as an option? We're moving away from creating documents with ID's except for the import so it didn't feel right to promote as an argument.

Hmm, good question... I'd just making it optional. We won't ever be able to remove it

options now accepts an id. Let me know if this works for you

spalger · 2017-06-23T20:29:25Z

src/server/saved_objects/routes/create.js

        payload: Joi.object({
-          attributes: Joi.object().required()
+          attributes: Joi.object().required(),
+          id: Joi.string().allow(null)


I feels like the url is the right place to put the id: POST /api/saved_objects/{type}/{id}

spalger · 2017-06-23T20:34:03Z

src/ui/public/saved_objects/angular_service.js

+import chrome from 'ui/chrome';
+import { SavedObjectsClient } from 'ui/saved_objects';
+
+uiModules.get('kibana').service('savedObjectsClient', function ($http, $q) {


Rather than relying on angular DI and ui/autoload/modules, can we just export a SavedObjectsClientProvider here and use Private(SavedObjectsClientProvider) to get the client?

…er-api

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

tylersmalley · 2017-06-26T22:41:45Z

Thanks @stacey-gammon and @spalger - all of your feedback should now be addressed.

stacey-gammon · 2017-06-27T14:40:33Z

src/ui/public/courier/saved_object/saved_object.js

-
    /**
     * Saves this object.
     *


SaveOptions is no longer typedef'ed so I think this should be changed to:

* @param {object} [options={}] * @property {boolean} options.confirmOverwrite - If true, attempts to create the source so it * can confirm an overwrite if a document with the id already exists.

👍 updated

stacey-gammon · 2017-06-27T14:41:34Z

src/ui/public/courier/saved_object/saved_object.js

@@ -302,14 +287,14 @@ export function SavedObjectProvider(esAdmin, kbnIndex, Promise, Private, Notifie
     * @resolved {String} - The id of the doc


Does this still resolve to the id? According to the change below, it looks like it now returns a response object with an id property on it.

Goot catch. It doesn't appear to have ever returned the id. doIndex is defined here, which calls sendToEs and resolves here

Updated the comment

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Uses the ES index action for create except when defing the _id_ without specifying options.overwrite=true

stacey-gammon · 2017-06-28T13:03:03Z

src/server/saved_objects/client/saved_objects_client.js

  */
  async create(type, attributes = {}, options = {}) {
-    const method = options.overwrite ? 'index' : 'create';
+    const method = get(options, 'overwrite', false) === false && options.id ? 'create' : 'index';


Above it says "force id on creation, not recommended", but it looks like with these new changes, the only way to create an object is to supply options.id. Or does index handle the automatic id creation if it doesn't exist?

index handles automatic id creation, in addition, it allows for overwriting of a document when an ID is specified.

spalger

A couple minor nits, feel free to merge as is, LGTM!

spalger · 2017-06-29T15:38:03Z

src/core_plugins/kibana/public/dashboard/__tests__/panel.js

+        };
+      });
+
      sinon.stub(esAdmin, 'mget').returns(Promise.resolve({ docs: [ mockDocResponse ] }));


Maybe we can put this off until we rid the frontend of esAdmin completely, but pretty sure we don't need esAdmin stuff here anymore.

Good catch - I was able to remove this esAdmin stub, but still some work to be done to remove the remaining fieldMapping call.

spalger · 2017-06-29T16:02:22Z

src/server/saved_objects/client/saved_objects_client.js

+   * @returns {promise} - { id, type, version, attributes }
+  */
+  async create(type, attributes = {}, options = {}) {
+    const method = get(options, 'overwrite', false) === false && options.id ? 'create' : 'index';


get(options, 'overwrite', false) === false && options.id

feels like a slightly more complicated way to write

options.id && !options.overwrite

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

stacey-gammon

🎉

spalger · 2017-06-29T18:08:56Z

jenkins, test this

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

tylersmalley · 2017-06-30T00:33:02Z

5.x: fa53e2e

tylersmalley added Team:Operations Kibana-Operations Team review v5.6.0 v6.0.0 labels Jun 19, 2017

tylersmalley requested a review from jbudz June 19, 2017 20:34

tylersmalley commented Jun 19, 2017

View reviewed changes

jbudz reviewed Jun 19, 2017

View reviewed changes

tylersmalley force-pushed the saved-object-loader-api branch 3 times, most recently from ccb5afc to 4d94db7 Compare June 21, 2017 16:00

tylersmalley removed the review label Jun 21, 2017

tylersmalley changed the title ~~Uses SavedObjects API in Courier~~ [WIP] Uses SavedObjects API in Courier Jun 21, 2017

Uses SavedObjectAPI for Courier SavedObject

5f7dfc2

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

tylersmalley force-pushed the saved-object-loader-api branch 4 times, most recently from e53e9fa to 70d7cc8 Compare June 22, 2017 21:00

Updates

455f1d3

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

tylersmalley force-pushed the saved-object-loader-api branch from 70d7cc8 to 455f1d3 Compare June 22, 2017 21:02

Allow null id

1d7041e

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

tylersmalley changed the title ~~[WIP] Uses SavedObjects API in Courier~~ Uses SavedObjects API in Courier Saved Object Jun 23, 2017

tylersmalley requested review from spalger and stacey-gammon June 23, 2017 00:33

tylersmalley added the review label Jun 23, 2017

stacey-gammon reviewed Jun 23, 2017

View reviewed changes

spalger reviewed Jun 23, 2017

View reviewed changes

stacey-gammon mentioned this pull request Jun 26, 2017

Migrate apps to use the SavedObject Client/API #12229

Closed

4 tasks

Merge remote-tracking branch 'upstream/master' into saved-object-load…

46d32d9

…er-api

tylersmalley added 5 commits June 26, 2017 12:50

Moves create id to options

b0ee97d

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Prevent destructuring in argument definition

c80800d

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

attributes is no longer private

972ea0d

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Use Private over module autoloader

973d9bf

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Filter objects array

5d00b4f

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

tylersmalley force-pushed the saved-object-loader-api branch from 49629ee to 5d00b4f Compare June 26, 2017 21:33

stacey-gammon reviewed Jun 27, 2017

View reviewed changes

tylersmalley added 2 commits June 27, 2017 08:05

Updates comments

51d3eb1

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Use correct method on create

8f80105

Uses the ES index action for create except when defing the _id_ without specifying options.overwrite=true

stacey-gammon reviewed Jun 28, 2017

View reviewed changes

spalger approved these changes Jun 29, 2017

View reviewed changes

tylersmalley added 2 commits June 29, 2017 10:02

Simplify ES action check

c13ec07

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Remove unused esAdmin mget stub

166254e

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

stacey-gammon approved these changes Jun 29, 2017

View reviewed changes

tylersmalley merged commit ead5754 into elastic:master Jun 29, 2017

tylersmalley added a commit to tylersmalley/kibana that referenced this pull request Jun 29, 2017

Uses SavedObjects API in Courier Saved Object (elastic#12407)

2c22d68

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

tylersmalley mentioned this pull request Jun 29, 2017

[5.x] Uses SavedObjects API in Courier Saved Object (#12407) #12584

Merged

tylersmalley added a commit that referenced this pull request Jun 30, 2017

Uses SavedObjects API in Courier Saved Object (#12407) (#12584)

fa53e2e

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

This was referenced Jul 5, 2017

Use SavedObjectsClient in Courier Index Pattern #12670

Closed

Use SavedObjectsClient for Courier Index Pattern #12719

Merged

jimgoodwin added the release_note:enhancement label Jul 25, 2017

chrisronline mentioned this pull request Nov 8, 2017

Kibana 6.0.0.beta2 sets wrong index names when saving and doesn't find existing indices when importing visualizations #13923

Closed

		@@ -302,14 +287,14 @@ export function SavedObjectProvider(esAdmin, kbnIndex, Promise, Private, Notifie
		* @resolved {String} - The id of the doc

Conversation

tylersmalley commented Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbudz commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylersmalley commented Jun 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylersmalley commented Jun 19, 2017 •

edited

Loading