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

Fix object empty check and minor perf issue in query editor extensions #7077

Merged
merged 9 commits into from
Jun 28, 2024

Conversation

joshuali925
Copy link
Member

@joshuali925 joshuali925 commented Jun 20, 2024

Description

Follow up for #7034, this PR

@AMoo-Miki I didn't use the object check '[object Object]' !== Object.prototype.toString.call(configMap). I don't know what access user has, but if an attacker can arbitrarily alter configMap, the code will still break with something like

        configMap={
          new Proxy(
            {},
            {
              ownKeys(target) {
                throw new Error('Accessing keys is not allowed.');
              },
            }
          )
        }

Given that our code creates the config map here, I think relying on static type check is enough, but feel free to comment if otherwise.

private queryEditorExtensionMap: Record<string, QueryEditorExtensionConfig> = {};

No test changes because this PR does not change code behavior.

Issues Resolved

Screenshot

Testing the changes

Changelog

  • fix: Fix object empty check and minor perf issue in query editor extensions

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: Joshua Li <joshuali925@gmail.com>
Copy link
Contributor

ℹ️ Manual Changeset Creation Reminder

Please ensure manual commit for changeset file 7077.yml under folder changelogs/fragments to complete this PR.

If you want to use the available OpenSearch Changeset Bot App to avoid manual creation of changeset file you can install it in your forked repository following this link.

For more information about formatting of changeset files, please visit OpenSearch Auto Changeset and Release Notes Tool.

QueryEditorExtensions requires the container divs to be mounted first,
but in the previous implementation, extensions will be mounted first and
it relied on rerendering of queryEditorTopRow. Moving them into query
editor fixes the render order and ensures refs are set.

Signed-off-by: Joshua Li <joshuali925@gmail.com>
Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

1 similar comment
Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link

codecov bot commented Jun 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.45%. Comparing base (170ac61) to head (1de91d9).
Report is 363 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7077      +/-   ##
==========================================
- Coverage   67.45%   67.45%   -0.01%     
==========================================
  Files        3448     3448              
  Lines       67957    67964       +7     
  Branches    11055    11057       +2     
==========================================
+ Hits        45843    45845       +2     
- Misses      19443    19446       +3     
- Partials     2671     2673       +2     
Flag Coverage Δ
Linux_1 33.10% <ø> (-0.01%) ⬇️
Linux_2 55.06% <ø> (+<0.01%) ⬆️
Linux_3 45.24% <100.00%> (-0.02%) ⬇️
Linux_4 34.83% <ø> (-0.01%) ⬇️
Windows_1 33.12% <ø> (-0.01%) ⬇️
Windows_2 55.01% <ø> (+<0.01%) ⬆️
Windows_3 45.25% <100.00%> (-0.01%) ⬇️
Windows_4 34.83% <ø> (-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.

@@ -92,6 +93,9 @@ export default class QueryEditorUI extends Component<Props, State> {
private services = this.props.opensearchDashboards.services;
private componentIsUnmounting = false;
private queryEditorDivRefInstance: RefObject<HTMLDivElement> = createRef();
Copy link
Member

@kavilla kavilla Jun 22, 2024

Choose a reason for hiding this comment

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

nit: tbh I think I carried this over from the query string input component and it was used for attaching suggestions. I wonder then if we should just delete this then since i think the header ref more or less solves it.
but not a blocker for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed the handleListUpdate related stuff in 5b3e358

@@ -120,6 +124,30 @@ export default class QueryEditorUI extends Component<Props, State> {
});
};

private renderQueryEditorExtensions() {
Copy link
Member

Choose a reason for hiding this comment

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

thanks! nice clean up!

/>
</EuiFlexItem>
);
}

function renderQueryEditorExtensions() {
Copy link
Member

Choose a reason for hiding this comment

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

this is a good clean up. that i need to do after restructuring. I have a clean up task in #6957 but some of the render functions i added are in the wrong location and that's cause i tried to not touch query string input. so thanks for starting the clean up!

kavilla
kavilla previously approved these changes Jun 28, 2024
@github-actions github-actions bot added the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Jun 28, 2024
kavilla
kavilla previously approved these changes Jun 28, 2024
Copy link
Contributor

❌ Entry Too Long

Entry is 152 characters long, which is 52 characters longer than the maximum allowed length of 100 characters. Please revise your entry to be within the maximum length.

@github-actions github-actions bot added failed changeset and removed Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry labels Jun 28, 2024
abbyhu2000
abbyhu2000 previously approved these changes Jun 28, 2024
@kavilla kavilla merged commit b85e177 into opensearch-project:main Jun 28, 2024
66 of 67 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-7077-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b85e1775c4c1b46f3a04b4e63ab60943fad318a5
# Push it to GitHub
git push --set-upstream origin backport/backport-7077-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7077-to-2.x.

opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 1, 2024
#7077)

Follow up for #7034, this PR
- addresses #7034 (comment)
- addresses #7034 (comment)
- fixes render order
   - QueryEditorExtensions requires the container divs to be mounted first,
but in the previous implementation, extensions will be mounted first and
it relied on rerendering of queryEditorTopRow. Moving them into query
editor fixes the render order and ensures refs are set.

@AMoo-Miki I didn't use the object check `'[object Object]' !== Object.prototype.toString.call(configMap)`. I don't know what access user has, but if an attacker can arbitrarily alter `configMap`, the code will still break with something like
```tsx
        configMap={
          new Proxy(
            {},
            {
              ownKeys(target) {
                throw new Error('Accessing keys is not allowed.');
              },
            }
          )
        }
```

Given that our code creates the config map here, I think relying on static type check is enough, but feel free to comment if otherwise.
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/7f0e39eb9809c95b98069cc971611edc2cbbc62b/src/plugins/data/public/ui/ui_service.ts#L31

Signed-off-by: Joshua Li <joshuali925@gmail.com>
(cherry picked from commit b85e177)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kavilla pushed a commit that referenced this pull request Jul 1, 2024
#7077) (#7140)

Follow up for #7034, this PR
- addresses #7034 (comment)
- addresses #7034 (comment)
- fixes render order
   - QueryEditorExtensions requires the container divs to be mounted first,
but in the previous implementation, extensions will be mounted first and
it relied on rerendering of queryEditorTopRow. Moving them into query
editor fixes the render order and ensures refs are set.

@AMoo-Miki I didn't use the object check `'[object Object]' !== Object.prototype.toString.call(configMap)`. I don't know what access user has, but if an attacker can arbitrarily alter `configMap`, the code will still break with something like
```tsx
        configMap={
          new Proxy(
            {},
            {
              ownKeys(target) {
                throw new Error('Accessing keys is not allowed.');
              },
            }
          )
        }
```

Given that our code creates the config map here, I think relying on static type check is enough, but feel free to comment if otherwise.
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/7f0e39eb9809c95b98069cc971611edc2cbbc62b/src/plugins/data/public/ui/ui_service.ts#L31


(cherry picked from commit b85e177)

Signed-off-by: Joshua Li <joshuali925@gmail.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>
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.

5 participants