-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Delete legacy URL aliases when objects are deleted or unshared #117056
Merged
jportner
merged 9 commits into
elastic:main
from
jportner:issue-116235-disable-aliases-after-delete-or-unshare
Nov 4, 2021
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
140e7d9
Refactor
jportner 1e8fc63
Add deleteLegacyUrlAliases module
jportner 7cbedaa
Change saved object delete API to use deleteLegacyUrlAliases
jportner 4236bb9
Change saved object updateObjectsSpaces API to use deleteLegacyUrlAli…
jportner 7373db6
Merge branch 'main' into issue-116235-disable-aliases-after-delete-or…
jportner 04db6f2
PR review feedback
jportner 450dd2a
More PR review feedback
jportner ee325f7
Merge branch 'main' into issue-116235-disable-aliases-after-delete-or…
jportner ed54e64
PR review feedback, part 3
jportner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
23 changes: 23 additions & 0 deletions
23
...erver/saved_objects/service/lib/legacy_url_aliases/delete_legacy_url_aliases.test.mock.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import type { getErrorMessage } from '../../../../elasticsearch'; | ||
|
||
export const mockGetEsErrorMessage = jest.fn() as jest.MockedFunction<typeof getErrorMessage>; | ||
|
||
jest.mock('../../../../elasticsearch', () => { | ||
return { getErrorMessage: mockGetEsErrorMessage }; | ||
}); | ||
|
||
// Mock these functions to return empty results, as this simplifies test cases and we don't need to exercise alternate code paths for these | ||
jest.mock('@kbn/es-query', () => { | ||
return { nodeTypes: { function: { buildNode: jest.fn() } } }; | ||
}); | ||
jest.mock('../search_dsl', () => { | ||
return { getSearchDsl: jest.fn() }; | ||
}); |
152 changes: 152 additions & 0 deletions
152
...ore/server/saved_objects/service/lib/legacy_url_aliases/delete_legacy_url_aliases.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,152 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import { mockGetEsErrorMessage } from './delete_legacy_url_aliases.test.mock'; // Note: importing this file applies default mocks for other functions too | ||
|
||
import { errors as EsErrors } from '@elastic/elasticsearch'; | ||
|
||
import { elasticsearchClientMock } from '../../../../elasticsearch/client/mocks'; | ||
import { typeRegistryMock } from '../../../saved_objects_type_registry.mock'; | ||
import { ALL_NAMESPACES_STRING } from '../utils'; | ||
import { deleteLegacyUrlAliases } from './delete_legacy_url_aliases'; | ||
import type { DeleteLegacyUrlAliasesParams } from './delete_legacy_url_aliases'; | ||
|
||
type SetupParams = Pick< | ||
DeleteLegacyUrlAliasesParams, | ||
'type' | 'id' | 'namespaces' | 'deleteBehavior' | ||
>; | ||
|
||
describe('deleteLegacyUrlAliases', () => { | ||
function setup(setupParams: SetupParams) { | ||
return { | ||
mappings: { properties: {} }, // doesn't matter, only used as an argument to getSearchDsl which is mocked | ||
registry: typeRegistryMock.create(), // doesn't matter, only used as an argument to getSearchDsl which is mocked | ||
client: elasticsearchClientMock.createElasticsearchClient(), | ||
getIndexForType: jest.fn(), // doesn't matter | ||
...setupParams, | ||
}; | ||
} | ||
|
||
const type = 'obj-type'; | ||
const id = 'obj-id'; | ||
|
||
it('throws an error if namespaces includes the "all namespaces" string', async () => { | ||
const namespaces = [ALL_NAMESPACES_STRING]; | ||
const params = setup({ type, id, namespaces, deleteBehavior: 'inclusive' }); | ||
|
||
await expect(() => deleteLegacyUrlAliases(params)).rejects.toThrowError( | ||
`Failed to delete legacy URL aliases for ${type}/${id}: "namespaces" cannot include the * string` | ||
); | ||
expect(params.client.updateByQuery).not.toHaveBeenCalled(); | ||
}); | ||
|
||
it('throws an error if updateByQuery fails', async () => { | ||
const namespaces = ['space-a', 'space-b']; | ||
const params = setup({ type, id, namespaces, deleteBehavior: 'inclusive' }); | ||
const esError = new EsErrors.ResponseError( | ||
elasticsearchClientMock.createApiResponse({ | ||
statusCode: 500, | ||
body: { error: { type: 'es_type', reason: 'es_reason' } }, | ||
}) | ||
); | ||
params.client.updateByQuery.mockResolvedValueOnce( | ||
elasticsearchClientMock.createErrorTransportRequestPromise(esError) | ||
); | ||
mockGetEsErrorMessage.mockClear(); | ||
mockGetEsErrorMessage.mockReturnValue('Oh no!'); | ||
|
||
await expect(() => deleteLegacyUrlAliases(params)).rejects.toThrowError( | ||
`Failed to delete legacy URL aliases for ${type}/${id}: Oh no!` | ||
); | ||
expect(params.client.updateByQuery).toHaveBeenCalledTimes(1); | ||
expect(mockGetEsErrorMessage).toHaveBeenCalledTimes(1); | ||
expect(mockGetEsErrorMessage).toHaveBeenCalledWith(esError); | ||
}); | ||
|
||
describe('deleteBehavior "inclusive"', () => { | ||
const deleteBehavior = 'inclusive' as const; | ||
|
||
it('when filtered namespaces is not empty, returns early', async () => { | ||
const namespaces = ['default']; | ||
const params = setup({ type, id, namespaces, deleteBehavior }); | ||
|
||
await deleteLegacyUrlAliases(params); | ||
expect(params.client.updateByQuery).not.toHaveBeenCalled(); | ||
}); | ||
|
||
it('when filtered namespaces is not empty, calls updateByQuery with expected script params', async () => { | ||
const namespaces = ['space-a', 'default', 'space-b']; | ||
const params = setup({ type, id, namespaces, deleteBehavior }); | ||
|
||
await deleteLegacyUrlAliases(params); | ||
expect(params.client.updateByQuery).toHaveBeenCalledTimes(1); | ||
expect(params.client.updateByQuery).toHaveBeenCalledWith( | ||
expect.objectContaining({ | ||
body: expect.objectContaining({ | ||
script: expect.objectContaining({ | ||
params: { | ||
namespaces: ['space-a', 'space-b'], // 'default' is filtered out | ||
matchTargetNamespaceOp: 'delete', | ||
notMatchTargetNamespaceOp: 'noop', | ||
}, | ||
}), | ||
}), | ||
}), | ||
expect.anything() | ||
); | ||
}); | ||
}); | ||
|
||
describe('deleteBehavior "exclusive"', () => { | ||
const deleteBehavior = 'exclusive' as const; | ||
|
||
it('when filtered namespaces is empty, calls updateByQuery with expected script params', async () => { | ||
const namespaces = ['default']; | ||
const params = setup({ type, id, namespaces, deleteBehavior }); | ||
|
||
await deleteLegacyUrlAliases(params); | ||
expect(params.client.updateByQuery).toHaveBeenCalledTimes(1); | ||
expect(params.client.updateByQuery).toHaveBeenCalledWith( | ||
expect.objectContaining({ | ||
body: expect.objectContaining({ | ||
script: expect.objectContaining({ | ||
params: { | ||
namespaces: [], // 'default' is filtered out | ||
matchTargetNamespaceOp: 'noop', | ||
notMatchTargetNamespaceOp: 'delete', | ||
}, | ||
}), | ||
}), | ||
}), | ||
expect.anything() | ||
); | ||
}); | ||
|
||
it('when filtered namespaces is not empty, calls updateByQuery with expected script params', async () => { | ||
const namespaces = ['space-a', 'default', 'space-b']; | ||
const params = setup({ type, id, namespaces, deleteBehavior }); | ||
|
||
await deleteLegacyUrlAliases(params); | ||
expect(params.client.updateByQuery).toHaveBeenCalledTimes(1); | ||
expect(params.client.updateByQuery).toHaveBeenCalledWith( | ||
expect.objectContaining({ | ||
body: expect.objectContaining({ | ||
script: expect.objectContaining({ | ||
params: { | ||
namespaces: ['space-a', 'space-b'], // 'default' is filtered out | ||
matchTargetNamespaceOp: 'noop', | ||
notMatchTargetNamespaceOp: 'delete', | ||
}, | ||
}), | ||
}), | ||
}), | ||
expect.anything() | ||
); | ||
}); | ||
}); | ||
}); |
113 changes: 113 additions & 0 deletions
113
src/core/server/saved_objects/service/lib/legacy_url_aliases/delete_legacy_url_aliases.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import * as esKuery from '@kbn/es-query'; | ||
|
||
import { getErrorMessage as getEsErrorMessage } from '../../../../elasticsearch'; | ||
import type { ISavedObjectTypeRegistry } from '../../../saved_objects_type_registry'; | ||
import type { IndexMapping } from '../../../mappings'; | ||
import { LEGACY_URL_ALIAS_TYPE } from '../../../object_types'; | ||
import type { RepositoryEsClient } from '../repository_es_client'; | ||
import { getSearchDsl } from '../search_dsl'; | ||
import { ALL_NAMESPACES_STRING, DEFAULT_NAMESPACE_STRING } from '../utils'; | ||
|
||
/** @internal */ | ||
export interface DeleteLegacyUrlAliasesParams { | ||
mappings: IndexMapping; | ||
registry: ISavedObjectTypeRegistry; | ||
client: RepositoryEsClient; | ||
getIndexForType: (type: string) => string; | ||
/** The object type. */ | ||
type: string; | ||
/** The object ID. */ | ||
id: string; | ||
/** | ||
* The namespaces to include or exclude when searching for legacy URL alias targets (depends on the `deleteBehavior` parameter). | ||
* Note that using `namespaces: [], deleteBehavior: 'exclusive'` will delete all aliases for this object in all spaces. | ||
*/ | ||
namespaces: string[]; | ||
/** | ||
* If this is equal to 'inclusive', all aliases with a `targetNamespace` in the `namespaces` array will be deleted. | ||
* If this is equal to 'exclusive', all aliases with a `targetNamespace` _not_ in the `namespaces` array will be deleted. | ||
*/ | ||
deleteBehavior: 'inclusive' | 'exclusive'; | ||
} | ||
|
||
/** | ||
* Deletes legacy URL aliases that point to a given object. | ||
* | ||
* Note that aliases are only created when an object is converted to become share-capable, and each targetId is deterministically generated | ||
* with uuidv5 -- this means that the chances of there actually being _multiple_ legacy URL aliases that target a given type/ID are slim to | ||
* none. However, we don't always know exactly what space an alias could be in (if an object exists in multiple spaces, or in all spaces), | ||
* so the most straightforward way for us to ensure that aliases are reliably deleted is to use updateByQuery, which is what this function | ||
* does. | ||
* | ||
* @internal | ||
*/ | ||
export async function deleteLegacyUrlAliases(params: DeleteLegacyUrlAliasesParams) { | ||
const { mappings, registry, client, getIndexForType, type, id, namespaces, deleteBehavior } = | ||
params; | ||
|
||
if (namespaces.includes(ALL_NAMESPACES_STRING)) { | ||
throwError(type, id, '"namespaces" cannot include the * string'); | ||
} | ||
|
||
// Legacy URL aliases cannot exist in the default space; filter that out | ||
const filteredNamespaces = namespaces.filter( | ||
(namespace) => namespace !== DEFAULT_NAMESPACE_STRING | ||
); | ||
if (!filteredNamespaces.length && deleteBehavior === 'inclusive') { | ||
// nothing to do, return early | ||
return; | ||
} | ||
|
||
const { buildNode } = esKuery.nodeTypes.function; | ||
const match1 = buildNode('is', `${LEGACY_URL_ALIAS_TYPE}.targetType`, type); | ||
const match2 = buildNode('is', `${LEGACY_URL_ALIAS_TYPE}.targetId`, id); | ||
const kueryNode = buildNode('and', [match1, match2]); | ||
|
||
try { | ||
await client.updateByQuery( | ||
{ | ||
index: getIndexForType(LEGACY_URL_ALIAS_TYPE), | ||
refresh: false, // This could be called many times in succession, intentionally do not wait for a refresh | ||
body: { | ||
...getSearchDsl(mappings, registry, { | ||
type: LEGACY_URL_ALIAS_TYPE, | ||
kueryNode, | ||
}), | ||
script: { | ||
// Intentionally use one script source with variable params to take advantage of ES script caching | ||
source: ` | ||
if (params['namespaces'].indexOf(ctx._source['${LEGACY_URL_ALIAS_TYPE}']['targetNamespace']) > -1) { | ||
ctx.op = params['matchTargetNamespaceOp']; | ||
} else { | ||
ctx.op = params['notMatchTargetNamespaceOp']; | ||
} | ||
`, | ||
params: { | ||
namespaces: filteredNamespaces, | ||
matchTargetNamespaceOp: deleteBehavior === 'inclusive' ? 'delete' : 'noop', | ||
notMatchTargetNamespaceOp: deleteBehavior === 'inclusive' ? 'noop' : 'delete', | ||
}, | ||
lang: 'painless', | ||
}, | ||
conflicts: 'proceed', | ||
}, | ||
}, | ||
{ ignore: [404] } | ||
); | ||
} catch (err) { | ||
const errorMessage = getEsErrorMessage(err); | ||
throwError(type, id, `${errorMessage}`); | ||
} | ||
} | ||
|
||
function throwError(type: string, id: string, message: string) { | ||
throw new Error(`Failed to delete legacy URL aliases for ${type}/${id}: ${message}`); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
12 changes: 12 additions & 0 deletions
12
src/core/server/saved_objects/service/lib/legacy_url_aliases/index.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
export { findLegacyUrlAliases } from './find_legacy_url_aliases'; | ||
|
||
export { deleteLegacyUrlAliases } from './delete_legacy_url_aliases'; | ||
export type { DeleteLegacyUrlAliasesParams } from './delete_legacy_url_aliases'; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't even aware of that tbh...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, aliases are only created when objects in non-default spaces are converted, and I also designed
resolve
with that assumption in mind. Technically we could have chosen to support aliases in the default space (with adefault:
prefix in the raw ID) but there wasn't any practical reason to do so.