migrate savedObjects routes to core #56734

pgayvallet · 2020-02-04T11:38:48Z

Summary

Fix #50321

Migrate the /api/saved_objects/* routes fromlegacy to core

Checklist

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

elasticmachine · 2020-02-04T11:39:21Z

Pinging @elastic/kibana-platform (Team:Platform)

src/core/server/saved_objects/routes/get.test.ts

…-so-routes

pgayvallet · 2020-02-11T12:24:19Z

src/core/server/saved_objects/export/get_sorted_objects_for_export.ts

+  if ((types?.length ?? 0) > 0 && (objects?.length ?? 0) > 0) {
+    throw Boom.badRequest(`Can't specify both "types" and "objects" properties when exporting`);
+  }


The route validation was using an nand(types, objects). As we don't have the equivalent in config-schema (probably for the best), I moved this check in the actual service method. This probably is it's place anyway.

I also had to fix some calls that were actually sending both (with types being used as they are exclude in the business logic)

pgayvallet · 2020-02-11T12:25:14Z

src/core/server/saved_objects/export/get_sorted_objects_for_export.test.ts

-      types: ['index-pattern', 'search'],
      objects: [
        {
          type: 'index-pattern',


See comment on get_sorted_objects_for_export.ts

src/core/server/saved_objects/routes/bulk_create.test.ts

pgayvallet · 2020-02-11T12:27:33Z

src/core/server/saved_objects/routes/bulk_update.ts

+    router.handleLegacyErrors(async (context, req, res) => {
+      const savedObject = await context.core.savedObjects.client.bulkUpdate(req.body);
+      return res.ok({ body: savedObject });
+    })


The savedObject service actually throws boom errors. I had to wrap the handlers using handleLegacyErrors.

pgayvallet · 2020-02-11T12:31:29Z

src/core/server/saved_objects/routes/export.ts

+import {
+  createPromiseFromStreams,
+  createMapStream,
+  createConcatStream,
+} from '../../../../legacy/utils/streams';


The whole SO import/export APIs rely on our legacy stream utilities. As we don't want to loose the (not yet used but anyway) http streaming feature for the export and import, I kept using them (changing that would mean some structural changes in the getSortedObjectsForExport method and it's underlying components. This is not the first time we are importing this in core SO code though. We probably will have to move it to core at some point (FTR services also uses them, so this will not be removed when we remove legacy, at least no without rewrite), but the PR is already big, did not want to do it here.

pgayvallet · 2020-02-11T12:35:53Z

src/core/server/saved_objects/saved_objects_config.ts

+export const savedObjectsConfig = {
+  path: 'savedObjects',
+  schema: schema.object({
+    maxImportPayloadBytes: schema.number({ defaultValue: 10485760 }),
+    maxImportExportSize: schema.number({ defaultValue: 10000 }),
+  }),
+};


For the import/export routes, I now need the 'real' savedObject config. What we called that previously was the config for the migrations config namespace. I created the SavedObjectConfig class to 'merge' them. I kept the migrations properties as an underlying object to avoid breaking every signature in the SO migrator code, and also because I think this isolation makes sense.

not a blocker, just curious: Do we want to move migrations --> savedObjects.migration in the future? Will we have migrations fields in v8 with saved objects v2? @rudolf

pgayvallet · 2020-02-11T12:38:33Z

src/legacy/core_plugins/kibana/migrations/types.ts

+// eslint-disable-next-line @kbn/eslint/no-restricted-paths
+import { SavedObjectReference } from '../../../../core/server';


This is server code (SO migrations), but outside of the server folder, which explains the disabled rule

pgayvallet · 2020-02-11T12:40:47Z

test/api_integration/apis/general/index.js

    loadTestFile(require.resolve('./csp'));
-    loadTestFile(require.resolve('./prototype_pollution'));


This test was using a legacy SO endpoint to test against prototype pollution. We don't have yet that kind of protection in NP. After discussion with @legrego, we agreed to delete the test, and to properly reimplement it when the feature would lands to NP.

pgayvallet · 2020-02-11T12:42:35Z

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

      objects: options.objects,
      savedObjectsClient,
-      types: eligibleTypes,
+      types: options.objects.length > 0 ? undefined : eligibleTypes,


As explained in a previous comment, When sending both objects and types, only objects were used. However the validation to not send both was performed in the route handler. As I moved it to the service, I had to adapt some calls to no longer send both.

@legrego do you happen to recall why we used to always be passing in eligibleTypes here? It looks like this was being ignored... I'm tempted to say we should just always pass in undefined here if that's the case.

The intent was to prevent namespace agnostic types from being specified, but that's handled on the subsequent call to importSavedObjects, so I agree we should just always pass undefined here, rather than eligibleTypes

Ack, will just remove the types parameter from the call then

pgayvallet · 2020-02-11T12:43:50Z

src/legacy/server/saved_objects/saved_objects_mixin.test.js

-  describe('Routes', () => {
-    it('should create 12 routes', () => {
-      savedObjectsMixin(mockKbnServer, mockServer);
-      expect(mockServer.route).toHaveBeenCalledTimes(12);
-    });
-    it('should add POST /api/saved_objects/_bulk_create', () => {
-      savedObjectsMixin(mockKbnServer, mockServer);
-      expect(mockServer.route).toHaveBeenCalledWith(
-        expect.objectContaining({ path: '/api/saved_objects/_bulk_create', method: 'POST' })
-      );
-    });


This is already covered by the API integration tests, so I did not move these tests to core. Tell me if I should.

src/core/server/saved_objects/routes/find.ts

…-so-routes

src/core/server/saved_objects/routes/bulk_create.test.ts

mshustov · 2020-02-14T14:06:39Z

src/core/server/saved_objects/routes/test_utils.ts


-  return server;
-}
+  httpSetup.registerRouteHandlerContext(coreId, 'core', async (ctx, req, res) => {


If you already created the mock, we can expose it and document a usage example. can be done in a separate PR ofc

created #57844

src/core/server/saved_objects/routes/find.ts

mshustov · 2020-02-14T14:13:25Z

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

+    router.handleLegacyErrors(async (context, req, res) => {
+      const { overwrite } = req.query;
+      const file = req.body.file as FileStream;
+      const fileExtension = extname(file.hapi.filename).toLowerCase();


I haven't checked, but believe we should be able to access filename via Content-Disposition header https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition

Content-Disposition is not a header in multipart/form-data, it's included in the actual payload. The only way to get the file name is to access the parsed payload, and HAPI do the parsing and exposes the name in this property. We can't do differently without tricky changes.

FYI, an example of such request in a 'json' format is

const request = { method: 'POST', url: '/api/saved_objects/_import', payload: [ '--EXAMPLE', 'Content-Disposition: form-data; name="file"; filename="export.ndjson"', 'Content-Type: application/ndjson', '', '{"type":"index-pattern","id":"my-pattern","attributes":{"title":"my-pattern-*"}}', '--EXAMPLE--', ].join('\r\n'), headers: { 'content-Type': 'multipart/form-data; boundary=EXAMPLE', }, };

commented in #56944 let's merge as is

src/core/server/saved_objects/saved_objects_config.ts

mshustov · 2020-02-14T14:21:42Z

src/core/server/saved_objects/saved_objects_config.ts

+export const savedObjectsConfig = {
+  path: 'savedObjects',
+  schema: schema.object({
+    maxImportPayloadBytes: schema.number({ defaultValue: 10485760 }),
+    maxImportExportSize: schema.number({ defaultValue: 10000 }),
+  }),
+};


not a blocker, just curious: Do we want to move migrations --> savedObjects.migration in the future? Will we have migrations fields in v8 with saved objects v2? @rudolf

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

…-so-routes

kibanamachine · 2020-02-18T10:32:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: aaae92d

History

💚 Build #26899 succeeded d25b471
💚 Build #26421 succeeded 4e97843
💚 Build #25860 succeeded d3c019e
💔 Build #25843 failed 459334d
💚 Build #25822 succeeded db4d79f

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

* migrate `get` route * migrate `create` route * migrate `delete` route * migrate `find` route * migrate `update` route * migrate `bulk_get` route * migrate `bulk_create` route * remove route-related mixin tests * migrate `bulk_update` route * fix expectTypeRequired assertion * migrate `log_legacy_imports` route * migrate `export` route * fix karma tests * array is better than object in some situations. * remove prototype pollution tests * adapt ftr assertions * adapt ftr assertions * adapt yet more ftr assertions * migrate `import` route * fix test tests * fix getSortedObjectsForExport usages * fix snapshots * fix so ui exports usages due to merge * create router with prefix * creates `savedObjects` namespace config in addition to `migrations` * migrate `resolve_import_errors` route * remove old types file * fix FTR assertion * remove types parameter from copy_to_space * move route tests to integration_tests * use byteSize instead of number * fix unit tests * add has_reference query parameter Co-authored-by: Mikhail Shustov <[email protected]>

* migrate `get` route * migrate `create` route * migrate `delete` route * migrate `find` route * migrate `update` route * migrate `bulk_get` route * migrate `bulk_create` route * remove route-related mixin tests * migrate `bulk_update` route * fix expectTypeRequired assertion * migrate `log_legacy_imports` route * migrate `export` route * fix karma tests * array is better than object in some situations. * remove prototype pollution tests * adapt ftr assertions * adapt ftr assertions * adapt yet more ftr assertions * migrate `import` route * fix test tests * fix getSortedObjectsForExport usages * fix snapshots * fix so ui exports usages due to merge * create router with prefix * creates `savedObjects` namespace config in addition to `migrations` * migrate `resolve_import_errors` route * remove old types file * fix FTR assertion * remove types parameter from copy_to_space * move route tests to integration_tests * use byteSize instead of number * fix unit tests * add has_reference query parameter Co-authored-by: Mikhail Shustov <[email protected]> Co-authored-by: Pierre Gayvallet <[email protected]>

migrate get route

d69bfaa

pgayvallet added v7.7.0 release_note:skip Skip the PR/issue when compiling release notes Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Feb 4, 2020

pgayvallet commented Feb 4, 2020

View reviewed changes

src/core/server/saved_objects/routes/get.test.ts Outdated Show resolved Hide resolved

pgayvallet added 19 commits February 4, 2020 15:54

migrate create route

97137df

migrate delete route

50ae2b8

migrate find route

7760136

migrate update route

3636265

migrate bulk_get route

ac30dad

migrate bulk_create route

77d1e8a

remove route-related mixin tests

e16802a

migrate bulk_update route

2a699c4

fix expectTypeRequired assertion

22f8ba7

migrate log_legacy_imports route

e617d7e

migrate export route

b05883c

fix karma tests

9b18fdd

array is better than object in some situations.

38e25a0

remove prototype pollution tests

cac5e14

adapt ftr assertions

3f9992f

adapt ftr assertions

03bb316

adapt yet more ftr assertions

27bd062

Merge remote-tracking branch 'upstream/master' into kbn-50321-migrate…

d517045

…-so-routes

Merge remote-tracking branch 'upstream/master' into kbn-50321-migrate…

5671fc6

…-so-routes

pgayvallet mentioned this pull request Feb 6, 2020

[NP] route validation does not properly parse multipart/form-data payloads #56951

Closed

migrate import route

ad0fcd1

pgayvallet added 4 commits February 11, 2020 11:21

creates savedObjects namespace config in addition to migrations

ca1a90c

Merge remote-tracking branch 'upstream/master' into kbn-50321-migrate…

db4d79f

…-so-routes

migrate resolve_import_errors route

cfbde19

remove old types file

459334d

pgayvallet commented Feb 11, 2020

View reviewed changes

pgayvallet marked this pull request as ready for review February 11, 2020 12:44

pgayvallet requested review from a team as code owners February 11, 2020 12:44

pgayvallet commented Feb 11, 2020

View reviewed changes

src/core/server/saved_objects/routes/find.ts Outdated Show resolved Hide resolved

pgayvallet added 3 commits February 11, 2020 14:37

fix FTR assertion

d3c019e

Merge remote-tracking branch 'upstream/master' into kbn-50321-migrate…

84ad5ce

…-so-routes

remove types parameter from copy_to_space

4e97843

kobelb approved these changes Feb 13, 2020

View reviewed changes

mshustov reviewed Feb 14, 2020

View reviewed changes

pgayvallet and others added 6 commits February 14, 2020 16:01

Merge remote-tracking branch 'upstream/master' into kbn-50321-migrate…

16a7599

…-so-routes

move route tests to integration_tests

178ea49

use byteSize instead of number

56fc21f

fix unit tests

d25b471

Merge branch 'master' into pr/56734

6ff7d55

add has_reference query parameter

aaae92d

mshustov approved these changes Feb 18, 2020

View reviewed changes

mshustov merged commit 055c611 into elastic:master Feb 18, 2020

mshustov mentioned this pull request Feb 18, 2020

[7.x] migrate savedObjects routes to core (#56734) #57846

Merged

mshustov added the backported label Feb 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate savedObjects routes to core #56734

migrate savedObjects routes to core #56734

pgayvallet commented Feb 4, 2020 •

edited

Loading

elasticmachine commented Feb 4, 2020

pgayvallet Feb 11, 2020

pgayvallet Feb 11, 2020

pgayvallet Feb 11, 2020

pgayvallet Feb 11, 2020

pgayvallet Feb 11, 2020

mshustov Feb 14, 2020

pgayvallet Feb 11, 2020

pgayvallet Feb 11, 2020

pgayvallet Feb 11, 2020

kobelb Feb 12, 2020

legrego Feb 12, 2020

pgayvallet Feb 13, 2020

pgayvallet Feb 11, 2020

mshustov Feb 14, 2020

mshustov Feb 18, 2020

mshustov Feb 14, 2020

pgayvallet Feb 14, 2020

mshustov Feb 18, 2020 •

edited

Loading

mshustov Feb 14, 2020

kibanamachine commented Feb 18, 2020

		// eslint-disable-next-line @kbn/eslint/no-restricted-paths
		import { SavedObjectReference } from '../../../../core/server';

		loadTestFile(require.resolve('./csp'));
		loadTestFile(require.resolve('./prototype_pollution'));

migrate savedObjects routes to core #56734

migrate savedObjects routes to core #56734

Conversation

pgayvallet commented Feb 4, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Feb 4, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Feb 18, 2020

💚 Build Succeeded

History

pgayvallet commented Feb 4, 2020 •

edited

Loading

mshustov Feb 18, 2020 •

edited

Loading