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.

Sign up for GitHub

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

[Security Solution] Invalidate updated rules instead of marking them instantly stale #153529

Closed
wants to merge 2 commits into from
Closed

[Security Solution] Invalidate updated rules instead of marking them instantly stale #153529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd6cc72
invalidate updated rules with different refetchType strategy
maximpn Mar 10, 2023
53f778f
do not mark fetch rule by id query results as instantly stale
maximpn Mar 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions ...ty_solution/public/detection_engine/rule_management/api/hooks/use_bulk_action_mutation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,8 @@ export const useBulkActionMutation = (
if (updatedRules) {
// We have a list of updated rules, no need to invalidate all
updateRulesCache(updatedRules);
} else {
// We failed to receive the list of update rules, invalidate all
invalidateFindRulesQuery();
}
invalidateFindRulesQuery({ refetchType: 'none' });
Comment on lines -60 to +61
Copy link
Contributor

Choose a reason for hiding this comment

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

It introduces a high probability that we will display stale data eventually. Refetch type none prevents hooks that are currently rendered to refetch their data. Therefore if there is no updatedRules in the response, they will continue to display outdated data.

break;
}
case BulkActionType.delete:
Expand Down
3 changes: 0 additions & 3 deletions ..._solution/public/detection_engine/rule_management/api/hooks/use_fetch_rule_by_id_query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,6 @@ export const useFetchRuleByIdQuery = (id: string, options?: UseQueryOptions<Rule
{
...DEFAULT_QUERY_OPTIONS,
...options,
// Mark this query as immediately stale helps to avoid problems related to filtering.
// e.g. enabled and disabled state filter require data update which happens at the backend side
staleTime: 0,
Copy link
Contributor

@xcrzx xcrzx Mar 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I gave it another thought, and keeping the stale time to 0 seems more appropriate. Rules contain dynamic data, such as "last run", "status", etc., that we may not know when it updates. As a result, we should consider the data as always stale and fetch updates each time a data fetching hook mounts. This approach helps avoid inconsistencies when a single rule is displayed under different filters, which could lead to conflicting information. For example, a rule might appear to have been executed 6 minutes ago in one view while displaying an execution time 1 minute ago in another. We can ensure that the information displayed remains consistent and accurate by fetching updates every time a hook mounts.

enabled: !!id,
}
);
Expand Down
24 changes: 11 additions & 13 deletions ...curity_solution/public/detection_engine/rule_management/api/hooks/use_find_rules_query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import type { UseQueryOptions } from '@tanstack/react-query';
import type { InvalidateQueryFilters, UseQueryOptions } from '@tanstack/react-query';
import { useQuery, useQueryClient } from '@tanstack/react-query';
import { useCallback } from 'react';
import { DETECTION_ENGINE_RULES_URL_FIND } from '../../../../../common/constants';
Expand Down Expand Up @@ -54,9 +54,6 @@ export const useFindRulesQuery = (
},
{
...DEFAULT_QUERY_OPTIONS,
// Mark this query as immediately stale helps to avoid problems related to filtering.
// e.g. enabled and disabled state filter require data update which happens at the backend side
staleTime: 0,
...queryOptions,
}
);
Expand All @@ -72,15 +69,16 @@ export const useFindRulesQuery = (
export const useInvalidateFindRulesQuery = () => {
const queryClient = useQueryClient();

return useCallback(() => {
/**
* Invalidate all queries that start with FIND_RULES_QUERY_KEY. This
* includes the in-memory query cache and paged query cache.
*/
queryClient.invalidateQueries(FIND_RULES_QUERY_KEY, {
refetchType: 'active',
});
}, [queryClient]);
return useCallback(
(filters: InvalidateQueryFilters = { refetchType: 'active' }) => {
/**
* Invalidate all queries that start with FIND_RULES_QUERY_KEY. This
* includes the in-memory query cache and paged query cache.
*/
queryClient.invalidateQueries(FIND_RULES_QUERY_KEY, filters);
},
[queryClient]
);
};

/**
Expand Down