Refactor loading saved objects

[stacey-gammon]

Every saved object type repeated essentially the same code to find and retrieve objects from the server. This refactors that out into a common place. Some saved objects did need to overwrite the default functions when there was some slight deviation from the norm.

Note this change includes a fix for #9402 and also changes the timelion default retrieval size to 100 to match the rest of the saved objects (used to be 1000 but I couldn't find any good reason for the discrepancy).

[thomasneirynck]

jenkins, test this

[thomasneirynck]

Don't have much to add :) I tested this with a few dashboards/vizzes/saved searches. Works!

this is also cool:

FWIW, since this is normal OO, I would use JS's built-in grammar to define the type, instead of recreating the methods inside the constructor function each time it's called.

either the old fashioned way, which Kibana frequently uses:

function SavedObjectSearch(){
 ...
}
SavedObjectSearch.prototype.get .= function()...
SavedObjectSearch.prototype.urlFor = function()...

or with ES6 class syntax.

class SavedObjectSearch(){
   constructor(...){...}
   get(){...}
   urlFor(){...}
}

There's a million ways to define types in JS (sadly), so I like keeping it conventional.

Anyway, that's minor.

LGTM.

[thomasneirynck]

FYI: there is an issue about this hardcoded limit. #8044

[stacey-gammon]

Ah cool, added a comment and a TODO to keep it on the radar.

[stacey-gammon]

Switched to es6 classes! I actually meant to use the revealing module pattern but did it wrong, doh. After some more thought I decided I like es6 classes better, and even though it doesn't give you true private scope, it's clean and looks nice. :) Not much in that file is private anyhow.

[tsullivan]

This mutates the hit parameter by adding ._source.id and ._source.url. Maybe the declaration of const source should clone hit._source first?

[stacey-gammon]

Hmmm, I'm not 100% convinced. I don't think the code is relying on hit._source being modified, but I don't know for sure. Also, creating the clone is inefficient, unnecessary, and potentially dangerous without fully understanding what is in ._source (deep clone via json parse/stringify or shallow via object.assign? any circular references? Are any class types important to preserve?).

I'm not super confident that making a clone wouldn't create subtle bugs since I don't fully understand what ._source is (or might be), so at this moment, I don't think creating a clone isn't worth the additional complexity and would prefer to leave the logic as is and make this a refactor only PR (well, minus the two situations that produced wrong results - the 1000 -> 100 change and getting rid of allowing wildcards for visualizations).

But I'm open to changing it if you feel strongly, or I am wrong in any of my thoughts above, or if you have a better understanding of what ._source is. Maybe I'm overcomplicating it. :)

I did add a comment, anyhow, so it's clear that hit._source is being modified and returned.

[tsullivan]

I don't feel strongly about it, so keeping it as-is is fine with me. I think the added comment is very helpful

[tsullivan]

I think we should use the ES6 object literal shortcut for these fields, just

{
  ...
  body,
  size
}

[stacey-gammon]

done!

[stacey-gammon]

@tsullivan all comments should be addressed, lmk if there is anything else!

[tsullivan]

LGTM

[stacey-gammon]

jenkins test this