Skip to content

Commit

Permalink
Don't migrate partial documents (elastic#198703)
Browse files Browse the repository at this point in the history
## Summary

Fixes a regression by adding back the functionality that was originally
introduced in elastic#162404: skipping
migrations (transform and forwardCompatibilitySchema) when the fields
option is specified

In working on this we uncovered some problem scenarios we previously
hadn't thought through (will create separate issues for these):
1. Even if we can't reliably run the transform part of migrations on a
partial document we should still run the `forwardCompatibilitySchema` so
that unknown fields from future versions are dropped.
However, we realised most teams missed the detail in our
[docs](https://github.com/elastic/kibana/blob/main/docs/developer/architecture/core/saved-objects-service.asciidoc?plain=1#L429-L457)
that call out that forwardCompatibilitySchemas shouldn't assert on data
but only drop unknown fields. This means if we would run the
forwardCompatibilitySchema today it would most often fail on partial
documents.
3. When `fields` contain an encrypted field but not all AAD fields,
encryption would fail and log an error (potentially triggering
serverless alerts).
  • Loading branch information
rudolf authored Nov 6, 2024
1 parent bf51662 commit 36f6d6f
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,33 @@ describe('find', () => {
),
});
});

it('does not perform migrations when a partial document is requested by specifying no fields should be retrieved', async () => {
const noNamespaceSearchResults = generateIndexPatternSearchResults();
client.search.mockResolvedValueOnce(
elasticsearchClientMock.createSuccessTransportRequestPromise(noNamespaceSearchResults)
);
migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
await expect(
repository.find({
type,
fields: [], // don't return any fields
})
).resolves.not.toHaveProperty('saved_objects.0.migrated');
expect(migrator.migrateDocument).not.toHaveBeenCalled();
});

it('does not perform migrations when a partial document is requested by specifying some fields', async () => {
const noNamespaceSearchResults = generateIndexPatternSearchResults();
client.search.mockResolvedValueOnce(
elasticsearchClientMock.createSuccessTransportRequestPromise(noNamespaceSearchResults)
);
migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
await expect(repository.find({ type, fields: ['title'] })).resolves.not.toHaveProperty(
'saved_objects.0.migrated'
);
expect(migrator.migrateDocument).not.toHaveBeenCalled();
});
});

describe('search dsl', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
CheckAuthorizationResult,
SavedObjectsRawDocSource,
GetFindRedactTypeMapParams,
SavedObjectUnsanitizedDoc,
} from '@kbn/core-saved-objects-server';
import {
DEFAULT_NAMESPACE_STRING,
Expand Down Expand Up @@ -57,6 +58,7 @@ export const performFind = async <T = unknown, A = unknown>(
common: commonHelper,
serializer: serializerHelper,
migration: migrationHelper,
encryption: encryptionHelper,
} = helpers;
const { securityExtension, spacesExtension } = extensions;
let namespaces!: string[];
Expand Down Expand Up @@ -266,13 +268,27 @@ export const performFind = async <T = unknown, A = unknown>(
const migratedDocuments: Array<SavedObjectsFindResult<T>> = [];
try {
for (const savedObject of savedObjects) {
let migrated: SavedObjectUnsanitizedDoc | undefined;
const { sort, score, ...rawObject } = savedObject;
const migrated = disableExtensions
? migrationHelper.migrateStorageDocument(rawObject)
: await migrationHelper.migrateAndDecryptStorageDocument({
document: rawObject,
typeMap: redactTypeMap,
});

if (fields !== undefined) {
// If the fields argument is set, don't migrate.
// This document may only contains a subset of it's fields meaning the migration
// (transform and forwardCompatibilitySchema) is not guaranteed to succeed. We
// still try to decrypt/redact the fields that are present in the document.
migrated = await encryptionHelper.optionallyDecryptAndRedactSingleResult(
savedObject,
redactTypeMap
);
} else if (disableExtensions) {
migrated = migrationHelper.migrateStorageDocument(rawObject);
} else {
migrated = await migrationHelper.migrateAndDecryptStorageDocument({
document: rawObject,
typeMap: redactTypeMap,
});
}

migratedDocuments.push({ ...migrated, sort, score } as SavedObjectsFindResult<T>);
}
} catch (error) {
Expand Down

0 comments on commit 36f6d6f

Please sign in to comment.