Fix #16: Repository layer code review: fix critical transaction bug and high-severity issues #17

Merged
hermes merged 2 commits from fix/issue-16 into master 2026-07-20 16:49:36 +00:00
4 changed files with 97 additions and 5 deletions

View File

@ -9,6 +9,7 @@ import (
"strings"
"time"
cerrors "git.loyso.art/frx/kurious/internal/common/errors"
"git.loyso.art/frx/kurious/internal/common/nullable"
"git.loyso.art/frx/kurious/internal/common/xslices"
"git.loyso.art/frx/kurious/internal/kurious/domain"
@ -76,6 +77,20 @@ func (r *sqliteCourseRepository) List(
if !params.Ascending {
direction = "DESC"
}
allowedOrderBy := map[string]struct{}{
Outdated
Review

Use map[string]struct{} for fields white-list.

Use `map[string]struct{}` for fields white-list.
"id": {},
"name": {},
"created_at": {},
"updated_at": {},
"full_price": {},
"discount": {},
"duration": {},
}
if _, ok := allowedOrderBy[params.OrderBy]; !ok {
return result, fmt.Errorf("invalid order by field: %s", params.OrderBy)
}
query += " ORDER BY " + params.OrderBy + " " + direction
if params.Limit > 0 {
@ -277,6 +292,9 @@ func (r *sqliteCourseRepository) Get(
var courseDB sqliteCourseDB
err = r.db.GetContext(ctx, &courseDB, query, id)
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
return course, cerrors.ErrNotFound
}
return course, fmt.Errorf("executing query: %w", err)
}
@ -289,7 +307,7 @@ func (r *sqliteCourseRepository) GetByExternalID(
return course, errors.New("not implemented")
}
func (r *sqliteCourseRepository) CreateBatch(ctx context.Context, params ...domain.CreateCourseParams) error {
func (r *sqliteCourseRepository) CreateBatch(ctx context.Context, params ...domain.CreateCourseParams) (err error) {
tx, err := r.db.BeginTxx(ctx, &sql.TxOptions{Isolation: sql.LevelDefault})
if err != nil {
return fmt.Errorf("beginning tx: %w", err)
@ -317,9 +335,9 @@ func (r *sqliteCourseRepository) CreateBatch(ctx context.Context, params ...doma
}
for _, param := range params {
_, err := stmt.ExecContext(ctx, createCourseParamsAsValues(param)...)
if err != nil {
return fmt.Errorf("executing statement query: %w", err)
_, execErr := stmt.ExecContext(ctx, createCourseParamsAsValues(param)...)
if execErr != nil {
return fmt.Errorf("executing statement query: %w", execErr)
}
}

View File

@ -1,10 +1,12 @@
package adapters
import (
"errors"
"strconv"
"testing"
"time"
cerrors "git.loyso.art/frx/kurious/internal/common/errors"
"git.loyso.art/frx/kurious/internal/common/nullable"
"git.loyso.art/frx/kurious/internal/kurious/domain"
@ -105,3 +107,33 @@ func (s *sqliteCourseRepositorySuite) TestListLimitOffset() {
s.NoError(err)
s.Empty(result.Courses)
}
func (s *sqliteCourseRepositorySuite) TestCreateBatchPartialFailure() {
cr := s.connection.CourseRepository()
baseParams := domain.CreateCourseParams{
SourceType: domain.SourceTypeManual,
}
first := baseParams
first.ID = "dup-id"
duplicate := baseParams
duplicate.ID = "dup-id"
err := cr.CreateBatch(s.ctx, first, duplicate)
s.Require().Error(err)
result, listErr := cr.List(s.ctx, domain.ListCoursesParams{})
s.Require().NoError(listErr)
s.Empty(result.Courses, "partial batch should be rolled back, not committed")
s.Zero(result.Count, "partial batch should be rolled back, not committed")
}
func (s *sqliteCourseRepositorySuite) TestGetNotFound() {
cr := s.connection.CourseRepository()
_, err := cr.Get(s.ctx, "does-not-exist")
s.Require().Error(err)
s.True(errors.Is(err, cerrors.ErrNotFound), "expected ErrNotFound, got %v", err)
}

View File

@ -187,7 +187,7 @@ func (r *sqliteOrganizationRepository) List(ctx context.Context, params domain.L
}()
organizations := make([]organizationDB, 0, 1<<8)
err = r.db.SelectContext(ctx, &organizations, query)
err = r.db.SelectContext(ctx, &organizations, query, args...)
if err != nil {
return nil, fmt.Errorf("executing query: %w", err)
}

View File

@ -126,3 +126,45 @@ func (s *sqliteOrganzationRepositorySuite) TestCreate() {
expectedOrganization.UpdatedAt = gotOrganization.UpdatedAt
s.Equal(expectedOrganization, gotOrganization)
}
func (s *sqliteOrganzationRepositorySuite) TestListByIDs() {
const itemscount = 3
baseOrg := domain.Organization{
Alias: "test-alias",
Name: "test-name",
Site: "test-site",
LogoLink: "test-logo",
}
ids := make([]string, 0, itemscount)
for i := 0; i < itemscount; i++ {
iStr := strconv.Itoa(i)
id := "test-id-" + iStr
ids = append(ids, id)
_, err := s.connection.Organization().Create(s.ctx, domain.CreateOrganizationParams{
ID: id,
ExternalID: nullable.NewValue("test-ext-id-" + iStr),
Alias: baseOrg.Alias,
Name: baseOrg.Name,
Site: baseOrg.Site,
LogoLink: baseOrg.LogoLink,
})
s.Require().NoError(err)
}
wantedIDs := ids[:2]
gotOrgs, err := s.connection.Organization().List(s.ctx, domain.ListOrganizationsParams{
IDs: wantedIDs,
})
s.Require().NoError(err)
gotIDs := make([]string, 0, len(gotOrgs))
for _, o := range gotOrgs {
gotIDs = append(gotIDs, o.ID)
}
s.Len(gotOrgs, len(wantedIDs))
s.ElementsMatch(wantedIDs, gotIDs)
}