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

[Cloud Posture] fix findings table sort casing bug #148529

Merged
merged 20 commits into from
Jan 15, 2023

Conversation

ofiriro3
Copy link
Contributor

@ofiriro3 ofiriro3 commented Jan 9, 2023

Summary

Currently our cloud-security-posture Findings page sort some fields with regard to case sensitivity, we would like them to be sorted in alphabetical order regardless of their upper case/lower case characters.

Related Findings fields

  • rule.section
  • resource.name
  • resource.type

Related issues:

@ofiriro3 ofiriro3 marked this pull request as ready for review January 9, 2023 11:54
@ofiriro3 ofiriro3 requested a review from a team as a code owner January 9, 2023 11:54
@ofiriro3 ofiriro3 requested a review from orouz January 9, 2023 11:54
@orouz orouz added release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related ci:cloud-deploy Create or update a Cloud deployment v8.7.0 labels Jan 9, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

@ofiriro3 ofiriro3 changed the title Sorting bug [Cloud Posture] fix findings table sort casing bug Jan 9, 2023
return { [sort.field]: sort.direction };
};

const requiredSortingByPainlessScript = ['rule.section', 'resource.name', 'resource.type'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @ofiriro3,

Forgive me if I need more context here. Would you mind adding a comment about why these fields require such a sorting instead of using a more native Elasticsearch solution like normalizer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @opauloh.

We had a long discussion about how should we solve the sorting problem.
One of the solutions that I suggested was indeed a normalizer, in the end, it was decided to use a painless script.

Long story short, we didn't want to change the schema/mapping.

You are more than welcome to read the discussion ->

Copy link
Contributor

@opauloh opauloh left a comment

Choose a reason for hiding this comment

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

code LGTM, just left a suggestion

Copy link
Contributor

@orouz orouz left a comment

Choose a reason for hiding this comment

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

looking good! 🥇

left some comments for your discretion


// We need to use a dataset for the tests to run
// We intentionally make some fields start with a capital letter to test that the query bar is case-insensitive/case-sensitive
const data = [
Copy link
Contributor

Choose a reason for hiding this comment

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

defining the data is now longer and very focused on sorting. i think we can narrow it down a bit. for example, using single chars or shorter sentences would achieve the same result. and instead of doing chance.integer() % 2 ... multiple times, we can do it once and do the same for the text casing.

@kibana-ci
Copy link
Collaborator

kibana-ci commented Jan 15, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
cloudSecurityPosture 139.6KB 139.8KB +180.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ofiriro3 ofiriro3 merged commit 47789da into elastic:main Jan 15, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 15, 2023
@ofiriro3 ofiriro3 deleted the sorting_bug branch January 15, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:cloud-deploy Create or update a Cloud deployment release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Kibana - Findings - "CIS Section" column sorting is case sensitive
6 participants