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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from Sep 3, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f991e39
Legacy SO import: Fix bug causing multiple overrides to only show the…
pgayvallet Sep 2, 2020
fd7f71b
eslint
pgayvallet Sep 2, 2020
d80ad52
fix for loops
pgayvallet Sep 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 15 additions & 16 deletions src/plugins/saved_objects_management/public/lib/resolve_saved_objects.ts
View file
Open in desktop
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 @@ -228,17 +224,17 @@ export async function resolveIndexPatternConflicts(
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,7 +249,7 @@ 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?
Expand All @@ -262,7 +258,7 @@ export async function resolveSavedSearches(
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

kibana/src/core/public/overlays/modal/modal_service.tsx

Lines 123 to 128 in 395d621

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
56 changes: 54 additions & 2 deletions test/functional/apps/management/_import_objects.js
View file
Open in desktop
Expand Up @@ -203,12 +203,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 +280,58 @@ export default function ({ getService, getPageObjects }) {
expect(isSuccessful).to.be(true);
});

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

////
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
26 changes: 26 additions & 0 deletions test/functional/apps/management/exports/_import_index_patterns_multiple_exists.json
View file
Open in desktop

Large diffs are not rendered by default.

36 changes: 36 additions & 0 deletions test/functional/apps/management/exports/_import_objects_multiple_exists.json
View file
Open in desktop
@@ -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 added test/functional/fixtures/es_archiver/saved_objects_imports/data.json.gz
View file
Open in desktop
Binary file not shown.