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] Consume workspace id in saved object client #6014

Conversation

SuZhou-Joe
Copy link
Member

@SuZhou-Joe SuZhou-Joe commented Mar 4, 2024

Description

This PR adds a new private property in saved objects client to make the client be aware of current workspace. Furthermore when user tries to do Create/Update/Find operations on saved objects, the property will take effect if set and append itself to request payload to indicate it is doing the operation under the specific workspace.

Issues Resolved

closes #6027

Screenshot

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@SuZhou-Joe SuZhou-Joe force-pushed the current-workspace-in-saved-objects-client-official branch from 86b2934 to 23f5907 Compare March 4, 2024 08:22
Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.08%. Comparing base (58fb588) to head (c553a36).
Report is 614 commits behind head on main.

Files with missing lines Patch % Lines
src/plugins/workspace/public/plugin.ts 71.42% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6014      +/-   ##
==========================================
- Coverage   67.12%   67.08%   -0.04%     
==========================================
  Files        3323     3324       +1     
  Lines       64291    64309      +18     
  Branches    10336    10341       +5     
==========================================
- Hits        43153    43144       -9     
- Misses      18615    18686      +71     
+ Partials     2523     2479      -44     
Flag Coverage Δ
Linux_1 31.67% <27.77%> (+<0.01%) ⬆️
Linux_2 ?
Linux_3 44.71% <0.00%> (-0.01%) ⬇️
Linux_4 35.09% <0.00%> (-0.01%) ⬇️
Windows_1 31.69% <27.77%> (+<0.01%) ⬆️
Windows_2 55.19% <100.00%> (+0.01%) ⬆️
Windows_3 44.72% <0.00%> (-0.02%) ⬇️
Windows_4 35.09% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SuZhou-Joe SuZhou-Joe changed the title [Workspace] Consume workspace id in saved object client and saved object management page [Workspace] Consume workspace id in saved object client Mar 5, 2024
@SuZhou-Joe SuZhou-Joe force-pushed the current-workspace-in-saved-objects-client-official branch 2 times, most recently from d801828 to 645d605 Compare March 5, 2024 02:30
Copy link
Collaborator

@BionIT BionIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments

@@ -227,6 +234,11 @@ export class SavedObjectsClient {
this.batchQueue = [];
}

public setCurrentWorkspace(workspaceId: string): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to have a separate set workspace id when using saved object client? Can workspaces field be within the options? If each workspace should have one saved object client instance, we can add workspace id in the constructor

Copy link
Member Author

@SuZhou-Joe SuZhou-Joe Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is used in workspace.start lifeCycle, which indicates current workspace id. We support workspaces field within options, but it is import that we add a internal currentWorkspaceId because we want to make workspaces totally transparent to other plugins so that no matter which plugin calls savedObjectsClient methods, the client will append workspaces fields when doing the real request automatically.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean to make the workspaces transparent? this is to set the workspace id not get, right? plus, why would plugins need to get current workspace id from the saved objects client. Can we have the workspace id in the global context instead of setting it specific for saved object client, is it the only place where it needs to know the current workspace?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setCurrentWorkspace will only be called by workspace plugin when setup, and once the currentWorkspaceId is set, the workspaces field will be automatically append into the final request payload accordingly.

if (options.hasOwnProperty('workspaces')) {
finalWorkspaces = options.workspaces;
} else if (typeof currentWorkspaceId === 'string') {
finalWorkspaces = [currentWorkspaceId];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference of the currentworkspace id vs the one from the payload/options?

Copy link
Member Author

@SuZhou-Joe SuZhou-Joe Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workspaces fields inside payload/options has a higher priority than currentWorkspaceId. Most of the cases there will not be workspaces fields in options, except some plugins want to overwrite the workspaces explicitly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just set workspace id into options when using the saved object client?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 sources of workspaces:

  • workspaces inside options.
  • currentWorkspaceId in the instance.

the workspaces inside options has higher priority than the current workspaceId, if somewhere calls a method by specifying workspaces inside options, we will honor that and use exactly what they pass to the request payload.

We add currentWorkspaceId in the instance because actually there is few case that plugins want to overwrite the workspaces inside options, by using the currentWorkspaceId in the instance, dashboards page / visualizations page / data source management page will support workspace automatically without a single line of code changes because currentWorkspaceId is already set by workspace plugin when they call any CRUD operations through saved objects client.

@@ -131,6 +131,7 @@ export async function resolveSavedObjectsImportErrors({
retries,
createNewCopies,
dataSourceId,
workspaces,
};
const checkConflictsResult = await checkConflicts(checkConflictsParams);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would workspaces checked in checkConflicts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public start(core: CoreStart) {
this.coreStart = core;

this.currentWorkspaceSubscription = this._changeSavedObjectCurrentWorkspace();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is this currentWorkspaceSubscription used? should it be observable instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The currentWorkspaceSubscription is used for stopping the subscription in stop lifeCycle.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, how is this observable currentWorkspaceId$ initialized?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The currentWorkspaceId$ will be initialized by a workspace client. You can find the usage here: #6094

src/plugins/workspace/public/plugin.ts Show resolved Hide resolved
@SuZhou-Joe
Copy link
Member Author

@BionIT As the PR will contain more than 30 files changes if including both saved objects client changes and saved objects management page changes. So I will reply to your comment on saved objects management page in #6026 .

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -227,6 +234,11 @@ export class SavedObjectsClient {
this.batchQueue = [];
}

public setCurrentWorkspace(workspaceId: string): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean to make the workspaces transparent? this is to set the workspace id not get, right? plus, why would plugins need to get current workspace id from the saved objects client. Can we have the workspace id in the global context instead of setting it specific for saved object client, is it the only place where it needs to know the current workspace?

public start(core: CoreStart) {
this.coreStart = core;

this.currentWorkspaceSubscription = this._changeSavedObjectCurrentWorkspace();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, how is this observable currentWorkspaceId$ initialized?

if (options.hasOwnProperty('workspaces')) {
finalWorkspaces = options.workspaces;
} else if (typeof currentWorkspaceId === 'string') {
finalWorkspaces = [currentWorkspaceId];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just set workspace id into options when using the saved object client?

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
@SuZhou-Joe SuZhou-Joe force-pushed the current-workspace-in-saved-objects-client-official branch from 75f5530 to c553a36 Compare March 9, 2024 01:13
@SuZhou-Joe SuZhou-Joe removed the v2.13.0 label Mar 9, 2024
@SuZhou-Joe SuZhou-Joe merged commit 8f0885d into opensearch-project:main Mar 9, 2024
58 of 59 checks passed
SuZhou-Joe added a commit to SuZhou-Joe/OpenSearch-Dashboards that referenced this pull request Mar 11, 2024
…roject#6014)

* feat: consume current workspace in saved objects management and saved objects client

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: add unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: add unit test for each change

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: update snapshot of unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update snapshot

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* revert: saved object management changes

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: add CHANGELOG

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: address some comment

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: address comment

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: remove useless return

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: bootstrap error

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update CHANGELOG

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update CHANGELOG

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: optimize code

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update comment

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

---------

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
SuZhou-Joe added a commit to SuZhou-Joe/OpenSearch-Dashboards that referenced this pull request Mar 11, 2024
…roject#6014)

* feat: consume current workspace in saved objects management and saved objects client

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: add unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: add unit test for each change

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: update snapshot of unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update snapshot

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* revert: saved object management changes

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: add CHANGELOG

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: address some comment

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: address comment

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: remove useless return

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: bootstrap error

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update CHANGELOG

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update CHANGELOG

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: optimize code

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update comment

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

---------

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
SuZhou-Joe added a commit to ruanyl/OpenSearch-Dashboards that referenced this pull request Mar 11, 2024
…roject#6014) (#290)

* feat: consume current workspace in saved objects management and saved objects client



* feat: add unit test



* feat: add unit test for each change



* fix: update snapshot of unit test



* fix: unit test



* fix: unit test



* fix: unit test



* feat: update snapshot



* revert: saved object management changes



* feat: add CHANGELOG



* feat: address some comment



* feat: address comment



* feat: remove useless return



* fix: bootstrap error



* feat: update CHANGELOG



* feat: update CHANGELOG



* feat: optimize code



* feat: update comment



---------

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 11, 2024
* feat: consume current workspace in saved objects management and saved objects client

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: add unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: add unit test for each change

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: update snapshot of unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update snapshot

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* revert: saved object management changes

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: add CHANGELOG

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: address some comment

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: address comment

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: remove useless return

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: bootstrap error

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update CHANGELOG

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update CHANGELOG

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: optimize code

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update comment

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

---------

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
(cherry picked from commit 8f0885d)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
SuZhou-Joe pushed a commit that referenced this pull request Apr 11, 2024
* feat: consume current workspace in saved objects management and saved objects client

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: add unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: add unit test for each change

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: update snapshot of unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: unit test

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update snapshot

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* revert: saved object management changes

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: add CHANGELOG

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: address some comment

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: address comment

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: remove useless return

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* fix: bootstrap error

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update CHANGELOG

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update CHANGELOG

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: optimize code

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

* feat: update comment

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

---------

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
(cherry picked from commit 8f0885d)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Workspace] Consume workspace in saved objects client.
4 participants