Import saved searches first

[w33ble]

Closes #7667

When importing saved objects from JSON, those objects are run through the saved object service, which means that visualizations and searches check their fields and other hierarchical requirements. For visualizations, this means that any associated saved searches are looked up on import, and things fail if the required saved search does not exist.

However, all saved objects were imported in parallel, which led to weird race conditions where visualizations would fail to import because their saved searches were not yet finished importing.

With this PR, saved searches are imported before anything else, so this race condition is no longer an issue.

---

To test, either:

• Import a large number of saved visualizations that use saved searches via the JSON importer, and keep importing and deleting until you run into the race condition
• Modify the source to introduce a little delay when importing saved searches

If you are interested in the later, here's a handy diff:

--- a/src/core_plugins/kibana/public/management/sections/objects/_objects.js
+++ b/src/core_plugins/kibana/public/management/sections/objects/_objects.js
@@ -194,7 +194,18 @@ uiModules.get('apps/management')
                 return;
               }
 
-              return service.get()
+              // return service.get()
+              function getDelay() {
+                if (doc._type === 'search') {
+                  return new Promise(resolve => {
+                    setTimeout(resolve, 3000);
+                  });
+                }
+                return Promise.resolve();
+              }
+
+              return getDelay()
+                .then(() => service.get())
                 .then(function (obj) {
                   obj.id = doc._id;
                   return obj.applyESResp(doc)

[kimjoar]

What about something like this?

const [searches, other] = _.partition(docs, doc => doc._type === "search")

[w33ble]

Slick, but that only allows me to create 2 collections. That works for now, but I'm planning to add more exports, and order will become important in those cases too. This structure makes it easy to add more ordered collections.

[kimjoar]

_.groupBy(docs, doc => {
  switch (doc._type) {
    case 'search':
      return 'searches'
    default:
      return 'other'
  }
})

[ycombinator]

Functionality LGTM. Reviewing code now...

[ycombinator]

I like @kjbekkelund's suggestion to use _.groupBy. This code is trying to group objects by their type and _.groupBy conveys that more explicitly than docs.reduce.

If you are trying to avoid using lodash, then I'd suggest creating a function named groupDocsByType or something that encapsulates your code. That way, its slightly more obvious what's going on.

[w33ble]

_.groupBy also requires some knowledge about that not-very-often-used lodash method. Plus, our stance is to prefer native methods of helpers. And I don't think the use of _.groupBy makes it any easier to read, most of what I have here is preserved even when using it.

Additionally, _.groupBy doesn't enforce an object structure, so later when the code attempts to iterate over docTypes.searches, that could be undefined if the user is not importing any saved searches.

[ycombinator]

For me, its less about whether we use a native method or a lodash method and more about descriptiveness of the operation. Array.prototype.reduce could be used for a lot of things so, as someone who didn't originally write this code, I'd have to dig into it to figure out what it does. If you could wrap that code in a descriptive helper function like groupDocsByType or something, that would make the code more readable IMO.

[ycombinator]

Functionality still LGTM and code LGTM as well.

[Bargs]

LGTM. One note, since this relies on _type it might need to be updated for 6.0 when we decide how to migrate the .kibana index away from types.

[w33ble]

@Bargs just seeing your comment now, sorry. Isn't that going to be a 7.0 concern? Either way, yeah, it's part of a larger change. This at least makes things work better right now.

[Bargs]

It'll be a 6.0 thing because new indices will only be able to have one type in 6.0 elastic/elasticsearch#15613 (comment)

[w33ble]

5.x ba92d47
5.3 2473029
5.2 42b925f
5.1 6cee4ec