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] Fix: keep disallowed types when importing with overwrite #6668

Merged

Conversation

SuZhou-Joe
Copy link
Member

@SuZhou-Joe SuZhou-Joe commented Apr 29, 2024

Description

When importing data sources and configuration into a workspace with overwrite: true, we will prevent the creation of such objects. However, to ensure that objects dependent on the disallowed ones continue to function properly, we must leave their references untouched.

How the changes can keep the references untouched.

When user tries to import with overwrite: true, the logic will go into https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/server/saved_objects/import/check_conflicts.ts#L90, and in order to keep the references untouched, we just need to make sure the logic won't go into the branch of https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/server/saved_objects/import/check_conflicts.ts#L105 , which sets a new id reference to the current item. And the result it is consumed comes from the savedObjectsClient.checkConflicts(), which can be wrapped by savedObjectsClientWrapper mechanism to intercept special logic on specific objects.

Issues Resolved

Screenshot

Before fix

20240429125045381

After fix

20240429125522801

Testing the changes

There is a integration test case, named checkConflicts when importing disallowed types inside src/plugins/workspace/server/saved_objects/integration_tests/saved_objects_wrapper_for_check_workspace_conflict.test.ts to cover this case.

Changelog

  • fix: keep disallowed types when importing with overwrite

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

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

codecov bot commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.81%. Comparing base (460428c) to head (1df34d1).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6668      +/-   ##
==========================================
+ Coverage   67.79%   67.81%   +0.02%     
==========================================
  Files        3413     3413              
  Lines       66755    66770      +15     
  Branches    10861    10864       +3     
==========================================
+ Hits        45254    45282      +28     
+ Misses      18857    18845      -12     
+ Partials     2644     2643       -1     
Flag Coverage Δ
Linux_1 33.21% <100.00%> (+0.02%) ⬆️
Linux_2 55.63% <ø> (ø)
Linux_3 45.24% <ø> (ø)
Linux_4 34.91% <ø> (ø)
Windows_1 33.26% <100.00%> (+0.04%) ⬆️
Windows_2 55.59% <ø> (ø)
Windows_3 45.26% <ø> (ø)
Windows_4 34.91% <ø> (ø)

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.

@opensearch-project opensearch-project deleted a comment from github-actions bot Apr 29, 2024
@opensearch-project opensearch-project deleted a comment from github-actions bot Apr 29, 2024
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
@ruanyl
Copy link
Member

ruanyl commented Apr 29, 2024

we must leave their references untouched.

@SuZhou-Joe could you help me to understand how this PR addressed the above issue?

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
@SuZhou-Joe
Copy link
Member Author

@ruanyl Sure, updated in the description and thanks for calling it out. It requires some context to figure the whole flow out.

const disallowedSavedObjects: SavedObjectsCheckConflictsObject[] = [];
const allowedSavedObjects: SavedObjectsCheckConflictsObject[] = [];
objects.forEach((item) => {
const isImportIntoWorkspace = !!workspaces?.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this line could move out the forEach loop

}

allowedSavedObjects.push(item);
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: return is not needed here

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

+1 @Hailong-am comments>

For clarification, we are talking about keeping the disallowed types in the destination workspace right?

Approving because seems fine to me

@SuZhou-Joe SuZhou-Joe merged commit de6a889 into opensearch-project:main Apr 29, 2024
71 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 29, 2024
…6668)

* fix: keep disallowed types when importing with overwrite

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

* Changeset file for PR #6668 created/updated

* feat: update comment

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

* feat: address some minor concern

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

---------

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit de6a889)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
SuZhou-Joe added a commit that referenced this pull request Apr 30, 2024
…with overwrite (#6673)

* [Workspace] Fix: keep disallowed types when importing with overwrite (#6668)

* fix: keep disallowed types when importing with overwrite

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

* Changeset file for PR #6668 created/updated

* feat: update comment

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

* feat: address some minor concern

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

---------

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit de6a889)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Update src/plugins/workspace/server/saved_objects/integration_tests/saved_objects_wrapper_for_check_workspace_conflict.test.ts

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

---------

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>
Co-authored-by: SuZhou-Joe <suzhou@amazon.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.

None yet

5 participants