Skip to content

Commit

Permalink
[Cloud Security] fix incorrect number of groups when there is a null …
Browse files Browse the repository at this point in the history
…group (#207360)

## Summary
This PR fixes group by logic regarding calculation of groups count when
there is a null group.

## Video Recording with the fix

https://github.com/user-attachments/assets/14e82bf3-b8d3-4287-8f90-04372be23cf2

## BUG description
The checking of null group is scoped to the findings on a specific page,
for example if we have two pages and the null group appears on the
second page the number of groups will not be correct on the first page
because the null group of was subtracted from the total number of
groups. On the second page the number is correct because the null group
was fetched in the second page.

### Closes
- #188138

### Definition of done
- [x] number of groups should be correct across all pages

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] The PR description includes the appropriate Release Notes section,
and the correct release_note:* label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
  • Loading branch information
alexreal1314 authored Jan 23, 2025
1 parent a97e8af commit a5051b4
Show file tree
Hide file tree
Showing 9 changed files with 204 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,9 @@ export const mockGroupingProps = {
unitsCountWithoutNull: {
value: 14,
},
nullGroupItems: {
doc_count: 1,
},
},
groupingId: 'test-grouping-id',
isLoading: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ describe('Grouping', () => {
);
expect(screen.getByTestId('group-count').textContent).toBe('3 groups');
});

it('calls custom groupsUnit callback correctly', () => {
// Provide a custom groupsUnit function in testProps
const customGroupsUnit = jest.fn(
Expand All @@ -223,5 +224,59 @@ describe('Grouping', () => {
expect(customGroupsUnit).toHaveBeenCalledWith(3, testProps.selectedGroup, true);
expect(screen.getByTestId('group-count').textContent).toBe('3 custom units');
});

it('calls custom groupsUnit callback with hasNullGroup = false and null group in current page', () => {
const customGroupsUnit = jest.fn(
(n, parentSelectedGroup, hasNullGroup) => `${n} custom units`
);

const customProps = {
...testProps,
groupsUnit: customGroupsUnit,
data: {
...testProps.data,
nullGroupItems: {
...testProps.data.nullGroupItems,
doc_count: 0,
},
},
};

render(
<I18nProvider>
<Grouping {...customProps} />
</I18nProvider>
);

expect(customGroupsUnit).toHaveBeenCalledWith(3, testProps.selectedGroup, true);
expect(screen.getByTestId('group-count').textContent).toBe('3 custom units');
});
});

it('calls custom groupsUnit callback with hasNullGroup = true and no null group in current page', () => {
const customGroupsUnit = jest.fn((n, parentSelectedGroup, hasNullGroup) => `${n} custom units`);

const customProps = {
...testProps,
groupsUnit: customGroupsUnit,
data: {
...testProps.data,
groupByFields: {
...testProps.data.groupByFields,
buckets: testProps?.data?.groupByFields?.buckets?.map(
(bucket, index) => (index === 2 ? { ...bucket, isNullGroup: undefined } : bucket) as any
),
},
},
};

render(
<I18nProvider>
<Grouping {...customProps} />
</I18nProvider>
);

expect(customGroupsUnit).toHaveBeenCalledWith(3, testProps.selectedGroup, true);
expect(screen.getByTestId('group-count').textContent).toBe('3 custom units');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,15 @@ const GroupingComponent = <T,>({

const groupCount = useMemo(() => data?.groupsCount?.value ?? 0, [data?.groupsCount?.value]);
const groupCountText = useMemo(() => {
const hasNullGroup =
const hasNullGroupInCurrentPage =
data?.groupByFields?.buckets?.some(
(groupBucket: GroupingBucket<T>) => groupBucket.isNullGroup
) || false;

return `${groupsUnit(groupCount, selectedGroup, hasNullGroup)}`;
}, [data?.groupByFields?.buckets, groupCount, groupsUnit, selectedGroup]);
const hasNullGroup = Boolean(data?.nullGroupItems?.doc_count);

return `${groupsUnit(groupCount, selectedGroup, hasNullGroupInCurrentPage || hasNullGroup)}`;
}, [data?.groupByFields?.buckets, data?.nullGroupItems, groupCount, groupsUnit, selectedGroup]);

const groupPanels = useMemo(
() =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ export interface RootAggregation<T> {
unitsCountWithoutNull?: {
value?: number | null;
};
nullGroupItems?: {
doc_count?: number;
};
}

export type ParsedRootAggregation<T> = RootAggregation<T> & {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export const MISCONFIGURATIONS_GROUPS_UNIT = (
});
default:
return i18n.translate('xpack.csp.findings.groupUnit', {
values: { groupCount: totalCount },
values: { groupCount },
defaultMessage: `{groupCount} {groupCount, plural, =1 {group} other {groups}}`,
});
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import React from 'react';
import { renderHook } from '@testing-library/react';
import { useLatestFindingsGrouping } from './use_latest_findings_grouping';
import { useCloudSecurityGrouping } from '../../../components/cloud_security_grouping';
import { useDataViewContext } from '../../../common/contexts/data_view_context';
import { useGetCspBenchmarkRulesStatesApi } from '@kbn/cloud-security-posture/src/hooks/use_get_benchmark_rules_state_api';
import { getGroupingQuery } from '@kbn/grouping';
import { useGroupedFindings } from './use_grouped_findings';

jest.mock('../../../components/cloud_security_grouping');
jest.mock('../../../common/contexts/data_view_context');
jest.mock('@kbn/cloud-security-posture/src/hooks/use_get_benchmark_rules_state_api');
jest.mock('@kbn/grouping', () => ({
getGroupingQuery: jest.fn().mockImplementation((params) => {
return {
query: { bool: {} },
};
}),
parseGroupingQuery: jest.fn().mockReturnValue({}),
}));
jest.mock('./use_grouped_findings');

describe('useLatestFindingsGrouping', () => {
const mockGroupPanelRenderer = (
selectedGroup: string,
fieldBucket: any,
nullGroupMessage?: string,
isLoading?: boolean
) => <div>Mock Group Panel Renderer</div>;
const mockGetGroupStats = jest.fn();

beforeEach(() => {
(useCloudSecurityGrouping as jest.Mock).mockReturnValue({
grouping: { selectedGroups: ['cloud.account.id'] },
});
(useDataViewContext as jest.Mock).mockReturnValue({ dataView: {} });
(useGetCspBenchmarkRulesStatesApi as jest.Mock).mockReturnValue({ data: {} });
(useGroupedFindings as jest.Mock).mockReturnValue({
data: {},
isFetching: false,
});
});

it('calls getGroupingQuery with correct rootAggregations', () => {
renderHook(() =>
useLatestFindingsGrouping({
groupPanelRenderer: mockGroupPanelRenderer,
getGroupStats: mockGetGroupStats,
groupingLevel: 0,
groupFilters: [],
selectedGroup: 'cloud.account.id',
})
);

expect(getGroupingQuery).toHaveBeenCalledWith(
expect.objectContaining({
rootAggregations: [
{
failedFindings: {
filter: {
term: {
'result.evaluation': { value: 'failed' },
},
},
},
passedFindings: {
filter: {
term: {
'result.evaluation': { value: 'passed' },
},
},
},
nullGroupItems: {
missing: { field: 'cloud.account.id' },
},
},
],
})
);
});

it('calls getGroupingQuery without nullGroupItems when selectedGroup is "none"', () => {
renderHook(() =>
useLatestFindingsGrouping({
groupPanelRenderer: mockGroupPanelRenderer,
getGroupStats: mockGetGroupStats,
groupingLevel: 0,
groupFilters: [],
selectedGroup: 'none',
})
);

expect(getGroupingQuery).toHaveBeenCalledWith(
expect.objectContaining({
rootAggregations: [
{
failedFindings: {
filter: {
term: {
'result.evaluation': { value: 'failed' },
},
},
},
passedFindings: {
filter: {
term: {
'result.evaluation': { value: 'passed' },
},
},
},
},
],
})
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,11 @@ export const useLatestFindingsGrouping = ({
},
},
},
...(!isNoneGroup([currentSelectedGroup]) && {
nullGroupItems: {
missing: { field: currentSelectedGroup },
},
}),
},
],
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,15 @@ export const useLatestVulnerabilitiesGrouping = ({
sort: [{ groupByField: { order: 'desc' } }],
statsAggregations: getAggregationsByGroupField(currentSelectedGroup),
runtimeMappings: getRuntimeMappingsByGroupField(currentSelectedGroup),
rootAggregations: [
{
...(!isNoneGroup([currentSelectedGroup]) && {
nullGroupItems: {
missing: { field: currentSelectedGroup },
},
}),
},
],
});

const { data, isFetching } = useGroupedVulnerabilities({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export const VULNERABILITIES_GROUPS_UNIT = (
});
default:
return i18n.translate('xpack.csp.vulnerabilities.groupUnit', {
values: { groupCount: totalCount },
values: { groupCount },
defaultMessage: `{groupCount} {groupCount, plural, =1 {group} other {groups}}`,
});
}
Expand Down

0 comments on commit a5051b4

Please sign in to comment.