Partially move server-side `savedObjects` domain implementation to packages #137448

pgayvallet · 2022-07-28T13:05:54Z

Summary

First part of third stage (yea, I know, hopefully the next one with be the last) of #135820

Now that all the server-side savedObjects public types have been moved to packages, we can start moving the internal implementation.

Unsurprisingly, this is probably the biggest Core domain in term of files, and will be the same in term of number of packages.

From my initial investigation, I'm currently planning on having the following packages (removed the ``@kbn/core-saved-objects` prefix here):

@kbn/core-saved-objects-utils-server
- the public concrete static code that is exposed to external consumers
  - SavedObjectsUtils
  - SavedObjectsErrorHelper
  - some other misc stuff
@kbn/core-saved-objects-base-internal
- the common bits of the implementation that are shared between the other internal packages
  - mappings
  - serialization
  - validation
  - version
  - type registry
  - legacy aliases
  - config (could have only put the config types here, but for the sake of simplicity the schemas were moved too)
@kbn/core-saved-objects-api-internal (depends on base)
- the SOR and SOC implementations
@kbn/core-saved-objects-import-export-internal (depends on base and api)
- the importer/exporter implementations
@kbn/core-saved-objects-migration-internal (depends on base and api)
- all the SO migration code
@kbn/core-saved-objects-internal (depends on all the others)
- SO service
- registered routes
the associated mock packages

This PR is the first step, handling the creation of and code extraction to the following packages:

@kbn/core-saved-objects-utils-server
@kbn/core-saved-objects-base-server-internal
@kbn/core-saved-objects-base-server-mocks

NOTE: I did not look yet at the problem of the cyclic dependency with types of CoreUsageData. This only impacts the higher-level internal package, @kbn/core-saved-objects-internal, and I will take a look when the time comes

…-ref HEAD~1..HEAD --fix'

…mentation-stage-1

…uild_manifest'

pgayvallet

Self-review

pgayvallet · 2022-07-29T13:00:20Z

...e/saved-objects/core-saved-objects-base-server-internal/src/serialization/serializer.test.ts

+const createMockedTypeRegistry = ({
+ isNamespaceAgnostic,
+ isSingleNamespace,
+ isMultiNamespace,
+}: {
+ isNamespaceAgnostic: boolean;
+ isSingleNamespace: boolean;
+ isMultiNamespace: boolean;
+}): ISavedObjectTypeRegistry => {
+ const typeRegistry: Partial<ISavedObjectTypeRegistry> = {
+ isNamespaceAgnostic: jest.fn().mockReturnValue(isNamespaceAgnostic),
+ isSingleNamespace: jest.fn().mockReturnValue(isSingleNamespace),
+ isMultiNamespace: jest.fn().mockReturnValue(isMultiNamespace),
+ };
+ return typeRegistry as ISavedObjectTypeRegistry;
+};


Only test of the @kbn/core-saved-objects-base-server-internal that was using a mock from code that was moved to this package. I KISS'ed and created an inline mock helper.

The alternative would have been to create 2 additional packages (impl+mock) just for the SO type registry, and it didn't feel worth it.

pgayvallet · 2022-07-29T13:03:29Z

packages/core/saved-objects/core-saved-objects-base-server-mocks/src/serializer.mock.ts

+const createSerializerMock = () => {
+ const mock: jest.Mocked<ISavedObjectsSerializer> = {
+ isRawSavedObject: jest.fn(),
+ rawToSavedObject: jest.fn(),
+ savedObjectToRaw: jest.fn(),
+ generateRawId: jest.fn(),
+ generateRawLegacyUrlAliasId: jest.fn(),
+ };
+ return mock;
+};


As we didn't have one yet, I introduced a mock for ISavedObjectsSerializer.

pgayvallet · 2022-07-29T13:08:17Z

packages/core/saved-objects/core-saved-objects-utils-server/src/index.ts

+ ALL_NAMESPACES_STRING,
+ DEFAULT_NAMESPACE_STRING,
+ FIND_DEFAULT_PAGE,
+ FIND_DEFAULT_PER_PAGE,


These constants are in practice only used internally by Core, but they are also used by the SavedObjectsUtils implementation that is publicly exposed. To avoid duplicating them, I exposed them from the public util package, as it didn't felt like it would be causing any harm for them to be publicly accessible.

Sounds reasonable to me.

pgayvallet · 2022-07-29T13:09:16Z

src/core/server/index.ts

- SavedObjectsUtils,
- mergeSavedObjectMigrationMaps,
-} from './saved_objects';
+export { SavedObjectsClient } from './saved_objects';


I need to check if we still need to export SavedObjectsClient and SavedObjectTypeRegistry from the entrypoint, but this will be done in the next PR or in the follow-up cleanup.

pgayvallet · 2022-07-29T13:20:12Z

src/core/server/index.ts

-export {
- SavedObjectsClient,
- SavedObjectsErrorHelpers,
- SavedObjectsSerializer,


Last external usage of SavedObjectsSerializer have been removed and replaced by the newly introduced mock. It's no longer publicly exposed.

…mentation-stage-1

elasticmachine · 2022-07-29T13:22:06Z

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

pmuellr

ResponseOps changes LGTM

TinaHeiligers · 2022-08-01T00:16:18Z

@elasticmachine merge upstream

TinaHeiligers · 2022-08-01T01:07:21Z

@pgayvallet I tried to help but seemed to have done the opposite 😨 . Sorry 'bout that!

Attempt to undo an incorrect merge.

TinaHeiligers · 2022-08-16T21:15:37Z

@elasticmachine merge upstream

kibanamachine · 2022-08-16T21:15:40Z

merge conflict between base and head

TinaHeiligers

LGTM thus far without aby obvious changes needed.

TinaHeiligers · 2022-08-16T23:03:00Z

packages/core/saved-objects/core-saved-objects-utils-server/package.json

+ "name": "@kbn/core-saved-objects-utils-server",
+ "private": true,
+ "version": "1.0.0",
+ "main": "./target_node/index.js",


We need to add Core as the author here and in the other packages as per recent changes. That being said, I wonde rif there's a way to share package ownership between teams, since the code in here is mostly related to the security wrappers.

@kibana-operations, is there a plan to reflect code-ownership in packages or do we rely on the CODEOWNER's file?

TinaHeiligers · 2022-08-16T23:04:21Z

packages/core/saved-objects/core-saved-objects-utils-server/src/index.ts

+ * Side Public License, v 1.
+ */
+
+export { mergeSavedObjectMigrationMaps } from './merge_migration_maps';


With recent changes to the build tooling, this and all the other src/index.ts files need to move to the package's root. See #138476 for more info.

TinaHeiligers · 2022-08-16T23:04:52Z

packages/core/saved-objects/core-saved-objects-utils-server/src/index.ts

+ ALL_NAMESPACES_STRING,
+ DEFAULT_NAMESPACE_STRING,
+ FIND_DEFAULT_PAGE,
+ FIND_DEFAULT_PER_PAGE,


Sounds reasonable to me.

TinaHeiligers · 2022-08-16T23:08:38Z

packages/core/saved-objects/core-saved-objects-utils-server/BUILD.bazel

+PKG_DIRNAME = "core-saved-objects-utils-server"
+PKG_REQUIRE_NAME = "@kbn/core-saved-objects-utils-server"
+
+SOURCE_FILES = glob(


I'm not sure if there's tooling to make changes required by #138476 after a package is created, so we may need to update the bazel configs manually here. From what I can tell, there's a pretty standard exclude list that we should be able to copy-paste.

TinaHeiligers · 2022-08-16T23:36:09Z

packages/core/saved-objects/core-saved-objects-base-server-internal/README.md

@@ -0,0 +1,4 @@
+# @kbn/core-saved-objects-base-server-internal
+
+This package contains the base parts of the server-side savedObjects internal implementation, 


I think we need to be a bit more specific as to what the package contains. "base parts" isn't very descriptive. The list as per the PR description would be fine.

TinaHeiligers · 2022-08-16T23:57:10Z

packages/core/saved-objects/core-saved-objects-base-server-mocks/README.md

+# @kbn/core-saved-objects-base-server-mocks
+
+This package contains the mocks for the base server-side savedObjects sub-domain:
+- `SavedObjectTypeRegistry` mock


Thank you for adding which mocks the package contains!

TinaHeiligers · 2022-08-17T00:05:53Z

FYI, it looks like the failing CI tests are related to recent changes in non-core code. I've tested resolving conflicts with main locally and the tests passed.

…mentation-stage-1

semd

SecuritySolution changes LGTM

kibana-ci · 2022-08-22T09:04:59Z

💚 Build Succeeded

Buildkite Build
Commit: 6700ce8

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	-	29	+29
`@kbn/core-saved-objects-base-server-mocks`	-	4	+4
`@kbn/core-saved-objects-utils-server`	-	86	+86
`core`	296	216	-80
total			+39

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id	before	after	diff
`@kbn/core-saved-objects-utils-server`	-	1	+1
`core`	2	1	-1
total			-0

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	-	1	+1
`core`	6	5	-1
total			-0

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	-	35	+35
`@kbn/core-saved-objects-base-server-mocks`	-	4	+4
`@kbn/core-saved-objects-utils-server`	-	99	+99
total			+138

ESLint disabled line counts

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	-	3	+3
`core`	32	29	-3
total			-0

Total ESLint disabled count

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	-	3	+3
`core`	37	34	-3
total			-0

History

💔 Build #66289 failed 50fa813
💔 Build #65510 failed e7a0ad1
💔 Build #61906 failed c3210cc
💚 Build #61727 succeeded db1b9fe
💚 Build #61682 succeeded a72136d

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

…ckages (elastic#137448) * create @kbn/core-saved-objects-utils-server package * create empty @kbn/core-saved-objects-base-server-internal package * fix more internal imports * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * start moving stuff to base package * Fix SS mocks * adapt some imports * start fixing internal imports * fix remaining core usages * start fixing external usages * lint * move type registry and mocks to packages * adapt usages of serializer in tests * fix test mocking * [CI] Auto-commit changed files from 'node scripts/generate packages_build_manifest' * fix more internal usages of SOTR/mock * fix package * fix external usages * fix more mocked packages * fix more mocked packages * self review * Fix mistake from main merge Attempt to undo an incorrect merge. * add root_input_dir Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Christiane (Tina) Heiligers <christiane.heiligers@elastic.co>

create @kbn/core-saved-objects-utils-server package

258e60d

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v8.5.0 labels Jul 28, 2022

pgayvallet and others added 11 commits July 28, 2022 15:14

create empty @kbn/core-saved-objects-base-server-internal package

08bfb2b

fix more internal imports

0b0aa51

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

51fa753

…-ref HEAD~1..HEAD --fix'

start moving stuff to base package

76f8c3d

Fix SS mocks

55c372c

adapt some imports

866fb52

start fixing internal imports

e18a800

fix remaining core usages

887c196

start fixing external usages

240219a

Merge remote-tracking branch 'upstream/main' into kbn-135820-so-imple…

044d5a9

…mentation-stage-1

lint

95837c8

pgayvallet force-pushed the kbn-135820-so-implementation-stage-1 branch from f4a2c19 to 95837c8 Compare July 29, 2022 06:45

pgayvallet and others added 10 commits July 29, 2022 09:15

move type registry and mocks to packages

8c6bd8b

adapt usages of serializer in tests

934f79c

fix test mocking

32c122a

[CI] Auto-commit changed files from 'node scripts/generate packages_b…

dc409a3

…uild_manifest'

fix more internal usages of SOTR/mock

45fca48

fix package

b648df6

fix external usages

21983bd

fix more mocked packages

cc66bf8

fix more mocked packages

a72136d

self review

1fe8ec8

pgayvallet commented Jul 29, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into kbn-135820-so-imple…

db1b9fe

…mentation-stage-1

pgayvallet marked this pull request as ready for review July 29, 2022 13:22

pgayvallet requested a review from a team as a code owner July 29, 2022 13:22

pgayvallet requested review from a team as code owners July 29, 2022 13:22

azasypkin approved these changes Jul 29, 2022

View reviewed changes

pmuellr approved these changes Jul 29, 2022

View reviewed changes

Merge branch 'main' into kbn-135820-so-implementation-stage-1

a94a10b

Merge branch 'main' into kbn-135820-so-implementation-stage-1

c3210cc

TinaHeiligers mentioned this pull request Aug 16, 2022

Delete saved objects in bulk #30503

Closed

Fix mistake from main merge

e7a0ad1

Attempt to undo an incorrect merge.

TinaHeiligers reviewed Aug 17, 2022

View reviewed changes

TinaHeiligers approved these changes Aug 17, 2022

View reviewed changes

pgayvallet added 2 commits August 22, 2022 09:14

Merge remote-tracking branch 'upstream/main' into kbn-135820-so-imple…

50fa813

…mentation-stage-1

add root_input_dir

6700ce8

semd approved these changes Aug 22, 2022

View reviewed changes

pgayvallet mentioned this pull request Aug 22, 2022

chore(NA): remove src folder requirement from packages (part 2) #138476

Merged

pgayvallet merged commit f7c0a0c into elastic:main Aug 22, 2022

kibanamachine added the backport:skip This commit does not require backporting label Aug 22, 2022

pgayvallet mentioned this pull request Aug 25, 2022

Migrate remaining parts of server-side SO domain to packages #139305

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partially move server-side `savedObjects` domain implementation to packages #137448

Partially move server-side `savedObjects` domain implementation to packages #137448

pgayvallet commented Jul 28, 2022 •

edited

Loading

pgayvallet left a comment

pgayvallet Jul 29, 2022

pgayvallet Jul 29, 2022

pgayvallet Jul 29, 2022

TinaHeiligers Aug 16, 2022

pgayvallet Jul 29, 2022

pgayvallet Jul 29, 2022

elasticmachine commented Jul 29, 2022

pmuellr left a comment

TinaHeiligers commented Aug 1, 2022

TinaHeiligers commented Aug 1, 2022

TinaHeiligers commented Aug 16, 2022

kibanamachine commented Aug 16, 2022

TinaHeiligers left a comment

TinaHeiligers Aug 16, 2022

TinaHeiligers Aug 16, 2022

TinaHeiligers Aug 16, 2022

TinaHeiligers Aug 16, 2022

TinaHeiligers Aug 16, 2022

TinaHeiligers Aug 16, 2022

TinaHeiligers commented Aug 17, 2022

semd left a comment

kibana-ci commented Aug 22, 2022

API count

ESLint disabled line counts

Total ESLint disabled count

		@@ -0,0 +1,4 @@
		# @kbn/core-saved-objects-base-server-internal

		This package contains the base parts of the server-side savedObjects internal implementation,

Partially move server-side savedObjects domain implementation to packages #137448

Partially move server-side savedObjects domain implementation to packages #137448

Conversation

pgayvallet commented Jul 28, 2022 • edited Loading

Summary

pgayvallet left a 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

elasticmachine commented Jul 29, 2022

pmuellr left a comment

Choose a reason for hiding this comment

TinaHeiligers commented Aug 1, 2022

TinaHeiligers commented Aug 1, 2022

TinaHeiligers commented Aug 16, 2022

kibanamachine commented Aug 16, 2022

TinaHeiligers left a 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

TinaHeiligers commented Aug 17, 2022

semd left a comment

Choose a reason for hiding this comment

kibana-ci commented Aug 22, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Any counts in public APIs

Public APIs missing exports

API count

ESLint disabled line counts

Total ESLint disabled count

History

Partially move server-side `savedObjects` domain implementation to packages #137448

Partially move server-side `savedObjects` domain implementation to packages #137448

pgayvallet commented Jul 28, 2022 •

edited

Loading