refactor: make unmarshalling easier and override json to RawMessage #4145
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
ogzhanolguncu
changed the title
📝 WalkthroughWalkthroughAdds a generic helper UnmarshalNullableJSONTo[T any] to centralize defensive JSON decoding and threads a logger through KeyData construction; replaces many inline json.Unmarshal calls in DB and API handlers with the helper and updates ToKeyData and its callers to accept a logger parameter. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant API as API Handler
participant DB as db package
rect rgba(214,234,248,0.6)
API->>DB: Call ToKeyData(row, logger)
DB->>DB: For each JSON column\ncall UnmarshalNullableJSONTo[T](field, logger)
DB-->>API: Return KeyData (decoded fields)
end
rect rgba(233,245,227,0.6)
API->>DB: UnmarshalNullableJSONTo[map[string]any](metaBytes, logger)
DB-->>API: map[string]any or zero-value (logs on issues)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
[pre_merge_check_pass] The implementation successfully translates the requirements from issue #3789 into working code without requiring RawMessage or sqlc plugin modifications, making it a practical solution to the underlying problem. The vast majority of changes are directly related to the PR's stated objectives of simplifying JSON unmarshalling and improving type safety. All modifications to database packages and handler files involve either introducing the ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)go/apps/api/routes/v2_identities_list_identities/handler.go (2)
⏰ 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)
🔇 Additional comments (3)
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 |
ogzhanolguncu
requested review from
Flo4604,
chronark,
imeyer and
perkinsjr
as code owners
|
Using json.rawmessage with inserts is a pain, that's why we are using []byte in the first place |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
go/pkg/db/custom_types.go(2 hunks)go/pkg/db/key_data.go(2 hunks)go/pkg/db/key_find_for_verification.sql_generated.go(2 hunks)go/pkg/db/key_find_live_by_hash.sql_generated.go(2 hunks)go/pkg/db/key_find_live_by_id.sql_generated.go(2 hunks)go/pkg/db/key_list_by_key_auth_id.sql_generated.go(2 hunks)go/pkg/db/key_list_live_by_auth_id.sql_generated.go(2 hunks)go/pkg/db/models_generated.go(4 hunks)go/pkg/db/sqlc.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
go/pkg/db/key_data.go (1)
go/pkg/db/custom_types.go (4)
UnmarshalJSONArray(51-66)RoleInfo(11-15)PermissionInfo(17-22)RatelimitInfo(24-32)
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
go/pkg/db/models_generated.go (1)
Workspace(814-832)
go/pkg/db/key_list_by_key_auth_id.sql_generated.go (1)
go/pkg/db/models_generated.go (2)
Key(640-664)EncryptedKey(609-616)
go/pkg/db/key_find_live_by_id.sql_generated.go (1)
go/pkg/db/models_generated.go (4)
Api(514-525)KeyAuth(666-677)Workspace(814-832)EncryptedKey(609-616)
⏰ 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). (3)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
🔇 Additional comments (18)
go/pkg/db/sqlc.json (1)
36-43: LGTM - Idiomatic JSON handling.The override mapping from JSON database type to
json.RawMessageis the idiomatic Go approach for representing JSON data. Whilejson.RawMessageis essentially[]byte, it explicitly signals JSON intent and integrates better with theencoding/jsonpackage.go/pkg/db/key_find_for_verification.sql_generated.go (2)
8-12: Generated code aligns with configuration.The addition of
encoding/jsonimport and the updatedIdentityMetafield type tojson.RawMessagecorrectly reflect the sqlc configuration override.
93-120: Type mapping correctly applied.The struct correctly uses
json.RawMessageforIdentityMeta(line 116) while aggregated fields likeRoles,Permissions, andRatelimitsremain asinterface{}since they're computed columns from subqueries, not actual database columns. This aligns with the PR's stated approach.go/pkg/db/key_find_live_by_id.sql_generated.go (2)
8-12: Consistent type handling.Import and type changes align with the project-wide JSON handling refactor.
106-142: Struct types correctly updated.The
IdentityMetafield now usesjson.RawMessage(line 135), consistent with the sqlc override. Aggregated relationship fields (Roles,Permissions,RolePermissions,Ratelimits) appropriately remain asinterface{}per the design constraints.go/pkg/db/key_list_by_key_auth_id.sql_generated.go (2)
8-12: Import added for JSON handling.Consistent with the JSON type refactor across generated files.
41-48: Type mapping applied correctly.The
IdentityMetafield usesjson.RawMessage(line 45), matching the global sqlc override for JSON columns.go/pkg/db/key_list_live_by_auth_id.sql_generated.go (2)
8-12: Import addition supports JSON handling.Consistent with other generated files in this refactor.
117-150: Field types correctly mapped.The
IdentityMetafield (line 143) correctly usesjson.RawMessage. Aggregated fields remaininterface{}as expected for computed JSON columns.go/pkg/db/key_find_live_by_hash.sql_generated.go (2)
8-12: JSON import added.Supports the
json.RawMessagetype used in the struct.
105-141: Type mappings consistent.The
IdentityMetafield (line 134) usesjson.RawMessage. Aggregated JSON columns appropriately remaininterface{}.go/pkg/db/custom_types.go (1)
3-7: Import added for JSON unmarshalling.The
encoding/jsonpackage supports the newUnmarshalJSONArrayhelper function.go/pkg/db/key_data.go (2)
84-88: Cleaner JSON array deserialization.Replacing nil initialization with
UnmarshalJSONArraycalls centralizes the unmarshalling logic and ensures consistent handling of JSON-aggregated fields. The helper always returns a valid (non-nil) slice, eliminating potential nil pointer issues.
134-138: Consistent pattern applied.Both
buildKeyDataandbuildKeyDataFromKeyAuthnow use the sameUnmarshalJSONArrayapproach for deserializing relationship fields, improving maintainability.go/pkg/db/models_generated.go (4)
555-567: Change from[]bytetojson.RawMessageforAuditLogTarget.Metais verified as safe.Downstream analysis found only three references to
AuditLogTargetin test code (ingo/apps/api/routes/v2_ratelimit_limit/200_test.go), accessing.Type,.ID, and.Name.Stringfields—not theMetafield. This confirms no breaking changes or required updates. Thejson.RawMessagetype is already established in the codebase as the standard pattern for JSON columns, making this change safe and correct.
629-638: LGTM! Type change verified across all query result structs.The change to
json.RawMessageforIdentity.Metais correct and consistently applied. All query result structIdentityMetafields have been updated to match (verified in 5 generated SQL query files), and all downstream usages in handlers and tests are compatible sincejson.RawMessageis a standard Go type alias for[]byte.
527-543: LGTM! No breaking changes found.The change from
[]bytetojson.RawMessageis compatible with all downstream code. The type flow is correct:
- Source
auditlog.AuditLog.ActorMetaismap[string]any- Converted via
json.Marshal()to[]byteininsert.go:69- Implicitly assigned to
db.AuditLog.ActorMetaasjson.RawMessage(which is defined astype RawMessage []byte)This is the idiomatic pattern for JSON database columns. All SQL-generated code and service code handle the field correctly without modification.
814-832: No breaking changes—field is unused in codebase.The
Subscriptionsfield is not referenced anywhere in the application code, tests, or migrations. Comprehensive searches across all Go files (excluding generated files) returned zero results for field access, struct initialization, or serialization patterns. The change from[]bytetojson.RawMessageis safe and idiomatic—both types are compatible at the database level, andjson.RawMessageis the Go standard for deferred JSON parsing. Since this is a sqlc-generated model file, the type change likely originates from an updated type mapping in the generation configuration, not manual code modification.Likely an incorrect or invalid review comment.
Wanna elaborate? |
|
See the error messages in the failing tests
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
That's not what I meant why don't we stick to that after fixing those errors? Whats the trade off of []byte vs raw.JsonMessage? Btw, I'm not saying you are wrong, but if its more idiomatic to use raw.Json thingy I say let's fix the issues and roll with this one |
|
ah I remember why we use []byte json.rawmessage doesn't support null JSON columns json.RawMessage does not support NULL · Issue #129 · sqlc-dev/sqlc So you will have to work around this |
|
@Flo4604 I'll check again if its really causing too much headache i'll revert it back to []byte and let you know 🫡 . Thanks for the explanation |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
103-118: Consider adding explicit NULL metadata test coverage.Given the PR's focus on improving NULL JSON column handling (see PR comments about
json.RawMessageNULL support issues), consider adding a test that creates an identity without metadata and explicitly verifies thatidentity.Metacan be safely unmarshaled or is properly represented as NULL/empty.Currently, this test creates an identity without metadata but doesn't verify the Meta field behavior.
Add a test case that verifies NULL metadata handling:
t.Run("create identity without metadata and verify NULL handling", func(t *testing.T) { externalTestID := uid.New("test_external_id") req := handler.Request{ExternalId: externalTestID} res := testutil.CallRoute[handler.Request, handler.Response](h, route, headers, req) require.Equal(t, 200, res.Status) identity, err := db.Query.FindIdentityByExternalID(ctx, h.DB.RO(), db.FindIdentityByExternalIDParams{ WorkspaceID: h.Resources().UserWorkspace.ID, ExternalID: externalTestID, Deleted: false, }) require.NoError(t, err) // Verify NULL or empty metadata is handled correctly dbMeta, err := db.UnmarshalNullableJSONTo[map[string]any](&identity.Meta) require.NoError(t, err) // Assert expected behavior for NULL/empty metadata // (adjust assertion based on your NullJSON implementation) })go/pkg/db/sqlc.json (1)
36-55: Consider a second override for non-nullable JSON → json.RawMessage.To avoid type fragmentation and keep non-nullable JSON fields strongly typed, add an override for nullable=false mapping to encoding/json.RawMessage.
Apply:
"overrides": [ { "db_type": "json", "go_type": { "type": "NullJSON", "package": "dbtype", "import": "github.com/unkeyed/unkey/go/pkg/db/types" }, "nullable": true }, + { + "db_type": "json", + "go_type": { + "type": "RawMessage", + "package": "json", + "import": "encoding/json" + }, + "nullable": false + }, { "column": "permissions.description", "go_type": { "type": "NullString", "package": "dbtype", "import": "github.com/unkeyed/unkey/go/pkg/db/types" }, "nullable": true } ]go/pkg/db/key_data.go (1)
84-87: LGTM! Cleaner JSON unmarshalling with centralized helpers.The refactoring replaces inline JSON unmarshalling with
UnmarshalJSONArrayTo, which provides consistent error handling (returning empty slices on failure) across all relationship fields. This improves maintainability and reduces code duplication.Also applies to: 134-137
go/apps/api/routes/v2_keys_verify_key/handler.go (1)
24-27: LGTM! Improved code organization.Grouping type aliases into a type block is more idiomatic and cleaner.
go/apps/api/routes/v2_identities_list_identities/handler.go (1)
19-22: LGTM! Improved code organization.Grouping type aliases is more idiomatic and consistent with other handlers in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
go/apps/api/routes/v2_apis_list_keys/handler.go(1 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/200_test.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/handler.go(2 hunks)go/apps/api/routes/v2_keys_get_key/handler.go(1 hunks)go/apps/api/routes/v2_keys_verify_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_whoami/handler.go(1 hunks)go/pkg/db/custom_types.go(2 hunks)go/pkg/db/identity_find_with_ratelimits.sql_generated.go(2 hunks)go/pkg/db/key_data.go(2 hunks)go/pkg/db/key_data_test.go(2 hunks)go/pkg/db/key_find_for_verification.sql_generated.go(2 hunks)go/pkg/db/key_find_live_by_hash.sql_generated.go(2 hunks)go/pkg/db/key_find_live_by_id.sql_generated.go(2 hunks)go/pkg/db/key_list_by_key_auth_id.sql_generated.go(2 hunks)go/pkg/db/key_list_live_by_auth_id.sql_generated.go(2 hunks)go/pkg/db/models_generated.go(4 hunks)go/pkg/db/sqlc.json(1 hunks)go/pkg/db/types/null_json.go(1 hunks)go/pkg/testutil/seed/seed.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- go/pkg/db/key_find_live_by_id.sql_generated.go
- go/pkg/db/key_list_by_key_auth_id.sql_generated.go
- go/pkg/db/custom_types.go
- go/pkg/db/key_list_live_by_auth_id.sql_generated.go
- go/pkg/db/models_generated.go
🧰 Additional context used
🧬 Code graph analysis (13)
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)
go/pkg/db/key_data.go (1)
go/pkg/db/custom_types.go (4)
UnmarshalJSONArrayTo(51-66)RoleInfo(11-15)PermissionInfo(17-22)RatelimitInfo(24-32)
go/apps/api/routes/v2_keys_verify_key/handler.go (3)
go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)go/pkg/fault/wrap.go (1)
Wrap(25-67)
go/apps/api/routes/v2_apis_list_keys/handler.go (3)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)go/pkg/db/models_generated.go (1)
Identity(629-638)go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
go/apps/api/routes/v2_identities_list_identities/handler.go (2)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)
go/pkg/db/identity_find_with_ratelimits.sql_generated.go (1)
go/pkg/db/types/null_json.go (1)
NullJSON(10-13)
go/apps/api/routes/v2_identities_get_identity/handler.go (2)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)
go/pkg/db/key_data_test.go (2)
go/pkg/db/types/null_json.go (1)
NullJSON(10-13)go/pkg/db/models_generated.go (1)
Identity(629-638)
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
go/pkg/db/types/null_json.go (1)
NullJSON(10-13)
go/apps/api/routes/v2_keys_get_key/handler.go (3)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)go/pkg/db/models_generated.go (1)
Identity(629-638)go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
go/pkg/testutil/seed/seed.go (3)
go/pkg/db/types/null_json.go (1)
NullJSON(10-13)go/pkg/db/identity_insert.sql_generated.go (1)
InsertIdentityParams(31-38)go/pkg/db/models_generated.go (2)
Environment(618-627)Identity(629-638)
go/apps/api/routes/v2_keys_whoami/handler.go (3)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)go/pkg/db/models_generated.go (1)
Identity(629-638)go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
go/pkg/db/key_find_for_verification.sql_generated.go (1)
go/pkg/db/types/null_json.go (1)
NullJSON(10-13)
⏰ 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)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
141-143: LGTM! Correct usage of the centralized JSON unmarshal helper.The replacement of direct
json.Unmarshalwithdb.UnmarshalNullableJSONTois correct and follows the new pattern introduced in this PR. Error handling is properly preserved.go/apps/api/routes/v2_identities_list_identities/200_test.go (1)
372-377: LGTM; aligns with NullJSON shape.Using Meta.Valid and comparing against NullJSON.Data with JSONEq is correct and order‑insensitive.
go/pkg/db/identity_find_with_ratelimits.sql_generated.go (1)
12-13: Verification complete—NullJSON integration is correct.The struct confirms Meta is
dbtype.NullJSON(line 73) and Scan properly handles NULL (returns Valid=false) and valid JSON (with a defensive buffer copy to prevent reuse corruption). Ratelimits correctly remainsinterface{}as a non-database field. All requirements are met.go/pkg/db/sqlc.json (1)
40-42: NullJSON properly implements Scanner/Valuer; no issues found.Verification confirms that
dbtype.NullJSONcorrectly implements bothdatabase/sql.Scanneranddriver.Valuerinterfaces with proper NULL handling. TheScan()method includes a defensive buffer copy to prevent corruption from driver buffer reuse—a critical safeguard. The sqlc override configuration is sound: nullable JSON columns map toNullJSONfor reads, while insert parameters usejson.RawMessagefor explicit values. This design is intentional and working correctly.go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
12-13: LGTM; NullJSON properly handles LEFT JOIN NULLs.The
NullJSONtype hasDataandValidfields, and itsScan()method correctly setsValid=falsewhen the database value is NULL. TheUnmarshalTo()method returnsnilwithout attempting to unmarshal when!Valid, preventing panics on NULL identity metadata from LEFT JOINs. All handlers safely useUnmarshalNullableJSONTo(), which wraps this behavior. The import change is correct.go/pkg/db/key_find_for_verification.sql_generated.go (1)
12-12: LGTM! Generated code reflects the new NullJSON type.The sqlc-generated code correctly imports the dbtype package and updates IdentityMeta to use dbtype.NullJSON, which properly handles nullable JSON columns.
Also applies to: 117-117
go/pkg/testutil/seed/seed.go (2)
380-380: LGTM! Naming convention improved.The rename from
identityIdtoidentityIDfollows Go naming conventions for acronyms, and all usages are correctly updated.Also applies to: 392-392, 397-397
372-387: Type inconsistency confirmed: verify if this is intended design.The verification confirms that
InsertIdentityParams.Metaisjson.RawMessagewhileIdentity.Metaisdbtype.NullJSON. Since both files are auto-generated, this pattern likely reflects intentional design: simpler raw types for insertion parameters and wrapped nullable types for model retrieval. However, the architectural intention needs confirmation—specifically whether the codebase applies this pattern consistently across other insert operations versus model definitions, and whether it introduces any conversion overhead or maintenance complexity that should be addressed at the code generator level.go/apps/api/routes/v2_keys_verify_key/handler.go (1)
232-238: LGTM! Consistent error handling with centralized helper.The refactoring replaces manual JSON unmarshalling with
db.UnmarshalNullableJSONTo, maintaining identical error handling behavior while improving code consistency across the codebase.go/pkg/db/types/null_json.go (3)
15-40: LGTM! Critical defensive copy prevents corruption.The defensive copy in the Scan method is essential to prevent data corruption from driver buffer reuse. This is a well-known issue with json.RawMessage and the implementation correctly addresses it with clear documentation.
42-47: LGTM! Correct SQL driver Value implementation.The Value method correctly returns
nilfor SQL NULL when the field is invalid, and properly converts the RawMessage to []byte when valid.
60-65: The silent no-op behavior is safe and works as intended.The
UnmarshalTomethod is only called through theUnmarshalNullableJSONTowrapper function (custom_types.go:79-83), which properly propagates the error to callers. All production usages check the returned error value. ReturningnilwhenValidisfalseis intentional and semantically correct: no error occurred, the field is null, and the caller receives a zero value—a reasonable default for nullable fields. The design properly distinguishes between actual unmarshaling errors (fromjson.Unmarshal) and null values.go/apps/api/routes/v2_identities_list_identities/handler.go (1)
139-146: LGTM! Consistent error handling pattern.The refactoring uses
db.UnmarshalNullableJSONTowith proper error wrapping. Unlike the list_keys handler which only logs errors, this returns the error to the caller, which is more appropriate for an API that lists identities where metadata is expected.go/pkg/db/key_data_test.go (1)
9-9: LGTM! Tests updated for new type.The test correctly uses the new
dbtype.NullJSONtype for IdentityMeta, ensuring the type change is properly validated.Also applies to: 125-125, 133-133
go/apps/api/routes/v2_apis_list_keys/handler.go (1)
306-311: Behavior change is intentional and consistent across handlers.The old implementation checked both
len(keyData.Identity.Meta) > 0andidentityMeta != nilbefore assigning, while the new code assigns&identityMetawhenever unmarshaling succeeds. This means empty JSON objects{}now result in a pointer to an empty map instead of nil.However, this change is part of the broader refactor commit "refactor: use NullJSON for mysql json type" and follows the identical pattern already established in
v2_keys_whoamiandv2_keys_get_keyhandlers. The refactor also improves error handling by logging failures instead of silently ignoring them.Confirm whether this behavior change is acceptable for the API contract (nil vs. empty map in the response).
|
@Flo4604 I think I finally landed on a good solution, but there is some caveat // CRITICAL: A defensive copy to prevent buffer reuse corruption
// json.RawMessage holds a reference to the underlying byte slice.
// When the driver reuses that buffer for the next row, your NullJSON.Data suddenly points to corrupted memory.
// This consumes more memory compared to copyless version and also guarantees data integrity.
clone := make([]byte, len(buf))
copy(clone, buf)When scanning, I actually noticed this only during tests, data is getting corrupted due to how 
|
|
why don't we then just use our byte[] version and add a helper that does the same thing? |
|
|
|
I guess, yeah. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
go/apps/api/routes/v2_keys_whoami/handler.go (1)
158-162: Honor omitempty by checking data presence before unmarshalling and setting Meta.When
keyData.Identity.Metais nil or empty, the helper returns a zero-value map; taking&identityMetacreates a pointer to that nil map, which serializes as"meta": nullinstead of being omitted. Check for data presence first and only setresponse.Identity.Metawhen valid data exists.Apply the fix suggested in the previous review:
- if identityMeta, err := db.UnmarshalNullableJSONTo[map[string]any](keyData.Identity.Meta); err != nil { + if len(keyData.Identity.Meta) > 0 { + identityMeta, err := db.UnmarshalNullableJSONTo[map[string]any](keyData.Identity.Meta) + if err != nil { - h.Logger.Error("failed to unmarshal identity meta", "error", err) - } else { + h.Logger.Error("failed to unmarshal identity meta", "error", err) + } else { - response.Identity.Meta = &identityMeta + response.Identity.Meta = &identityMeta + } }go/apps/api/routes/v2_identities_get_identity/handler.go (1)
99-105: Log meta unmarshal errors instead of failing the request, and honor omitempty.Failing the entire request when identity metadata cannot be parsed degrades UX. Other endpoints (e.g.,
v2_keys_whoami) log the error and continue. Additionally, always settingMeta: &metaMapproduces"meta": nulleven when the DB column is empty or NULL; gate assignment on data presence to respect theomitemptytag.Apply the fix suggested in the previous review:
- metaMap, err := db.UnmarshalNullableJSONTo[map[string]any](identity.Meta) - if err != nil { - return fault.Wrap(err, - fault.Internal("unable to unmarshal metadata"), - fault.Public("We're unable to parse the identity's metadata."), - ) - } + var metaPtr *map[string]any + if len(identity.Meta) > 0 { + metaMap, err := db.UnmarshalNullableJSONTo[map[string]any](identity.Meta) + if err != nil { + h.Logger.Error("failed to unmarshal identity meta", "error", err) + } else { + metaPtr = &metaMap + } + }And update the response assignment:
Data: openapi.Identity{ Id: identity.ID, ExternalId: identity.ExternalID, - Meta: &metaMap, + Meta: metaPtr, Ratelimits: &responseRatelimits, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
go/apps/api/routes/v2_apis_list_keys/handler.go(1 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/handler.go(2 hunks)go/apps/api/routes/v2_keys_get_key/handler.go(1 hunks)go/apps/api/routes/v2_keys_verify_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_whoami/handler.go(1 hunks)go/pkg/db/custom_types.go(2 hunks)go/pkg/testutil/seed/seed.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- go/pkg/testutil/seed/seed.go
- go/apps/api/routes/v2_apis_list_keys/handler.go
- go/pkg/db/custom_types.go
- go/apps/api/routes/v2_keys_get_key/handler.go
🧰 Additional context used
🧬 Code graph analysis (5)
go/apps/api/routes/v2_keys_verify_key/handler.go (4)
go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)go/pkg/db/models_generated.go (1)
Identity(629-638)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-94)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)
go/apps/api/routes/v2_keys_whoami/handler.go (3)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-94)go/pkg/db/models_generated.go (1)
Identity(629-638)go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-94)
go/apps/api/routes/v2_identities_list_identities/handler.go (3)
go/apps/api/openapi/gen.go (3)
V2IdentitiesListIdentitiesRequestBody(708-714)V2IdentitiesListIdentitiesResponseBody(717-726)Meta(279-282)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-94)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)
go/apps/api/routes/v2_identities_get_identity/handler.go (2)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-94)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)
⏰ 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). (3)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
🔇 Additional comments (3)
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
141-141: LGTM!The test code correctly uses the new unmarshalling helper. The pattern is consistent across all test cases and appropriately validates metadata handling.
Also applies to: 241-241, 316-316, 425-425
go/apps/api/routes/v2_keys_verify_key/handler.go (1)
24-27: LGTM!Grouping related type aliases in a single
typeblock is idiomatic Go and improves readability.go/apps/api/routes/v2_identities_list_identities/handler.go (1)
19-22: LGTM!Grouping related type aliases in a single
typeblock is idiomatic Go and improves readability.
|
yeah also we shouldn't fail when json parsing the meta object, as we discussed in slack the object should be valid since its validated as object when sending via the API and the frontend also validates it afaik. |
Yeah I didn't change the existing behavior. We just log errors move on |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
go/pkg/db/custom_types.go (1)
51-66: Critical: Silent error handling issue remains unaddressed from previous review.The function still silently discards JSON unmarshal errors at line 62-63, returning an empty slice instead. This prevents callers from distinguishing between legitimate empty arrays and corrupted/malformed data, masking data integrity issues.
The past review provided evidence showing that other code paths (e.g.,
go/apps/api/routes/v2_keys_set_roles/handler.go:286-289andgo/internal/services/keys/get.go:161-185) log or propagate unmarshal failures. This inconsistency is problematic.Note that your new
UnmarshalNullableJSONTofunction correctly returns errors (line 92-93), demonstrating the right pattern.Refactor
UnmarshalJSONArrayToto match:-func UnmarshalJSONArrayTo[T any](data any) []T { +func UnmarshalJSONArrayTo[T any](data any) ([]T, error) { if data == nil { - return []T{} + return []T{}, nil } bytes, ok := data.([]byte) if !ok || len(bytes) == 0 { - return []T{} + return []T{}, nil } var result []T if err := json.Unmarshal(bytes, &result); err != nil { - return []T{} + return nil, fmt.Errorf("failed to unmarshal JSON array: %w", err) } - return result + return result, nil }Then update all call sites in
go/pkg/db/key_data.goto handle the error appropriately (log with context or propagate).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
go/pkg/db/custom_types.go(2 hunks)
⏰ 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). (3)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
go/apps/api/routes/v2_identities_get_identity/handler.go (1)
99-121: Honor omitempty for Meta; set pointer only when meta is present.Current code always sets Meta: &metaMap, yielding
"meta": nullwhen absent/invalid. Gate it so Meta is omitted unless parsed.- metaMap := db.UnmarshalNullableJSONTo[map[string]any](identity.Meta, h.Logger) + var metaMap map[string]any + if identity.Meta != nil { // or check specific NullString/Valid shape if applicable + metaMap = db.UnmarshalNullableJSONTo[map[string]any](identity.Meta, h.Logger) + } @@ - Meta: &metaMap, + Meta: func() *map[string]any { if metaMap == nil { return nil }; return &metaMap }(),Optional: also use the same helper for ratelimits for consistency/logging.
- var ratelimits []db.RatelimitInfo - if ratelimitBytes, ok := identity.Ratelimits.([]byte); ok && ratelimitBytes != nil { - _ = json.Unmarshal(ratelimitBytes, &ratelimits) - } + ratelimits := db.UnmarshalNullableJSONTo[[]db.RatelimitInfo](identity.Ratelimits, h.Logger)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
go/apps/api/routes/v2_apis_list_keys/handler.go(2 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/handler.go(2 hunks)go/apps/api/routes/v2_keys_get_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_reroll_key/200_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/handler.go(1 hunks)go/apps/api/routes/v2_keys_verify_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_whoami/handler.go(2 hunks)go/pkg/db/custom_types.go(2 hunks)go/pkg/db/key_data.go(5 hunks)go/pkg/db/key_data_test.go(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go/apps/api/routes/v2_apis_list_keys/handler.go
- go/apps/api/routes/v2_keys_get_key/handler.go
🧰 Additional context used
🧬 Code graph analysis (9)
go/apps/api/routes/v2_keys_verify_key/handler.go (3)
go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)go/pkg/db/models_generated.go (1)
Identity(629-638)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(57-86)
go/apps/api/routes/v2_keys_reroll_key/200_test.go (1)
go/pkg/db/key_data.go (1)
ToKeyData(30-47)
go/pkg/db/key_data.go (4)
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
FindLiveKeyByHashRow(104-140)go/pkg/db/key_find_live_by_id.sql_generated.go (1)
FindLiveKeyByIDRow(105-141)go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
ListLiveKeysByKeyAuthIDRow(116-149)go/pkg/db/custom_types.go (4)
UnmarshalNullableJSONTo(57-86)RoleInfo(13-17)PermissionInfo(19-24)RatelimitInfo(26-34)
go/apps/api/routes/v2_identities_get_identity/handler.go (1)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(57-86)
go/pkg/db/key_data_test.go (1)
go/pkg/db/key_data.go (1)
ToKeyData(30-47)
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(57-86)
go/apps/api/routes/v2_identities_list_identities/handler.go (2)
go/apps/api/openapi/gen.go (3)
V2IdentitiesListIdentitiesRequestBody(708-714)V2IdentitiesListIdentitiesResponseBody(717-726)Meta(279-282)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(57-86)
go/apps/api/routes/v2_keys_reroll_key/handler.go (1)
go/pkg/db/key_data.go (1)
ToKeyData(30-47)
go/apps/api/routes/v2_keys_whoami/handler.go (4)
go/pkg/db/key_data.go (1)
ToKeyData(30-47)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(57-86)go/pkg/db/models_generated.go (1)
Identity(629-638)go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
⏰ 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). (3)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
🔇 Additional comments (1)
go/apps/api/routes/v2_keys_reroll_key/handler.go (1)
82-82: LGTM: pass logger into ToKeyData.Signature updated correctly; aligns with helper-based JSON handling.
What does this PR do?
This PR simplifies casting aggregated json data to concrete types. Also, replaces
[]byteoverride withjson.RawMessagebecause it represents our JSON Db columns better. Regarding dynamically castinginterface{}to associated type, I tried to override them insqlc.json, but since those values are aggregated and not really a column in the db, override fails. I also tried to do plugin but cons are overweighed the pros and finally I tried to do replace them using a script then I ended up with too much magic and very error prone code.UnmarshalJSONArraythis is the most idiomatic way to solve this.https://linear.app/unkey/issue/ENG-1991/improve-type-safety-for-json-columns-in-database-structs
Fixes #3789
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated