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

[Workspace][Bug] Unable to copy assets to a workspace without assigning related data source #7690

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
2 changes: 2 additions & 0 deletions changelogs/fragments/7690.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- [Workspace] Unable to copy assets to a workspace without assigning related data source ([#7690](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7690))
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,11 @@ describe('#createSavedObjects', () => {
id: 'baz-id',
error: { type: 'missing_references' },
} as SavedObjectsImportError,
{
type: 'baz',
id: 'baz-id',
error: { type: 'missing_data_source' },
} as SavedObjectsImportError,
];
const unresolvableErrors: SavedObjectsImportError[] = [
{ type: 'qux', id: 'qux-id', error: { type: 'unsupported_type' } } as SavedObjectsImportError,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,12 @@ export const createSavedObjects = async <T>({
return { ...object, ...(references && { references }) };
})
);
const resolvableErrors = ['conflict', 'ambiguous_conflict', 'missing_references'];
const resolvableErrors = [
'conflict',
'ambiguous_conflict',
'missing_references',
'missing_data_source',
];
let expectedResults = objectsToCreate;
if (!accumulatedErrors.some(({ error: { type } }) => resolvableErrors.includes(type))) {
const bulkCreateResponse = await savedObjectsClient.bulkCreate(objectsToCreate, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
SavedObjectsType,
SavedObject,
SavedObjectsImportError,
SavedObjectsBaseOptions,
} from '../types';
import { savedObjectsClientMock } from '../../mocks';
import { SavedObjectsImportOptions, ISavedObjectTypeRegistry } from '..';
Expand All @@ -48,14 +49,17 @@ import { checkConflicts } from './check_conflicts';
import { checkOriginConflicts } from './check_origin_conflicts';
import { createSavedObjects } from './create_saved_objects';
import { checkConflictsForDataSource } from './check_conflict_for_data_source';
import { validateDataSources } from './validate_data_sources';

jest.mock('./collect_saved_objects');
jest.mock('./regenerate_ids');
jest.mock('./validate_references');
jest.mock('./validate_data_sources');
jest.mock('./check_conflicts');
jest.mock('./check_origin_conflicts');
jest.mock('./create_saved_objects');
jest.mock('./check_conflict_for_data_source');
jest.mock('./utils');

const getMockFn = <T extends (...args: any[]) => any, U>(fn: (...args: Parameters<T>) => U) =>
fn as jest.MockedFunction<(...args: Parameters<T>) => U>;
Expand All @@ -71,6 +75,7 @@ describe('#importSavedObjectsFromStream', () => {
});
getMockFn(regenerateIds).mockReturnValue(new Map());
getMockFn(validateReferences).mockResolvedValue([]);
getMockFn(validateDataSources).mockResolvedValue([]);
getMockFn(checkConflicts).mockResolvedValue({
errors: [],
filteredObjects: [],
Expand Down Expand Up @@ -101,7 +106,9 @@ describe('#importSavedObjectsFromStream', () => {
const setupOptions = (
createNewCopies: boolean = false,
dataSourceId: string | undefined = undefined,
dataSourceEnabled: boolean | undefined = false
dataSourceEnabled: boolean | undefined = false,
workspaces: SavedObjectsBaseOptions['workspaces'] = undefined,
isCopy: boolean = false
): SavedObjectsImportOptions => {
readStream = new Readable();
savedObjectsClient = savedObjectsClientMock.create();
Expand All @@ -122,6 +129,9 @@ describe('#importSavedObjectsFromStream', () => {
namespace,
createNewCopies,
dataSourceId,
workspaces,
isCopy,
dataSourceEnabled,
};
};
const createObject = (
Expand Down Expand Up @@ -195,6 +205,25 @@ describe('#importSavedObjectsFromStream', () => {
);
});

test('validates data sources', async () => {
const options = setupOptions(true, undefined, true, ['workspace-1'], true);
const collectedObjects = [createObject()];
const errorAccumulator: SavedObjectsImportError[] = [];
getMockFn(collectSavedObjects).mockResolvedValue({
errors: [],
collectedObjects,
importIdMap: new Map(),
});

await importSavedObjectsFromStream(options);
expect(validateDataSources).toHaveBeenCalledWith(
collectedObjects,
savedObjectsClient,
errorAccumulator,
['workspace-1']
);
});

describe('with createNewCopies disabled', () => {
test('does not regenerate object IDs', async () => {
const options = setupOptions();
Expand Down Expand Up @@ -602,6 +631,37 @@ describe('#importSavedObjectsFromStream', () => {
expect(result).toEqual({ success: false, successCount: 0, errors: expectedErrors });
});

test('performs a copy operation accumulates multiple errors', async () => {
const options = setupOptions(true, undefined, true, ['workspace-1'], true);
const errors = [createError(), createError(), createError(), createError()];
getMockFn(collectSavedObjects).mockResolvedValue({
errors: [errors[0]],
collectedObjects: [],
importIdMap: new Map(), // doesn't matter
});
getMockFn(validateReferences).mockResolvedValue([errors[1]]);
getMockFn(validateDataSources).mockResolvedValue([errors[2]]);
getMockFn(createSavedObjects).mockResolvedValue({ errors: [errors[3]], createdObjects: [] });

// it will not accumulate
getMockFn(checkConflicts).mockResolvedValue({
errors: [errors[2]],
filteredObjects: [],
importIdMap: new Map(), // doesn't matter
pendingOverwrites: new Set(),
});
// it will not accumulate
getMockFn(checkOriginConflicts).mockResolvedValue({
errors: [errors[3]],
importIdMap: new Map(), // doesn't matter
pendingOverwrites: new Set(),
});

const result = await importSavedObjectsFromStream(options);
const expectedErrors = errors.map(({ type, id }) => expect.objectContaining({ type, id }));
expect(result).toEqual({ success: false, successCount: 0, errors: expectedErrors });
});

test('early return if import data source objects to non-MDS cluster', async () => {
const options = setupOptions(false, testDataSourceId, false);
const dsObj = createDataSourceObject();
Expand Down
16 changes: 16 additions & 0 deletions src/core/server/saved_objects/import/import_saved_objects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import { checkConflicts } from './check_conflicts';
import { regenerateIds } from './regenerate_ids';
import { checkConflictsForDataSource } from './check_conflict_for_data_source';
import { isSavedObjectWithDataSource } from './validate_object_id';
import { validateDataSources } from './validate_data_sources';

/**
* Import saved objects from given stream. See the {@link SavedObjectsImportOptions | options} for more
Expand All @@ -61,6 +62,7 @@ export async function importSavedObjectsFromStream({
dataSourceTitle,
workspaces,
dataSourceEnabled,
isCopy,
}: SavedObjectsImportOptions): Promise<SavedObjectsImportResponse> {
let errorAccumulator: SavedObjectsImportError[] = [];
const supportedTypes = typeRegistry.getImportableAndExportableTypes().map((type) => type.name);
Expand Down Expand Up @@ -108,6 +110,20 @@ export async function importSavedObjectsFromStream({
);
errorAccumulator = [...errorAccumulator, ...validateReferencesResult];

if (isCopy) {
yubonluo marked this conversation as resolved.
Show resolved Hide resolved
// Unable to copy data source among workspaces.
yubonluo marked this conversation as resolved.
Show resolved Hide resolved
collectSavedObjectsResult.collectedObjects = collectSavedObjectsResult.collectedObjects.filter(
(obj) => obj.type !== 'data-source'
);
const validateDataSourcesResult = await validateDataSources(
collectSavedObjectsResult.collectedObjects,
savedObjectsClient,
errorAccumulator,
workspaces
);
errorAccumulator = [...errorAccumulator, ...validateDataSourcesResult];
}

if (createNewCopies) {
// randomly generated id
importIdMap = regenerateIds(collectSavedObjectsResult.collectedObjects, dataSourceId);
Expand Down
13 changes: 12 additions & 1 deletion src/core/server/saved_objects/import/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,15 @@ export interface SavedObjectsImportMissingReferencesError {
references: Array<{ type: string; id: string }>;
}

/**
* Represents a failure to import due to missing target workspace assigned data source.
yubonluo marked this conversation as resolved.
Show resolved Hide resolved
* @public
*/
export interface SavedObjectsImportMissingDataSourceError {
type: 'missing_data_source';
dataSource: string;
}

/**
* Represents a failure to import.
* @public
Expand All @@ -126,7 +135,8 @@ export interface SavedObjectsImportError {
| SavedObjectsImportAmbiguousConflictError
| SavedObjectsImportUnsupportedTypeError
| SavedObjectsImportMissingReferencesError
| SavedObjectsImportUnknownError;
| SavedObjectsImportUnknownError
| SavedObjectsImportMissingDataSourceError;
}

/**
Expand Down Expand Up @@ -191,6 +201,7 @@ export interface SavedObjectsImportOptions {
dataSourceTitle?: string;
dataSourceEnabled?: boolean;
workspaces?: SavedObjectsBaseOptions['workspaces'];
isCopy?: boolean;
}

/**
Expand Down
102 changes: 102 additions & 0 deletions src/core/server/saved_objects/import/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
getUpdatedTSVBVisState,
updateDataSourceNameInVegaSpec,
updateDataSourceNameInTimeline,
findReferenceDataSourceForObject,
} from './utils';
import { parse } from 'hjson';
import { isEqual } from 'lodash';
Expand Down Expand Up @@ -342,3 +343,104 @@ describe('getUpdatedTSVBVisState', () => {
}
);
});

describe('findReferenceDataSourceForObject', () => {
const savedObjects: Array<SavedObject<{ title?: string }>> = [
{
id: '1',
references: [{ id: '5', type: 'data-source', name: '5' }],
type: 'index-pattern',
attributes: {},
},
{
id: '2',
references: [{ id: '3', type: 'non-data-source', name: '3' }],
type: 'index-pattern',
attributes: {},
},
{
id: '3',
references: [],
type: 'non-data-source',
attributes: {},
},
{
id: '4',
references: [{ id: '1', type: 'index-pattern', name: '1' }],
type: 'non-data-source',
attributes: {},
},
{
id: '6',
references: [{ id: '7', type: 'index-pattern', name: '7' }],
type: 'non-data-source',
attributes: {},
},
];

const ObjectsMap = new Map(savedObjects.map((so) => [so.id, so]));

test('returns the data-source reference if it exists in the references', () => {
const result = findReferenceDataSourceForObject(savedObjects[0], ObjectsMap);
expect(result).toEqual({ id: '5', type: 'data-source', name: '5' });
});

test('returns null if there is no data-source reference and no nested references', () => {
expect(findReferenceDataSourceForObject(savedObjects[1], ObjectsMap)).toBeNull();
});

test('returns null if no references exist', () => {
expect(findReferenceDataSourceForObject(savedObjects[2], ObjectsMap)).toBeNull();
});

test('returns nested data-source reference if found', () => {
const result = findReferenceDataSourceForObject(savedObjects[3], ObjectsMap);
expect(result).toEqual({ id: '5', type: 'data-source', name: '5' });
});

test('returns null if the nested references have no data-source reference', () => {
expect(findReferenceDataSourceForObject(savedObjects[4], ObjectsMap)).toBeNull();
});

test('returns null if circular reference', () => {
const circularAssets: Array<SavedObject<{ title?: string }>> = [
{
id: '1',
references: [{ id: '2', type: 'non-data-source', name: '2' }],
type: 'non-data-source',
attributes: {},
},
{
id: '2',
references: [{ id: '3', type: 'non-data-source', name: '3' }],
type: 'non-data-source',
attributes: {},
},
{
id: '3',
references: [{ id: '1', type: 'non-data-source', name: '1' }],
type: 'non-data-source',
attributes: {},
},
];
const circularAssetsMap = new Map(circularAssets.map((so) => [so.id, so]));

const result = findReferenceDataSourceForObject(circularAssets[0], circularAssetsMap);
expect(result).toBeNull();
});

test('returns null if circular reference is itself', () => {
const circularAssets: Array<SavedObject<{ title?: string }>> = [
{
id: '1',
references: [{ id: '1', type: 'non-data-source', name: '1' }],
type: 'non-data-source',
attributes: {},
},
];
const circularAssetsMap = new Map(circularAssets.map((so) => [so.id, so]));

const result = findReferenceDataSourceForObject(circularAssets[0], circularAssetsMap);
expect(result).toBeNull();
});
});
35 changes: 35 additions & 0 deletions src/core/server/saved_objects/import/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,3 +183,38 @@ const parseJSONSpec = (spec: string) => {

return undefined;
};

export function findReferenceDataSourceForObject(
savedObject: SavedObject,
savedObjects: Map<string, SavedObject>,
visited = new Set<string>()
): SavedObjectReference | null {
const { references } = savedObject;
if (!references || references.length === 0) {
return null;
}

const dataSourceReference = references.find((ref) => ref.type === 'data-source');
if (dataSourceReference) {
return dataSourceReference;
}

visited.add(savedObject.id);

for (const { id } of references) {
if (visited.has(id)) {
continue;
}
const referenceObject = savedObjects.get(id);
if (referenceObject) {
const dataSource = findReferenceDataSourceForObject(referenceObject, savedObjects, visited);
if (dataSource) {
return dataSource;
}
}
}

visited.delete(savedObject.id);

return null;
}
Loading
Loading