Skip to content

Conversation

@aaguiarz
Copy link
Member

@aaguiarz aaguiarz commented Jan 17, 2026

Description

What problem is being solved?

Fixes #618

How is it being solved?

It batches assertions in groups of 100.

What changes are made to solve it?

References

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

Summary by CodeRabbit

  • New Features

    • Assertion imports now process in batches, optimizing handling of large import volumes.
  • Tests

    • Added test coverage for batched assertion imports to ensure correct behavior with large datasets.
  • Refactor

    • Optimized memory allocation and improved code efficiency across import and test utilities.

✏️ Tip: You can customize this high-level summary in your review settings.

@aaguiarz aaguiarz requested a review from a team as a code owner January 17, 2026 01:18
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 17, 2026

Walkthrough

This PR addresses a limitation where importing store files with more than 100 assertions would fail. The changes implement batching of assertion imports in groups of 100, precompute total assertion counts for efficient slice allocation, introduce comprehensive test coverage for batching behavior, and apply performance-oriented slice preallocation optimizations across test utilities.

Changes

Cohort / File(s) Summary
Assertion batching implementation
cmd/store/import.go, cmd/store/import_test.go
Added batching logic in getCheckAssertions to split assertions into groups of maxAssertionsPerWrite (100). Precomputes totalAssertions to preallocate slice capacity. New test TestImportStoreWithBatchedAssertions validates batching with 150 assertions split into two batches. Introduced test constants testModelID and testStoreID, and helper setupBatchedWriteAssertionsMock to configure mock expectations for multiple batched calls. Updated existing tests to use new constants.
Test utility slice preallocation
internal/storetest/localtest.go, internal/storetest/remotetest.go
Replaced empty slice initializations with preallocated slices in RunLocalCheckTest, RunLocalListObjectsTest, RunLocalListUsersTest, RunRemoteCheckTest, RunRemoteListObjectsTest, RunRemoteListUsersTest, and RunRemoteTest. Capacity calculated based on product of input dimensions (e.g., len(users)\*len(objects)\*len(assertions)). No functional logic changes.
String parsing optimization
internal/storetest/testresult.go
Replaced strings.Index and slicing with strings.Cut to locate "# Test Summary #" header in FriendlyBody. Functionally equivalent but more idiomatic.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

codex

Suggested reviewers

  • Siddhant-K-code
  • rhamzeh
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Write assertions in batches' clearly and concisely describes the main change: implementing batching functionality for assertions during import operations.
Linked Issues check ✅ Passed The PR successfully implements batching of assertions into groups of 100 as required by issue #618, addressing the server validation error for large imports.
Out of Scope Changes check ✅ Passed All changes are directly related to the batching objective: assertion batching logic, supporting test infrastructure, and slice preallocation optimizations for performance.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@dosubot
Copy link

dosubot bot commented Jan 17, 2026

Related Documentation

Checked 6 published document(s) in 1 knowledge base(s). No updates required.

How did I do? Any feedback?  Join Discord

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failure when importing a store file with more than 100 assertions

2 participants