Refactor Backend Architecture for Service Layer Isolation & Comprehensive Test Coverage #253
Conversation
…dicated setup module.
📝 WalkthroughWalkthroughCentralizes HTTP routing into routes.SetupRouter, introduces Mongo-backed repositories (User, Debate), extracts auth logic into a new AuthService with JWT/email flows, adds a VotingService for AI judging, converts the seeder to a standalone main command, and adds test scaffolding and utilities for integration and concurrency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthController
participant AuthService
participant UserRepository
participant Database
participant EmailService
rect rgba(100, 150, 255, 0.5)
Note over Client,EmailService: SignUp Flow
Client->>AuthController: POST /signup (email, password)
AuthController->>AuthService: SignUp(ctx, email, password)
AuthService->>UserRepository: FindByEmail(ctx, email)
UserRepository->>Database: Query user by email
Database-->>UserRepository: User or not found
UserRepository-->>AuthService: result
AuthService->>AuthService: Hash password & build user
AuthService->>UserRepository: Create(ctx, newUser)
UserRepository->>Database: Insert user
Database-->>UserRepository: Insert result
UserRepository-->>AuthService: created user
AuthService->>EmailService: SendVerificationEmail(email, code)
EmailService-->>AuthService: success/error
AuthService-->>AuthController: success/error
AuthController-->>Client: 200 / 4xx/5xx
end
sequenceDiagram
participant Client
participant AuthController
participant AuthService
participant UserRepository
participant Database
rect rgba(150, 200, 150, 0.5)
Note over Client,Database: Verify Email & Login Flow
Client->>AuthController: POST /verify-email (email, code)
AuthController->>AuthService: VerifyEmail(ctx, email, code)
AuthService->>UserRepository: FindByEmail(ctx, email)
UserRepository->>Database: Query user
Database-->>UserRepository: User
UserRepository-->>AuthService: User
AuthService->>AuthService: Validate code & expiry
AuthService->>UserRepository: UpdateByEmail(ctx, email, verified)
UserRepository->>Database: Update user
Database-->>UserRepository: Success
UserRepository-->>AuthService: Success
AuthService->>AuthService: Generate JWT
AuthService-->>AuthController: User + Token
AuthController-->>Client: 200 (user, accessToken)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/cmd/seeder/main.go (1)
40-59: Seeded users are missing required fields.The test users lack several required fields from the User model:
Password,Nickname,IsVerified,LastRatingUpdate,UpdatedAt,Score, andCurrentStreak. These fields have noomitemptyBSON tags, meaning they must be populated. Either set default values for these fields in the seeded data or handle their initialization in the seeding logic.
🤖 Fix all issues with AI agents
In `@backend/cmd/seeder/main.go`:
- Around line 28-34: The call to collection.CountDocuments in main.go ignores
its returned error which can make the seeder proceed on failures; update the
seeding logic around the CountDocuments call (the variable named collection and
the CountDocuments(context.Background(), bson.M{}) invocation) to capture the
returned error, check if err != nil, and on error log the detailed error (via
log.Printf or similar) and abort/return instead of continuing; only proceed to
check count and seed when err is nil.
In `@backend/repositories/debate_repository.go`:
- Around line 40-50: The Create method on MongoDebateRepository currently
performs an unguarded type assertion on result.InsertedID to primitive.ObjectID
which can panic; update MongoDebateRepository.Create to use the comma-ok form
when converting result.InsertedID to primitive.ObjectID (same approach as
UserRepository.Create), and if the assertion fails return a clear typed error
(e.g., errors.New("failed to convert inserted ID to ObjectID")) instead of
panicking; ensure you reference result.InsertedID and primitive.ObjectID in the
check and return the hex string only after successful assertion.
- Around line 66-75: The UpdateOutcome implementation in MongoDebateRepository
is using a userId parameter and filtering on "userId", but the interface and
DebateVsBot model use email; update the function signature and internal
references so the parameter is named email (UpdateOutcome(ctx context.Context,
email, outcome string) error) and change the filter to bson.M{"email": email}
before calling UpdateOne (ensure any callers are updated to pass email if
needed).
In `@backend/repositories/user_repository.go`:
- Around line 64-74: In MongoUserRepository.Create, guard the untyped assertion
on result.InsertedID before assigning to user.ID: check whether
result.InsertedID is a primitive.ObjectID (e.g. via a type switch or ok :=
result.InsertedID.(primitive.ObjectID)), only set user.ID when the assertion
succeeds, and return a clear error if the InsertedID is of an unexpected type;
update the Create function to mirror the safe pattern used in
services/debatevsbot.go so it never panics on non-ObjectID _id values.
In `@backend/routes/setup.go`:
- Around line 52-54: SetupRouter currently hardcodes "./config/config.prod.yml"
when wiring middlewares.AuthMiddleware which ignores the cfg passed in and can
mismatch JWT secrets; change SetupRouter to accept and use the effective config
path or pass the loaded cfg object into the middleware instead of the hard-coded
string: update the SetupRouter signature (and its call sites such as
backend/cmd/server/main.go and tests) to forward the config path or cfg to
middlewares.AuthMiddleware and replace the hard-coded "./config/config.prod.yml"
usage in the auth and admin middleware wiring so the middleware reads the same
config used by LoadConfig.
- Around line 22-23: router.SetTrustedProxies is being called with an invalid
"localhost" entry and its returned error is ignored; replace invalid names with
valid IPs/CIDR (e.g., "127.0.0.1/32", "::1/128" or your proxy network ranges)
and check the returned error from SetTrustedProxies, handling it (log and exit
or return the error) so the router does not silently default to trusting all
proxies; update the call site where SetTrustedProxies is used to validate the
input slice and handle the error accordingly.
In `@backend/services/auth_service.go`:
- Around line 112-114: The expiry check currently uses user.CreatedAt (the code:
if time.Since(user.CreatedAt) > 24*time.Hour), which ties verification validity
to account creation; change the logic to use a dedicated timestamp for when the
verification code was issued (e.g., user.VerificationCodeSentAt or
user.CodeIssuedAt) and check time.Since(user.VerificationCodeSentAt) >
24*time.Hour instead; add/update the corresponding field on the User model,
ensure it is set when issuing/resending codes (where verification codes are
generated/sent), and update persistence/DB migrations and any tests that assume
CreatedAt-based expiry.
- Around line 40-45: The SignUp method currently treats any error from
userRepo.FindByEmail as "user not found", which masks DB errors; update SignUp
in authServiceImpl to distinguish a not-found sentinel error from other errors:
call s.userRepo.FindByEmail(ctx, email) and if err == nil return
errors.New("user already exists"); if err != nil then if errors.Is(err,
repo.ErrNotFound) or errors.Is(err, mongo.ErrNoDocuments) proceed to create the
user, otherwise return the error (propagate DB/connection errors). If the
repository lacks a sentinel ErrNotFound, add one (or translate
mongo.ErrNoDocuments) in the repository implementation so SignUp can reliably
use errors.Is.
In `@backend/services/voting_service_test.go`:
- Around line 36-39: The mock method MockDebateRepository.FindByID currently
does an unchecked type assertion which will panic if the mock returns nil;
change it to first retrieve args.Get(0) into a variable, check if that value is
nil and if so return nil and args.Error(1), otherwise perform the type assertion
and return the typed *models.DebateVsBot and args.Error(1) so it matches the
pattern used in the other mock methods.
- Around line 102-110: The "Invalid JSON" subtest currently returns early and
performs no assertions; update the subtest handling for the case where tt.name
== "Invalid JSON" (within the loop that calls ParseJudgeResult) to assert that
ParseJudgeResult returned an error (e.g., require/Error or assert.Error on the
returned error) and optionally assert that the result is the zero/default value
if desired; for all other cases keep the existing assert.Equal(t, tt.expected,
result). Reference: ParseJudgeResult and the test loop variable tt in
voting_service_test.go.
In `@backend/services/voting_service.go`:
- Around line 37-57: JudgeDebate currently discards GetBotPersonality(botName),
ignores botLevel and topic, and uses a placeholder prompt; fix by capturing the
personality (p := GetBotPersonality(botName)) and incorporating p, botLevel and
topic into the prompt built before calling FormatHistory and
s.gemini.GenerateContent (e.g., include personality description, judging
criteria based on botLevel, and the debate topic), and if you intend to keep a
placeholder instead of full logic add a clear TODO comment in JudgeDebate to
indicate incomplete implementation.
In `@backend/websocket/concurrency_test.go`:
- Line 36: The UserID construction uses string(rune(id)) which converts the
integer to a Unicode code point (producing control characters for small ids)
instead of a numeric string; update the UserID generation (where UserID:
"user"+string(rune(id)) is set) to use a proper integer-to-string conversion
such as strconv.Itoa(id) or fmt.Sprintf("user%d", id), and add the corresponding
import (strconv or fmt) to the test file.
🧹 Nitpick comments (14)
backend/websocket/concurrency_test.go (1)
62-107: Concurrency pattern is valid for race detection.The test correctly exercises concurrent read/write patterns with proper mutex usage. Running with
-racewill verify thread safety.Optional improvement: The snapshot simulation (lines 97-100) could actually build the slice to more closely mirror production broadcast logic:
♻️ Optional enhancement
room.Mutex.Lock() // Simulate snapshot - _ = make([]*Client, 0, len(room.Clients)) + snapshot := make([]*Client, 0, len(room.Clients)) for _, cl := range room.Clients { - _ = cl + snapshot = append(snapshot, cl) } room.Mutex.Unlock() + _ = snapshot // Use snapshotbackend/cmd/seeder/main.go (2)
17-17: Hardcoded relative config path is fragile.The path
../../config/config.prod.ymldepends on the working directory when the binary is executed. Consider using an environment variable or command-line flag for flexibility.💡 Suggested improvement using environment variable
+import ( + "os" + ... +) + func main() { - cfg, err := config.LoadConfig("../../config/config.prod.yml") + configPath := os.Getenv("CONFIG_PATH") + if configPath == "" { + configPath = "../../config/config.prod.yml" + } + cfg, err := config.LoadConfig(configPath)
61-64: Consider simplifying slice conversion.The conversion loop is correct but can be slightly more concise.
💡 Alternative approach
- var documents []interface{} - for _, user := range testUsers { - documents = append(documents, user) - } + documents := make([]interface{}, len(testUsers)) + for i, user := range testUsers { + documents[i] = user + }This pre-allocates the slice to avoid repeated allocations during append.
backend/repositories/user_repository.go (1)
29-37: Consider failing fast when the DB isn’t initialized.Returning an empty repo defers failure to later calls; consider returning an error (or at least logging) to avoid a “successful” startup with a nil collection.
backend/services/debatevsbot.go (1)
204-206: Remove trailing empty fmt.Sprintf argument (optional cleanup).The empty string only fills the final
%sand adds trailing whitespace; if no value is intended, drop the extra placeholder + argument.♻️ Suggested cleanup
-[Your opening argument] -%s %s`, +[Your opening argument] +%s`, @@ - limitInstruction, - baseInstruction, - "", + limitInstruction, + baseInstruction,backend/routes/setup.go (1)
26-32: Make allowed origins configurable (avoid localhost-only in prod).Hard-coding
http://localhost:5173can break CORS in non-dev environments. Consider pulling origins from config/env with a safe default.backend/services/voting_service_test.go (2)
46-54: Avoid duplicateMockGeminiClientdefinitions.You already have
MockGeminiClientinbackend/utils/test_utils.go. Consider reusing it to reduce drift between test helpers.
56-71: Assert mock expectations to ensure the Gemini call happens.Without
AssertExpectations, the test can pass even ifGenerateContentis never called.🔧 Proposed fix
resp, err := service.JudgeDebate(ctx, history, "Bot", "Medium", "Topic") assert.NoError(t, err) assert.Equal(t, expectedResponse, resp) + mockGemini.AssertExpectations(t) }) }backend/tests/integration_test.go (1)
3-29: Reuseutils.LoadTestConfigto avoid config drift.This duplicates test config initialization already provided in
backend/utils/test_utils.go. Reusing it centralizes defaults.🔧 Proposed fix
import ( "net/http" "net/http/httptest" "testing" "arguehub/config" "arguehub/routes" + "arguehub/utils" "github.com/gin-gonic/gin" "github.com/stretchr/testify/assert" ) @@ func SetupTestRouter() *gin.Engine { gin.SetMode(gin.TestMode) - // Create minimal test config - cfg := &config.Config{} - cfg.Database.URI = "mongodb://localhost:27017/test_db" - cfg.JWT.Secret = "testsecret" - cfg.JWT.Expiry = 60 + cfg := utils.LoadTestConfig() + cfg.Database.URI = "mongodb://localhost:27017/test_db" // override if needed // Redis defautls to zero value (empty Addr), which is fine for tests that don't need it or handle it gracefullybackend/utils/test_utils.go (1)
29-33: Add a timeout to the initial DB drop to avoid hanging tests.Using
context.Background()can stall the test suite if Mongo stalls. Match the teardown’s timeout pattern.🔧 Proposed fix
- // Ensure we are starting with a clean slate - err = db.MongoDatabase.Drop(context.Background()) + // Ensure we are starting with a clean slate + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + err = db.MongoDatabase.Drop(ctx) if err != nil { log.Printf("Warning: Failed to drop test database: %v", err) }backend/services/voting_service.go (2)
23-35: UnuseddebateRepodependency.The
debateRepofield is injected but never used in the current implementation. If it's intended for future use, consider adding a TODO comment or removing it until needed to keep the code clean.♻️ If not needed, consider removing
type votingServiceImpl struct { - debateRepo repositories.DebateRepository - gemini GeminiClientInterface + gemini GeminiClientInterface } -func NewVotingService(debateRepo repositories.DebateRepository, gemini GeminiClientInterface) VotingService { +func NewVotingService(gemini GeminiClientInterface) VotingService { return &votingServiceImpl{ - debateRepo: debateRepo, - gemini: gemini, + gemini: gemini, } }
61-86: Silent fallback to "loss" may mask parsing issues.The function defaults to
"loss"when:
verdictkey is missing or not a map (line 69)winnerkey is missing or not a string (line 74)winnervalue is unrecognized (line 85)This makes it difficult to distinguish between a legitimate bot win and a malformed response. Consider returning an error for unexpected structures, or at least logging when defaults are applied.
♻️ Alternative: Return error for malformed verdict
func ParseJudgeResult(jsonResult string) (string, error) { var result map[string]interface{} if err := json.Unmarshal([]byte(jsonResult), &result); err != nil { return "", err } verdict, ok := result["verdict"].(map[string]interface{}) if !ok { - return "loss", nil // Default failure + return "", errors.New("missing or invalid verdict field") } winner, ok := verdict["winner"].(string) if !ok { - return "loss", nil + return "", errors.New("missing or invalid winner field") } if strings.EqualFold(winner, "User") { return "win", nil } else if strings.EqualFold(winner, "Bot") { return "loss", nil } else if strings.EqualFold(winner, "Draw") { return "draw", nil } - return "loss", nil + return "", fmt.Errorf("unrecognized winner value: %s", winner) }backend/controllers/auth.go (2)
426-447: Duplicate JWT generation logic.The private
generateJWTfunction duplicatesservices.GenerateJWT. The only difference is logging statements. Consider using the service function and adding logging there if needed, or removing this function onceGoogleLoginis refactored to use the service.♻️ Consider consolidating
// In GoogleLogin and other methods still using generateJWT: -token, err := generateJWT(existingUser.Email, cfg.JWT.Secret, cfg.JWT.Expiry) +token, err := services.GenerateJWT(existingUser.Email, cfg.JWT.Secret, cfg.JWT.Expiry)Then remove the local
generateJWTfunction once all callers are migrated.
33-122: Inconsistent service layer usage across auth endpoints.
GoogleLogin,ForgotPassword,VerifyForgotPassword, andVerifyTokenstill accessdb.MongoDatabasedirectly, whileSignUp,Login, andVerifyEmailnow use the service layer. Consider tracking this as follow-up work to complete the refactor and achieve consistent architecture.Would you like me to open an issue to track migrating the remaining auth endpoints to the service layer?
| collection := db.MongoDatabase.Collection("users") | ||
| count, _ := collection.CountDocuments(context.Background(), bson.M{}) | ||
|
|
||
| if count > 0 { | ||
| log.Println("Users already exist, skipping seed.") | ||
| return | ||
| } |
There was a problem hiding this comment.
Ignored error from CountDocuments can cause unexpected behavior.
If CountDocuments fails (e.g., network issue, permissions), count defaults to 0 and the seeder proceeds to insert, potentially causing duplicate key errors or masking the real failure.
🐛 Proposed fix
collection := db.MongoDatabase.Collection("users")
- count, _ := collection.CountDocuments(context.Background(), bson.M{})
+ count, err := collection.CountDocuments(context.Background(), bson.M{})
+ if err != nil {
+ log.Fatal("Failed to count users: " + err.Error())
+ }
if count > 0 {
log.Println("Users already exist, skipping seed.")
return
}🤖 Prompt for AI Agents
In `@backend/cmd/seeder/main.go` around lines 28 - 34, The call to
collection.CountDocuments in main.go ignores its returned error which can make
the seeder proceed on failures; update the seeding logic around the
CountDocuments call (the variable named collection and the
CountDocuments(context.Background(), bson.M{}) invocation) to capture the
returned error, check if err != nil, and on error log the detailed error (via
log.Printf or similar) and abort/return instead of continuing; only proceed to
check count and seed when err is nil.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/models/user.go`:
- Around line 23-28: Add JSON omit tags to the sensitive fields on the User
struct: update the struct fields Password, VerificationCode,
VerificationCodeSentAt, and ResetPasswordCode to include json:"-" in addition to
their existing bson tags (e.g., Password string `bson:"password" json:"-"`),
leaving other tags and omitempty behavior intact so these fields cannot be
accidentally marshaled to JSON.
1 participant
Issue #230
This PR executes a major architectural refactor of the backend to enable strict dependency injection, resolving circular dependencies and allowing for robust automated testing. It isolates business logic from the controller layer into dedicated Services (AuthService,
VotingService) and implements a multi-layered testing strategy covering unit, integration, and concurrency vectors.Technical Changes
Architecture & Refactoring
VotingServicefromcontrollers, moving core business logic intoservices/package.repositoriesinterfaces (UserRepository,DebateRepository) and EmailSender interface to abstract external dependencies (MongoDB, SMTP) for mocking.utils/to a standalonecmd/seederbinary, resolving theutils<->servicesimport cycle.routes.SetupRouterto facilitatehttptestintegration.Testing Infrastructure
VotingService(JudgeDebate, result parsing) usingtestify/mock.httptestto verify HTTP route registration, middleware binding, and status codes (403 vs 404).utils/test_utils.gofor test-specific configuration loading and mock generation.Bug Fixes
fmt.Sprintfargument mismatch in debatevsbot.go.configvstestConfigstruct mismatches in test files.Testing Strategy
All tests pass with race detection enabled: