diff --git a/dev_docs/tutorials/saved_objects.mdx b/dev_docs/tutorials/saved_objects.mdx index bd7d231218af..644afadb268c 100644 --- a/dev_docs/tutorials/saved_objects.mdx +++ b/dev_docs/tutorials/saved_objects.mdx @@ -122,7 +122,7 @@ Since Elasticsearch has a default limit of 1000 fields per index, plugins should fields they add to the mappings. Similarly, Saved Object types should never use `dynamic: true` as this can cause an arbitrary amount of fields to be added to the .kibana index. - ## References +## References Declare by adding an id, type and name to the `references` array. @@ -155,12 +155,14 @@ identify this reference. This guarantees that the id the reference points to alw visualization id was directly stored in `dashboard.panels[0].visualization` there is a risk that this id gets updated without updating the reference in the references array. -## Writing migrations +## Migrations Saved Objects support schema changes between Kibana versions, which we call migrations. Migrations are - applied when a Kibana installation is upgraded from one version to the next, when exports are imported via + applied when a Kibana installation is upgraded from one version to a newer version, when exports are imported via the Saved Objects Management UI, or when a new object is created via the HTTP API. +### Writing migrations + Each Saved Object type may define migrations for its schema. Migrations are specified by the Kibana version number, receive an input document, and must return the fully migrated document to be persisted to Elasticsearch. @@ -241,10 +243,11 @@ export const dashboardVisualization: SavedObjectsType = { in which this migration was released. So if you are creating a migration which will be part of the v7.10.0 release, but will also be backported and released as v7.9.3, the migration version should be: 7.9.3. - Migrations should be written defensively, an exception in a migration function will prevent a Kibana upgrade from succeeding and will cause downtime for our users. - Having said that, if a - document is encountered that is not in the expected shape, migrations are encouraged to throw an exception to abort the upgrade. In most scenarios, it is better to - fail an upgrade than to silently ignore a corrupt document which can cause unexpected behaviour at some future point in time. +Migrations should be written defensively, an exception in a migration function will prevent a Kibana upgrade from succeeding and will cause downtime for our users. +Having said that, if a document is encountered that is not in the expected shape, migrations are encouraged to throw an exception to abort the upgrade. In most scenarios, it is better to +fail an upgrade than to silently ignore a corrupt document which can cause unexpected behaviour at some future point in time. When such a scenario is encountered, +the error should be verbose and informative so that the corrupt document can be corrected, if possible. -It is critical that you have extensive tests to ensure that migrations behave as expected with all possible input documents. Given how simple it is to test all the branch -conditions in a migration function and the high impact of a bug in this code, there’s really no reason not to aim for 100% test code coverage. +### Testing Migrations + +Bugs in a migration function cause downtime for our users and therefore have a very high impact. Follow the . diff --git a/dev_docs/tutorials/testing_plugins.mdx b/dev_docs/tutorials/testing_plugins.mdx index 96e13555a36a..55b662421cbd 100644 --- a/dev_docs/tutorials/testing_plugins.mdx +++ b/dev_docs/tutorials/testing_plugins.mdx @@ -569,7 +569,7 @@ describe('renderApp', () => { }); ``` -### SavedObjects +### SavedObjectsClient #### Unit Tests @@ -794,6 +794,192 @@ Kibana and esArchiver to load fixture data into Elasticsearch. _todo: fully worked out example_ +### Saved Objects migrations + +_Also see ._ + +It is critical that you have extensive tests to ensure that migrations behave as expected with all possible input +documents. Given how simple it is to test all the branch conditions in a migration function and the high impact of a +bug in this code, there’s really no reason not to aim for 100% test code coverage. + +It's recommend that you primarily leverage unit testing with Jest for testing your migration function. Unit tests will +be a much more effective approach to testing all the different shapes of input data and edge cases that your migration +may need to handle. With more complex migrations that interact with several components or may behave different depending +on registry contents (such as Embeddable migrations), we recommend that you use the Jest Integration suite which allows +you to create a full instance Kibana and all plugins in memory and leverage the import API to test migrating documents. + +#### Throwing exceptions +Keep in mind that any exception thrown by your migration function will cause Kibana to fail to upgrade. This should almost +never happen for our end users and we should be exhaustive in our testing to be sure to catch as many edge cases that we +could possibly handle. This entails ensuring that the migration is written defensively; we should try to avoid every bug +possible in our implementation. + +In general, exceptions should only be thrown when the input data is corrupted and doesn't match the expected schema. In +such cases, it's important that an informative error message is included in the exception and we do not rely on implicit +runtime exceptions such as "null pointer exceptions" like `TypeError: Cannot read property 'foo' of undefined`. + +#### Unit testing + +Unit testing migration functions is typically pretty straight forward and comparable to other types of Jest testing. In +general, you should focus this tier of testing on validating output and testing input edge cases. One focus of this tier +should be trying to find edge cases that throw exceptions the migration shouldn't. As you can see in this simple +example, the coverage here is very exhaustive and verbose, which is intentional. + +```ts +import { migrateCaseFromV7_9_0ToV7_10_0 } from './case_migrations'; + +const validInput_7_9_0 = { + id: '1', + type: 'case', + attributes: { + connector_id: '1234'; + } +} + +describe('Case migrations v7.7.0 -> v7.8.0', () => { + it('transforms the connector field', () => { + expect(migrateCaseFromV7_9_0ToV7_10_0(validInput_7_9_0)).toEqual({ + id: '1', + type: 'case', + attributes: { + connector: { + id: '1234', // verify id was moved into subobject + name: 'none', // verify new default field was added + } + } + }); + }); + + it('handles empty string', () => { + expect(migrateCaseFromV7_9_0ToV7_10_0({ + id: '1', + type: 'case', + attributes: { + connector_id: '' + } + })).toEqual({ + id: '1', + type: 'case', + attributes: { + connector: { + id: 'none', + name: 'none', + } + } + }); + }); + + it('handles null', () => { + expect(migrateCaseFromV7_9_0ToV7_10_0({ + id: '1', + type: 'case', + attributes: { + connector_id: null + } + })).toEqual({ + id: '1', + type: 'case', + attributes: { + connector: { + id: 'none', + name: 'none', + } + } + }); + }); + + it('handles undefined', () => { + expect(migrateCaseFromV7_9_0ToV7_10_0({ + id: '1', + type: 'case', + attributes: { + // Even though undefined isn't a valid JSON or Elasticsearch value, we should test it anyways since there + // could be some JavaScript layer that casts the field to `undefined` for some reason. + connector_id: undefined + } + })).toEqual({ + id: '1', + type: 'case', + attributes: { + connector: { + id: 'none', + name: 'none', + } + } + }); + + expect(migrateCaseFromV7_9_0ToV7_10_0({ + id: '1', + type: 'case', + attributes: { + // also test without the field present at all + } + })).toEqual({ + id: '1', + type: 'case', + attributes: { + connector: { + id: 'none', + name: 'none', + } + } + }); + }); +}); +``` + +#### Integration testing +With more complicated migrations, the behavior of the migration may be dependent on values from other plugins which may +be difficult or even impossible to test with unit tests. You need to actually bootstrap Kibana, load the plugins, and +then test the full end-to-end migration. This type of set up will also test ingesting your documents into Elasticsearch +against the mappings defined by your Saved Object type. + +This can be achieved using the `jest_integration` suite and the `kbnTestServer` utility for starting an in-memory +instance of Kibana. You can then leverage the import API to test migrations. This API applies the same migrations to +imported documents as are applied at Kibana startup and is much easier to work with for testing. + +```ts +// You may need to adjust these paths depending on where your test file is located. +// The absolute path is src/core/test_helpers/so_migrations +import { createTestHarness, SavedObjectTestHarness } from '../../../../src/core/test_helpers/so_migrations'; + +describe('my plugin migrations', () => { + let testHarness: SavedObjectTestHarness; + + beforeAll(async () => { + testHarness = createTestHarness(); + await testHarness.start(); + }); + + afterAll(async () => { + await testHarness.stop(); + }); + + it('successfully migrates valid case documents', async () => { + expect( + await testHarness.migrate([ + { type: 'case', id: '1', attributes: { connector_id: '1234' }, references: [] }, + { type: 'case', id: '2', attributes: { connector_id: '' }, references: [] }, + { type: 'case', id: '3', attributes: { connector_id: null }, references: [] }, + ]) + ).toEqual([ + expect.objectContaining( + { type: 'case', id: '1', attributes: { connector: { id: '1234', name: 'none' } } }), + expect.objectContaining( + { type: 'case', id: '2', attributes: { connector: { id: 'none', name: 'none' } } }), + expect.objectContaining( + { type: 'case', id: '3', attributes: { connector: { id: 'none', name: 'none' } } }), + ]) + }) +}) +``` + +There are some caveats about using the import/export API for testing migrations: +- You cannot test the startup behavior of Kibana this way. This should not have any effect on type migrations but does + mean that this method cannot be used for testing the migration algorithm itself. +- While not yet supported, if support is added for migrations that affect multiple types, it's possible that the + behavior during import may vary slightly from the upgrade behavior. + ### Elasticsearch _How to test ES clients_