Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 24 commits into from
Apr 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
1124f42
Support generating legacy URL aliases for objects that change IDs dur…
azasypkin Jan 17, 2023
84c4132
Merge branch 'main' into issue-123550-weak-links
azasypkin Feb 28, 2023
dadf104
Review#1: Create legacy URL aliases only for objects that were succes…
azasypkin Feb 28, 2023
3df1447
Merge branch 'main' into issue-123550-weak-links
azasypkin Mar 7, 2023
3780626
Merge branch 'main' into issue-123550-weak-links
azasypkin Mar 9, 2023
7295394
Add tests.
azasypkin Mar 9, 2023
79790e5
Merge branch 'main' into issue-123550-weak-links
azasypkin Mar 10, 2023
79f8be8
Add more tests.
azasypkin Mar 13, 2023
f3d9b13
Merge branch 'main' into issue-123550-weak-links
azasypkin Mar 13, 2023
76597ba
Adjust references.
azasypkin Mar 14, 2023
261502f
Merge branch 'main' into issue-123550-weak-links
azasypkin Mar 14, 2023
8a8c8e5
Merge branch 'main' into issue-123550-weak-links
azasypkin Mar 16, 2023
4af273f
Add support for `compatibilityMode` in import/copy resolve-errors APIs.
azasypkin Mar 16, 2023
95cdcbe
Merge branch 'main' into issue-123550-weak-links
azasypkin Mar 16, 2023
5c9b344
Update telemetry tests.
azasypkin Mar 16, 2023
a1e9192
Merge branch 'main' into issue-123550-weak-links
azasypkin Mar 20, 2023
0adb905
Merge branch 'main' into issue-123550-weak-links
azasypkin Mar 27, 2023
c7ff265
Update import/copy API docs to mention new `compatibilityMode` option.
azasypkin Mar 27, 2023
c7e8451
Merge branch 'main' into issue-123550-weak-links
azasypkin Mar 29, 2023
255bfea
Review#2: handle review feedback.
azasypkin Mar 29, 2023
9e3552e
Merge branch 'main' into issue-123550-weak-links
azasypkin Mar 31, 2023
ade2e1d
Merge branch 'main' into issue-123550-weak-links
azasypkin Mar 31, 2023
9ebaa3d
Review#3: handle review feedback.
azasypkin Mar 31, 2023
2a006ca
Merge branch 'main' into issue-123550-weak-links
azasypkin Apr 3, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions docs/api/saved-objects/import.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,19 @@ Saved objects can only be imported into the same version, a newer minor on the s
(Optional, boolean) Creates copies of saved objects, regenerates each object ID, and resets the origin. When used, potential conflict
errors are avoided.
+
NOTE: This cannot be used with the `overwrite` option.
NOTE: This option cannot be used with the `overwrite` and `compatibilityMode` options.

`overwrite`::
(Optional, boolean) Overwrites saved objects when they already exist. When used, potential conflict errors are automatically resolved by
overwriting the destination object.
+
NOTE: This cannot be used with the `createNewCopies` option.
NOTE: This option cannot be used with the `createNewCopies` option.

`compatibilityMode`::
(Optional, boolean) Applies various adjustments to the saved objects that are being imported to maintain compatibility between different {kib}
versions. Use this option only if you encounter issues with imported saved objects.
+
NOTE: This option cannot be used with the `createNewCopies` option.

[[saved-objects-api-import-request-body]]
==== Request body
Expand Down
10 changes: 8 additions & 2 deletions docs/api/spaces-management/copy_saved_objects.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,19 @@ You can request to overwrite any objects that already exist in the target space
(Optional, boolean) Creates new copies of saved objects, regenerates each object ID, and resets the origin. When used, potential conflict
errors are avoided. The default value is `true`.
+
NOTE: This cannot be used with the `overwrite` option.
NOTE: This option cannot be used with the `overwrite` and `compatibilityMode` options.

`overwrite`::
(Optional, boolean) When set to `true`, all conflicts are automatically overridden. When a saved object with a matching `type` and `id`
exists in the target space, that version is replaced with the version from the source space. The default value is `false`.
+
NOTE: This cannot be used with the `createNewCopies` option.
NOTE: This option cannot be used with the `createNewCopies` option.

`compatibilityMode`::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@azasypkin azasypkin Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷‍♂️

(Optional, boolean) Applies various adjustments to the saved objects that are being copied to maintain compatibility between different {kib}
versions. Use this option only if you encounter issues with copied saved objects.
+
NOTE: This option cannot be used with the `createNewCopies` option.

[role="child_attributes"]
[[spaces-api-copy-saved-objects-response-body]]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -878,6 +878,14 @@ describe('SavedObjectsRepository Spaces Extension', () => {
);
});
});

test('#getCurrentNamespace', () => {
mockSpacesExt.getCurrentNamespace.mockReturnValue('ns-from-ext');

expect(repository.getCurrentNamespace('ns-from-arg')).toBe('ns-from-ext');
expect(mockSpacesExt.getCurrentNamespace).toBeCalledTimes(1);
expect(mockSpacesExt.getCurrentNamespace).toHaveBeenCalledWith('ns-from-arg');
});
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5228,4 +5228,24 @@ describe('SavedObjectsRepository', () => {
await expect(repository.updateObjectsSpaces([], [], [])).rejects.toEqual(expectedResult);
});
});

describe('#getCurrentNamespace', () => {
it('returns `undefined` for `undefined` namespace argument', async () => {
expect(repository.getCurrentNamespace()).toBeUndefined();
});

it('throws if `*` namespace argument is provided', async () => {
expect(() => repository.getCurrentNamespace('*')).toThrowErrorMatchingInlineSnapshot(
`"\\"options.namespace\\" cannot be \\"*\\": Bad Request"`
);
});

it('properly handles `default` namespace', async () => {
expect(repository.getCurrentNamespace('default')).toBeUndefined();
});

it('properly handles non-`default` namespace', async () => {
expect(repository.getCurrentNamespace('space-a')).toBe('space-a');
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -2553,6 +2553,16 @@ export class SavedObjectsRepository implements ISavedObjectsRepository {
});
}

/**
* {@inheritDoc ISavedObjectsRepository.getCurrentNamespace}
*/
getCurrentNamespace(namespace?: string) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-)?

Copy link
Member Author

@azasypkin azasypkin Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (this._spacesExtension) {
return this._spacesExtension.getCurrentNamespace(namespace);
}
return normalizeNamespace(namespace);
}

/**
* Returns index specified by the given type or the default index
*
Expand Down Expand Up @@ -2664,20 +2674,6 @@ export class SavedObjectsRepository implements ISavedObjectsRepository {
// any other error from this check does not matter
}

/**
* If the spaces extension is enabled, we should use that to get the current namespace (and optionally throw an error if a consumer
* attempted to specify the namespace option).
*
* If the spaces extension is *not* enabled, we should simply normalize the namespace option so that `'default'` can be used
* interchangeably with `undefined`.
*/
private getCurrentNamespace(namespace?: string) {
if (this._spacesExtension) {
return this._spacesExtension.getCurrentNamespace(namespace);
}
return normalizeNamespace(namespace);
}

/** The `initialNamespaces` field (create, bulkCreate) is used to create an object in an initial set of spaces. */
private validateInitialNamespaces(type: string, initialNamespaces: string[] | undefined) {
if (!initialNamespaces) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const createRepositoryMock = () => {
removeReferencesTo: jest.fn(),
collectMultiNamespaceReferences: jest.fn(),
updateObjectsSpaces: jest.fn(),
getCurrentNamespace: jest.fn(),
};

return mock;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -326,4 +326,12 @@ describe('SavedObjectsClient', () => {
);
expect(result).toBe(returnValue);
});

test(`#getCurrentNamespace`, () => {
mockRepository.getCurrentNamespace.mockReturnValue('ns');
const client = new SavedObjectsClient(mockRepository);

expect(client.getCurrentNamespace()).toEqual('ns');
expect(mockRepository.getCurrentNamespace).toHaveBeenCalledWith();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -208,4 +208,9 @@ export class SavedObjectsClient implements SavedObjectsClientContract {
options
);
}

/** {@inheritDoc SavedObjectsClientContract.getCurrentNamespace} */
getCurrentNamespace() {
return this._repository.getCurrentNamespace();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const create = () => {
removeReferencesTo: jest.fn(),
collectMultiNamespaceReferences: jest.fn(),
updateObjectsSpaces: jest.fn(),
getCurrentNamespace: jest.fn(),
};

mock.createPointInTimeFinder = savedObjectsPointInTimeFinderMock.create({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const create = () => {
removeReferencesTo: jest.fn(),
collectMultiNamespaceReferences: jest.fn(),
updateObjectsSpaces: jest.fn(),
getCurrentNamespace: jest.fn(),
} as unknown as jest.Mocked<SavedObjectsClientContract>;

mock.createPointInTimeFinder = savedObjectsPointInTimeFinderMock.create({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ import type {
} from './apis';

/**
* Saved Objects is Kibana's data persisentence mechanism allowing plugins to
* Saved Objects is Kibana's data persistence mechanism allowing plugins to
* use Elasticsearch for storing plugin state.
*
* ## SavedObjectsClient errors
Expand Down Expand Up @@ -288,7 +288,7 @@ export interface SavedObjectsClientContract {
*
* @param type - the type of the object to remove references to
* @param id - the ID of the object to remove references to
* @param options {@link SavedObjectsRemoveReferencesToOptions} - options for the remove references opertion
* @param options {@link SavedObjectsRemoveReferencesToOptions} - options for the remove references operation
* @returns the {@link SavedObjectsRemoveReferencesToResponse}
*/
removeReferencesTo(
Expand Down Expand Up @@ -377,7 +377,7 @@ export interface SavedObjectsClientContract {
* ```
*
* @param findOptions {@link SavedObjectsCreatePointInTimeFinderOptions} - options for the create PIT finder operation
* @param dependencies {@link SavedObjectsCreatePointInTimeFinderDependencies} - dependencies for the create PIT fimder operation
* @param dependencies {@link SavedObjectsCreatePointInTimeFinderDependencies} - dependencies for the create PIT finder operation
* @returns the created PIT finder
*/
createPointInTimeFinder<T = unknown, A = unknown>(
Expand Down Expand Up @@ -412,4 +412,9 @@ export interface SavedObjectsClientContract {
spacesToRemove: string[],
options?: SavedObjectsUpdateObjectsSpacesOptions
): Promise<SavedObjectsUpdateObjectsSpacesResponse>;

/**
* Returns the namespace associated with the client. If the namespace is the default one, this method returns `undefined`.
*/
getCurrentNamespace(): string | undefined;
}
Original file line number Diff line number Diff line change
Expand Up @@ -533,4 +533,14 @@ export interface ISavedObjectsRepository {
findOptions: SavedObjectsCreatePointInTimeFinderOptions,
dependencies?: SavedObjectsCreatePointInTimeFinderDependencies
): ISavedObjectsPointInTimeFinder<T, A>;

/**
* If the spaces extension is enabled, it's used to get the current namespace (and optionally throws an error if a
* consumer attempted to specify the namespace explicitly).
*
* If the spaces extension is *not* enabled, this function simply normalizes the specified namespace so that
* `'default'` can be used interchangeably with `undefined` i.e. the method always returns `undefined` for the default
* namespace.
*/
getCurrentNamespace(namespace?: string): string | undefined;
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ export interface ImportSavedObjectsOptions {
namespace?: string;
/** If true, will create new copies of import objects, each with a random `id` and undefined `originId`. */
createNewCopies: boolean;
/**
* 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;
}

/**
Expand All @@ -67,6 +72,7 @@ export async function importSavedObjectsFromStream({
importHooks,
namespace,
refresh,
compatibilityMode,
}: ImportSavedObjectsOptions): Promise<SavedObjectsImportResponse> {
let errorAccumulator: SavedObjectsImportFailure[] = [];
const supportedTypes = typeRegistry.getImportableAndExportableTypes().map((type) => type.name);
Expand Down Expand Up @@ -147,6 +153,7 @@ export async function importSavedObjectsFromStream({
overwrite,
namespace,
refresh,
compatibilityMode,
};
const createSavedObjectsResult = await createSavedObjects(createSavedObjectsParams);
errorAccumulator = [...errorAccumulator, ...createSavedObjectsResult.errors];
Expand Down
Loading