Unicron fix one more bug#5035
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR stops converting DynamoDB items through intermediate ChangesDynamoDB Native Persistence Pattern
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
There was a problem hiding this comment.
Pull request overview
Fixes DynamoDB attribute-type coercion introduced by store.InterfaceMapToItem (via isNumericString) by avoiding JSON-style map round-trips in a couple of legacy API flows, ensuring digit-only string fields remain DynamoDB S (string) attributes.
Changes:
- Build DynamoDB
map[string]types.AttributeValueitems directly when creating/updating/v1/projectrecords to prevent digit-onlyproject_namevalues from being stored asN. - Update GitHub OAuth “get or create user” flow to patch/persist raw DynamoDB AttributeValue maps, avoiding unintended
S→Ncoercion. - Preserve pre-cutover API response shape for
user_github_id(return a JSON number) while still storing it as DynamoDBN.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cla-backend-legacy/internal/api/handlers.go | Creates/updates Project items using raw DynamoDB AttributeValue maps to prevent numeric-string coercion. |
| cla-backend-legacy/internal/api/github_oauth.go | Updates user create/update during GitHub OAuth using raw AttributeValue maps and maintains legacy user_github_id response shape. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cla-backend-legacy/internal/api/github_oauth.go (2)
306-319: 💤 Low valueMinor inconsistency in
user_github_usernamehandling.In the update path (line 273-275),
user_github_usernameis only set ifgithubLogin != "", preserving existing values. Here in the create path, it's always set (line 312), potentially storing an empty string. While this is unlikely in practice (a valid GitHub user should have a login), consider applying the same guard for consistency:- "user_github_username": &types.AttributeValueMemberS{Value: githubLogin}, } + if githubLogin != "" { + itemAV["user_github_username"] = &types.AttributeValueMemberS{Value: githubLogin} + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cla-backend-legacy/internal/api/github_oauth.go` around lines 306 - 319, The create-path always sets itemAV["user_github_username"] to githubLogin which can store an empty string; mirror the update-path behavior by only adding the "user_github_username" entry to the itemAV map when githubLogin != "" (same guard used in the update branch) so existing/empty values aren't persisted—modify the create branch where itemAV is constructed to conditionally add the "user_github_username" key based on githubLogin.
282-286: 💤 Low valueUnreachable branch: emails are already validated non-empty.
The
elsebranch that deletesuser_emailsis unreachable because lines 228-230 already return an error iflen(emails) < 1. While this defensive code isn't harmful, it could be removed for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cla-backend-legacy/internal/api/github_oauth.go` around lines 282 - 286, Remove the unreachable else block that deletes "user_emails": since emails is already validated to be non-empty earlier, simply set userAV["user_emails"] = &types.AttributeValueMemberSS{Value: emails} (or assign unconditionally) and delete the conditional else branch that calls delete(userAV, "user_emails"); this cleans up dead code around the userAV, emails and types.AttributeValueMemberSS usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cla-backend-legacy/internal/api/github_oauth.go`:
- Around line 306-319: The create-path always sets
itemAV["user_github_username"] to githubLogin which can store an empty string;
mirror the update-path behavior by only adding the "user_github_username" entry
to the itemAV map when githubLogin != "" (same guard used in the update branch)
so existing/empty values aren't persisted—modify the create branch where itemAV
is constructed to conditionally add the "user_github_username" key based on
githubLogin.
- Around line 282-286: Remove the unreachable else block that deletes
"user_emails": since emails is already validated to be non-empty earlier, simply
set userAV["user_emails"] = &types.AttributeValueMemberSS{Value: emails} (or
assign unconditionally) and delete the conditional else branch that calls
delete(userAV, "user_emails"); this cleans up dead code around the userAV,
emails and types.AttributeValueMemberSS usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91530e31-900d-4b71-892d-d55efc090e05
📒 Files selected for processing (2)
cla-backend-legacy/internal/api/github_oauth.gocla-backend-legacy/internal/api/handlers.go
|
No longer needed in favor of #5037 |
Followup after switchover - safe to postpone to next week.