ROX-28697: use Walk instead of GetAll in policy by janisz · Pull Request #14747 · stackrox/stackrox

janisz · 2025-03-25T16:57:50Z

Description

Use Walk instead of GetAll in policies

CHANGELOG update is not needed
Documentation is not needed

Testing

inspected CI results

Automated testing

modified existing tests
contributed no automated tests

How I validated my change

CI

Summary by Sourcery

Replace GetAll method with Walk method in policy-related code to improve iteration and memory efficiency

Enhancements:

Refactored policy store and datastore to use Walk method instead of GetAll, reducing memory overhead by iterating over policies without loading all of them into memory at once

Tests:

Updated test cases to use Walk method instead of GetAll
Modified mock generation and test expectations to support the new Walk method

Chores:

Removed GetAll method from policy store interfaces
Updated go:generate directive to remove GetAll-related generation

rhacs-bot · 2025-03-25T17:24:30Z

Images are ready for the commit at 652f9b4.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-423-g652f9b4f42.

codecov · 2025-03-25T17:55:20Z

Codecov Report

Attention: Patch coverage is 43.75000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 48.96%. Comparing base (af2f4e3) to head (652f9b4).
Report is 15 commits behind head on master.

Files with missing lines	Patch %	Lines
central/policy/datastore/datastore_impl.go	58.33%	4 Missing and 1 partial ⚠️
central/policy/datastore/singleton.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #14747   +/-   ##
=======================================
  Coverage   48.96%   48.96%           
=======================================
  Files        2550     2550           
  Lines      187222   187225    +3     
=======================================
+ Hits        91665    91667    +2     
- Misses      88304    88305    +1     
  Partials     7253     7253

Flag	Coverage Δ
go-unit-tests	`48.96% <43.75%> (+<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.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

janisz · 2025-03-27T17:06:51Z

It's draft as it's waiting for:

chore(tests): remove SkipIfPostgresEnabled #14749

Signed-off-by: Tomasz Janiszewski <tomek@redhat.com> # Conflicts: # central/policy/datastore/datastore_impl_test.go # central/policy/datastore/singleton.go # central/policy/store/store.go

Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>

janisz · 2025-04-08T10:34:44Z

Since #14749 (review) is still in review I decided to fix tests here too but not unskip them as they will be unskipped in the other PR with fix over error string that contains id instead of name

sourcery-ai · 2025-04-08T10:36:03Z

Reviewer's Guide by Sourcery

This pull request replaces the GetAll function with the Walk function in the policy datastore. This change improves performance by allowing iteration over policies without loading all of them into memory at once. The pull request also includes updates to the test suite to reflect this change.

Sequence diagram for fetching all policies using Walk

sequenceDiagram
    participant DS as DataStore
    participant S as Store

    DS->>S: Walk(ctx, fn)
    activate S
    loop For each policy in the database
        S->>S: fn(policy)
        activate S
        S-->>DS: error (if any)
        deactivate S
    end
    S-->>DS: error (if any)
    deactivate S

Updated class diagram for the Policy Store

classDiagram
    class Store {
        <<interface>>
        +Search(ctx context.Context, q *v1.Query) ([]search.Result, error)
        +Get(ctx context.Context, id string) (*storage.Policy, bool, error)
        +GetMany(ctx context.Context, ids []string) ([]*storage.Policy, []int, error)
        +Walk(ctx context.Context, fn func(obj *storage.Policy) error) error
        +GetIDs(ctx context.Context) ([]string, error)
        +Upsert(ctx context.Context, obj *storage.Policy) error
        +UpsertMany(ctx context.Context, objs []*storage.Policy) error
        +Delete(ctx context.Context, id string) error
    }

    note for Store "GetAll has been removed and Walk has been added"

File-Level Changes

Change	Details	Files
Replaced `GetAll` calls with `Walk` in the policy datastore.	Replaced `GetAll` with `Walk` in `ImportPolicies` to check for duplicate IDs and names. Replaced `GetAll` with `Walk` in `GetAllPolicies` to retrieve all policies. Adjusted mock store to remove `GetAll` and add `Walk`. Adjusted test assertions and expectations to align with the change from `GetAll` to `Walk`. Added a helper function `eq` to compare policies, excluding the `Categories` field, in test assertions.	`central/policy/datastore/datastore_impl_test.go` `central/policy/store/mocks/store.go` `central/policy/datastore/datastore_impl.go` `central/policy/datastore/singleton.go` `central/policy/store/postgres/store_test.go` `central/policy/store/gen.go` `central/policy/store/store.go` `central/policy/store/postgres/store.go`
Modified test cases to accommodate the change from GetAll to Walk and to improve test coverage.	Removed the `TestReplacingResourceAccess` test case. Updated the `TestImportPolicyDuplicateID` test case to use `Walk` and verify category retrieval. Updated the `TestImportPolicyDuplicateName` test case to use `Walk`. Updated the `TestImportPolicyMixedSuccessAndFailure` test case to use `Walk`. Updated the `TestImportOverwrite` test case to use `Walk` and verify category retrieval and setting. Updated the `TestRemoveScopesAndNotifiers` test case to use `Walk`. Updated the `TestDoesNotRemoveScopesAndNotifiers` test case to use `Walk`.	`central/policy/datastore/datastore_impl_test.go`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!
Generate a plan of action for an issue: Comment @sourcery-ai plan on
an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @janisz - I've reviewed your changes - here's some feedback:

Overall Comments:

It looks like you've removed the GetAll method from the Store interface, but the mock still has it.
The eq matcher in the tests could be improved by using proto.Equal for a more robust comparison.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

janisz requested a review from a team as a code owner March 25, 2025 16:57

github-actions bot added the area/central label Mar 25, 2025

janisz changed the title ~~fix(policy): use Walk instead of GetAll~~ ROX-28697: use Walk instead of GetAll in policy Mar 26, 2025

janisz marked this pull request as draft March 26, 2025 11:29

openshift-ci bot added the do-not-merge/work-in-progress label Mar 26, 2025

janisz force-pushed the policies_get_all branch from a55bdac to 57b0887 Compare March 27, 2025 17:00

rhybrillou approved these changes Apr 3, 2025

View reviewed changes

janisz added 2 commits April 8, 2025 12:19

fix(policy): use Walk instead of GetAll

ead622b

Signed-off-by: Tomasz Janiszewski <tomek@redhat.com> # Conflicts: # central/policy/datastore/datastore_impl_test.go # central/policy/datastore/singleton.go # central/policy/store/store.go

rebase

652f9b4

Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>

janisz force-pushed the policies_get_all branch from 57b0887 to 652f9b4 Compare April 8, 2025 10:31

janisz marked this pull request as ready for review April 8, 2025 10:34

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 8, 2025

sourcery-ai bot reviewed Apr 8, 2025

View reviewed changes

janisz merged commit d174ead into master Apr 9, 2025
90 of 91 checks passed

janisz deleted the policies_get_all branch April 9, 2025 08:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROX-28697: use Walk instead of GetAll in policy#14747

ROX-28697: use Walk instead of GetAll in policy#14747
janisz merged 2 commits intomasterfrom
policies_get_all

janisz commented Mar 25, 2025 •

edited by sourcery-ai bot

Loading

Uh oh!

rhacs-bot commented Mar 25, 2025 •

edited

Loading

Uh oh!

codecov bot commented Mar 25, 2025 •

edited

Loading

Uh oh!

janisz commented Mar 27, 2025

Uh oh!

janisz commented Apr 8, 2025

Uh oh!

sourcery-ai bot commented Apr 8, 2025

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

janisz commented Mar 25, 2025 • edited by sourcery-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Use Walk instead of GetAll in policies

Testing

Automated testing

How I validated my change

Summary by Sourcery

Uh oh!

rhacs-bot commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

janisz commented Mar 27, 2025

Uh oh!

janisz commented Apr 8, 2025

Uh oh!

sourcery-ai bot commented Apr 8, 2025

Reviewer's Guide by Sourcery

Sequence diagram for fetching all policies using Walk

Updated class diagram for the Policy Store

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

janisz commented Mar 25, 2025 •

edited by sourcery-ai bot

Loading

rhacs-bot commented Mar 25, 2025 •

edited

Loading

codecov bot commented Mar 25, 2025 •

edited

Loading