chore: Cleanup seeder functions to return full structs instead of IDs#4110
chore: Cleanup seeder functions to return full structs instead of IDs#4110
Conversation
mcstepp
requested review from
Flo4604,
chronark,
imeyer and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
📝 WalkthroughWalkthroughTest helpers and seed utilities now return full db entity structs; tests were updated to capture those structs and use their Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test
participant Harness as TestHarness
participant Seeder as Seeder
participant DB as Database
Note over Test,Harness: Test requests seeded entity
Test->>Harness: CreatePermission(req)
Harness->>Seeder: CreatePermission(ctx, req)
Seeder->>DB: INSERT permission (returns row)
DB-->>Seeder: db row
Seeder-->>Harness: db.Permission (full struct)
Harness-->>Test: db.Permission (full struct)
Note over Test: Test uses permission.ID / permission.Name in subsequent requests/assertions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used🧬 Code graph analysis (6)go/apps/api/routes/v2_keys_reroll_key/200_test.go (1)
go/apps/api/routes/v2_keys_add_permissions/200_test.go (1)
go/pkg/testutil/http.go (2)
go/apps/api/routes/v2_keys_verify_key/200_test.go (1)
go/pkg/testutil/seed/seed.go (6)
go/apps/api/routes/v2_keys_add_roles/200_test.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go/pkg/testutil/seed/seed.go (1)
317-361: Masked insert error when both IdentityID and KeyID are set.The second insert overwrites
err; a first‑insert failure can be hidden.-func (s *Seeder) CreateRatelimit(ctx context.Context, req CreateRatelimitRequest) db.Ratelimit { +func (s *Seeder) CreateRatelimit(ctx context.Context, req CreateRatelimitRequest) db.Ratelimit { ratelimitID := uid.New(uid.RatelimitPrefix) createdAt := time.Now().UnixMilli() - var err error - if req.IdentityID != nil { - err = db.Query.InsertIdentityRatelimit(ctx, s.DB.RW(), db.InsertIdentityRatelimitParams{ + if err := db.Query.InsertIdentityRatelimit(ctx, s.DB.RW(), db.InsertIdentityRatelimitParams{ ID: ratelimitID, WorkspaceID: req.WorkspaceID, IdentityID: sql.NullString{String: *req.IdentityID, Valid: true}, Name: req.Name, Limit: req.Limit, Duration: req.Duration, AutoApply: req.AutoApply, CreatedAt: createdAt, - }) + }); err != nil { + require.NoError(s.t, err) + } } if req.KeyID != nil { - err = db.Query.InsertKeyRatelimit(ctx, s.DB.RW(), db.InsertKeyRatelimitParams{ + if err := db.Query.InsertKeyRatelimit(ctx, s.DB.RW(), db.InsertKeyRatelimitParams{ ID: ratelimitID, WorkspaceID: req.WorkspaceID, KeyID: sql.NullString{String: *req.KeyID, Valid: true}, Name: req.Name, Limit: req.Limit, Duration: req.Duration, AutoApply: req.AutoApply, CreatedAt: createdAt, - }) + }); err != nil { + require.NoError(s.t, err) + } } - - require.NoError(s.t, err)Additionally fix:
- Line 420
CreateRole: useuid.RolePrefixinstead ofuid.PermissionPrefix- Lines 462–463
CreatePermission: assertions should checkreq.Nameandreq.Slugrespectively, notreq.WorkspaceID
🧹 Nitpick comments (4)
go/apps/api/routes/v2_keys_add_roles/200_test.go (1)
139-142: Make assertion order‑independent to avoid flakes.Response order isn’t guaranteed; compare as sets.
Based on learnings
- roleNames := []string{res.Body.Data[0].Name, res.Body.Data[1].Name} - require.Equal(t, []string{"admin_idempotent", "editor_idempotent"}, roleNames) + roleNames := []string{res.Body.Data[0].Name, res.Body.Data[1].Name} + expected := []string{"admin_idempotent", "editor_idempotent"} + require.ElementsMatch(t, expected, roleNames)go/pkg/testutil/seed/seed.go (1)
371-405: Reuse a single createdAt for deterministic returns.Insert and return can diverge by milliseconds. Capture once and reuse.
-func (s *Seeder) CreateIdentity(ctx context.Context, req CreateIdentityRequest) db.Identity { +func (s *Seeder) CreateIdentity(ctx context.Context, req CreateIdentityRequest) db.Identity { + createdAt := time.Now().UnixMilli() ... - err := db.Query.InsertIdentity(ctx, s.DB.RW(), db.InsertIdentityParams{ + err := db.Query.InsertIdentity(ctx, s.DB.RW(), db.InsertIdentityParams{ ID: identityId, ExternalID: req.ExternalID, WorkspaceID: req.WorkspaceID, Environment: "", - CreatedAt: time.Now().UnixMilli(), + CreatedAt: createdAt, Meta: metaBytes, }) ... return db.Identity{ ID: identityId, ExternalID: req.ExternalID, WorkspaceID: req.WorkspaceID, Environment: "", Meta: metaBytes, Deleted: false, - CreatedAt: time.Now().UnixMilli(), + CreatedAt: createdAt, UpdatedAt: sql.NullInt64{Valid: false}, }go/apps/api/routes/v2_keys_whoami/200_test.go (2)
101-105: Swap expected/actual inrequire.Equal.Conventional order is
expected, actualto get clearer diffs.- require.Equal(t, res.Body.Data.KeyId, keyID) + require.Equal(t, keyID, res.Body.Data.KeyId)
174-181: Optional: use test clock for time derivations.Using
h.Clock.Now()avoids wall‑clock flakiness in CI.- futureDate := time.Now().Add(24 * time.Hour).Truncate(time.Hour) + futureDate := h.Clock.Now().Add(24 * time.Hour).Truncate(time.Hour)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
go/apps/api/routes/v2_identities_get_identity/200_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/200_test.go(9 hunks)go/apps/api/routes/v2_keys_add_roles/200_test.go(2 hunks)go/apps/api/routes/v2_keys_get_key/200_test.go(2 hunks)go/apps/api/routes/v2_keys_remove_permissions/200_test.go(7 hunks)go/apps/api/routes/v2_keys_remove_permissions/404_test.go(2 hunks)go/apps/api/routes/v2_keys_reroll_key/200_test.go(2 hunks)go/apps/api/routes/v2_keys_verify_key/200_test.go(3 hunks)go/apps/api/routes/v2_keys_verify_key/412_test.go(2 hunks)go/apps/api/routes/v2_keys_verify_key/multilimit_test.go(5 hunks)go/apps/api/routes/v2_keys_whoami/200_test.go(2 hunks)go/pkg/testutil/http.go(1 hunks)go/pkg/testutil/seed/seed.go(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
go/apps/api/routes/v2_keys_remove_permissions/404_test.go (1)
go/pkg/testutil/seed/seed.go (1)
CreatePermissionRequest(453-458)
go/apps/api/routes/v2_keys_verify_key/multilimit_test.go (1)
go/pkg/testutil/seed/seed.go (1)
CreateKeyRequest(188-208)
go/apps/api/routes/v2_identities_get_identity/200_test.go (2)
go/pkg/db/models_generated.go (1)
Identity(629-638)go/apps/api/openapi/gen.go (1)
Identity(161-173)
go/apps/api/routes/v2_keys_add_roles/200_test.go (1)
go/pkg/testutil/seed/seed.go (1)
CreateRoleRequest(408-414)
go/apps/api/routes/v2_keys_whoami/200_test.go (1)
go/pkg/testutil/seed/seed.go (1)
CreateIdentityRequest(364-369)
go/apps/api/routes/v2_keys_get_key/200_test.go (1)
go/pkg/testutil/seed/seed.go (1)
CreateIdentityRequest(364-369)
go/apps/api/routes/v2_keys_verify_key/200_test.go (1)
go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
go/pkg/testutil/http.go (3)
go/apps/api/integration/harness.go (1)
Harness(34-44)go/pkg/testutil/seed/seed.go (4)
CreateIdentityRequest(364-369)CreateRatelimitRequest(307-315)CreateRoleRequest(408-414)CreatePermissionRequest(453-458)go/pkg/db/models_generated.go (4)
Identity(629-638)Ratelimit(736-747)Role(772-779)Permission(704-712)
go/apps/api/routes/v2_keys_reroll_key/200_test.go (1)
go/pkg/testutil/seed/seed.go (1)
CreateIdentityRequest(364-369)
go/pkg/testutil/seed/seed.go (7)
go/pkg/db/key_role_insert.sql_generated.go (1)
InsertKeyRoleParams(27-32)go/pkg/db/key_permission_insert.sql_generated.go (1)
InsertKeyPermissionParams(27-33)go/pkg/db/models_generated.go (5)
Ratelimit(736-747)Identity(629-638)Environment(618-627)Role(772-779)Permission(704-712)go/pkg/uid/uid.go (2)
RatelimitPrefix(29-29)PermissionPrefix(27-27)go/pkg/db/role_insert.sql_generated.go (1)
InsertRoleParams(30-36)go/pkg/db/role_permission_insert.sql_generated.go (1)
InsertRolePermissionParams(27-32)go/pkg/db/permission_insert.sql_generated.go (1)
InsertPermissionParams(33-40)
go/apps/api/routes/v2_keys_remove_permissions/200_test.go (2)
go/pkg/testutil/seed/seed.go (1)
CreatePermissionRequest(453-458)go/pkg/db/key_permission_insert.sql_generated.go (1)
InsertKeyPermissionParams(27-33)
go/apps/api/routes/v2_keys_add_permissions/200_test.go (1)
go/pkg/testutil/seed/seed.go (1)
CreatePermissionRequest(453-458)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
🔇 Additional comments (19)
go/apps/api/routes/v2_keys_add_roles/200_test.go (1)
98-116: LGTM on switching to struct return (admin.ID).Correct adaptation to seeder changes; params align with db.InsertKeyRoleParams.
go/apps/api/routes/v2_keys_whoami/200_test.go (1)
49-70: LGTM on switching to identity struct and.ID.Matches seeder changes and pointer usage
&identity.IDis correct.go/apps/api/routes/v2_keys_verify_key/412_test.go (2)
52-56: LGTM on IdentityID wiring.Passing
ptr.P(identity.ID)aligns with seeder changes.
70-75: LGTM on error detail checks.Asserting presence of
identity.IDand external ID is appropriate.go/pkg/testutil/http.go (1)
221-235: Code changes verified—no call-site issues detected.All call sites properly access the
.IDfield on returned db structs. Examples verified:
IdentityID: &identity.ID(v2_keys_whoami/200_test.go)IdentityID: ptr.P(identity.ID)(v2_keys_verify_key/200_test.go)Id: identity.ID(v2_keys_verify_key/200_test.go:685)The test codebase correctly handles the return type changes. No fixes or refactoring needed.
go/apps/api/routes/v2_keys_remove_permissions/404_test.go (1)
49-61: LGTM!The refactoring correctly updates the variable from
permissionIDtopermissionand accesses the ID viapermission.ID. The change improves type safety and consistency with the broader PR objective.go/apps/api/routes/v2_keys_reroll_key/200_test.go (1)
47-76: LGTM!The identity variable is now captured as a full object and correctly referenced via
identity.IDwhen constructing the key. This aligns with the PR's objective to return full structs from seeder functions.go/apps/api/routes/v2_keys_get_key/200_test.go (1)
49-69: LGTM!The identity object is correctly captured and its ID is properly accessed via
identity.IDfor theIdentityIDfield. This follows the consistent pattern established throughout the PR.go/apps/api/routes/v2_keys_remove_permissions/200_test.go (2)
117-148: LGTM!The permission objects are correctly captured and their IDs are properly accessed when inserting key-permission associations. The refactoring improves type safety while maintaining the same test logic.
336-384: LGTM!All three permission objects are correctly created and their IDs are properly used in the database insert operations. The pattern is consistent with the other test cases in this file.
go/apps/api/routes/v2_identities_get_identity/200_test.go (1)
380-387: LGTM!The identity object is captured and its ID is correctly used in the request. This change enables the test to access the internal identity ID when needed, which aligns with the PR's goal of returning full structs.
go/apps/api/routes/v2_keys_verify_key/200_test.go (2)
593-613: LGTM!The identity object is correctly captured and its ID is properly used in the key creation. This enables the test to access identity fields if needed in the future.
642-685: LGTM!The identity object is correctly captured and used in two ways: (1) setting
IdentityID: ptr.P(identity.ID)when creating the key, and (2) asserting the response identity matches viaidentity.ID. The refactoring maintains test correctness while improving type safety.go/apps/api/routes/v2_keys_verify_key/multilimit_test.go (2)
112-134: LGTM!The identity object is correctly captured and its ID is properly used when creating the key. This pattern is consistently applied across multiple test cases in this file.
162-184: LGTM!All identity objects throughout the multilimit tests are correctly captured and their IDs are properly accessed via
identity.ID. The refactoring is consistent and maintains test correctness.Also applies to: 217-239, 282-306, 392-414
go/apps/api/routes/v2_keys_add_permissions/200_test.go (4)
12-12: LGTM!The
ptrpackage import is correctly added to support theptr.P()helper for creating pointer values for optional fields likeDescription.
61-96: LGTM!The permission object is correctly captured with proper field initialization using
ptr.P()for the description. The permission's name and ID are correctly accessed throughout the test, improving type safety and reducing the need for intermediate variables.
119-167: LGTM!Both permission objects are correctly created and their names and IDs are properly accessed. The pattern is consistent with the single-permission test case and aligns with the PR's objectives.
248-301: LGTM!The new permission object is correctly created and its name and ID are properly used in the request and assertions. The refactoring maintains test correctness while eliminating the need for separate permission ID variables.
|
lgtm except the things rabbit commented |
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (10/21/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
mcstepp
force-pushed
the
ENG-1921-cleanup-seeder-functions
branch
from
ce6eb52 to
c0ef69f
Compare
3 participants
What does this PR do?
This PR refactors the test seeder functions to return full database structs instead of just IDs, improving type safety and reducing the need for additional database queries in tests.
Changes:
CreateRatelimit,CreateIdentity,CreateRole,CreatePermission) to return completedb.*structs instead of string IDs.IDfield when only the ID is neededMotivation:
Previously, tests that needed information beyond just the ID of a seeded entity had to make additional database queries. By returning the full structs, tests can access all fields directly, making them cleaner and more efficient.
Fixes #3545
Type of change
How should this be tested?
Test Steps:
cd go make test-unitExpected Behavior:
Note: There is a pre-existing race condition in the upstream libopenapi-validator library (unrelated to these changes) that may appear when running tests with -race flag. This is a known issue in the validator library itself.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated