Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughAdds member registration and session authentication backed by PostgreSQL, exposes member REST endpoints, wires them into the server, adds migrations and integration tests, and updates project documentation and tooling. ChangesMember and session management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MemberHandler
participant MemberRepositoryPGX
participant SessionRepositoryPGX
Client->>MemberHandler: Login with email and password
MemberHandler->>MemberRepositoryPGX: Load member by email
MemberRepositoryPGX-->>MemberHandler: Member record
MemberHandler->>SessionRepositoryPGX: Create session
SessionRepositoryPGX-->>MemberHandler: Session key and expiry
MemberHandler-->>Client: Set HttpOnly session_key cookie
Client->>MemberHandler: Request current member with cookie
MemberHandler->>SessionRepositoryPGX: Load session by key
SessionRepositoryPGX-->>MemberHandler: Session record
MemberHandler->>MemberRepositoryPGX: Load member by ID
MemberRepositoryPGX-->>MemberHandler: Member record
MemberHandler-->>Client: Return member response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (4)
Makefile (1)
9-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the target name match its new behavior.
Without
-v,db-rmno longer removes PostgreSQL volumes or data; it only stops and removes containers. Rename it to something likedb-stop, or add a separate explicit reset target for destructive cleanup.🤖 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 `@Makefile` around lines 9 - 10, Rename the Makefile target db-rm to db-stop to reflect that docker compose down only stops and removes containers without volumes. Update any references to the target, or add a separate explicitly named destructive reset target if volume removal is still required.plan/member-session-plan.md (1)
153-153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
To resolve the markdown linting warning (
MD040), add a language identifier (such astext) to this fenced code block.♻️ Proposed fix
-``` +```text🤖 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 `@plan/member-session-plan.md` at line 153, Update the fenced code block in member-session-plan.md by adding an appropriate language identifier, such as text, to its opening fence to satisfy Markdown lint rule MD040.Source: Linters/SAST tools
src/api/member_handler.go (2)
192-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUtilize domain sentinel errors for session authentication.
Rather than creating new generic
errors.New("unauthorized")instances, leverage the domain sentinel errors defined insrc/domain/errors.go(e.g.,domain.ErrSessionNotFound,domain.ErrSessionExpired). This improves error semantics and properly passes internal repository errors up the stack.♻️ Proposed refactor
func (h *MemberHandler) authenticateMember(r *http.Request) (*domain.Member, error) { cookie, err := r.Cookie("session_key") if err != nil { - return nil, errors.New("unauthorized") + return nil, domain.ErrSessionNotFound } session, err := h.sessionRepo.GetByKey(context.Background(), cookie.Value) if err != nil { - return nil, errors.New("unauthorized") + return nil, err } if session == nil { - return nil, errors.New("unauthorized") + return nil, domain.ErrSessionNotFound } if session.ExpiresAt.Before(time.Now()) { - return nil, errors.New("session expired") + return nil, domain.ErrSessionExpired } member, err := h.memberRepo.GetByID(context.Background(), session.MemberID) if err != nil { - return nil, errors.New("unauthorized") + return nil, err } if member == nil { - return nil, errors.New("unauthorized") + return nil, domain.ErrMemberNotFound } return member, nil }🤖 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 `@src/api/member_handler.go` around lines 192 - 219, Update MemberHandler.authenticateMember to return the domain sentinel errors from domain/errors.go: use the session-not-found sentinel for missing cookies, absent sessions, and missing members; return domain.ErrSessionExpired for expired sessions; and propagate repository errors from GetByKey and GetByID instead of replacing them with generic unauthorized errors.
77-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUtilize the defined sentinel error for invalid credentials.
You defined
domain.ErrInvalidCredentialsin this PR. Using it here instead of hardcoding the string promotes consistency with your domain error handling.♻️ Proposed fix
if member == nil { - writeError(w, http.StatusUnauthorized, "invalid credentials") + writeError(w, http.StatusUnauthorized, domain.ErrInvalidCredentials.Error()) return } if err := bcrypt.CompareHashAndPassword([]byte(member.Password), []byte(req.Password)); err != nil { - writeError(w, http.StatusUnauthorized, "invalid credentials") + writeError(w, http.StatusUnauthorized, domain.ErrInvalidCredentials.Error()) return }🤖 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 `@src/api/member_handler.go` around lines 77 - 85, Update the unauthorized error responses in the member authentication flow to use the defined domain.ErrInvalidCredentials sentinel instead of the hardcoded "invalid credentials" string. Apply this consistently for both the nil-member check and bcrypt comparison failure while preserving the existing unauthorized status and early returns.
🤖 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.
Inline comments:
In `@src/api/member_handler.go`:
- Around line 31-35: Validate required fields in the registration and login
handlers after JSON decoding and before proceeding: registration must require
non-empty req.Email, req.Password, and req.Name, while login must require
non-empty req.Email and req.Password. Return the existing bad-request response
for missing fields; apply these changes at src/api/member_handler.go lines 31-35
and 66-70, using the handler symbols and preserving the existing authentication
and registration flows.
In `@src/database/member_repo.go`:
- Around line 70-94: Update MemberRepositoryPGX.Update to retrieve the new
updated_at value atomically within the UPDATE statement by using a RETURNING
clause and scanning the result directly, replacing the separate QueryRow read.
Preserve the existing duplicate-email, not-found, and update-error handling,
ensuring a missing returned row maps to domain.ErrMemberNotFound.
- Around line 10-12: Generate and commit the repository’s go.sum file so it
contains checksums for pgx and all other module dependencies. This applies to
the dependency imports in src/database/member_repo.go lines 10-12 and
src/database/session_repo.go lines 10-11; no source changes are required at
either site.
In `@src/test/database/session_repo_test.go`:
- Around line 98-111: Update TestSessionRepositoryPGX_DeleteByMemberID to retain
each created session key while seeding the three sessions, then call
repo.GetByKey for every retained key after DeleteByMemberID and assert each
result is nil.
---
Nitpick comments:
In `@Makefile`:
- Around line 9-10: Rename the Makefile target db-rm to db-stop to reflect that
docker compose down only stops and removes containers without volumes. Update
any references to the target, or add a separate explicitly named destructive
reset target if volume removal is still required.
In `@plan/member-session-plan.md`:
- Line 153: Update the fenced code block in member-session-plan.md by adding an
appropriate language identifier, such as text, to its opening fence to satisfy
Markdown lint rule MD040.
In `@src/api/member_handler.go`:
- Around line 192-219: Update MemberHandler.authenticateMember to return the
domain sentinel errors from domain/errors.go: use the session-not-found sentinel
for missing cookies, absent sessions, and missing members; return
domain.ErrSessionExpired for expired sessions; and propagate repository errors
from GetByKey and GetByID instead of replacing them with generic unauthorized
errors.
- Around line 77-85: Update the unauthorized error responses in the member
authentication flow to use the defined domain.ErrInvalidCredentials sentinel
instead of the hardcoded "invalid credentials" string. Apply this consistently
for both the nil-member check and bcrypt comparison failure while preserving the
existing unauthorized status and early returns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 970ba299-edbf-45ba-b534-24ae4243fd4f
📒 Files selected for processing (19)
MakefileREADME.mddb/migrations/002_create_members.sqlgo.modmain.goopencode.jsonplan/member-session-plan.mdsrc/api/member_handler.gosrc/api/router.gosrc/database/member_repo.gosrc/database/session_repo.gosrc/domain/errors.gosrc/domain/member.gosrc/domain/repository.gosrc/test/api/handler_test.gosrc/test/api/member_handler_test.gosrc/test/database/member_repo_test.gosrc/test/database/product_repo_test.gosrc/test/database/session_repo_test.go
| var req domain.RegisterRequest | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| writeError(w, http.StatusBadRequest, "invalid request body") | ||
| return | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate required fields in request payloads.
Unlike UpdateMember, both the registration and login flows decode the JSON payload and proceed without verifying that the required fields are populated. This can allow users to register with an empty email and password, bypassing intended business rules because an empty string satisfies the database's NOT NULL constraint.
src/api/member_handler.go#L31-L35: validate thatreq.Email,req.Password, andreq.Nameare not empty before proceeding with registration.src/api/member_handler.go#L66-L70: validate thatreq.Emailandreq.Passwordare not empty before attempting authentication.
📍 Affects 1 file
src/api/member_handler.go#L31-L35(this comment)src/api/member_handler.go#L66-L70
🤖 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 `@src/api/member_handler.go` around lines 31 - 35, Validate required fields in
the registration and login handlers after JSON decoding and before proceeding:
registration must require non-empty req.Email, req.Password, and req.Name, while
login must require non-empty req.Email and req.Password. Return the existing
bad-request response for missing fields; apply these changes at
src/api/member_handler.go lines 31-35 and 66-70, using the handler symbols and
preserving the existing authentication and registration flows.
| "github.com/jackc/pgx/v5" | ||
| "github.com/jackc/pgx/v5/pgconn" | ||
| "github.com/jackc/pgx/v5/pgxpool" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sum_file="$(fd -a '^go\.sum$' | head -n1)"
test -n "$sum_file" || { echo "go.sum is missing"; exit 1; }
rg -n '^github\.com/jackc/pgx/v5 v5\.10\.0 ' "$sum_file"Repository: Neelein/Product_Manger_Backend
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Top-level files:\n'
git ls-files | sed -n '1,120p'
printf '\nGo module files:\n'
fd -a '^(go\.mod|go\.sum)$' .
printf '\nGo sources in src/database:\n'
fd -a '\.go$' src/database
printf '\nReference to pgx/go.sum in repo:\n'
rg -n 'github\.com/jackc/pgx/v5|go\.sum|module ' -g '!*vendor*' .Repository: Neelein/Product_Manger_Backend
Length of output: 2097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Mentions of go.sum / module hygiene:\n'
rg -n 'go\.sum|module|checksum|sumdb|vendor' README.md AGENTS.md decisions plan go.mod .github -g '!**/node_modules/**' || true
printf '\nGo module contents:\n'
cat -n go.mod
printf '\nDatabase repo imports:\n'
sed -n '1,80p' src/database/member_repo.go
printf '\n---\n'
sed -n '1,80p' src/database/session_repo.goRepository: Neelein/Product_Manger_Backend
Length of output: 5113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in [Path('README.md'), Path('AGENTS.md'), Path('go.mod')]:
if p.exists():
print(f'\n== {p} ==')
print(p.read_text()[:4000])
PYRepository: Neelein/Product_Manger_Backend
Length of output: 3360
Commit the generated go.sum. The repo has no go.sum at all, so the pgx entries and the rest of the module checksums are missing.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 10-10: could not import github.com/jackc/pgx/v5 (src/database/member_repo.go:10:2: missing go.sum entry for module providing package github.com/jackc/pgx/v5 (imported by backend/src/database); to add:
go get backend/src/database)
(typecheck)
[error] 11-11: could not import github.com/jackc/pgx/v5/pgconn (src/database/member_repo.go:11:2: missing go.sum entry for module providing package github.com/jackc/pgx/v5/pgconn (imported by backend/src/database); to add:
go get backend/src/database)
(typecheck)
📍 Affects 2 files
src/database/member_repo.go#L10-L12(this comment)src/database/session_repo.go#L10-L11
🤖 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 `@src/database/member_repo.go` around lines 10 - 12, Generate and commit the
repository’s go.sum file so it contains checksums for pgx and all other module
dependencies. This applies to the dependency imports in
src/database/member_repo.go lines 10-12 and src/database/session_repo.go lines
10-11; no source changes are required at either site.
Source: Linters/SAST tools
| func (r *MemberRepositoryPGX) Update(ctx context.Context, member *domain.Member) error { | ||
| ct, err := r.pool.Exec(ctx, | ||
| `UPDATE members | ||
| SET email = $1, name = $2, updated_at = now() | ||
| WHERE id = $3`, | ||
| member.Email, member.Name, member.ID, | ||
| ) | ||
| if err != nil { | ||
| var pgErr *pgconn.PgError | ||
| if errors.As(err, &pgErr) && pgErr.Code == "23505" { | ||
| return domain.ErrEmailAlreadyExists | ||
| } | ||
| return fmt.Errorf("updating member: %w", err) | ||
| } | ||
|
|
||
| if ct.RowsAffected() == 0 { | ||
| return domain.ErrMemberNotFound | ||
| } | ||
|
|
||
| err = r.pool.QueryRow(ctx, | ||
| `SELECT updated_at FROM members WHERE id = $1`, member.ID, | ||
| ).Scan(&member.UpdatedAt) | ||
| if err != nil { | ||
| return fmt.Errorf("reading updated timestamp: %w", err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return updated_at from the update atomically.
The UPDATE commits before the second query. If that read fails or the row is concurrently deleted, callers receive an error even though their update succeeded.
Proposed fix
func (r *MemberRepositoryPGX) Update(ctx context.Context, member *domain.Member) error {
- ct, err := r.pool.Exec(ctx,
+ err := r.pool.QueryRow(ctx,
`UPDATE members
SET email = $1, name = $2, updated_at = now()
- WHERE id = $3`,
+ WHERE id = $3
+ RETURNING updated_at`,
member.Email, member.Name, member.ID,
- )
+ ).Scan(&member.UpdatedAt)
if err != nil {
+ if errors.Is(err, pgx.ErrNoRows) {
+ return domain.ErrMemberNotFound
+ }
var pgErr *pgconn.PgError
if errors.As(err, &pgErr) && pgErr.Code == "23505" {
return domain.ErrEmailAlreadyExists
}
return fmt.Errorf("updating member: %w", err)
}
-
- if ct.RowsAffected() == 0 {
- return domain.ErrMemberNotFound
- }
-
- err = r.pool.QueryRow(ctx,
- `SELECT updated_at FROM members WHERE id = $1`, member.ID,
- ).Scan(&member.UpdatedAt)
- if err != nil {
- return fmt.Errorf("reading updated timestamp: %w", err)
- }
-
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *MemberRepositoryPGX) Update(ctx context.Context, member *domain.Member) error { | |
| ct, err := r.pool.Exec(ctx, | |
| `UPDATE members | |
| SET email = $1, name = $2, updated_at = now() | |
| WHERE id = $3`, | |
| member.Email, member.Name, member.ID, | |
| ) | |
| if err != nil { | |
| var pgErr *pgconn.PgError | |
| if errors.As(err, &pgErr) && pgErr.Code == "23505" { | |
| return domain.ErrEmailAlreadyExists | |
| } | |
| return fmt.Errorf("updating member: %w", err) | |
| } | |
| if ct.RowsAffected() == 0 { | |
| return domain.ErrMemberNotFound | |
| } | |
| err = r.pool.QueryRow(ctx, | |
| `SELECT updated_at FROM members WHERE id = $1`, member.ID, | |
| ).Scan(&member.UpdatedAt) | |
| if err != nil { | |
| return fmt.Errorf("reading updated timestamp: %w", err) | |
| } | |
| func (r *MemberRepositoryPGX) Update(ctx context.Context, member *domain.Member) error { | |
| err := r.pool.QueryRow(ctx, | |
| `UPDATE members | |
| SET email = $1, name = $2, updated_at = now() | |
| WHERE id = $3 | |
| RETURNING updated_at`, | |
| member.Email, member.Name, member.ID, | |
| ).Scan(&member.UpdatedAt) | |
| if err != nil { | |
| if errors.Is(err, pgx.ErrNoRows) { | |
| return domain.ErrMemberNotFound | |
| } | |
| var pgErr *pgconn.PgError | |
| if errors.As(err, &pgErr) && pgErr.Code == "23505" { | |
| return domain.ErrEmailAlreadyExists | |
| } | |
| return fmt.Errorf("updating member: %w", err) | |
| } | |
| return nil | |
| } |
🤖 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 `@src/database/member_repo.go` around lines 70 - 94, Update
MemberRepositoryPGX.Update to retrieve the new updated_at value atomically
within the UPDATE statement by using a RETURNING clause and scanning the result
directly, replacing the separate QueryRow read. Preserve the existing
duplicate-email, not-found, and update-error handling, ensuring a missing
returned row maps to domain.ErrMemberNotFound.
| func TestSessionRepositoryPGX_DeleteByMemberID(t *testing.T) { | ||
| defer cleanupSessions(t) | ||
| repo := database.NewSessionRepositoryPGX(testPool) | ||
| member := createTestMemberForSession(t) | ||
|
|
||
| for i := 0; i < 3; i++ { | ||
| session := domain.Session{MemberID: member.ID} | ||
| err := repo.Create(context.Background(), &session) | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| err := repo.DeleteByMemberID(context.Background(), member.ID) | ||
| assert.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify that every member session was deleted.
This test passes even if DeleteByMemberID is a no-op. Retain each generated key and assert that GetByKey returns nil afterward.
🤖 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 `@src/test/database/session_repo_test.go` around lines 98 - 111, Update
TestSessionRepositoryPGX_DeleteByMemberID to retain each created session key
while seeding the three sessions, then call repo.GetByKey for every retained key
after DeleteByMemberID and assert each result is nil.
1 participant
Changes
POST /api/members/updateauthenticateMemberhelper — Shared auth logic used by bothGetCurrentMemberandUpdateMemberMemberRepository.Updatemethod with duplicate email detection and not-found handlingErrMemberNotFoundsentinel errorFiles changed
src/domain/member.goUpdateMemberRequestsrc/domain/errors.goErrMemberNotFoundsrc/domain/repository.goUpdatetoMemberRepositoryinterfacesrc/database/member_repo.goUpdate(email, name, updated_at)src/api/member_handler.goUpdateMemberhandler, extractedauthenticateMembersrc/api/router.goPOST /api/members/updaterouteREADME.mddb/migrations/002_create_members.sqlAPI
Closes member profile update requirement.
Summary by CodeRabbit