[Dashboard First] Genericize Attribute Service #76057

ThomThomson · 2020-08-27T00:34:13Z

Summary

This PR genericizes the Attribute service by adding an options prop.

The options prop contains:

An optional custom unwrap method that can be used to determine how a savedObject is transformed into the by value input. If left undefined, the attribute service returns the attributes key from the savedObject
An optional custom save method that can be run when the attribute service is wrapping attributes or changing the input to its by reference type. If left undefined, the attribute service will use savedObjectClient.update or savedObjectClient.create. If provided, the attribute service will use the same custom save method in both the update and create cases.

Using the AttributeService

To accept the defaults, you can get the attribute service via:

getAttributeService<MyCoolEmbeddableAttributes>('myCoolEmbeddableType');

Getting an attribute service with custom save, and unwrap methods:

getAttributeService<
  MyCoolEmbeddableAttributes,
  { id: string; attributes: MyCoolEmbeddableAttributes; },    // The by value input is optional and requires a key called 'attributes' which contains the byValueInput
  { id: string; savedObjectId: string; }  // The by reference input type is optional used SavedObjectEmbeddableInput by default
>(this.type, {
  customSaveMethod: (type, attributes, savedObjectId) => { ..... },
  customUnwrapMethod: (savedObject) => { ..... },
});

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…buteServiceCustomMethods

ThomThomson · 2020-08-27T13:42:20Z

@elasticmachine merge upstream

elasticmachine · 2020-08-27T14:34:05Z

Pinging @elastic/kibana-app (Team:KibanaApp)

majagrubic

I'm getting this error when trying to add a book embeddable:

Uncaught (in promise) SyntaxError: Unexpected token u in JSON at position 0
    at JSON.parse (<anonymous>)
    at Object.showSavedObject (visualize_embeddable_factory.tsx:50)
    at saved_object_finder.tsx:53
    at Array.filter (<anonymous>)
    at saved_object_finder.tsx:49

So I couldn't test this. Love addition of unit tests for the attribute service!
Some general thoughts below

majagrubic · 2020-08-27T14:56:48Z

src/plugins/dashboard/public/attribute_service/attribute_service.tsx

-          const savedItem = await this.savedObjectsClient.create(this.type, newAttributes);
-          return { savedObjectId: savedItem.id } as RefType;
+          const savedItem = this.options?.customSaveMethod
+            ? await this.options.customSaveMethod(this.type, newAttributes)


This assumes customSaveMethod will take type and attributes parameters by default.

customSaveMethod currently uses this signature:

customSaveMethod?: ( type: string, attributes: A, savedObjectId?: string ) => Promise<{ id: string }>;

It could easily be changed (and updated in the lens PR as well), but I can't think of a better signature to use.

majagrubic · 2020-08-27T15:00:38Z

src/plugins/dashboard/public/attribute_service/attribute_service.tsx

 > {
-  private embeddableFactory: EmbeddableFactory;
+  private embeddableFactory?: EmbeddableFactory;
+  private attributesKey: AttributesKey;


Do we really need this? It feels like we're trying to be a bit too generic at this point and it's just adding a lot of code branching. Visualize embeddable can wrap its input in attributes, and with having a custom save function, things should work fine. Ie.

I made this for the case where the only difference between the regular attribute service and the embeddable input that will be using it is the name of the key. This may or may not be the case for Visualize, but I think it's a good feature to support, especially if the 'by value' paradigm will be used this way in many different embeddables. The only new logic this results in is the attribute key line:

kibana/src/plugins/dashboard/public/attribute_service/attribute_service.tsx

Line 91 in 0249784

this.attributesKey = options?.attributesKey || (ATTRIBUTE_SERVICE_DEFAULT_KEY as AttributesKey);

Afterwards, the logic is all the same, accessing the attributes via:

input[this.attributesKey]

or setting the attributes via:

{ [this.attributesKey]: newAttributes }

I actually ended up removing the attributesKey to make this PR a little bit simpler. If we experiment with visualize and see that it could be helpful, it would be trivial to re-implement it in a followup PR.

majagrubic · 2020-08-27T15:02:03Z

src/plugins/dashboard/public/attribute_service/attribute_service.tsx

-          await this.savedObjectsClient.update(this.type, savedObjectId, newAttributes);
-          return { savedObjectId } as RefType;
+          if (this.options?.customSaveMethod) {
+            await this.options.customSaveMethod(this.type, newAttributes, savedObjectId);


Should this be a customUpdateMethod?

This is a good idea, and I considered it, but made the decision that the customSaveMethod would handle both the update and save cases.

I thought that forgoing this bit of logic in the attribute service

"if you have a custom update method, use that, else if you have a custom save method, use that, else use the default".

... in favor of a little more logic in the custom save method i.e. this little bit of logic from the Lens saved object store:

const result = await (id ? this.safeUpdate(id, attributes, references) : this.client.create(DOC_TYPE, attributes, { references, }));

... was a little bit cleaner. This all said, it would be possible to add the custom update method. What do you think works better?

ThomThomson · 2020-08-27T18:00:32Z

I am unable to re-create the error on adding a book embeddable. I double checked creation of a new book and adding it to library, creation of a new book by value, and adding an existing book from the library. Not sure where the problem came from!

ThomThomson · 2020-08-31T15:21:08Z

@elasticmachine merge upstream

…attributesKey

…buteServiceCustomMethods

ppisljar

code LGTM, didn't test out

streamich

Checked on Mac/Chrome, LGTM.

examples/embeddable_examples/public/book/book_embeddable.tsx

…l title instead of embeddable

ThomThomson · 2020-09-04T14:34:33Z

@elasticmachine merge upstream

dej611

Just a couple of observations. Tested with Safari 👍 .

src/plugins/dashboard/public/attribute_service/attribute_service.tsx

…omthomson/kibana into feature/attributeServiceCustomMethods

dej611

LGTM

kibanamachine · 2020-09-04T17:29:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 796b19e

Build metrics

page load bundle size

id	value	diff	baseline
dashboard	709.8KB	+1.7KB	708.2KB

History

💚 Build #72634 succeeded dcfa531
💚 Build #72064 succeeded e6c8e8b
💚 Build #71718 succeeded 522019f
💔 Build #71709 failed fd4a440
💚 Build #71658 succeeded f67fbdc

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

Genericized attribute service, with custom save and unwrap methods and added unit tests.

* master: (47 commits) Do not require id & description when creating a logstash pipeline (elastic#76616) Remove commented src/core/tsconfig file (elastic#76792) Replaced whitelistedHosts with allowedHosts in actions ascii docs (elastic#76731) [Dashboard First] Genericize Attribute Service (elastic#76057) [ci-metrics] unify distributable file count metrics (elastic#76448) [Security Solution][Detections] Handle conflicts on alert status update (elastic#75492) [eslint] convert to @typescript-eslint/no-unused-expressions (elastic#76471) [DOCS] Add default time range filter to advanced settings (elastic#76414) [Security Solution] Refactor NetworkTopNFlow to use Search Strategy (elastic#76249) [Dashboard] Update Index Patterns when Child Index Patterns Change (elastic#76356) [ML] Add option to Advanced Settings to set default time range filter for AD jobs (elastic#76347) Add CSM app to CODEOWNERS (elastic#76793) [Security Solution][Exceptions] - Updates exception item find sort field (elastic#76685) [Security Solution][Detections][Tech Debt] - Move to using common io-ts types (elastic#75009) [Lens] Drag dimension to replace (elastic#75895) URI encode the index names we fetch in the fetchIndices lib function. (elastic#76584) [Security Solution] Resolver retrieve entity id of documents without field mapped (elastic#76562) [Ingest Manager] validate agent route using AJV instead kbn-config-schema (elastic#76546) Updated non-dev usages of node-forge (elastic#76699) [Ingest Pipelines] Processor forms for processors K-S (elastic#75638) ...

ThomThomson added 3 commits August 26, 2020 18:07

genericized attribute service, started tests

11bf2a8

Merge branch 'master' of github.com:elastic/kibana into feature/attri…

e5d6dc7

…buteServiceCustomMethods

finished tests. Changed signature for custom save method

eae8d34

ThomThomson added Feature:Dashboard Dashboard related features Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Aug 27, 2020

Merge branch 'master' into feature/attributeServiceCustomMethods

0249784

ThomThomson marked this pull request as ready for review August 27, 2020 14:34

ThomThomson requested a review from a team August 27, 2020 14:34

ThomThomson requested a review from a team as a code owner August 27, 2020 14:34

majagrubic reviewed Aug 27, 2020

View reviewed changes

elasticmachine and others added 5 commits August 31, 2020 09:21

Merge branch 'master' into feature/attributeServiceCustomMethods

1d3e6fc

More explicit customSaveMethod call

fb07e63

Simplified attribute_service genericization by removing customizable …

f67fbdc

…attributesKey

Merge branch 'master' of github.com:elastic/kibana into feature/attri…

fd4a440

…buteServiceCustomMethods

merge cont

522019f

ppisljar approved these changes Sep 2, 2020

View reviewed changes

streamich approved these changes Sep 2, 2020

View reviewed changes

examples/embeddable_examples/public/book/book_embeddable.tsx Outdated Show resolved Hide resolved

added readonly, made attribute_service getInputAsRefType take optiona…

e6c8e8b

…l title instead of embeddable

ThomThomson mentioned this pull request Sep 3, 2020

[Dashboard First] Lens By Value #70272

Closed

7 tasks

Merge branch 'master' into feature/attributeServiceCustomMethods

dcfa531

dej611 reviewed Sep 4, 2020

View reviewed changes

src/plugins/dashboard/public/attribute_service/attribute_service.tsx Show resolved Hide resolved

src/plugins/dashboard/public/attribute_service/attribute_service.tsx Outdated Show resolved Hide resolved

Implemented review suggestions

94b9dd0

Merge branch 'feature/attributeServiceCustomMethods' of github.com:th…

796b19e

…omthomson/kibana into feature/attributeServiceCustomMethods

dej611 approved these changes Sep 4, 2020

View reviewed changes

ThomThomson merged commit 2606434 into elastic:master Sep 4, 2020

ThomThomson mentioned this pull request Sep 4, 2020

[7.x] [Dashboard First] Genericize Attribute Service (#76057) #76806

Merged

ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Sep 4, 2020

[Dashboard First] Genericize Attribute Service (elastic#76057)

4e9d6f0

Genericized attribute service, with custom save and unwrap methods and added unit tests.

ThomThomson added a commit that referenced this pull request Sep 4, 2020

[Dashboard First] Genericize Attribute Service (#76057) (#76806)

338a679

Genericized attribute service, with custom save and unwrap methods and added unit tests.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard First] Genericize Attribute Service #76057

[Dashboard First] Genericize Attribute Service #76057

ThomThomson commented Aug 27, 2020 •

edited

Loading

ThomThomson commented Aug 27, 2020

elasticmachine commented Aug 27, 2020

majagrubic left a comment

majagrubic Aug 27, 2020

ThomThomson Aug 27, 2020

majagrubic Aug 27, 2020

ThomThomson Aug 27, 2020

ThomThomson Sep 1, 2020

majagrubic Aug 27, 2020

ThomThomson Aug 27, 2020

ThomThomson commented Aug 27, 2020

ThomThomson commented Aug 31, 2020

ppisljar left a comment

streamich left a comment

ThomThomson commented Sep 4, 2020

dej611 left a comment

dej611 left a comment

kibanamachine commented Sep 4, 2020

[Dashboard First] Genericize Attribute Service #76057

[Dashboard First] Genericize Attribute Service #76057

Conversation

ThomThomson commented Aug 27, 2020 • edited Loading

Summary

Using the AttributeService

Checklist

For maintainers

ThomThomson commented Aug 27, 2020

elasticmachine commented Aug 27, 2020

majagrubic left a comment

Choose a reason for hiding this comment

majagrubic Aug 27, 2020

Choose a reason for hiding this comment

ThomThomson Aug 27, 2020

Choose a reason for hiding this comment

majagrubic Aug 27, 2020

Choose a reason for hiding this comment

ThomThomson Aug 27, 2020

Choose a reason for hiding this comment

ThomThomson Sep 1, 2020

Choose a reason for hiding this comment

majagrubic Aug 27, 2020

Choose a reason for hiding this comment

ThomThomson Aug 27, 2020

Choose a reason for hiding this comment

ThomThomson commented Aug 27, 2020

ThomThomson commented Aug 31, 2020

ppisljar left a comment

Choose a reason for hiding this comment

streamich left a comment

Choose a reason for hiding this comment

ThomThomson commented Sep 4, 2020

dej611 left a comment

Choose a reason for hiding this comment

dej611 left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 4, 2020

💚 Build Succeeded

Build metrics

page load bundle size

History

ThomThomson commented Aug 27, 2020 •

edited

Loading