[Management] Handle saved search import better by chrisronline · Pull Request #14625 · elastic/kibana

chrisronline · 2017-10-26T20:30:36Z

This PR changes how we handle the new import conflict handling so we process saved searches separately from other objects (like dashboards and visualizations) to ensure that objects imported that reference saved searches resolve properly.

Specifically, this PR separates saved object import conflicts for saved searches from other saved objects (dashboards, visualizations). This separation then allows us to process the saved searches first to ensure those are written before we attempt to process other saved objects, which could rely on those saved searches to exist.

tylersmalley · 2017-10-26T21:43:37Z

jenkins, test it

bhavyarm · 2017-10-26T23:30:19Z

Copied the src/core_plugins/kibana/public/management/sections/objects/_objects.js to my local and tested the blocker #14592. Import works fine now for my export.json in the bug 14592.

bhavyarm

LGTM ( I tested the blocker)

cjcenizal

I haven't tested this in the browser but overall this LGTM! I have some suggestions on ways to make the code more readable, but nothing here is a blocker.

cjcenizal · 2017-10-26T23:28:53Z

-                      return;
-                    }
+                    if (swallowErrors && err instanceof SavedObjectNotFound) {
+                      if (err.savedObjectType === 'search') {


Minor nit, but a switch seems to make slightly more sense here:

switch (err.savedObjectType) { case 'search': conflictedSearchDocs.push(doc); return; case 'index-pattern': conflictedIndexPatterns.push({ obj, doc }); return;

cjcenizal · 2017-10-26T23:38:19Z

@@ -205,8 +205,9 @@ uiModules.get('apps/management')
        })
        .then((overwriteAll) => {
          const conflictedIndexPatterns = [];


Can we add a comment above these to provide some context?

// We'll collect all index patterns which {don't correspond to a saved object? are missing? I'm not sure}, // so we can ask the user to {resolve them? reassign them?}. We'll also collect all saved searches // which {don't correspond exist?}, so we can ask the user to {resolve them? recreate them?}. const conflictedIndexPatterns = []; const conflictedSearchDocs = [];

// Collect all index patterns and saved searches referenced in other documents, like visualizations, // but which have not been imported yet.

I think the naming here is a tad confusing. It makes more sense to call a set of index-patterns that could be associated with a savedsearch conflictedIndexPatterns, but doesn't make sense for savedsearches. The searchDocs are not conflicted. They just don't exist yet, haven't been imported yet, and so visualizations that reference them can't be imported.

Ideas:
referencedSavedSearches --> the viz references it. that's all we can say for sure.
notYetImportedSavedSearches --> this assumes that they will be imported. it could be that someone fucked with the id and the savedsearch doesn't actually exist.
referencedAndNotFoundSavedSearches -- verbose but accurate?

I added some comments, does that help clarify this?

cjcenizal · 2017-10-26T23:43:18Z

+          const conflictedSearchDocs = [];

-          function importDocument(doc) {
+          function importDocument(swallowErrors, doc) {


I mentioned this to @chrisronline on Slack. The side effects of pushing docs into conflictedIndexPatterns and conflictedSearchDocs into this function make it difficult to follow the flow of the code, especially since we call this function later and pass in conflictedSearchDocs.

It looks like the logic within the if (swallowErrors ...) conditional is the only part which changes from call site to call site. Can we extract this logic into its own function and pass that function in when we need it?

cjcenizal · 2017-10-26T23:45:23Z

+            const oldIndexId = obj.searchSource.getOwn('index');
+            const newIndexId = objs.find(({ oldId }) => oldId === oldIndexId).newId;
+            if (newIndexId === oldIndexId) {
+              // Skip


Can we add a few words here to elaborate? e.g. The index hasn't changed (or whatever conclusion we can draw from this equality check) so we don't need to {do whatever it is we're doing on line 275}.

rhoboat · 2017-10-26T23:53:14Z

Please wait.. i'm testing this as well...

rhoboat

Couple of naming things, but otherwise, looks good enough for solving this blocker. But I really don't like the code style. If I have some more time, I'll post a refactor PR.

rhoboat · 2017-10-27T00:00:57Z

@@ -205,8 +205,9 @@ uiModules.get('apps/management')
        })
        .then((overwriteAll) => {
          const conflictedIndexPatterns = [];


// Collect all index patterns and saved searches referenced in other documents, like visualizations, // but which have not been imported yet.

rhoboat · 2017-10-27T00:06:32Z

@@ -205,8 +205,9 @@ uiModules.get('apps/management')
        })
        .then((overwriteAll) => {
          const conflictedIndexPatterns = [];


I think the naming here is a tad confusing. It makes more sense to call a set of index-patterns that could be associated with a savedsearch conflictedIndexPatterns, but doesn't make sense for savedsearches. The searchDocs are not conflicted. They just don't exist yet, haven't been imported yet, and so visualizations that reference them can't be imported.

Ideas:
referencedSavedSearches --> the viz references it. that's all we can say for sure.
notYetImportedSavedSearches --> this assumes that they will be imported. it could be that someone fucked with the id and the savedsearch doesn't actually exist.
referencedAndNotFoundSavedSearches -- verbose but accurate?

rhoboat · 2017-10-27T00:11:54Z

+                        return;
+                      }
+                    }
                    // swallow errors here so that the remaining promise chain executes


This is so confusing now. I get that we want to swallow errors in the original code--so we can continue execution and use notify. But the fact that we do that makes passing in a new arg called swallowErrors not make much sense. Its purpose is to avoid hitting the notify line. Maybe a better name for that arg is handleConflicts or collectDocsNotFound.

rhoboat

Couple of naming things, but otherwise, looks good enough for solving this blocker. But I really don't like the code style. If I have some more time, I'll post a refactor PR.

epixa · 2017-10-27T13:05:14Z

@chrisronline Can you forward port this today?

chrisronline · 2017-10-27T14:07:43Z

@epixa Sure thing. PRs open and will merge on green CI.

* Port fixes from #14625 to 6.x * Adding missing test file * Fix linting issue

* Port fixes from #14625 to 7.0 * Fix linting issue

* Port fixes from elastic#14625 to 7.0 * Fix linting issue

Handle saved search import better

b844151

chrisronline added the :Management label Oct 26, 2017

Add functional test

08b3410

chrisronline requested a review from tylersmalley October 26, 2017 20:39

chrisronline self-assigned this Oct 26, 2017

chrisronline added review v6.0.0-rc2 v6.1.0 v7.0.0 labels Oct 26, 2017

chrisronline requested a review from cjcenizal October 26, 2017 20:41

tylersmalley approved these changes Oct 26, 2017

View reviewed changes

bhavyarm self-requested a review October 26, 2017 23:29

bhavyarm reviewed Oct 26, 2017

View reviewed changes

cjcenizal approved these changes Oct 26, 2017

View reviewed changes

rhoboat self-requested a review October 26, 2017 23:49

rhoboat suggested changes Oct 27, 2017

View reviewed changes

Better comments

0b2ba0c

rhoboat approved these changes Oct 27, 2017

View reviewed changes

tylersmalley merged commit 6ef803b into elastic:6.0 Oct 27, 2017

tylersmalley mentioned this pull request Oct 27, 2017

Importing reindexed saved objects is not working. #14592

Closed

tylersmalley added the v5.6.4 label Oct 27, 2017

chrisronline added a commit to chrisronline/kibana that referenced this pull request Oct 27, 2017

Port fixes from elastic#14625 to 6.x

2fea87c

chrisronline mentioned this pull request Oct 27, 2017

[Management] [6.x] Handle saved search import better #14636

Merged

chrisronline added a commit to chrisronline/kibana that referenced this pull request Oct 27, 2017

Port fixes from elastic#14625 to 7.0

95dd2a1

chrisronline mentioned this pull request Oct 27, 2017

[Management] [7.0] Handle saved search import better #14637

Merged

chrisronline deleted the fix/import_export_saved_searches branch October 27, 2017 14:08

chrisronline added a commit that referenced this pull request Oct 27, 2017

[Management] [6.x] Handle saved search import better (#14636)

4fc92bb

* Port fixes from #14625 to 6.x * Adding missing test file * Fix linting issue

chrisronline added a commit that referenced this pull request Oct 27, 2017

[Management] [7.0] Handle saved search import better (#14637)

677742e

* Port fixes from #14625 to 7.0 * Fix linting issue

patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026

[Management] [7.0] Handle saved search import better (elastic#14637)

029b619

* Port fixes from elastic#14625 to 7.0 * Fix linting issue

Conversation

chrisronline commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tylersmalley commented Oct 26, 2017

Uh oh!

bhavyarm commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bhavyarm left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal 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!

cjcenizal Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhoboat commented Oct 26, 2017

Uh oh!

rhoboat left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rhoboat left a comment

Choose a reason for hiding this comment

Uh oh!

epixa commented Oct 27, 2017

Uh oh!

chrisronline commented Oct 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chrisronline commented Oct 26, 2017 •

edited

Loading

bhavyarm commented Oct 26, 2017 •

edited

Loading

cjcenizal Oct 26, 2017 •

edited

Loading

rhoboat left a comment •

edited

Loading