Enforce `SavedObject.attributes` to have a serializable object shape #123605

pgayvallet · 2022-01-24T14:45:02Z

Summary

This PR is an attempt to enforce better attributes type for the SavedObject type and all the related APIs.

Note: this PR only impact typescript types and interfaces. The real runtime validation of the shape of the attributes when calling create/bulkCreate will be done in another PR.

Checklist

Documentation was added for features that require explanation or tutorials

…ttributes

pgayvallet

This is an attempt to add better type checking around the saved object attributes shape, to avoid the usage of types such as undefined or javascript primitives or arrays.

As the SavedObject type is used in a lot of places, this required to modify quite a lot of files to propagate the type changes.

To be honest, I'm not 100% sure these changes are really worth it, which is why I'd like to get the team's opinion on these changes.

Note: to address #123575, we'll add proper runtime validation for create and bulkCreate in another PR anyway.

pgayvallet · 2022-01-25T12:20:09Z

src/core/types/saved_objects.ts

@@ -66,7 +68,7 @@ export interface SavedObjectsMigrationVersion {
  [pluginName: string]: string;
 }

-export interface SavedObject<T = unknown> {
+export interface SavedObject<T extends SavedObjectAttributes = SavedObjectAttributes> {


So this is the root change of the PR. We're no longer defaulting to unknown and we're enforcing that T extends a serializable object/record, which will allow to detect misusages such as

soClient.create('dashboard', undefined, { id: 'test-undefined' });

I've tried something like this long ago but ended up abandoning it.

A serializable type means that specific types like https://github.com/elastic/kibana/blob/main/src/plugins/dashboard/public/application/lib/load_dashboard_by_title.ts#L16 fail because they don't have an index signature. But if we change the type to have an index signature it becomes less type safe because it allows properties not specified by the plugin's own generic.

So ideally we want to maybe say, if you provide your own type we'll take that as is (without the index signature) but if you don't provide a type we'll use Serializable because that's safer than unknown. It also seems like this should only apply to create operations, because when reading unknown is a safer type, and nudges plugins to create better types whereas Serializable will allow any property access.

So I don't know if there's a way to always provide better type safety, but IMO we're improving some type safety but reducing it in other places.

Yea, that's my feeling exactly.

There's no way to tell typescript that what we want as a type is an object WITHOUT index signature, without any property by default, and that can be extended, but only by objects having the correct type for their underlying properties (e.g serializable), which is a pain.

pgayvallet · 2022-01-25T12:22:23Z

src/plugins/data/common/query/types.ts

+// type required for index signature
+// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
+export type SavedQueryAttributes = {


This is one of the side-effect of having T extends SavedObjectAttributes. We're forced to use TS types instead of interfaces when overloading the default type of T to avoid an index signature error.

interface SavedObjectAttributes { [key: string]: SavedObjectAttribute; } interface SavedObject<T extends SavedObjectAttributes = SavedObjectAttributes> { ... } // valid let t: SavedObject<{ foo: string; bar: number }> // valid but eslint error because we should be using an interface type Foo = { foo: string; bar: number }; let t: SavedObject<Foo>; // TS2344: Type 'Foo' does not satisfy the constraint 'SavedObjectAttributes'. Index signature for type 'string' is missing in type 'Foo'. interface Foo { foo: string; bar: number }; let t: SavedObject<Foo>;

See microsoft/TypeScript#15300 for more details

it's not really blocker for the changes, but with our lint rules enforcing that we're using interfaces instead of types, this is awkward as it forces to add a linter exception for all type-based SO properties structures

IMO interface Foo extends SavedObjectAttributes { ... } should work.

However, there's the annoyance of importing the type everywhere. Also, it would allow devs to add additional non-typed keys because of the index signature 😕

I wonder if we should find an alternative to the current definition of SavedObjectAttributes. The current index signature does not stop us from storing an empty saved object:

await context.core.savedObjects.client.create('my_type', {});

So, probably, defining SavedObjectAttributes as type SavedObjectAttributes = object is enough to ensure that an object is specified (vs. undefined, null or any other primitive), and we avoid the issue type vs. interface issue. WDYT?

we hit a similar issue with persistable state interfaces and went on changing a bunch of interfaces to types

pgayvallet · 2022-01-25T12:38:10Z

src/plugins/data_views/public/saved_objects_client_wrapper.ts

    const response = (await this.savedObjectClient.find<T>(options)).savedObjects;
    return response.map<SavedObject<T>>(simpleSavedObjectToSavedObject);
  }

-  async get<T = unknown>(type: string, id: string) {
+  async get<T extends SavedObjectAttributes = SavedObjectAttributes>(type: string, id: string) {


This is the second thing a bit bothersome with that T extends SavedObjectAttributes = SavedObjectAttributes type change. We can no longer use unknown as a default for higher-level APIs when the shape of the attributes is not relevant, forcing to adapt all these higher-level APIs to also define the correct generic types.

elasticmachine · 2022-01-25T12:52:57Z

Pinging @elastic/kibana-core (Team:Core)

…ttributes

kibana-ci · 2022-01-31T07:13:16Z

💔 Build Failed

Failed CI Steps

Checks

Metrics [docs]

✅ unchanged

History

💔 Build #19334 failed 645f45c
💔 Build #19331 failed e40651c
💔 Build #19324 failed 9226fd4
💔 Build #19317 failed 90ad7d8
💔 Build #19315 failed 2ff807d

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

afharo · 2022-01-31T11:12:07Z

src/core/public/index.ts

@@ -115,7 +115,6 @@ export type {
  SavedObject,
  SavedObjectAttribute,
  SavedObjectAttributes,
-  SavedObjectAttributeSingle,


NIT: Do we need to run yarn docs:acceptApiChanges --docs?

afharo · 2022-01-31T11:14:41Z

src/core/public/saved_objects/saved_objects_client.ts

@@ -533,7 +548,9 @@ export class SavedObjectsClient {
    };
  };

-  private async performBulkResolve<T>(objects: ObjectTypeAndId[]) {
+  private async performBulkResolve<T extends SavedObjectAttributes = SavedObjectAttributes>(


nit: Previously, T wasn't optional. Should we keep it that way?

afharo · 2022-01-31T11:35:08Z

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

@@ -176,7 +176,10 @@ export interface SavedObjectsResolveImportErrorsOptions {
  createNewCopies: boolean;
 }

-export type CreatedObject<T> = SavedObject<T> & { destinationId?: string };
+export type CreatedObject<T extends SavedObjectAttributes = SavedObjectAttributes> =


Are we OK with making T optional when it was required?

afharo · 2022-01-31T11:45:22Z

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

@@ -11,7 +11,7 @@ import { SavedObjectsImportFailure, CreatedObject } from '../types';

 export function extractErrors(
  // TODO: define saved object type
-  savedObjectResults: Array<CreatedObject<unknown>>,
+  savedObjectResults: CreatedObject[],
  savedObjectsToImport: Array<SavedObject<any>>


nit: Removing anys

Suggested change

savedObjectsToImport: Array<SavedObject<any>>

savedObjectsToImport: Array<SavedObject<{ title?: string }>>

afharo · 2022-01-31T12:21:44Z

src/core/types/saved_objects.ts

@@ -33,7 +22,7 @@ export type SavedObjectAttribute = SavedObjectAttributeSingle | SavedObjectAttri
 * @public
 */
 export interface SavedObjectAttributes {
-  [key: string]: SavedObjectAttribute;
+  [key: string]: Serializable;


This change is not needed. Is it?

afharo · 2022-01-31T12:42:46Z

src/plugins/data/common/query/types.ts

+// type required for index signature
+// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
+export type SavedQueryAttributes = {


IMO interface Foo extends SavedObjectAttributes { ... } should work.

However, there's the annoyance of importing the type everywhere. Also, it would allow devs to add additional non-typed keys because of the index signature 😕

I wonder if we should find an alternative to the current definition of SavedObjectAttributes. The current index signature does not stop us from storing an empty saved object:

await context.core.savedObjects.client.create('my_type', {});

So, probably, defining SavedObjectAttributes as type SavedObjectAttributes = object is enough to ensure that an object is specified (vs. undefined, null or any other primitive), and we avoid the issue type vs. interface issue. WDYT?

ppisljar

app services changes LGTM

pgayvallet · 2022-02-09T19:56:53Z

After discussing it with @rudolf we came to the conclusion that this wasn't worth it because of the TS limitation around what we're trying to achieve (#123605 (comment)). Closing

pgayvallet added 13 commits January 24, 2022 15:44

Force savedObject attributes to have an object shape

9fa4958

fix more types

03663a6

chaining type violation fixes

92c851c

Merge remote-tracking branch 'upstream/main' into kbn-123575-so-obj-a…

8f0db00

…ttributes

trying something else

85037d5

where am I going?

c0dca5d

more fixes

483efcb

one more

2ff807d

fix repo code

7be4539

fix type type

90ad7d8

yet more fixes

9226fd4

getting close maybe

e40651c

maybe last ones?

645f45c

pgayvallet changed the title ~~Force savedObject attributes to have an object shape~~ Enforce SavedObject.attributes to have a serializable object shape Jan 25, 2022

remove old type

65cc833

pgayvallet commented Jan 25, 2022

View reviewed changes

pgayvallet added Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jan 25, 2022

pgayvallet requested a review from a team January 25, 2022 12:51

pgayvallet marked this pull request as ready for review January 25, 2022 12:52

pgayvallet requested a review from a team as a code owner January 25, 2022 12:52

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes v8.1.0 labels Jan 25, 2022

Merge remote-tracking branch 'upstream/main' into kbn-123575-so-obj-a…

0b2d1c1

…ttributes

afharo reviewed Jan 31, 2022

View reviewed changes

afharo requested a review from a team January 31, 2022 13:02

ppisljar approved these changes Feb 9, 2022

View reviewed changes

pgayvallet closed this Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce `SavedObject.attributes` to have a serializable object shape #123605

Enforce `SavedObject.attributes` to have a serializable object shape #123605

pgayvallet commented Jan 24, 2022 •

edited

Loading

pgayvallet left a comment

pgayvallet Jan 25, 2022 •

edited

Loading

rudolf Jan 31, 2022

pgayvallet Feb 1, 2022

pgayvallet Jan 25, 2022

afharo Jan 31, 2022

ppisljar Feb 9, 2022

pgayvallet Jan 25, 2022

elasticmachine commented Jan 25, 2022

kibana-ci commented Jan 31, 2022 •

edited

Loading

afharo Jan 31, 2022

afharo Jan 31, 2022

afharo Jan 31, 2022

afharo Jan 31, 2022

afharo Jan 31, 2022

afharo Jan 31, 2022

ppisljar left a comment

pgayvallet commented Feb 9, 2022

	savedObjectsToImport: Array<SavedObject<any>>
	savedObjectsToImport: Array<SavedObject<{ title?: string }>>

Enforce SavedObject.attributes to have a serializable object shape #123605

Enforce SavedObject.attributes to have a serializable object shape #123605

Conversation

pgayvallet commented Jan 24, 2022 • edited Loading

Summary

Checklist

pgayvallet left a comment

Choose a reason for hiding this comment

pgayvallet Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jan 25, 2022

kibana-ci commented Jan 31, 2022 • edited Loading

💔 Build Failed

Failed CI Steps

Metrics [docs]

History

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

pgayvallet commented Feb 9, 2022

Enforce `SavedObject.attributes` to have a serializable object shape #123605

Enforce `SavedObject.attributes` to have a serializable object shape #123605

pgayvallet commented Jan 24, 2022 •

edited

Loading

pgayvallet Jan 25, 2022 •

edited

Loading

kibana-ci commented Jan 31, 2022 •

edited

Loading