[UI setting] feat: keep backward compatibility for UI setting client. (#9854) (#9924)

* feat: keep backward compatibility for scope



* Changeset file for PR #9854 created/updated

* feat: update warning messages



* feat: update logic to keep backward compatibility



* fix: UT failure



* feat: optimize code



* fix: bootstrap



* fix: boostrap



* fix: bootstrap error



---------



(cherry picked from commit 027baef)

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

Author: 3 people

changelogs/fragments/9854.yml

@@ -0,0 +1,2 @@
+feat:
+- Keep backward compatibility for UI setting client ([#9854](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/9854))

src/core/server/ui_settings/ui_settings_client.test.ts

@@ -137,19 +137,39 @@ describe('ui settings', () => {
       );
     });
 
-    it('shall throw error when trying to update multi-scope settings without passing a scope', async () => {
+    it('shall give deprecation warnings when trying to update multi-scope settings without passing a scope', async () => {
       const value = chance.word();
       const defaults = {
         user: { value, scope: 'user' },
         workspace: { value, scope: ['workspace', 'user'] },
+        globalAndWorkspace: { value, scope: ['workspace', 'global'] },
       };
-      const { uiSettings } = setup({ defaults });
+      const { uiSettings, savedObjectsClient } = setup({ defaults });
 
-      try {
-        await uiSettings.setMany({ one: 'value', user: 'val', workspace: 'value' });
-      } catch (error) {
-        expect(error.message).toBe('Unable to update "workspace", because it has multiple scopes');
-      }
+      await expect(
+        uiSettings.setMany({
+          one: 'value',
+          user: 'val',
+          workspace: 'value',
+          globalAndWorkspace: 'value',
+        })
+      ).resolves.not.toThrow();
+
+      expect(savedObjectsClient.update).toBeCalledWith('config', ID, {
+        globalAndWorkspace: 'value',
+        one: 'value',
+      });
+      expect(savedObjectsClient.update).toBeCalledWith('config', `<current_workspace>_${ID}`, {
+        workspace: 'value',
+      });
+
+      expect(logger.warn).toBeCalledWith(
+        'Deprecation warning: The setting "workspace" has multiple scopes. Please specify a scope when updating it.'
+      );
+
+      expect(logger.warn).toBeCalledWith(
+        'Deprecation warning: The setting "globalAndWorkspace" has multiple scopes. Please specify a scope when updating it.'
+      );
     });
 
     it('automatically creates the savedConfig if it is missing', async () => {

src/core/server/ui_settings/ui_settings_client.ts

@@ -326,27 +326,45 @@ export class UiSettingsClient implements IUiSettingsClient {
    * @returns [global, user, workspace]
    */
   private groupChanges(changes: Record<string, any>) {
-    const userChanges = {} as Record<string, any>;
-    const globalChanges = {} as Record<string, any>;
-    const adminSettingChanges = {} as Record<string, any>;
-    const workspaceChanges = {} as Record<string, any>;
+    const groupedChanges: Record<UiSettingScope, any> = {
+      [UiSettingScope.GLOBAL]: {},
+      [UiSettingScope.USER]: {},
+      [UiSettingScope.WORKSPACE]: {},
+      [UiSettingScope.DASHBOARD_ADMIN]: {},
+    };
 
     Object.entries(changes).forEach(([key, val]) => {
       if (Array.isArray(this.defaults[key]?.scope) && (this.defaults[key].scope?.length ?? 0) > 1) {
-        // if this setting has more than one scope, we should not update this setting without specifying scope
-        throw new Error(`Unable to update "${key}", because it has multiple scopes`);
+        const keyScopes = Array<UiSettingScope>().concat(this.defaults[key].scope || []);
+        // If this setting applies to multiple scopes including global
+        // categorize it as global to maintain backward compatibility.
+        if (keyScopes.includes(UiSettingScope.GLOBAL)) {
+          groupedChanges[UiSettingScope.GLOBAL][key] = val;
+        } else {
+          // else we use first scope as fallback
+          groupedChanges[keyScopes[0]][key] = val;
+        }
+
+        this.log.warn(
+          `Deprecation warning: The setting "${key}" has multiple scopes. Please specify a scope when updating it.`
+        );
       } else if (this.userLevelSettingsKeys.includes(key)) {
-        userChanges[key] = val;
+        groupedChanges[UiSettingScope.USER][key] = val;
       } else if (this.adminUiSettingsKeys.includes(key)) {
-        adminSettingChanges[key] = val;
+        groupedChanges[UiSettingScope.DASHBOARD_ADMIN][key] = val;
       } else if (this.workspaceLevelSettingsKeys.includes(key)) {
-        workspaceChanges[key] = val;
+        groupedChanges[UiSettingScope.WORKSPACE][key] = val;
       } else {
-        globalChanges[key] = val;
+        groupedChanges[UiSettingScope.GLOBAL][key] = val;
       }
     });
 
-    return [globalChanges, userChanges, workspaceChanges, adminSettingChanges];
+    return [
+      groupedChanges[UiSettingScope.GLOBAL],
+      groupedChanges[UiSettingScope.USER],
+      groupedChanges[UiSettingScope.WORKSPACE],
+      groupedChanges[UiSettingScope.DASHBOARD_ADMIN],
+    ];
   }
 
   private async write({

src/plugins/workspace/server/plugin.ts

@@ -70,6 +70,7 @@ export class WorkspacePlugin implements Plugin<WorkspacePluginSetup, WorkspacePl
   private workspaceSavedObjectsClientWrapper?: WorkspaceSavedObjectsClientWrapper;
   private workspaceUiSettingsClientWrapper?: WorkspaceUiSettingsClientWrapper;
   private workspaceConfig$: Observable<ConfigSchema>;
+  private env: PluginInitializerContext['env'];
 
   private proxyWorkspaceTrafficToRealHandler(setupDeps: CoreSetup) {
     /**
@@ -218,6 +219,7 @@ export class WorkspacePlugin implements Plugin<WorkspacePluginSetup, WorkspacePl
     this.logger = initializerContext.logger.get();
     this.globalConfig$ = initializerContext.config.legacy.globalConfig$;
     this.workspaceConfig$ = initializerContext.config.create();
+    this.env = initializerContext.env;
   }
 
   public async setup(core: CoreSetup, deps: WorkspacePluginDependencies) {
@@ -245,7 +247,10 @@ export class WorkspacePlugin implements Plugin<WorkspacePluginSetup, WorkspacePl
     );
     this.proxyWorkspaceTrafficToRealHandler(core);
 
-    const workspaceUiSettingsClientWrapper = new WorkspaceUiSettingsClientWrapper(this.logger);
+    const workspaceUiSettingsClientWrapper = new WorkspaceUiSettingsClientWrapper(
+      this.logger,
+      this.env
+    );
     this.workspaceUiSettingsClientWrapper = workspaceUiSettingsClientWrapper;
     core.savedObjects.addClientWrapper(
       PRIORITY_FOR_WORKSPACE_UI_SETTINGS_WRAPPER,
@@ -303,6 +308,9 @@ export class WorkspacePlugin implements Plugin<WorkspacePluginSetup, WorkspacePl
     this.workspaceConflictControl?.setSerializer(core.savedObjects.createSerializer());
     this.workspaceSavedObjectsClientWrapper?.setScopedClient(core.savedObjects.getScopedClient);
     this.workspaceUiSettingsClientWrapper?.setScopedClient(core.savedObjects.getScopedClient);
+    this.workspaceUiSettingsClientWrapper?.setAsScopedUISettingsClient(
+      core.uiSettings.asScopedToClient
+    );
 
     return {
       client: this.client as IWorkspaceClientImpl,

src/plugins/workspace/server/saved_objects/workspace_ui_settings_client_wrapper.test.ts

@@ -4,12 +4,19 @@
  */
 
 import { loggerMock } from '@osd/logging/target/mocks';
-import { httpServerMock, savedObjectsClientMock, coreMock } from '../../../../core/server/mocks';
+import {
+  httpServerMock,
+  savedObjectsClientMock,
+  coreMock,
+  uiSettingsServiceMock,
+} from '../../../../core/server/mocks';
 import { WorkspaceUiSettingsClientWrapper } from './workspace_ui_settings_client_wrapper';
 import {
   WORKSPACE_TYPE,
   CURRENT_WORKSPACE_PLACEHOLDER,
   SavedObjectsErrorHelpers,
+  PackageInfo,
+  UiSettingScope,
 } from '../../../../core/server';
 import {
   DEFAULT_DATA_SOURCE_UI_SETTINGS_ID,
@@ -19,13 +26,34 @@ import * as utils from '../../../../core/server/utils';
 
 jest.mock('../../../../core/server/utils');
 
+const WORKSPACE_SCOPE_SETTING_WITHOUT_VALUE_ID = 'workspace_scope_setting_without_value';
+const GLOBAL_SCOPE_SETTING_ID = 'global_scope_setting';
+
 describe('WorkspaceUiSettingsClientWrapper', () => {
   const createWrappedClient = () => {
     const clientMock = savedObjectsClientMock.create();
     const getClientMock = jest.fn().mockReturnValue(clientMock);
     const requestHandlerContext = coreMock.createRequestHandlerContext();
     const requestMock = httpServerMock.createOpenSearchDashboardsRequest();
     const logger = loggerMock.create();
+    const uiSettingsMock = coreMock.createStart().uiSettings;
+    const uiSettingsClientMock = uiSettingsServiceMock.createClient();
+    uiSettingsMock.asScopedToClient.mockReturnValue(uiSettingsClientMock);
+    const pluginInitializerContext = coreMock.createPluginInitializerContext();
+    (pluginInitializerContext.env.packageInfo as PackageInfo).version = '3.0.0';
+
+    uiSettingsClientMock.getRegistered.mockReturnValue({
+      [DEFAULT_DATA_SOURCE_UI_SETTINGS_ID]: {
+        scope: [UiSettingScope.GLOBAL, UiSettingScope.WORKSPACE],
+      },
+      [DEFAULT_INDEX_PATTERN_UI_SETTINGS_ID]: {
+        scope: UiSettingScope.WORKSPACE,
+      },
+      [WORKSPACE_SCOPE_SETTING_WITHOUT_VALUE_ID]: {
+        scope: UiSettingScope.WORKSPACE,
+      },
+      [GLOBAL_SCOPE_SETTING_ID]: {},
+    });
 
     clientMock.get.mockImplementation(async (type, id) => {
       if (type === 'config') {
@@ -55,8 +83,20 @@ describe('WorkspaceUiSettingsClientWrapper', () => {
       return Promise.reject();
     });
 
-    const wrapper = new WorkspaceUiSettingsClientWrapper(logger);
+    clientMock.update.mockResolvedValue({
+      id: '3.0.0',
+      references: [],
+      type: 'config',
+      attributes: {
+        defaultDashboard: 'default-dashboard-global',
+        [DEFAULT_DATA_SOURCE_UI_SETTINGS_ID]: 'default-ds-global',
+        [DEFAULT_INDEX_PATTERN_UI_SETTINGS_ID]: 'default-index-global',
+      },
+    });
+
+    const wrapper = new WorkspaceUiSettingsClientWrapper(logger, pluginInitializerContext.env);
     wrapper.setScopedClient(getClientMock);
+    wrapper.setAsScopedUISettingsClient(uiSettingsMock.asScopedToClient);
 
     return {
       wrappedClient: wrapper.wrapperFactory({
@@ -94,14 +134,18 @@ describe('WorkspaceUiSettingsClientWrapper', () => {
 
     const { wrappedClient } = createWrappedClient();
 
-    const result = await wrappedClient.get(`config`, `${CURRENT_WORKSPACE_PLACEHOLDER}_3.0.0`);
-    expect(result).toEqual({
+    const result = await wrappedClient.get<{
+      [key: string]: unknown;
+    }>(`config`, `${CURRENT_WORKSPACE_PLACEHOLDER}_3.0.0`);
+    expect(result).toStrictEqual({
       id: '3.0.0',
       references: [],
       type: 'config',
       attributes: {
         defaultDashboard: 'default-dashboard-workspace',
         [DEFAULT_DATA_SOURCE_UI_SETTINGS_ID]: 'default-ds-workspace',
+        [DEFAULT_INDEX_PATTERN_UI_SETTINGS_ID]: undefined,
+        [WORKSPACE_SCOPE_SETTING_WITHOUT_VALUE_ID]: undefined,
       },
     });
   });
@@ -182,7 +226,7 @@ describe('WorkspaceUiSettingsClientWrapper', () => {
     expect(error.message).toEqual('Bad Request');
   });
 
-  it('should update global ui settings', async () => {
+  it('should update global ui settings when out of workspace', async () => {
     // Currently NOT in a workspace
     jest.spyOn(utils, 'getWorkspaceState').mockReturnValue({});
 
@@ -199,4 +243,28 @@ describe('WorkspaceUiSettingsClientWrapper', () => {
       {}
     );
   });
+
+  it('should update workspace settings when inside workspace and config id equals global setting', async () => {
+    jest.spyOn(utils, 'getWorkspaceState').mockReturnValue({ requestWorkspaceId: 'workspace-id' });
+
+    const { wrappedClient, clientMock, logger } = createWrappedClient();
+
+    await wrappedClient.update('config', '3.0.0', {
+      [DEFAULT_DATA_SOURCE_UI_SETTINGS_ID]: 'data_source_id',
+    });
+
+    expect(clientMock.update).toHaveBeenCalledWith(
+      WORKSPACE_TYPE,
+      'workspace-id',
+      {
+        uiSettings: expect.objectContaining({
+          [DEFAULT_DATA_SOURCE_UI_SETTINGS_ID]: 'data_source_id',
+        }),
+      },
+      {}
+    );
+    expect(logger.warn).toBeCalledWith(
+      'Deprecation warning: updating workspace settings through global scope will no longer be supported.'
+    );
+  });
 });