Fixes #9680 (wrong title when overwriting saved objects) #44441

friol · 2019-08-29T22:46:54Z

Summary

As reported in issue #9680, currently, when overwriting a saved object with a new one with a different title, the confirmation message box will ask (erroneously) if you want to overwrite the object with the new title.
This fix shows the correct (old) title of the object you are going to overwrite.

Also, I updated the code to pass jest and Chrome functional tests ("copy saved object").

elasticmachine · 2019-08-29T22:46:56Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

kobelb

I will defer to others on whether or not we should be displaying the title of the object being imported, or the existing title of the saved-object in these situations. However, there are some existing issues with the implementation which we'll want addressed if we do proceed with these changes which I've commented on below.

src/core/server/saved_objects/import/import_saved_objects.ts

src/core/server/saved_objects/import/extract_errors.ts

src/core/server/saved_objects/service/index.ts

x-pack/legacy/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.ts

friol · 2019-08-30T05:35:47Z

Hello @kobelb and thanks for your comments.

The reason why the related Issue was opened is that, currently, when you are about to import a saved object with the same id, Kibana asks "Are you sure you want to overwrite <title of the object in .ndjson file>?", which is wrong in any case.

I'm passing down the source space Id, because, as far as I can see, "namespace" is the name of the target space. So, to recover the title of the object that is about to be overwritten, I need the source space - parameter to savedObjectsClient.get().

Ok for the bulkGet.

Thanks

kobelb · 2019-09-04T13:58:22Z

I'm passing down the source space Id, because, as far as I can see, "namespace" is the name of the target space. So, to recover the title of the object that is about to be overwritten, I need the source space - parameter to savedObjectsClient.get().

The target namespace is where we're going to be writing the saved object. If I understand correctly, we're wanting to display the title of the saved object we're about to overwrite, we'd be retrieving the title from the target namespace. Would you mind providing an example scenario where the "namespace" property on the "options" is different than the "sourceSpaceId"?

…d in extractErrors

friol · 2019-09-05T12:47:33Z

I'm passing down the source space Id, because, as far as I can see, "namespace" is the name of the target space. So, to recover the title of the object that is about to be overwritten, I need the source space - parameter to savedObjectsClient.get().

The target namespace is where we're going to be writing the saved object. If I understand correctly, we're wanting to display the title of the saved object we're about to overwrite, we'd be retrieving the title from the target namespace. Would you mind providing an example scenario where the "namespace" property on the "options" is different than the "sourceSpaceId"?

@kobelb: You are totally right.
I have removed the changes for srcSpaceId and used destSpaceId in the query.
I think one functional test will fail, but I suspect the test may be wrong, at this point.
Thanks.

src/core/server/saved_objects/import/extract_errors.ts

src/core/server/saved_objects/import/extract_errors.test.ts

rudolf

Thanks for your contributions @friol and sorry for taking so long to review, this one slipped under the radar.

I left some comments on the current implementation, but wonder if it wouldn't be much simpler to move all of the existing title resolution to the frontend? If you do savedObjectsClient.bulkGet on the frontend it's a single API call and with a bulk operation like importing all of your saved objects a little bit of extra latency shouldn't matter.

rudolf · 2019-10-18T10:21:01Z

src/core/server/saved_objects/import/extract_errors.ts

+  const errorSavedObjects = new Array();
+  for (const savedObject of savedObjectResults) {
+    if (savedObject.error) {
+      errorSavedObjects.push(savedObject);
+    }
+  }
+
+  // return an empty array if there are no errors
+  if (errorSavedObjects.length === 0) {
+    return [];
+  }


Can we test for savedObject.error.statusCode === 409 instead of just savedObject.error.

Nit: This code would be much more compact and readable if you used an array.filter like:

const errorSavedObjects = savedObjectResults.filter((so) => so.error && so.error.statusCode === 409)

rudolf · 2019-10-18T10:46:24Z

src/core/server/saved_objects/import/extract_errors.ts

        errors.push({
          id: savedObject.id,
          type: savedObject.type,
-          title,
+          title: realTitle,


When the import API returns an error, I think the correct response is to include the title of the object that failed to import. The API is saying "I failed while trying to import the title you gave me". Perhaps we can add a new field to the response like existingTitle.

rudolf · 2019-10-18T10:50:43Z

src/core/server/saved_objects/import/extract_errors.test.ts

+  test('extracts errors from saved objects', async () => {
+    const clientResponse = {
+      saved_objects: [
+        { id: '2', type: 'dashboard', attributes: { title: 'My Dashboard 2' } },


Can you change these titles to something like 'Existing Dashboard 2' otherwise we're not testing the existing title finding logic you added. I think this will also surface the the bug where we resolve the existing title of "My Dashboard 3" even though this wasn't a conflict error.

rudolf · 2019-10-18T10:52:42Z

src/core/server/saved_objects/import/import_saved_objects.test.ts

+
+    const clientResponse = {
+      saved_objects: [
+        { id: '1', type: 'index-pattern', attributes: { title: 'My Index Pattern' } },


Can we also change these titles to verify the existing title matching in the test.

lukeelmers · 2020-12-30T18:11:48Z

Hi @friol - thanks for this contribution. I'm going to go ahead and close the PR as stale since there hasn't been any activity for quite some time. If you'd still like to contribute, feel free to re-open & we can pick the conversation back up.

Thanks!

Fixes elastic#9680 (wrong title when overwriting saved objects)

96d3640

friol requested review from a team as code owners August 29, 2019 22:46

kobelb requested changes Aug 29, 2019

View reviewed changes

As asked by @kobelb, fetching all the titles with one bulk call

cfd882d

Reverted the srcSpaceId changes and modified srcSpaceId in destSpaceI…

5a2e961

…d in extractErrors

kobelb reviewed Sep 12, 2019

View reviewed changes

friol added 2 commits September 14, 2019 18:51

Merge remote-tracking branch 'upstream/master' into wrongtitlefix

ff9c4f0

Changes requested in review

b42bed1

friol requested a review from kobelb September 17, 2019 19:59

rudolf reviewed Oct 18, 2019

View reviewed changes

streamich added the Feature:Saved Objects label Feb 7, 2020

kibanamachine added the 💝community label Mar 23, 2020

lukeelmers closed this Dec 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #9680 (wrong title when overwriting saved objects) #44441

Fixes #9680 (wrong title when overwriting saved objects) #44441

friol commented Aug 29, 2019 •

edited

Loading

elasticmachine commented Aug 29, 2019

kobelb left a comment

friol commented Aug 30, 2019

kobelb commented Sep 4, 2019

friol commented Sep 5, 2019

rudolf left a comment

rudolf Oct 18, 2019

rudolf Oct 18, 2019

rudolf Oct 18, 2019

rudolf Oct 18, 2019

lukeelmers commented Dec 30, 2020

Fixes #9680 (wrong title when overwriting saved objects) #44441

Fixes #9680 (wrong title when overwriting saved objects) #44441

Conversation

friol commented Aug 29, 2019 • edited Loading

Summary

elasticmachine commented Aug 29, 2019

kobelb left a comment

Choose a reason for hiding this comment

friol commented Aug 30, 2019

kobelb commented Sep 4, 2019

friol commented Sep 5, 2019

rudolf left a comment

Choose a reason for hiding this comment

rudolf Oct 18, 2019

Choose a reason for hiding this comment

rudolf Oct 18, 2019

Choose a reason for hiding this comment

rudolf Oct 18, 2019

Choose a reason for hiding this comment

rudolf Oct 18, 2019

Choose a reason for hiding this comment

lukeelmers commented Dec 30, 2020

friol commented Aug 29, 2019 •

edited

Loading