Refactor: move workspace logic from repository to saved objects client wrapper

CHANGELOG.md

@@ -41,6 +41,11 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - [Multiple Datasource] Add interfaces to register add-on authentication method from plug-in module ([#5851](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5851))
 - [Multiple Datasource] Able to Hide "Local Cluster" option from datasource DropDown ([#5827](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5827))
 - [Multiple Datasource] Add api registry and allow it to be added into client config in data source plugin ([#5895](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5895))
+- [Multiple Datasource] Concatenate data source name with index pattern name and change delimiter to double colon ([#5907](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5907))
+- [Multiple Datasource] Refactor client and legacy client to use authentication registry ([#5881](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5881))
+- [Multiple Datasource] Improved error handling for the search API when a null value is passed for the dataSourceId ([#5882](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5882))
+- [Multiple Datasource] Hide/Show authentication method in multi data source plugin based on configuration ([#5916](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5916))
+- [Workspace] Optional workspaces params in repository ([#5949](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5949))
 
 ### 🐛 Bug Fixes
 

src/core/server/saved_objects/export/get_sorted_objects_for_export.test.ts

@@ -128,7 +128,6 @@ describe('getSortedObjectsForExport()', () => {
                 index-pattern,
                 search,
               ],
-              workspaces: undefined,
             },
           ],
         ],
@@ -219,7 +218,6 @@ describe('getSortedObjectsForExport()', () => {
                 index-pattern,
                 search,
               ],
-              workspaces: undefined,
             },
           ],
         ],
@@ -370,7 +368,6 @@ describe('getSortedObjectsForExport()', () => {
                 index-pattern,
                 search,
               ],
-              workspaces: undefined,
             },
           ],
         ],
@@ -462,7 +459,6 @@ describe('getSortedObjectsForExport()', () => {
                 index-pattern,
                 search,
               ],
-              workspaces: undefined,
             },
           ],
         ],

src/core/server/saved_objects/export/get_sorted_objects_for_export.ts

@@ -125,7 +125,7 @@ async function fetchObjectsToExport({
       search,
       perPage: exportSizeLimit,
       namespaces: namespace ? [namespace] : undefined,
-      workspaces,
+      ...(workspaces ? { workspaces } : {}),
     });
     if (findResponse.total > exportSizeLimit) {
       throw Boom.badRequest(`Can't export more than ${exportSizeLimit} objects`);

src/core/server/saved_objects/import/import_saved_objects.test.ts

@@ -42,7 +42,7 @@ import { typeRegistryMock } from '../saved_objects_type_registry.mock';
 import { importSavedObjectsFromStream } from './import_saved_objects';
 
 import { collectSavedObjects } from './collect_saved_objects';
-import { regenerateIds, regenerateIdsWithReference } from './regenerate_ids';
+import { regenerateIds } from './regenerate_ids';
 import { validateReferences } from './validate_references';
 import { checkConflicts } from './check_conflicts';
 import { checkOriginConflicts } from './check_origin_conflicts';
@@ -70,7 +70,6 @@ describe('#importSavedObjectsFromStream', () => {
       importIdMap: new Map(),
     });
     getMockFn(regenerateIds).mockReturnValue(new Map());
-    getMockFn(regenerateIdsWithReference).mockReturnValue(Promise.resolve(new Map()));
     getMockFn(validateReferences).mockResolvedValue([]);
     getMockFn(checkConflicts).mockResolvedValue({
       errors: [],
@@ -279,15 +278,6 @@ describe('#importSavedObjectsFromStream', () => {
           ]),
         });
         getMockFn(validateReferences).mockResolvedValue([errors[1]]);
-        getMockFn(regenerateIdsWithReference).mockResolvedValue(
-          Promise.resolve(
-            new Map([
-              ['foo', {}],
-              ['bar', {}],
-              ['baz', {}],
-            ])
-          )
-        );
         getMockFn(checkConflicts).mockResolvedValue({
           errors: [errors[2]],
           filteredObjects,

src/core/server/saved_objects/import/import_saved_objects.ts

@@ -38,7 +38,7 @@ import { validateReferences } from './validate_references';
 import { checkOriginConflicts } from './check_origin_conflicts';
 import { createSavedObjects } from './create_saved_objects';
 import { checkConflicts } from './check_conflicts';
-import { regenerateIds, regenerateIdsWithReference } from './regenerate_ids';
+import { regenerateIds } from './regenerate_ids';
 import { checkConflictsForDataSource } from './check_conflict_for_data_source';
 
 /**
@@ -86,13 +86,6 @@ export async function importSavedObjectsFromStream({
     // randomly generated id
     importIdMap = regenerateIds(collectSavedObjectsResult.collectedObjects, dataSourceId);
   } else {
-    importIdMap = await regenerateIdsWithReference({
-      savedObjects: collectSavedObjectsResult.collectedObjects,
-      savedObjectsClient,
-      workspaces,
-      objectLimit,
-      importIdMap,
-    });
     // in check conclict and override mode
     // Check single-namespace objects for conflicts in this namespace, and check multi-namespace objects for conflicts across all namespaces
     const checkConflictsParams = {
@@ -152,6 +145,7 @@ export async function importSavedObjectsFromStream({
     ...(workspaces ? { workspaces } : {}),
     dataSourceId,
     dataSourceTitle,
+    ...(workspaces ? { workspaces } : {}),
   };
   const createSavedObjectsResult = await createSavedObjects(createSavedObjectsParams);
   errorAccumulator = [...errorAccumulator, ...createSavedObjectsResult.errors];

src/core/server/saved_objects/import/regenerate_ids.ts

@@ -29,8 +29,7 @@
  */
 
 import { v4 as uuidv4 } from 'uuid';
-import { SavedObject, SavedObjectsClientContract } from '../types';
-import { SavedObjectsUtils } from '../service';
+import { SavedObject } from '../types';
 
 /**
  * Takes an array of saved objects and returns an importIdMap of randomly-generated new IDs.
@@ -48,40 +47,3 @@ export const regenerateIds = (objects: SavedObject[], dataSourceId: string | und
   }, new Map<string, { id: string; omitOriginId?: boolean }>());
   return importIdMap;
 };
-
-export const regenerateIdsWithReference = async (props: {
-  savedObjects: SavedObject[];
-  savedObjectsClient: SavedObjectsClientContract;
-  workspaces?: string[];
-  objectLimit: number;
-  importIdMap: Map<string, { id?: string; omitOriginId?: boolean }>;
-}): Promise<Map<string, { id?: string; omitOriginId?: boolean }>> => {
-  const { savedObjects, savedObjectsClient, workspaces, importIdMap } = props;
-  if (!workspaces || !workspaces.length) {
-    return savedObjects.reduce((acc, object) => {
-      return acc.set(`${object.type}:${object.id}`, { id: object.id, omitOriginId: false });
-    }, importIdMap);
-  }
-
-  const bulkGetResult = await savedObjectsClient.bulkGet(
-    savedObjects.map((item) => ({ type: item.type, id: item.id }))
-  );
-
-  return bulkGetResult.saved_objects.reduce((acc, object) => {
-    if (object.error?.statusCode === 404) {
-      acc.set(`${object.type}:${object.id}`, { id: object.id, omitOriginId: true });
-      return acc;
-    }
-
-    const filteredWorkspaces = SavedObjectsUtils.filterWorkspacesAccordingToBaseWorkspaces(
-      workspaces,
-      object.workspaces
-    );
-    if (filteredWorkspaces.length) {
-      acc.set(`${object.type}:${object.id}`, { id: uuidv4(), omitOriginId: true });
-    } else {
-      acc.set(`${object.type}:${object.id}`, { id: object.id, omitOriginId: false });
-    }
-    return acc;
-  }, importIdMap);
-};

src/core/server/saved_objects/import/types.ts

@@ -187,7 +187,7 @@ export interface SavedObjectsImportOptions {
   namespace?: string;
   /** If true, will create new copies of import objects, each with a random `id` and undefined `originId`. */
   createNewCopies: boolean;
-  /** if specified, will import in given workspaces, else will import as global object */
+  /** if specified, will import in given workspaces */
   workspaces?: string[];
   dataSourceId?: string;
   dataSourceTitle?: string;
@@ -212,7 +212,7 @@ export interface SavedObjectsResolveImportErrorsOptions {
   namespace?: string;
   /** If true, will create new copies of import objects, each with a random `id` and undefined `originId`. */
   createNewCopies: boolean;
-  /** if specified, will import in given workspaces, else will import as global object */
+  /** if specified, will import in given workspaces */
   workspaces?: string[];
   dataSourceId?: string;
   dataSourceTitle?: string;

src/core/server/saved_objects/import/validate_references.ts

@@ -73,7 +73,10 @@ export async function getNonExistingReferenceAsKeys(
   }
 
   // Fetch references to see if they exist
-  const bulkGetOpts = Array.from(collector.values()).map((obj) => ({ ...obj, fields: ['id'] }));
+  const bulkGetOpts = Array.from(collector.values()).map((obj) => ({
+    ...obj,
+    fields: ['id'],
+  }));
   const bulkGetResponse = await savedObjectsClient.bulkGet(bulkGetOpts, { namespace });
 
   // Error handling

src/core/server/saved_objects/service/lib/repository.test.js

@@ -629,6 +629,7 @@ describe('SavedObjectsRepository', () => {
       references: [{ name: 'ref_0', type: 'test', id: '2' }],
     };
     const namespace = 'foo-namespace';
+    const workspace = 'foo-workspace';
 
     const getMockBulkCreateResponse = (objects, namespace) => {
       return {
@@ -651,7 +652,6 @@ describe('SavedObjectsRepository', () => {
     };
 
     const bulkCreateSuccess = async (objects, options) => {
-      const originalObjects = JSON.parse(JSON.stringify(objects));
       const multiNamespaceObjects = objects.filter(
         ({ type, id }) => registry.isMultiNamespace(type) && id
       );
@@ -666,9 +666,7 @@ describe('SavedObjectsRepository', () => {
         opensearchClientMock.createSuccessTransportRequestPromise(response)
       );
       const result = await savedObjectsRepository.bulkCreate(objects, options);
-      expect(client.mget).toHaveBeenCalledTimes(
-        multiNamespaceObjects?.length || originalObjects?.some((item) => item.id) ? 1 : 0
-      );
+      expect(client.mget).toHaveBeenCalledTimes(multiNamespaceObjects?.length ? 1 : 0);
       return result;
     };
 
@@ -726,10 +724,7 @@ describe('SavedObjectsRepository', () => {
         await bulkCreateSuccess(objects);
         expect(client.bulk).toHaveBeenCalledTimes(1);
         expect(client.mget).toHaveBeenCalledTimes(1);
-        const docs = [
-          expect.objectContaining({ _id: `${obj1.type}:${obj1.id}` }),
-          expect.objectContaining({ _id: `${MULTI_NAMESPACE_TYPE}:${obj2.id}` }),
-        ];
+        const docs = [expect.objectContaining({ _id: `${MULTI_NAMESPACE_TYPE}:${obj2.id}` })];
         expect(client.mget.mock.calls[0][0].body).toEqual({ docs });
       });
 
@@ -922,6 +917,16 @@ describe('SavedObjectsRepository', () => {
         await bulkCreateSuccess(objects, { namespace });
         expectClientCallArgsAction(objects, { method: 'create', getId });
       });
+
+      it(`adds workspaces to request body for any types`, async () => {
+        await bulkCreateSuccess([obj1, obj2], { workspaces: [workspace] });
+        const expected = expect.objectContaining({ workspaces: [workspace] });
+        const body = [expect.any(Object), expected, expect.any(Object), expected];
+        expect(client.bulk).toHaveBeenCalledWith(
+          expect.objectContaining({ body }),
+          expect.anything()
+        );
+      });
     });
 
     describe('errors', () => {
@@ -1007,12 +1012,6 @@ describe('SavedObjectsRepository', () => {
         const response1 = {
           status: 200,
           docs: [
-            {
-              found: true,
-              _source: {
-                type: obj1.type,
-              },
-            },
             {
               found: true,
               _source: {
@@ -1036,11 +1035,7 @@ describe('SavedObjectsRepository', () => {
         expect(client.mget).toHaveBeenCalled();
 
         const body1 = {
-          docs: [
-            expect.objectContaining({ _id: `${obj1.type}:${obj1.id}` }),
-            expect.objectContaining({ _id: `${obj.type}:${obj.id}` }),
-            expect.objectContaining({ _id: `${obj2.type}:${obj2.id}` }),
-          ],
+          docs: [expect.objectContaining({ _id: `${obj.type}:${obj.id}` })],
         };
         expect(client.mget).toHaveBeenCalledWith(
           expect.objectContaining({ body: body1 }),
@@ -2050,17 +2045,9 @@ describe('SavedObjectsRepository', () => {
 
     const createSuccess = async (type, attributes, options) => {
       const result = await savedObjectsRepository.create(type, attributes, options);
-      let count = 0;
-      if (options?.overwrite && options?.id) {
-        /**
-         * workspace will call extra one to get latest status of current object
-         */
-        count++;
-      }
-      if (registry.isMultiNamespace(type) && options.overwrite) {
-        count++;
-      }
-      expect(client.get).toHaveBeenCalledTimes(count);
+      expect(client.get).toHaveBeenCalledTimes(
+        registry.isMultiNamespace(type) && options.overwrite ? 1 : 0
+      );
       return result;
     };