Skip to content

fix(user): propagate DB error in CheckEmailExist before existence check#2718

Open
nankingjing wants to merge 1 commit into
coze-dev:mainfrom
nankingjing:fix-checkemailexist-swallow-db-error
Open

fix(user): propagate DB error in CheckEmailExist before existence check#2718
nankingjing wants to merge 1 commit into
coze-dev:mainfrom
nankingjing:fix-checkemailexist-swallow-db-error

Conversation

@nankingjing

Copy link
Copy Markdown

Problem

backend/domain/user/internal/dal/user.go line 124, UserDAO.CheckEmailExist has a reversed check order that swallows real DB errors.

func (dao *UserDAO) CheckEmailExist(ctx context.Context, email string) (bool, error) {
	_, exist, err := dao.GetUsersByEmail(ctx, email)
	if !exist {
		return false, nil   // returns first, even when err != nil
	}
	if err != nil {
		return false, err   // dead code when exist == false
	}
	return true, nil
}

GetUsersByEmail returns three cases:

case return
record not found (nil, false, nil)
real DB error (nil, false, err)
found (user, true, nil)

On a real DB error it returns exist=false and err != nil. Because CheckEmailExist checks !exist first, it returns (false, nil) and never reaches the if err != nil branch, so that branch is dead code. The transient DB error is silently swallowed and reported as "email does not exist".

External consequence

userImpl.Create (backend/domain/user/service/user_impl.go, the signup path) does:

exist, err := u.UserRepo.CheckEmailExist(ctx, req.Email)
if err != nil {
	return nil, err
}
if exist {
	return nil, errorx.New(errno.ErrUserEmailAlreadyExistCode, ...)
}
// ... otherwise proceeds to create the user

When the existence query hits a transient DB error, CheckEmailExist returns (false, nil), so Create treats the email as free and proceeds to CreateUser instead of aborting with the error. This masks the failure and leads to a wrong signup outcome (duplicate-account attempt / unique-constraint failure on insert) instead of a clean, retryable error.

Fix

Return the error before the existence check, so real DB errors propagate while the not-found and found cases keep their exact prior behavior:

_, exist, err := dao.GetUsersByEmail(ctx, email)
if err != nil {
	return false, err
}
if !exist {
	return false, nil
}
return true, nil

Behavior table (unchanged for correct cases, fixed for the error case):

case before after
not found (false, nil) (false, nil)
DB error (false, nil) swallowed (false, err) propagated
found (true, nil) (true, nil)

The (bool, error) contract is preserved. Minimal, focused diff (statement reorder only).

Verification

No Go toolchain in the environment (go: command not found), so verified by reading and tracing the two functions and the caller; not executed.

CheckEmailExist checked !exist before err, but GetUsersByEmail returns
exist=false on a real DB error too. The !exist branch returned (false, nil)
first, swallowing the error and making the err!=nil branch dead code. A
transient DB error thus made an email look free, so signup (userImpl.Create)
proceeded instead of failing. Reorder to return the error first.
@nankingjing

Copy link
Copy Markdown
Author

Review: fix-checkemailexist-swallow-db-error (#2718)

Verdict: LGTM

The original code checked !exist before checking err, so a DB error that happened to return exist=false would be silently swallowed. The fix reorders the checks: error first, then existence. This is the correct Go idiom -- always check the error before trusting the value.

Before: GetUsersByEmail fails with DB error, exist=false -> returns (false, nil) -- error swallowed.
After: GetUsersByEmail fails with DB error -> returns (false, err) -- error propagated.

This is a minimal, correct one-line-adjacent fix. No test included, but the logic is straightforward. Merge-ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant