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

Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal #76482

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,6 @@ function groupByType(docs: SavedObjectsRawDoc[]): Record<string, SavedObjectsRaw
}, defaultDocTypes);
}

async function awaitEachItemInParallel<T, R>(list: T[], op: (item: T) => R) {
return await Promise.all(list.map((item) => op(item)));
}

export async function resolveIndexPatternConflicts(
resolutions: Array<{ oldId: string; newId: string }>,
conflictedIndexPatterns: any[],
Expand All @@ -175,7 +171,7 @@ export async function resolveIndexPatternConflicts(
) {
let importCount = 0;

await awaitEachItemInParallel(conflictedIndexPatterns, async ({ obj, doc }) => {
for (const { obj, doc } of conflictedIndexPatterns) {
const serializedSearchSource = JSON.parse(
doc._source.kibanaSavedObjectMeta?.searchSourceJSON || '{}'
);
Expand Down Expand Up @@ -220,25 +216,25 @@ export async function resolveIndexPatternConflicts(

if (!allResolved) {
// The user decided to skip this conflict so do nothing
return;
continue;
}
obj.searchSource = await dependencies.search.searchSource.create(
serializedSearchSourceWithInjectedReferences
);
if (await saveObject(obj, overwriteAll)) {
importCount++;
}
});
}
return importCount;
}

export async function saveObjects(objs: SavedObject[], overwriteAll: boolean) {
let importCount = 0;
await awaitEachItemInParallel(objs, async (obj) => {
for (const obj of objs) {
if (await saveObject(obj, overwriteAll)) {
importCount++;
}
});
}
return importCount;
}

Expand All @@ -253,16 +249,16 @@ export async function resolveSavedSearches(
overwriteAll: boolean
) {
let importCount = 0;
await awaitEachItemInParallel(savedSearches, async (searchDoc) => {
for (const searchDoc of savedSearches) {
const obj = await getSavedObject(searchDoc, services);
if (!obj) {
// Just ignore?
return;
continue;
}
if (await importDocument(obj, searchDoc, overwriteAll)) {
importCount++;
}
});
}
return importCount;
}

Expand All @@ -280,7 +276,10 @@ export async function resolveSavedObjects(
let importedObjectCount = 0;
const failedImports: FailedImport[] = [];
// Start with the index patterns since everything is dependent on them
await awaitEachItemInParallel(docTypes.indexPatterns, async (indexPatternDoc) => {
// As the confirmation opens a modal, and as we only allow one modal at a time
// (opening a new one close the previous with a rejection)
// we can't do that in parallel
for (const indexPatternDoc of docTypes.indexPatterns) {
Comment on lines -283 to +282
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, this was present for a very long time (since core's ModalService presence), as we are dismissing any previous modal when opening a new one

open: (mount: MountPoint, options: OverlayModalOpenOptions = {}): OverlayRef => {
// If there is an active modal, close it before opening a new one.
if (this.activeModal) {
this.activeModal.close();
this.cleanupDom();
}

The awaitEachItemInParallel was causing multiple modals to eventually open at the same time, dismissing the previous one when the next one kicked in.

Using a sort of confirmation queue here to pool the modal promises and only open the next one when the previous one is answered by the user is a nightmare, as some confirmations modal are opened as down as savedobject loaders underlying implementations in some cases. As this is only impacting the legacy imports, removing the parallel work and doing one object at a time was the only sane option.

From a quick manual benchmark, it doesn't even impact the process time that much.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the modern approach won't be affected by this fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea. the modern approach is not impacted by this bug, and is not relying on this mechanism to prompt user confirmations, so the PR has no impact on it

try {
const importedIndexPatternId = await importIndexPattern(
indexPatternDoc,
Expand All @@ -294,7 +293,7 @@ export async function resolveSavedObjects(
} catch (error) {
failedImports.push({ obj: indexPatternDoc as any, error });
}
});
}

// We want to do the same for saved searches, but we want to keep them separate because they need
// to be applied _first_ because other saved objects can be dependent on those saved searches existing
Expand All @@ -311,7 +310,7 @@ export async function resolveSavedObjects(
// likely that these saved objects will work once resaved so keep them around to resave them.
const conflictedSavedObjectsLinkedToSavedSearches: any[] = [];

await awaitEachItemInParallel(docTypes.searches, async (searchDoc) => {
for (const searchDoc of docTypes.searches) {
const obj = await getSavedObject(searchDoc, services);

try {
Expand All @@ -329,9 +328,9 @@ export async function resolveSavedObjects(
failedImports.push({ obj, error });
}
}
});
}

await awaitEachItemInParallel(docTypes.other, async (otherDoc) => {
for (const otherDoc of docTypes.other) {
const obj = await getSavedObject(otherDoc, services);

try {
Expand All @@ -350,7 +349,7 @@ export async function resolveSavedObjects(
failedImports.push({ obj, error });
}
}
});
}

return {
conflictedIndexPatterns,
Expand Down
54 changes: 52 additions & 2 deletions test/functional/apps/management/_import_objects.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import expect from '@kbn/expect';
import path from 'path';
import { keyBy } from 'lodash';

const delay = (ms) => new Promise((resolve) => setTimeout(resolve, ms));

export default function ({ getService, getPageObjects }) {
const kibanaServer = getService('kibanaServer');
const esArchiver = getService('esArchiver');
Expand Down Expand Up @@ -203,12 +205,12 @@ export default function ({ getService, getPageObjects }) {
// delete .kibana index and then wait for Kibana to re-create it
await kibanaServer.uiSettings.replace({});
await PageObjects.settings.navigateTo();
await esArchiver.load('management');
await esArchiver.load('saved_objects_imports');
await PageObjects.settings.clickKibanaSavedObjects();
});

afterEach(async function () {
await esArchiver.unload('management');
await esArchiver.unload('saved_objects_imports');
});

it('should import saved objects', async function () {
Expand Down Expand Up @@ -280,6 +282,54 @@ export default function ({ getService, getPageObjects }) {
expect(isSuccessful).to.be(true);
});

it('should allow the user to confirm overriding multiple duplicate saved objects', async function () {
// This data has already been loaded by the "visualize" esArchive. We'll load it again
// so that we can override the existing visualization.
await PageObjects.savedObjects.importFile(
path.join(__dirname, 'exports', '_import_objects_multiple_exists.json'),
false
);

await PageObjects.savedObjects.checkImportLegacyWarning();
await PageObjects.savedObjects.checkImportConflictsWarning();

await PageObjects.settings.associateIndexPattern('logstash-*', 'logstash-*');
await PageObjects.savedObjects.clickConfirmChanges();

// Override the visualizations.
await PageObjects.common.clickConfirmOnModal(false);
// as the second confirm can pop instantly, we can't wait for it to be hidden
// with is why we call clickConfirmOnModal with ensureHidden: false in previous statement
// but as the initial popin can take a few ms before fading, we need to wait a little
// to avoid clicking twice on the same modal.
await delay(1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

can be a source of flakiness.... ok, we will see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, there is a risk here. But I just couldn't find any better solution for these blinking confirmation modals

await PageObjects.common.clickConfirmOnModal(false);

const isSuccessful = await testSubjects.exists('importSavedObjectsSuccess');
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: can be combined with an assertion as await testSubjects.exists('importSavedObjectsSuccess');

expect(isSuccessful).to.be(true);
});

it('should allow the user to confirm overriding multiple duplicate index patterns', async function () {
// This data has already been loaded by the "visualize" esArchive. We'll load it again
// so that we can override the existing visualization.
await PageObjects.savedObjects.importFile(
path.join(__dirname, 'exports', '_import_index_patterns_multiple_exists.json'),
false
);

// Override the index patterns.
await PageObjects.common.clickConfirmOnModal(false);
// as the second confirm can pop instantly, we can't wait for it to be hidden
// with is why we call clickConfirmOnModal with ensureHidden: false in previous statement
// but as the initial popin can take a few ms before fading, we need to wait a little
// to avoid clicking twice on the same modal.
await delay(1000);
await PageObjects.common.clickConfirmOnModal(false);

const isSuccessful = await testSubjects.exists('importSavedObjectsSuccess');
expect(isSuccessful).to.be(true);
});

it('should import saved objects linked to saved searches', async function () {
await PageObjects.savedObjects.importFile(
path.join(__dirname, 'exports', '_import_objects_saved_search.json')
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
[
{
"_id": "test-1",
"_type": "visualization",
"_source": {
"title": "Visualization test 1",
"visState": "{\"title\":\"New Visualization\",\"type\":\"area\",\"params\":{\"shareYAxis\":true,\"addTooltip\":true,\"addLegend\":true,\"smoothLines\":false,\"scale\":\"linear\",\"interpolate\":\"linear\",\"mode\":\"stacked\",\"times\":[],\"addTimeMarker\":false,\"defaultYExtents\":false,\"setYExtents\":false,\"yAxis\":{}},\"aggs\":[{\"id\":\"1\",\"type\":\"count\",\"schema\":\"metric\",\"params\":{}},{\"id\":\"2\",\"type\":\"date_histogram\",\"schema\":\"segment\",\"params\":{\"field\":\"@timestamp\",\"interval\":\"auto\",\"customInterval\":\"2h\",\"min_doc_count\":1,\"extended_bounds\":{}}}],\"listeners\":{}}",
"uiStateJSON": "{}",
"description": "AreaChart",
"version": 1,
"kibanaSavedObjectMeta": {
"searchSourceJSON": "{\"index\":\"logstash-*\",\"query\":{\"query_string\":{\"query\":\"*\",\"analyze_wildcard\":true}},\"filter\":[]}"
}
},
"_meta": {
"savedObjectVersion": 2
}
},
{
"_id": "test-2",
"_type": "visualization",
"_source": {
"title": "Visualization test 2",
"visState": "{\"title\":\"New Visualization\",\"type\":\"area\",\"params\":{\"shareYAxis\":true,\"addTooltip\":true,\"addLegend\":true,\"smoothLines\":false,\"scale\":\"linear\",\"interpolate\":\"linear\",\"mode\":\"stacked\",\"times\":[],\"addTimeMarker\":false,\"defaultYExtents\":false,\"setYExtents\":false,\"yAxis\":{}},\"aggs\":[{\"id\":\"1\",\"type\":\"count\",\"schema\":\"metric\",\"params\":{}},{\"id\":\"2\",\"type\":\"date_histogram\",\"schema\":\"segment\",\"params\":{\"field\":\"@timestamp\",\"interval\":\"auto\",\"customInterval\":\"2h\",\"min_doc_count\":1,\"extended_bounds\":{}}}],\"listeners\":{}}",
"uiStateJSON": "{}",
"description": "AreaChart",
"version": 1,
"kibanaSavedObjectMeta": {
"searchSourceJSON": "{\"index\":\"logstash-*\",\"query\":{\"query_string\":{\"query\":\"*\",\"analyze_wildcard\":true}},\"filter\":[]}"
}
},
"_meta": {
"savedObjectVersion": 2
}
}
]
Binary file not shown.
Loading