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

Per review feedback on PR #17, comment #137: use map[string]struct{}
(set semantics) instead of map[string]bool for the ORDER BY field
whitelist. Updates the lookup to comma-ok idiom.

docs/ARCHITECTURE.md

@@ -0,0 +1,349 @@
+# Architecture
+
+## Patterns
+
+### Hexagonal Architecture (Ports & Adapters)
+
+The project follows **hexagonal architecture** — business logic lives at the center
+and all I/O (databases, HTTP, external APIs) is pushed to the periphery through
+interfaces. The domain never imports infrastructure packages.
+
+```
+┌─────────────────────────────────────────────────────┐
+│  Ports (delivery mechanisms)                         │
+│    ports/http/       — HTTP server (gorilla/mux)     │
+│    ports/background/ — Cron jobs (robfig/cron)       │
+└──────────────────────┬──────────────────────────────┘
+                       │ depends on
+┌──────────────────────▼──────────────────────────────┐
+│  Application layer (use cases)                      │
+│    app/command/   — write-side handlers             │
+│    app/query/     — read-side handlers              │
+│    decorator/     — handler interfaces + logging     │
+└──────────────────────┬──────────────────────────────┘
+                       │ depends on
+┌──────────────────────▼──────────────────────────────┐
+│  Domain (pure business rules)                        │
+│    domain/  — entities (Course, Organization, ...)    │
+│    domain/  — repository interfaces (ports)          │
+│    domain/  — mapper interface (CourseMapper)        │
+└──────────────────────┬──────────────────────────────┘
+                       │ implemented by
+┌──────────────────────▼──────────────────────────────┐
+│  Adapters (infrastructure)                           │
+│    adapters/sqlite_*_repository.go — SQLite repos     │
+│    adapters/memory_mapper.go      — in-memory mapper │
+│    adapters/ydb_*                 — legacy YDB stubs │
+│    adapters/not_implemented.go    — sentinel stubs  │
+└─────────────────────────────────────────────────────┘
+```
+
+### CQRS (Command-Query Responsibility Segregation)
+
+The application layer splits all use cases into **commands** (writes) and
+**queries** (reads), each with its own handler type:
+
+- **Commands** implement `decorator.CommandHandler[T]` — `Handle(ctx, params) error`
+- **Queries** implement `decorator.QueryHandler[Q, U]` — `Handle(ctx, query) (result, error)`
+
+Both are aggregated into `app.Application{Commands, Queries}` and exposed through
+`service.Application`.
+
+### Decorator Pattern
+
+Every handler is wrapped with a logging + tracing decorator at construction time
+(`decorator.ApplyCommandDecorators` / `decorator.AddQueryDecorators`). The
+decorator:
+
+1. Creates an OpenTelemetry span (`"command <Name>"` / `"query <Name>"`)
+2. Logs structured fields (handler name, serialized args)
+3. Measures elapsed time
+4. Records errors on the span
+5. Delegates to the base handler
+
+The decorator is applied inside `New*Handler()` constructors — callers never
+see the raw handler.
+
+### Composition Root
+
+`service.NewApplication` is the single place where all wiring happens: it selects
+the DB engine, creates connections, instantiates adapters, registers handlers
+on the `Application` struct, and injects the `CourseMapper`. Binaries in `cmd/`
+call this function.
+
+### Repository Interface Segregation
+
+Each aggregate has its own repository interface defined in `domain/`:
+
+| Interface                 | Methods (key)                              |
+|---------------------------|---------------------------------------------|
+| `CourseRepository`        | List, Get, GetByExternalID, Create, CreateBatch, Delete, UpdateCourseDescription, ListStatistics |
+| `OrganizationRepository`  | List, Get, ListStats, Create, Delete        |
+| `LearningCategoryRepository` | List, Get, Upsert                         |
+
+Adapters implement these interfaces. Unimplemented methods use
+`ErrNotImplemented` sentinel stubs.
+
+### External Client Abstraction
+
+The sravni.ru HTTP client (`common/client/sravni/Client` interface) is abstracted
+behind an interface with a `noop.Client` fallback. The background sync handler
+depends on the interface, not the concrete HTTP implementation.
+
+## Logic Placement
+
+| Concern                        | Where it lives                                    |
+|-------------------------------|---------------------------------------------------|
+| Business entities             | `internal/kurious/domain/`                        |
+| Repository interfaces (ports) | `internal/kurious/domain/repository.go`           |
+| Mapper interface              | `internal/kurious/domain/mapper.go`               |
+| Write-side use cases          | `internal/kurious/app/command/`                   |
+| Read-side use cases           | `internal/kurious/app/query/`                     |
+| Handler interfaces + decorators| `internal/common/decorator/`                      |
+| Repository implementations    | `internal/kurious/adapters/`                      |
+| HTTP routing + handlers       | `internal/kurious/ports/http/`                    |
+| Templ templates               | `internal/kurious/ports/http/bootstrap/*.templ`   |
+| Background jobs               | `internal/kurious/ports/background/`              |
+| Composition root             | `internal/kurious/service/service.go`             |
+| HTTP client (sravni.ru)      | `internal/common/client/sravni/`                   |
+| Config structs                | `internal/common/config/`                        |
+| Error sentinels               | `internal/common/errors/`                        |
+| Logging helpers               | `internal/common/xcontext/`, `common/xlog/`       |
+| Utility functions             | `internal/common/xslices/`, `common/nullable/`    |
+| ID generation                 | `internal/common/generator/`                     |
+| Entry points (binaries)       | `cmd/kuriweb/`, `cmd/background/`, `cmd/dev/`    |
+| SQL migrations                | `migrations/sqlite/` (embedded via `go:embed`)     |
+| Static web assets             | `assets/kurious/static/` (embedded via `go:embed`) |
+| Public package                | `pkg/xdefault/`                                    |
+
+## Layers
+
+### 1. Domain Layer (`internal/kurious/domain/`)
+
+Pure Go types — no framework imports, no I/O. Contains:
+
+- **Entities:** `Course`, `Organization`, `LearningCategory` — structs with
+  fields and no behavior beyond data representation.
+- **Repository interfaces:** `CourseRepository`, `OrganizationRepository`,
+  `LearningCategoryRepository` — the ports that adapters must implement.
+- **Mapper interface:** `CourseMapper` — maps external dictionary IDs to
+  human-readable names; implemented by `adapters/memory_mapper.go`.
+- **Value objects:** `SourceType`, `ListCoursesParams`, `CreateCourseParams`,
+  etc. — parameter types used by repository interfaces.
+
+### 2. Application Layer (`internal/kurious/app/`)
+
+Orchestrates domain objects through repository ports:
+
+- **`app.go`** — `Application{Commands, Queries}` aggregate struct. Groups all
+  handlers into a single type.
+- **`command/`** — Write handlers: `CreateCourse`, `CreateCourses`,
+  `DeleteCourse`, `UpdateCourseDescription`, `CreateOrganization`. Each is a
+  struct holding repository dependencies, with a `New*Handler()` constructor
+  that wraps the base with decorators.
+- **`query/`** — Read handlers: `GetCourse`, `ListCourses`, `ListLearningTypes`,
+  `ListCourseThematics`, `ListCoursesStats`, `ListOrganizations`,
+  `ListOrganizationsStats`, `GetOrganization`. Some queries (e.g. `ListCourses`)
+  implement cursor-based pagination with configurable batch sizes.
+
+### 3. Common Infrastructure (`internal/common/`)
+
+Shared packages used across the application:
+
+- **`decorator/`** — Generic `CommandHandler[T]` and `QueryHandler[Q, U]`
+  interfaces + logging/tracing decorator implementations. Uses OpenTelemetry spans
+  and structured slog logging.
+- **`client/sravni/`** — HTTP client for sravni.ru with `Client` interface,
+  `noop.Client` fallback, entities, and helpers. Uses `go-resty` internally.
+- **`config/`** — Typed config structs: `Log`, `HTTP`, `Sqlite`, `Trace`, `YDB`,
+  `Duration`.
+- **`errors/`** — `SimpleError`, `ValidationError`, sentinel errors
+  (`ErrNotFound`, `ErrNotImplemented`). Mapped to HTTP status codes in `ports/http`.
+- **`xslices/`** — Generic slice utilities: `Map`, `Filter`, `ForEach`, `LRU`.
+- **`nullable/`** — Generic `Value[T]` for nullable fields (wraps a value + valid flag).
+- **`xcontext/`** — Context helpers for propagating request-scoped log fields.
+- **`xlog/`** — Adapters bridging `slog` to `cron` logger interface.
+- **`generator/`** — ID generators (`RandomInt64ID`).
+
+### 4. Adapters Layer (`internal/kurious/adapters/`)
+
+Concrete implementations of domain interfaces:
+
+- **`sqlite_*_repository.go`** — SQLite-backed implementations using `sqlx`
+  named queries. Each defines a row type with `AsDomain()` for entity mapping.
+- **`sqlite_connection.go`** — Connection factory; applies embedded migrations,
+  returns ready-to-use repository instances.
+- **`memory_mapper.go`** — In-memory `CourseMapper` implementation; loads
+  dictionary counts from the course repository, maps IDs ↔ names.
+- **`not_implemented.go`** — Sentinel adapter types returning `ErrNotImplemented`
+  for methods not yet wired up.
+- **`ydb_*`** — Legacy YDB adapter stubs (engine no longer supported at runtime).
+
+All DB operations create OpenTelemetry spans with `db.*` semantic attributes.
+
+### 5. Ports Layer (`internal/kurious/ports/`)
+
+Delivery mechanisms that drive the application:
+
+- **`http/`** — HTTP server using `gorilla/mux`. `Server` struct holds
+  `service.Application` and delegates to handlers. `course.go` contains the main
+  `List` and `Index` handlers that call query handlers, map results to templ
+  params, and render HTML.
+- **`http/bootstrap/`** — Templ (`.templ`) template files compiled to Go.
+  Defines view parameter structs and HTML rendering.
+- **`background/`** — `BackgroundProcess` wrapping `robfig/cron`. Registers
+  scheduled handlers like `SyncSravniHandler`.
+- **`background.go`** — Cron scheduler with job registration, shutdown, and
+  next-run stats.
+- **`services.go`** — `Services{HTTP, Background}` aggregate — the outermost
+  composition point.
+
+### 6. Service Layer (`internal/kurious/service/`)
+
+The **composition root**:
+
+- **`service.go`** — `NewApplication()` selects DB engine, creates connections,
+  builds adapter instances, constructs all command/query handlers (with logging
+  decorators), and assembles the `Application` struct. Also owns `Close()` for
+  resource cleanup.
+
+### 7. Entry Points (`cmd/`)
+
+Three binaries, each in its own package:
+
+| Binary | Location | Purpose |
+|--------|----------|---------|
+| `kuriousweb` | `cmd/kuriweb/` | HTTP web server — starts server + optional background process |
+| `kuriousbg` | `cmd/background/` | Background sync process — runs cron scheduler only |
+| `sravnicli` | `cmd/dev/sravnicli/` | Developer CLI for inspecting sravni.ru data |
+
+Each binary reads a JSON config file, constructs `service.Application` via
+`service.NewApplication`, and starts its delivery mechanism.
+
+## Structure
+
+```
+.
+├── cmd/                                # Entry points
+│   ├── kuriweb/                        #   HTTP server binary
+│   ├── background/                     #   Background sync binary
+│   └── dev/sravnicli/                   #   Developer CLI
+│
+├── internal/
+│   ├── kurious/                        # Application core (hexagon)
+│   │   ├── domain/                     #   Entities + repository ports
+│   │   │   ├── course.go               #     Course entity
+│   │   │   ├── organization.go         #     Organization entity
+│   │   │   ├── category.go             #     LearningCategory entity
+│   │   │   ├── kurious.go              #     SourceType enum, shared domain types
+│   │   │   ├── mapper.go               #     CourseMapper interface
+│   │   │   └── repository.go           #     Repository interfaces + params
+│   │   │
+│   │   ├── app/                        #   Application layer (use cases)
+│   │   │   ├── app.go                  #     Application{Commands, Queries}
+│   │   │   ├── command/                #     Write-side handlers
+│   │   │   │   ├── createcourse.go     #       CreateCourse, CreateCourses
+│   │   │   │   ├── createorganization.go
+│   │   │   │   ├── deletecourse.go
+│   │   │   │   └── updatecoursedescription.go
+│   │   │   └── query/                  #     Read-side handlers
+│   │   │       ├── getcourse.go
+│   │   │       ├── getorganization.go
+│   │   │       ├── listcourses.go
+│   │   │       ├── listcoursesstats.go
+│   │   │       ├── listcoursethematics.go
+│   │   │       ├── listlearningtypes.go
+│   │   │       ├── listorganizations.go
+│   │   │       └── listorganizationstats.go
+│   │   │
+│   │   ├── adapters/                   #   Repository implementations
+│   │   │   ├── sqlite_course_repository.go
+│   │   │   ├── sqlite_organization_repository.go
+│   │   │   ├── sqlite_learning_category_repository.go
+│   │   │   ├── sqlite_connection.go    #     Connection factory + migrations
+│   │   │   ├── memory_mapper.go        #     In-memory CourseMapper
+│   │   │   ├── not_implemented.go      #     ErrNotImplemented stubs
+│   │   │   ├── adapters.go             #     Shared DB helpers, tracing
+│   │   │   └── ydb_course_repository.go #   Legacy YDB stub
+│   │   │
+│   │   ├── ports/                      #   Delivery mechanisms
+│   │   │   ├── http/                   #     HTTP server
+│   │   │   │   ├── server.go           #       Server, error handling, pagination
+│   │   │   │   ├── course.go           #       Course list/index handlers
+│   │   │   │   └── bootstrap/          #       Templ templates + params
+│   │   │   ├── background/             #     Cron job handlers
+│   │   │   │   └── synchandler.go      #       Sravni sync handler
+│   │   │   ├── background.go           #     Cron scheduler
+│   │   │   └── services.go             #     Services aggregate
+│   │   │
+│   │   └── service/                    #   Composition root
+│   │       └── service.go              #     NewApplication wiring
+│   │
+│   └── common/                         # Shared infrastructure
+│       ├── decorator/                  #   Handler interfaces + logging decorators
+│       ├── client/sravni/             #   Sravni.ru HTTP client
+│       ├── config/                     #   Config structs
+│       ├── errors/                     #   Error types + sentinels
+│       ├── generator/                  #   ID generation
+│       ├── nullable/                   #   Generic nullable Value[T]
+│       ├── xcontext/                   #   Context log helpers
+│       ├── xlog/                       #   Logger adapters
+│       └── xslices/                    #   Slice utilities
+│
+├── pkg/xdefault/                        # Public package (WithFallback helper)
+├── migrations/sqlite/                  # SQL migrations (go:embed)
+├── assets/kurious/static/               # Static assets (go:embed)
+├── docs/                                # Documentation
+│   └── ARCHITECTURE.md                  #   This file
+└── htmlexamples/                        # Templ prototyping
+```
+
+## Data Flow
+
+### HTTP Request (read path)
+
+```
+HTTP request
+  → gorilla/mux router (ports/http/server.go)
+    → handler method (ports/http/course.go)
+      → parse query params + pagination
+        → app.Queries.ListCourses.Handle()     [application layer]
+          → decorator: create span, log args    [decorator]
+            → courseHandler.Handle()            [application handler]
+              → domain.CourseRepository.List() [domain port]
+                → sqlite_course_repository      [adapter]
+                  → sqlx named query → SQLite   [infrastructure]
+          ← domain.ListCoursesResult
+      ← result → map to templ params
+  ← templ.Render(ctx, w) → HTML response
+```
+
+### Background Sync (write path)
+
+```
+cron tick
+  → BackgroundProcess scheduler (ports/background.go)
+    → syncSravniHandler.Handle()                  [port handler]
+      → fill caches (known external IDs, orgs)
+      → sravni.Client.GetMainPageState()          [external API]
+      → for each learning type:
+          → client.ListEducationalProducts()     [rate-limited]
+            → deduplicate courses/orgs
+        → app.Commands.InsertCourses.Handle()   [application layer]
+          → domain.CourseRepository.CreateBatch() [domain port]
+            → sqlite_course_repository            [adapter]
+```
+
+## Key Design Decisions
+
+1. **Pure-Go SQLite** (`modernc.org/sqlite`) — no CGO dependency, builds anywhere.
+2. **Embedded migrations** — SQL files in `migrations/sqlite/` are compiled into
+   the binary via `go:embed` and applied at startup in a transaction.
+3. **Templ for HTML** — type-safe templates compiled to Go; generated `_templ.go`
+   files are committed to the repo.
+4. **CGO_ENABLED=0 everywhere** — enforced by Taskfile and build flags.
+5. **Go 1.26** — latest toolchain; see `go.mod`.
+6. **OpenTelemetry** — spans and metrics on every command/query handler and DB
+   operation; exports to stdout or OTLP (HTTP/gRPC).
+7. **Single composition root** — `service.NewApplication` is the only place that
+   knows about concrete adapter types. Tests can swap adapters via interfaces.

internal/kurious/adapters/sqlite_course_repository.go

@@ -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{}{
+		"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)...)
+		_, execErr := stmt.ExecContext(ctx, createCourseParamsAsValues(param)...)
-		if err != nil {
+		if execErr != nil {
-			return fmt.Errorf("executing statement query: %w", err)
+			return fmt.Errorf("executing statement query: %w", execErr)
 		}
 	}
 

internal/kurious/adapters/sqlite_course_repository_test.go

@@ -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)
+}

internal/kurious/adapters/sqlite_organization_repository.go

@@ -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)
 	}

internal/kurious/adapters/sqlite_organization_repository_test.go

@@ -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)
+}

internal/kurious/service/service.go

@@ -55,7 +55,14 @@ func NewApplication(ctx context.Context, cfg ApplicationConfig, mapper domain.Co
 		organizationrepo = sqliteConnection.Organization()
 		repoCloser = sqliteConnection
 	case RepositoryEngineYDB:
-		return Application{}, errors.New("ydb is no longer supported")
+		ydbConn, err := adapters.NewYDBConnection(ctx, cfg.YDB, log.With(slog.String("db", "ydb")))
+		if err != nil {
+			return Application{}, fmt.Errorf("making ydb connection: %w", err)
+		}
+
+		courseadapter = ydbConn.CourseRepository()
+		organizationrepo = ydbConn.Organization()
+		repoCloser = ydbConn
 	default:
 		return Application{}, errors.New("unable to decide which db engine to use")
 	}