Support generating legacy URL aliases for objects that change IDs during import. #149021

azasypkin · 2023-01-17T11:21:01Z

Summary

This PR introduces a new Saved Objects Import/Copy API compatibilityMode boolean parameter that instructs import API to generate legacy URL aliases for all Saved Objects that have to change ID during import/copy.

The aforementioned parameter isn't set by default to avoid any breaking changes.

When importing/copying in the compatibility mode, legacy URL aliases are generated with the purpose field set to savedObjectImport to suppress "We redirected you to a new URL" toast notification. This notification doesn't make sense in this case since the API consumer explicitly opted-in to the compatibility mode.

How to test

HINT: To test compatibilityMode you can either use APIs for import/copy, or you can 1) import/copy in the UI with the enabled Network Dev Tools panel, 2) delete imported/copied content, 3) edit import/copy request in the Dev Tools to include compatibilityMode parameter (or just Copy as cURL if your browser doesn't support request editing) and 4) re-send request.

Import

Create a data view (Data View A) and two dashboards in the default space (Dashboard A and Dashboard B). One of the dashboards (Dashboard B) should include a Markdown visualization with a link (only the part of the URL that starts with #) to another dashboard (Dashboard A), so called weak link.
Export created data view, visualizations and dashboards.
Create a new space (Space A) and import to it the data exported from the default space (via API with compatibilityMode set to true).
Once the import is done, make sure that the weak link works as expected: the link in the Dashboard B in the Space A leads to the Dashboard A in the Space A
Repeat steps 1-4 in the reverse order: create a data view (Data View B) and two dashboards in the Space A (Dashboard C and Dashboard D) etc.

Copy

Create a data view (Data View A) and two dashboards in the default space (Dashboard A and Dashboard B). One of the dashboards (Dashboard B) should include a Markdown visualization with a link (only the part of the URL that starts with #) to another dashboard (Dashboard A), so called weak link.
Create a new space (Space A) and copy created data view, dashboards and visualizations from the default space to this new space (via API with compatibilityMode set to true).
Once the copy is done, make sure that the weak link works as expected: the link in the Dashboard B in the Space A leads to the Dashboard A in the Space A
Repeat steps 1-3 in the reverse order: create a data view (Data View B) and two dashboards in the Space A (Dashboard C and Dashboard D) etc.

Fixes: #123550

…ing import.

azasypkin · 2023-02-13T18:51:55Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts

+  /**
+   * {@inheritDoc ISavedObjectsRepository.getCurrentNamespace}
+   */
+  getCurrentNamespace(namespace?: string) {


note: I had to expose this method from the repository and the client to get the current namespace in the import code. The current namespace is required to properly generate legacy URL alias ID.

The fact that getCurrentNamespace accepts an optional namespace parameter always bothered me (to a point where I always forget why it is so).

Do we really need to mirror the API definition of SpaceExtension.getCurrentNamespace? Or would it be acceptable to just have a SOR.getCurrentNamespace(-no-parameters-)?

Do we really need to mirror the API definition of SpaceExtension.getCurrentNamespace? Or would it be acceptable to just have a SOR.getCurrentNamespace(-no-parameters-)?

That's what I would like to explore further, I agree this namespace argument is super confusing. If I cannot find any use case for that parameter for this newly exposed API, I'll happily get rid of it.

Fully agreed on the universally despised namespace parameter (kind of wish I had changed this during the refactor). Would there ever be a scenario where spaces as a feature is enabled but the spaces SO extension is not?
As the client doesn't include the parameter, and it is the only external consumer of the new repo method. I say we just get rid of the parameter. I'd be happy to replace the internal repo calls and extension, but I don't want to hold up this PR either.

Would there ever be a scenario where spaces as a feature is enabled but the spaces SO extension is not?

I don't think it can be the case (otherwise having or not having the weird namespace parameter in getCurrentNamespace method would be the least of our problems 🙂 )

As the client doesn't include the parameter, and it is the only external consumer of the new repo method.

I removed it from the client's public method signature, but kept in the repository's signature since it's still used internally.

azasypkin · 2023-02-13T18:56:24Z

...-objects/core-saved-objects-import-export-server-internal/src/import/import_saved_objects.ts

+  /**
+   * If true, Kibana will apply various adjustments to the data that's being imported to maintain compatibility between
+   * different Kibana versions (e.g. generate legacy URL aliases for all imported objects that have to change IDs).
+   */
+  compatibilityMode: boolean;


note: I went back and forth trying to find the appropriate name for this parameter without making a notion of the legacy URL alias a part of the public contract, and ended up with something that's close to what we really do (maintain not-officially-supported "weak-links" behavior) and vague enough to not expose implementation detail. I'd be happy to accept any other better name suggestion though.

azasypkin · 2023-02-13T18:59:29Z

...ects/core-saved-objects-import-export-server-internal/src/import/lib/create_saved_objects.ts

+      // origin ID is maintained.
+      const requiresLegacyUrlAlias =
+        compatibilityMode &&
+        originId !== importStateValue.destinationId &&


note: The importStateValue.destinationId === originId is true when, for example, we copy object from default space to a custom space, and then copy that object from a custom space back to a default space

azasypkin · 2023-02-13T19:01:19Z

...ed-objects/core-saved-objects-import-export-server-internal/src/import/lib/extract_errors.ts

+    const originalSavedObject = isLegacyUrlAlias(savedObject)
+      ? originalSavedObjectsMap.get(
+          `${savedObject.attributes.targetType}:${savedObject.attributes.sourceId}`
+        ) ??
+        originalSavedObjectsMap.get(
+          `${savedObject.attributes.targetType}:${savedObject.attributes.targetId}`
+        )
+      : originalSavedObjectsMap.get(`${savedObject.type}:${savedObject.id}`);


note: I'm not yet sure if we should expose legacy URL alias related errors in the API response, maybe we just need to log them.... I cannot find a non-edge use case when we'd get errors for the legacy URL aliases assuming all "preflight" checks we do in the import code. What do you think?

If the errors related to legacy url aliases are only edge-case errors (e.g connectivity issue with ES, concurrency conflicts and so on) and non-actionable, it's probably fine to just surface them in the API so that there's a trace somewhere.

azasypkin · 2023-02-13T19:02:16Z

packages/core/saved-objects/core-saved-objects-server-internal/src/routes/import.ts

+              if (object.createNewCopies && object.compatibilityMode) {
+                return 'cannot use [createNewCopies] with [compatibilityMode]';
+              }


note: If the API consumer explicitly requires new IDs then compatibility mode doesn't make sense.

azasypkin · 2023-02-13T19:05:01Z

Hey @pgayvallet,

Here's the draft version for the broken-weak-links fix that we've discussed recently. Would you mind sharing your preliminary feedback on the approach and assumptions I made before I proceed with tests, docs etc.?

Thanks!

pgayvallet · 2023-02-14T09:35:31Z

Ack: will TAL today

pgayvallet

Some comments and remarks, but the approach looks good to me.

pgayvallet · 2023-02-14T12:16:37Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts

+  /**
+   * {@inheritDoc ISavedObjectsRepository.getCurrentNamespace}
+   */
+  getCurrentNamespace(namespace?: string) {


The fact that getCurrentNamespace accepts an optional namespace parameter always bothered me (to a point where I always forget why it is so).

Do we really need to mirror the API definition of SpaceExtension.getCurrentNamespace? Or would it be acceptable to just have a SOR.getCurrentNamespace(-no-parameters-)?

pgayvallet · 2023-02-14T12:24:30Z

...ects/core-saved-objects-import-export-server-internal/src/import/lib/create_saved_objects.ts

+  // is specified explicitly we should use it instead of the namespace the saved objects client is scoped to. In certain
+  // scenarios (e.g. copying to a default space) both current namespace and namespace from the parameter aren't defined.
+  const legacyUrlTargetNamespace = SavedObjectsUtils.namespaceIdToString(
+    namespace ?? savedObjectsClient.getCurrentNamespace(namespace)


NIT: namespace ?? savedObjectsClient.getCurrentNamespace()

pgayvallet · 2023-02-14T12:33:17Z

...ects/core-saved-objects-import-export-server-internal/src/import/lib/create_saved_objects.ts

  // filter out the 'version' field of each object, if it exists, and set the originId appropriately
+  const legacyAliases = new Map<string, SavedObjectsBulkCreateObject>();


NIT: comment should probably remain at the same position (before the next line)

pgayvallet · 2023-02-14T12:48:23Z

...ed-objects/core-saved-objects-import-export-server-internal/src/import/lib/extract_errors.ts

+    const originalSavedObject = isLegacyUrlAlias(savedObject)
+      ? originalSavedObjectsMap.get(
+          `${savedObject.attributes.targetType}:${savedObject.attributes.sourceId}`
+        ) ??
+        originalSavedObjectsMap.get(
+          `${savedObject.attributes.targetType}:${savedObject.attributes.targetId}`
+        )
+      : originalSavedObjectsMap.get(`${savedObject.type}:${savedObject.id}`);


If the errors related to legacy url aliases are only edge-case errors (e.g connectivity issue with ES, concurrency conflicts and so on) and non-actionable, it's probably fine to just surface them in the API so that there's a trace somewhere.

pgayvallet · 2023-02-14T12:48:47Z

...ects/core-saved-objects-import-export-server-internal/src/import/lib/create_saved_objects.ts

+      const requiresLegacyUrlAlias =
+        compatibilityMode &&
+        originId !== importStateValue.destinationId &&
+        !importStateValue.omitOriginId &&
+        !legacyAliases.has(originId);


That's a big if.

Indeed, let me see if I can split it into separate variables just for the readability sake.

pgayvallet · 2023-02-14T12:53:49Z

...ects/core-saved-objects-import-export-server-internal/src/import/lib/create_saved_objects.ts

+  const remappedResults = expectedResults.flatMap<CreatedObject<T>>((result) => {
+    // Only keep `error` results for the legacy URL aliases to report errors.
+    if (result.type === LEGACY_URL_ALIAS_TYPE) {
+      return result.error ? [result] : [];
+    }


NIT: took me some time understanding what we're doing with the legacy url aliases in that loop. I would have separated normal results and legacy url alias from expectedResults before the flatMap, and then passed the legacy url alias to extractErrors more explicitly, like with an additional parameter.

That's my attempt of the premature performance optimization, tried to avoid looping through the list twice, but based on the previous comment we might split these calls so it won't be a problem anymore.

then passed the legacy url alias to extractErrors more explicitly, like with an additional parameter.

++, sounds good.

pgayvallet · 2023-02-14T13:00:18Z

...ects/core-saved-objects-import-export-server-internal/src/import/lib/create_saved_objects.ts

+    const bulkCreateResponse = await savedObjectsClient.bulkCreate(
+      [...objectsToCreate, ...Array.from(legacyAliases.values())],
+      { namespace, overwrite, refresh }
+    );


As discussed sync: Ideally, to avoid impact on edge cases, we would create the objects first, and then the legacy url aliases. But that's fine.

My bad, for some reason I though we agreed on the opposite 🙂 Anyway, I'll separate these calls just to be on the safe side.

My understanding was that we agreed that having a single bulkCreate was totally acceptable as it was easier, compared to do 2 distinct calls 😅

Right, right, I've just "heard" what I wanted to hear 🙂

azasypkin · 2023-02-15T12:04:34Z

Some comments and remarks, but the approach looks good to me.

Thanks for the review! I'll handle your feedback and proceed with the tests, and tag you/core for review once it's ready.

…sfully imported. Remove `namespace` parameter from the `getCurrentNamespace` SO Client API. Include legacy URL aliases errors in the import API response.

pgayvallet

Looking good to me

pgayvallet · 2023-03-28T07:10:41Z

packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_client.ts

+  /**
+   * Returns namespace associated with the client, if any.
+   */


NIT: (and because I'm not even sure myself that early in the morning) we may want to explain/document what the expected return is when we are in the default namespace (default or undefined?)

Yeah, good point, I'll mention this in the comment - for default namespace (or when space extension isn't enabled) this method returns undefined

pgayvallet · 2023-03-28T07:18:59Z

...ects/core-saved-objects-import-export-server-internal/src/import/lib/create_saved_objects.ts

+    const objectRequiresLegacyUrlAlias = !!result.originId && result.originId !== result.id;
+    if (compatibilityMode && objectRequiresLegacyUrlAlias && objectSuccessfullyImported) {


That's a way better if compared to first version 😄

I did my best! 😆

pgayvallet · 2023-03-28T07:23:15Z

packages/core/usage-data/core-usage-data-server-internal/src/core_usage_stats_client.test.ts

        CORE_USAGE_STATS_TYPE,
        CORE_USAGE_STATS_ID,
        [
          `${RESOLVE_IMPORT_STATS_PREFIX}.total`,
          `${RESOLVE_IMPORT_STATS_PREFIX}.namespace.default.total`,
          `${RESOLVE_IMPORT_STATS_PREFIX}.namespace.default.kibanaRequest.yes`,
          `${RESOLVE_IMPORT_STATS_PREFIX}.createNewCopiesEnabled.yes`,
+          // 'compatibilityModeEnabled.yes` and `compatibilityModeEnabled.no` when createNewCopies is true


NIT: incorrect or partially copied comment?

jeramysoucy

Looks great! I just had some questions and a minor nit.

jeramysoucy · 2023-03-29T17:42:24Z

docs/api/spaces-management/copy_saved_objects.asciidoc

-NOTE: This cannot be used with the `createNewCopies` option.
+NOTE: This option cannot be used with the `createNewCopies` option.
+
+`compatibilityMode`::


Question (just for my own sanity) - is there a reason why some API's expect options as query parameters and others as fields within the request body?

That's a good question, and I'm not quite sure. I don't think there is any good reason why import and copy APIs accept similar options differently.

The import API uses query string parameters because the body is the raw file content, so it's understandable why it uses both body and query string parameters. As for the copy API, having spaces and objects parameters in the body makes sense since they might be quite large potentially, but I'd rather use query string for the rest of the parameters to be consistent with the import API. I guess, we put everything in the body just to make it simpler for us 🤷‍♂️

jeramysoucy · 2023-03-29T19:33:23Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts

+  /**
+   * {@inheritDoc ISavedObjectsRepository.getCurrentNamespace}
+   */
+  getCurrentNamespace(namespace?: string) {


Fully agreed on the universally despised namespace parameter (kind of wish I had changed this during the refactor). Would there ever be a scenario where spaces as a feature is enabled but the spaces SO extension is not?
As the client doesn't include the parameter, and it is the only external consumer of the new repo method. I say we just get rid of the parameter. I'd be happy to replace the internal repo calls and extension, but I don't want to hold up this PR either.

jeramysoucy · 2023-03-30T12:59:04Z

...core-saved-objects-import-export-server-internal/src/import/lib/create_saved_objects.test.ts

    });
  });

  describe('with a defined namespace', () => {
    const namespace = 'some-namespace';
    test('calls bulkCreate once with input objects', async () => {
-      await testBulkCreateObjects(namespace);
+      await testBulkCreateObjects({ namespace });
+      await testBulkCreateObjects({ namespace, compatibilityMode: true });


Just a nit: update test description 'calls bulkCreate according to input objects and compatibilityMode option', as with compatibilityMode it is called twice. Or separate the compatibilityMode tests into a subset for more specific description wording.

Good catch, will update method description!

kibana-ci · 2023-04-03T08:49:23Z

💚 Build Succeeded

Buildkite Build
Commit: 2a006ca

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-api-server`	4	5	+1
`@kbn/core-saved-objects-api-server-internal`	54	55	+1
total			+2

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-api-server`	338	341	+3
`@kbn/core-saved-objects-api-server-internal`	75	77	+2
`@kbn/core-saved-objects-server`	491	493	+2
total			+7

ESLint disabled line counts

id	before	after	diff
`securitySolution`	432	435	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	512	515	+3

History

💔 Build #117178 failed 9ebaa3d
💚 Build #116298 succeeded 255bfea
💚 Build #115587 succeeded c7ff265
💚 Build #113967 succeeded a1e9192
💚 Build #113850 succeeded 5c9b344

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

TinaHeiligers · 2023-04-21T19:29:01Z

...objects/core-saved-objects-import-export-server-internal/src/import/resolve_import_errors.ts

+   * If true, Kibana will apply various adjustments to the data that's being retried to import to maintain compatibility between
+   * different Kibana versions (e.g. generate legacy URL aliases for all imported objects that have to change IDs).
+   */
+  compatibilityMode?: boolean;


@azasypkin I'm working in code related to import saved objects and went hunting down how compatibilityMode works in resolve_import_errors. The docs for this HTTP API need to be updated to include the new query option.

Ah, good catch, thanks @TinaHeiligers! I'll update docs for this API as well.

## Summary The broken weak links problem wasn't fixed in 8.1, so this statement is now confusing. An aliasing workaround [was added](#149021) in 8.8 for those customers who need it. Better solutions - use the [new links panel](https://www.elastic.co/guide/en/kibana/current/dashboard-links.html) for navigation between dashboards - use [shareable dashboards](#167901) to make analytics stuff available in multiple spaces instead of copying (not yet available)

## Summary The broken weak links problem wasn't fixed in 8.1, so this statement is now confusing. An aliasing workaround [was added](elastic#149021) in 8.8 for those customers who need it. Better solutions - use the [new links panel](https://www.elastic.co/guide/en/kibana/current/dashboard-links.html) for navigation between dashboards - use [shareable dashboards](elastic#167901) to make analytics stuff available in multiple spaces instead of copying (not yet available)

azasypkin added skip-ci ci:no-auto-commit Disable auto-committing changes on CI labels Jan 17, 2023

azasypkin force-pushed the issue-123550-weak-links branch 3 times, most recently from 97e1f5b to 1271d15 Compare February 9, 2023 12:52

Support generating legacy URL aliases for objects that change IDs dur…

1124f42

…ing import.

azasypkin force-pushed the issue-123550-weak-links branch from 1271d15 to 1124f42 Compare February 13, 2023 18:41

azasypkin commented Feb 13, 2023

View reviewed changes

azasypkin requested a review from a team February 13, 2023 19:03

azasypkin added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels Feb 13, 2023

azasypkin requested review from pgayvallet and removed request for a team February 13, 2023 19:05

pgayvallet reviewed Feb 14, 2023

View reviewed changes

azasypkin added 4 commits February 28, 2023 13:18

Merge branch 'main' into issue-123550-weak-links

84c4132

Review#1: Create legacy URL aliases only for objects that were succes…

dadf104

…sfully imported. Remove `namespace` parameter from the `getCurrentNamespace` SO Client API. Include legacy URL aliases errors in the import API response.

Merge branch 'main' into issue-123550-weak-links

3df1447

Merge branch 'main' into issue-123550-weak-links

3780626

azasypkin removed the skip-ci label Mar 10, 2023

azasypkin added 4 commits March 10, 2023 11:20

Add tests.

7295394

Merge branch 'main' into issue-123550-weak-links

79790e5

Add more tests.

79f8be8

Merge branch 'main' into issue-123550-weak-links

f3d9b13

azasypkin requested a review from pgayvallet March 20, 2023 09:16

azasypkin added the release_note:enhancement label Mar 20, 2023

azasypkin added 2 commits March 27, 2023 09:48

Merge branch 'main' into issue-123550-weak-links

0adb905

Update import/copy API docs to mention new compatibilityMode option.

c7ff265

pgayvallet approved these changes Mar 28, 2023

View reviewed changes

azasypkin added 2 commits March 29, 2023 09:30

Merge branch 'main' into issue-123550-weak-links

c7e8451

Review#2: handle review feedback.

255bfea

azasypkin added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Mar 29, 2023

jeramysoucy self-requested a review March 29, 2023 12:11

jeramysoucy approved these changes Mar 30, 2023

View reviewed changes

azasypkin added 4 commits March 31, 2023 13:42

Merge branch 'main' into issue-123550-weak-links

9e3552e

Merge branch 'main' into issue-123550-weak-links

ade2e1d

Review#3: handle review feedback.

9ebaa3d

Merge branch 'main' into issue-123550-weak-links

2a006ca

azasypkin merged commit a1fccfd into elastic:main Apr 3, 2023

azasypkin deleted the issue-123550-weak-links branch April 3, 2023 08:54

kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Apr 3, 2023

TinaHeiligers reviewed Apr 21, 2023

View reviewed changes

azasypkin mentioned this pull request Apr 25, 2023

Document compatibilityMode query string parameter in Resolve import errors API. #155696

Merged

drewdaemon mentioned this pull request Jan 16, 2024

[Docs] Remove broken weak links fix plan #174932

Merged

drewdaemon mentioned this pull request Jan 26, 2024

Metrics system overview dashboard redirects to a 404 Dashboard not found page in Kibana #175072

Closed

jeramysoucy mentioned this pull request Feb 2, 2024

Update legacy URL alias developer guide #176164

Closed

legrego mentioned this pull request Feb 9, 2024

[Saved objects] Create URL aliases during "Copy on save" #176552

Open

		// filter out the 'version' field of each object, if it exists, and set the originId appropriately
		const legacyAliases = new Map<string, SavedObjectsBulkCreateObject>();

		const objectRequiresLegacyUrlAlias = !!result.originId && result.originId !== result.id;
		if (compatibilityMode && objectRequiresLegacyUrlAlias && objectSuccessfullyImported) {

Support generating legacy URL aliases for objects that change IDs during import. #149021

Support generating legacy URL aliases for objects that change IDs during import. #149021

Conversation

azasypkin commented Jan 17, 2023 • edited Loading

Summary

How to test

Import

Copy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Feb 15, 2023 • 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

Choose a reason for hiding this comment

azasypkin commented Feb 13, 2023

pgayvallet commented Feb 14, 2023

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

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

azasypkin commented Feb 15, 2023

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

jeramysoucy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Mar 31, 2023 • 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

kibana-ci commented Apr 3, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

ESLint disabled line counts

Total ESLint disabled count

History

TinaHeiligers Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented Jan 17, 2023 •

edited

Loading

azasypkin Feb 15, 2023 •

edited

Loading

azasypkin Mar 31, 2023 •

edited

Loading

TinaHeiligers Apr 21, 2023 •

edited

Loading