feat: Create a user API to synchronously return the user ID and simul… - #2251
feat: Create a user API to synchronously return the user ID and simul…#2251bddiudiu wants to merge 40 commits into
Conversation
…taneously generate a default token according to the configuration. (cherry picked from commit 90ea83a)
WalkthroughCreateUser now assigns input Group to the created user, returns the created userId in JSON, and — when Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as controller/user.go
participant DB as Database
Client->>Controller: CreateUser / Register request (may include Group)
Controller->>DB: Insert user record
activate DB
DB-->>Controller: Insert result (userId)
deactivate DB
alt constant.GenerateDefaultToken == true
Controller->>Controller: Generate token key & build token payload (UserId, Name, Key, timestamps, quotas, Group)
Controller->>DB: Insert default token
activate DB
DB-->>Controller: Token insert result
deactivate DB
alt Token insert failed
Controller-->>Client: Error response (token error)
else Token insert succeeded
Controller-->>Client: Success response (includes userId)
end
else
Controller-->>Client: Success response (includes userId)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/user.go (1)
226-233: Now let me search for the CreateUser function to understand the inconsistency mentioned in the review:Now I need to search for the CreateUser function to complete the verification. Let me look for it:
Based on my verification of the GORM behavior and examination of the Insert() method, I can now provide a rewritten review comment:
Remove the redundant database query—GORM populates the ID after Create().
GORM's Create() method automatically backfills primary key values on the model struct, making the query at lines 226-233 unnecessary. Notably, after calling Create(), you can access the inserted ID directly via the object's primary key field. This is demonstrated in the Insert() method itself at line 415 where
user.Idis used immediately afterDB.Create(user)without a separate query.The code currently performs an additional database lookup to fetch the user by username, but this extra query can be eliminated by directly using
cleanUser.IdaftercleanUser.Insert(inviterId)succeeds, consistent with GORM's intended usage pattern.
♻️ Duplicate comments (1)
controller/user.go (1)
234-267: Duplicate code with same issues as CreateUser token generation.This token generation logic is nearly identical to lines 871-904 in
CreateUser, with the same issues:
- Contradictory quota settings (
RemainQuota: 500000withUnlimitedQuota: true)- Hard-coded values and Chinese text
- No transaction wrapping user+token creation
- Partial failure risk
Please apply the same refactoring recommended in the comment for lines 871-904: extract this into a shared helper function and wrap both operations in a database transaction to ensure atomicity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controller/user.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controller/user.go (5)
constant/env.go (1)
GenerateDefaultToken(14-14)common/utils.go (2)
GenerateKey(256-259)GetTimestamp(266-268)common/sys_log.go (1)
SysLog(11-14)model/token.go (1)
Token(14-31)setting/auto_group.go (1)
DefaultUseAutoGroup(11-11)
🔇 Additional comments (1)
controller/user.go (1)
909-909: LGTM! Useful API improvement.Adding the
userIdfield to the response is a valuable enhancement that allows API consumers to immediately reference the newly created user without additional queries.
| // 生成默认令牌 | ||
| if constant.GenerateDefaultToken { | ||
| key, err := common.GenerateKey() | ||
| if err != nil { | ||
| c.JSON(http.StatusOK, gin.H{ | ||
| "success": false, | ||
| "message": "生成默认令牌失败", | ||
| }) | ||
| common.SysLog("failed to generate token key: " + err.Error()) | ||
| return | ||
| } | ||
| // 生成默认令牌 | ||
| token := model.Token{ | ||
| UserId: cleanUser.Id, // 使用插入后的用户ID | ||
| Name: cleanUser.Username + "的初始令牌", | ||
| Key: key, | ||
| CreatedTime: common.GetTimestamp(), | ||
| AccessedTime: common.GetTimestamp(), | ||
| ExpiredTime: -1, // 永不过期 | ||
| RemainQuota: 500000, // 示例额度 | ||
| UnlimitedQuota: true, | ||
| ModelLimitsEnabled: false, | ||
| } | ||
| if setting.DefaultUseAutoGroup { | ||
| token.Group = "auto" | ||
| } | ||
| if err := token.Insert(); err != nil { | ||
| c.JSON(http.StatusOK, gin.H{ | ||
| "success": false, | ||
| "message": "创建默认令牌失败", | ||
| }) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical inconsistency and multiple concerns with token generation logic.
Several issues identified:
-
Inconsistency in ID retrieval: This code uses
cleanUser.Iddirectly afterInsert()(line 884), but theRegisterfunction (lines 226-233) queries the database to retrieve the inserted user's ID. This inconsistency suggests uncertainty about whether GORM populates the ID field after insertion. -
Contradictory quota settings: Setting both
RemainQuota: 500000andUnlimitedQuota: trueis contradictory. If the quota is unlimited, why set a remain quota? -
Hard-coded values: The quota value (500000) and Chinese text ("的初始令牌") are hard-coded, reducing configurability and creating i18n issues.
-
Code duplication: This token generation logic is nearly identical to lines 235-267 in the
Registerfunction, violating DRY principles. -
Partial failure without rollback: If token creation fails after user creation, the user exists without a token. Without a transaction wrapping both operations, this creates an inconsistent state.
Recommended actions:
- Verify whether
cleanUser.Idis populated afterInsert(). If not, fetch it from the database as done inRegister. - Remove the contradictory
RemainQuotasetting whenUnlimitedQuotais true, or make quota configuration explicit. - Extract token generation into a shared helper function to eliminate duplication.
- Use a database transaction to ensure atomicity of user+token creation.
- Make the token name configurable or at least support i18n.
// Example: Extract to helper function
func createDefaultToken(userId int, username string) error {
if !constant.GenerateDefaultToken {
return nil
}
key, err := common.GenerateKey()
if err != nil {
return fmt.Errorf("failed to generate token key: %w", err)
}
token := model.Token{
UserId: userId,
Name: fmt.Sprintf("%s - Initial Token", username), // or use i18n
Key: key,
CreatedTime: common.GetTimestamp(),
AccessedTime: common.GetTimestamp(),
ExpiredTime: -1,
UnlimitedQuota: true,
ModelLimitsEnabled: false,
}
if setting.DefaultUseAutoGroup {
token.Group = "auto"
}
return token.Insert()
}Then wrap both operations in a transaction:
err := model.DB.Transaction(func(tx *gorm.DB) error {
if err := cleanUser.Insert(0); err != nil {
return err
}
return createDefaultToken(cleanUser.Id, cleanUser.Username)
})
if err != nil {
common.ApiError(c, err)
return
}
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 (1)
.github/workflows/docker-image-arm64.yml(3 hunks)
🔇 Additional comments (1)
.github/workflows/docker-image-arm64.yml (1)
1-117: Misalignment between PR objectives and changes.The PR objectives describe a feature to "Create a user API to synchronously return the user ID and generate a default token," but this file contains only Docker infrastructure changes (repository name substitution). The actual user API feature implementation (referenced in the AI summary as occurring in
controller/user.go) is not provided for review.Clarify whether:
- These Docker workflow changes are the only changes in this PR (which would not align with the stated objectives)
- Additional files (
controller/user.goand related) should be included in this review- The PR objectives need to be updated
| with: | ||
| images: | | ||
| calciumion/new-api | ||
| bddiudiu/new-api |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Docker image repository migration is coordinated across infrastructure and deployments.
The repository name has been changed from calciumion/new-api to bddiudiu/new-api across the workflow. While the changes are consistent within this file, ensure that:
- This repository migration is intentional and coordinated with your deployment infrastructure
- Access credentials (secrets) are properly configured for the new Docker Hub account (
bddiudiu) - Existing deployments pulling from
calciumion/new-apihave a migration plan - No other CI/CD pipelines or manifests reference the old image name
Run the following script to verify there are no remaining references to the old repository name:
[/request_verification]
Additionally, verify that the DOCKERHUB_USERNAME and DOCKERHUB_TOKEN secrets are correctly configured for the new Docker Hub account.
Also applies to: 76-77, 107-109, 114-116
🏁 Script executed:
#!/bin/bash
# Description: Search for any remaining references to the old Docker image repository
echo "Searching for references to old repository (calciumion/new-api)..."
rg -n "calciumion/new-api" --type yaml --type yml
echo ""
echo "Verifying all instances in workflow are updated..."
rg -n "bddiudiu/new-api" --type yaml --type ymlLength of output: 174
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Description: Search for Docker image repository references with corrected syntax
echo "Searching for old repository references (calciumion/new-api)..."
rg -n "calciumion/new-api" --type yaml
echo ""
echo "Verifying new repository references (bddiudiu/new-api)..."
rg -n "bddiudiu/new-api" --type yaml
echo ""
echo "Also checking .yml files directly..."
find .github -name "*.yml" -o -name "*.yaml" | xargs grep -n "calciumion/new-api\|bddiudiu/new-api" 2>/dev/null || echo "No matches found in .github directory"Length of output: 1871
Incomplete Docker repository migration across workflows—multiple files still reference old repository.
Verification found the migration is inconsistent: docker-image-arm64.yml was updated to use bddiudiu/new-api, but other critical files still reference calciumion/new-api:
docker-image-alpha.yml: 8 references tocalciumion/new-api(lines 69, 79-80, 121-123, 128-130)docker-compose.yml: 1 reference tocalciumion/new-api(line 19)
Update these files to complete the repository migration consistently, or clarify if the alpha workflow and compose file should intentionally use different registries.
🤖 Prompt for AI Agents
In .github/workflows/docker-image-arm64.yml around line 66 and also in
.github/workflows/docker-image-alpha.yml (lines 69, 79-80, 121-123, 128-130) and
docker-compose.yml (line 19), fix the incomplete migration by replacing
occurrences of "calciumion/new-api" with "bddiudiu/new-api" so all workflows and
compose file reference the same repository; if the alpha workflow or compose
file are intentionally meant to point to a different registry, add a clear
comment explaining that intent instead of leaving mixed references.
18c9976 to
47020e4
Compare
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.42.0 to 0.45.0. - [Commits](golang/crypto@v0.42.0...v0.45.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-version: 0.45.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
… use context in multiple channels
…for token tracking
This reverts commit 8837132.
This reverts commit e84a873.
This reverts commit 75902d2.
This reverts commit 4f94c3b.
This reverts commit ee96ca6.
…Handler for token tracking" This reverts commit 83c4924.
…ettings" This reverts commit 817082d.
This reverts commit 50ca4e3.
This reverts commit 305e069.
This reverts commit 92c5b44.
…logic" This reverts commit 63c1af6.
This reverts commit 2e42b73.
This reverts commit 2799c94.
This reverts commit c1b8983.
…Usage to use context in multiple channels" This reverts commit f88e14b.
…t the first enabled one." This reverts commit 7cb6fc7.
…sing the OpenAI format" This reverts commit ba1a158.
This reverts commit f95cf0c.
This reverts commit 542447a.
6 participants
Create a user API to synchronously return the user ID and simultaneously generate a default token according to the configuration.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.