[Lens] Make Lens saved object share-capable

[mbondyra]

Fixes #105808

Step 4 of https://www.elastic.co/guide/en/kibana/master/sharing-saved-objects.html#sharing-saved-objects-step-4

I was thinking about waiting with this one closer to 8.0.0, but it's such a small change that I don't think it'll cause any conflicts.

[elasticmachine]

Pinging @elastic/kibana-vis-editors (Team:VisEditors)

[dej611]

Code changes LGTM

[jportner]

The code changes LGTM!

I think your tests are likely failing due to the fixtures in x-pack/test/functional/es_archives/lens/basic/data.json.
I did a quick look at the objects and I noticed that the lens objects in that file don't have a migrationVersion field defined.
Because of that, the document migrator thinks that the object is up-to-date, and doesn't attempt to convert it. But our SOR deserializer also expects multiple-isolated object types to have a namespaces field.
So as a quick fix you could add these fields to your lens objects in that test fixture file:

namespaces: ['default'],
migrationVersion: { lens: '8.0.0' }

If that doesn't work, perhaps it would be a good idea to add some ES queries before the test assertions to see what saved object documents actually exist, and make sure that the lens was converted properly.

Feel free to ping me if you need a hand troubleshooting.

(Also note: I am not sure why x-pack/test/functional/es_archives/lens/basic/data.json.gz also exists, you should be able to delete that)

[mbondyra]

Found the cause of the breaking test. We use a bit hacky safeUpdate. When updating Lens, we firstly clean all the attributes and then save the new ones, that’s because update method merges instead of replacing (it's very well explained in the linked PR)

I wanted to replace update with index but I don't see it exposed in saved object code, unless I am missing something?

We use bulkUpdate assuming that it works in order.

https://github.com/elastic/kibana/blob/master/x-pack/plugins/lens/public/persistence/saved_object_store.ts#L84-L90

return (
      await this.client.bulkUpdate([
        { type: DOC_TYPE, id: savedObjectId, attributes: resetAttributes, references },
        { type: DOC_TYPE, id: savedObjectId, attributes, references },
      ])
    ).savedObjects[1];

I think something about bulkUpdate works different with my changes because now it returns empty attributes for the second object, not sure why. @jportner you know what could have caused that?
I am thinking to simply change it to two awaited updates, is there a better option?

EDIT: I found overwrite=true this comment so I've switched to using single create instead of two updates 🙌🏼

[jportner]

Found the cause of the breaking test. We use a bit hacky safeUpdate. When updating Lens, we firstly clean all the attributes and then save the new ones, that’s because update method merges instead of replacing (it's very well explained in the linked PR)

Nice sleuthing!

I think something about bulkUpdate works different with my changes because now it returns empty attributes for the second object, not sure why. @jportner you know what could have caused that?

I don't believe anything in the Elasticsearch Bulk API guarantees that operations will be executed in the order that they are specified. The SavedObjectsClient doesn't explicitly prevent specifying the same object twice in a single operation, but this hacky approach certainly isn't recommended.

I wanted to replace update with index but I don't see it exposed in saved object code, unless I am missing something?
...
EDIT: I found overwrite=true this comment but I don't see it exposed in SO client.

The SavedObjectsClient abstracts the bulk API details away from the consumer. To use an index operation, you'll want to make this call:

return await this.client.create(DOC_TYPE, attributes, { id: savedObjectId, overwrite: true });

[mbondyra]

@elasticmachine merge upstream

[mbondyra]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 7e8d7d2
• Storybooks Preview

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
lens	1.6MB	1.6MB	-375.0B

History

• 💚 Build #152621 succeeded debd125
• 💚 Build #152515 succeeded 0c2af3f
• 💔 Build #151812 failed 874977fb1d6c75d132c0cf3f4cda07fcc1577f7b
• 💔 Build #151532 failed 5e73d37a31c0e3d97323dc0028eabae588a3281b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[stratoula]

Very nice investigation Marta! Code LGTM I didn't test it locally but everything looks fine :)