Skip to content

fix: remove go-safecast dependency - #738

Merged
Siddhant-K-code merged 1 commit into
openfga:mainfrom
Siddhant-K-code:fix/remove-go-safecast-dependency
Aug 5, 2026
Merged

fix: remove go-safecast dependency#738
Siddhant-K-code merged 1 commit into
openfga:mainfrom
Siddhant-K-code:fix/remove-go-safecast-dependency

Conversation

@Siddhant-K-code

@Siddhant-K-code Siddhant-K-code commented Aug 5, 2026

Copy link
Copy Markdown
Member

Summary

  • replace go-safecast conversions with explicit math.MaxInt32 bounds checks in shared import-parameter validation
  • remove go-safecast from module and lint configuration
  • add coverage for values exceeding the int32 range, including 32-bit compilation behavior

Fixes #672

Testing

  • go mod verify
  • go test ./...
  • GOARCH=386 go test ./internal/tuple -run TestImportTuplesRejectsValuesOutsideInt32Range -count=1

Summary by CodeRabbit

  • Bug Fixes

    • Added validation to reject import settings that exceed supported 32-bit integer limits.
    • Improved error reporting for invalid tuple import configuration values.
  • Tests

    • Added coverage for out-of-range import settings across supported platforms.

@Siddhant-K-code
Siddhant-K-code requested a review from a team as a code owner August 5, 2026 07:25
Copilot AI lite review requested due to automatic review settings August 5, 2026 07:25
@linux-foundation-easycla

linux-foundation-easycla Bot commented Aug 5, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: Siddhant-K-code / name: Siddhant Khare (3fe6922)

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

ImportTuples now validates tuple and parallel-request limits against math.MaxInt32 before converting them to int32. The change removes go-safecast from the implementation, module requirements, and lint allowlist, and adds overflow validation tests.

Changes

Tuple import validation

Layer / File(s) Summary
Int32 bounds validation and dependency removal
internal/tuple/import.go, internal/tuple/import_test.go, go.mod, .golangci.yaml
ImportTuples rejects values above math.MaxInt32 before direct int32 conversion. Tests cover both limits. The go-safecast dependency and allowlist entry are removed.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: aaguiarz, copilot

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes remove go-safecast, add int32 bounds checks, update dependency configuration, and add focused validation tests.
Out of Scope Changes check ✅ Passed All reviewed changes support dependency removal or verify the new int32 bounds-check behavior.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: removing the go-safecast dependency.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Removes the github.com/rung/go-safecast dependency by replacing safecast.Int32() conversions with explicit upper-bound checks before casting to int32, and adds tests to cover the new validation behavior.

Changes:

  • Replaced safecast.Int32(...) calls with math.MaxInt32 bounds checks and direct int32(...) casts.
  • Removed go-safecast from go.mod, go.sum, and .golangci.yaml allowlist/dep configuration.
  • Added a unit test to ensure values exceeding the int32 range are rejected.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
internal/tuple/import.go Removes go-safecast usage and adds explicit math.MaxInt32 checks before int32 casting.
internal/tuple/import_test.go Adds coverage for rejecting values that exceed int32 bounds.
go.mod Drops the github.com/rung/go-safecast dependency requirement.
go.sum Removes go-safecast checksum entries.
.golangci.yaml Removes go-safecast from the dependency allowlist/config.
Suppressed comments (1)

internal/tuple/import_test.go:38

  • This test hard-codes the numeric value of math.MaxInt32 in the expected error string. Using math.MaxInt32 directly (formatted via strconv) would keep the test aligned with the production error message and avoid a magic number.
			expectedError:       "maxParallelRequests must be at most 2147483647",

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/tuple/import.go Outdated
Comment thread internal/tuple/import_test.go Outdated
Copilot AI review requested due to automatic review settings August 5, 2026 07:28
@Siddhant-K-code
Siddhant-K-code force-pushed the fix/remove-go-safecast-dependency branch from 3fe6922 to efa7051 Compare August 5, 2026 07:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 5, 2026 07:30
@Siddhant-K-code
Siddhant-K-code force-pushed the fix/remove-go-safecast-dependency branch from efa7051 to e82ad2b Compare August 5, 2026 07:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

internal/tuple/import_test.go:33

  • Avoid hard-coding 2147483647 in the expected error string; building it from math.MaxInt32 keeps the test aligned with the production error message if it changes.
	}{
		{
			name:                "max tuples per write",
			maxTuplesPerWrite:   maxInt32PlusOne,
			maxParallelRequests: 1,
			expectedError:       "maxTuplesPerWrite must be at most " + maxInt32,

internal/tuple/import_test.go:39

  • Same as above: derive the expected max value from math.MaxInt32 instead of duplicating the literal, to keep the test resilient to message changes.
		},
		{
			name:                "max parallel requests",
			maxTuplesPerWrite:   1,
			maxParallelRequests: maxInt32PlusOne,
			expectedError:       "maxParallelRequests must be at most " + maxInt32,

Comment thread internal/tuple/import_test.go Outdated
Copilot AI review requested due to automatic review settings August 5, 2026 07:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 5, 2026 07:36
@Siddhant-K-code
Siddhant-K-code force-pushed the fix/remove-go-safecast-dependency branch from e82ad2b to 9316192 Compare August 5, 2026 07:36
@Siddhant-K-code

Copy link
Copy Markdown
Member Author

Addressed the 32-bit compilation comment in 9316192. The test now increments a non-constant int value only after the 32-bit skip. Verified with GOARCH=386 go test ./internal/tuple -run TestImportTuplesRejectsValuesOutsideInt32Range -count=1.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

internal/tuple/import_test.go:48

  • The subtests run in parallel, but the range variable test is captured by the closure. This can cause flaky failures / wrong inputs because test is reused across iterations. Create a per-iteration copy before starting the parallel subtest.
	for _, test := range tests {
		t.Run(test.name, func(t *testing.T) {
			t.Parallel()

@Siddhant-K-code
Siddhant-K-code added this pull request to the merge queue Aug 5, 2026
Merged via the queue into openfga:main with commit 379f941 Aug 5, 2026
24 checks passed
@Siddhant-K-code
Siddhant-K-code deleted the fix/remove-go-safecast-dependency branch August 5, 2026 14:00
@openfga-releaser-bot openfga-releaser-bot Bot mentioned this pull request Aug 10, 2026
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.

fix: remove go-safecast dependency

3 participants