fix(nim-allographer/baradb): critical client fixes and bug audit

- Export hidden symbols in baradb_client.nim (socket, nextId, readQueryResponse, toString, toBytes) - Add missing imports in baradb_exec.nim (asyncnet, enums, baradb_query) - Rewrite toJson() to use wire protocol columnTypes instead of guessing types from strings - Fix aggregate functions (count, avg, sum) to handle JInt/JFloat/JString correctly - Fix paginate/fastPaginate discard issues - Add placeholder arguments for whereIn/whereNotIn/whereBetween/whereNotBetween - Add placeholder count validation in formatSql() - Fix getRowPlain() to avoid IndexDefect on empty results - Harden transactionStart/transactionEnd against server crashes - Reset placeHolder in insertSql() to avoid stale values - Fix test_query.nim to use backtick quoting for BaraDB compatibility Known server bugs documented in BARADB_CLIENT_BUGS.md
2026-05-21 22:01:55 +03:00
parent bb843b9a03
commit 837b4d56fc
6 changed files with 202 additions and 31 deletions
@@ -0,0 +1,104 @@
+# Оставащи бъгове — nim-allographer (BaraDB клиент) + BaraDB сървър
+
+> Дата: 2026-05-21
+> Поправени в тази сесия: компилационни грешки, `toJson()`, агрегати, `whereIn`/`whereBetween` placeholder-и, `paginate`, transaction cleanup, `insertSql` state.
+
+---
+
+## 🔴 Критични — блокират функционалност
+
+### 1. [СЪРВЪР] SIGSEGV при INSERT в транзакция
+
+**Описание:** `BEGIN` работи, но при първи `INSERT` след него сървърът пада с `Illegal storage access. (Attempt to read from nil?)`.
+
+**Компонент:** BaraDB сървър — транзакционен engine.
+
+**Възпроизвеждане:**
+```sql
+BEGIN;
+INSERT INTO `users` (`name`) VALUES ('Alice');  -- тук пада
+```
+
+**Лог:**
+```
+[1] Query: BEGIN
+[1] Query: INSERT INTO `users` (`name`, `email`, `age`) VALUES ('Alice', 'alice@example.com', 30)
+SIGSEGV: Illegal storage access.
+```
+
+**Забележка:** Не е клиентски бъг. Клиентът изпраща коректно; сървърът крашва при обработка.
+
+---
+
+### 2. [СЪРВЪР] Parser error за резервирани думи като колони
+
+**Описание:** `CREATE TABLE` с колони на име `label` или `count` хвърля `Expected tkIdent but got tkLabels at line 3`. Вероятно `label` е token тип `tkLabels` и lexer/parser-ът не го разпознава като валиден идентификатор.
+
+**Компонент:** BaraDB сървър — lexer/parser.
+
+**Възпроизвеждане:**
+```sql
+CREATE TABLE test (
+  id SERIAL PRIMARY KEY,
+  label VARCHAR(255),   -- грешка тук
+  count INTEGER         -- и тук
+);
+```
+
+**Забележка:** Като workaround в клиента може да се използват backtick-ове (`` ` ``), но сървърът трябва да поддържа всякакви имена.
+
+---
+
+## 🟡 Важни — качество и сигурност
+
+### 3. [КЛИЕНТ] SQL injection риск в `formatSql()`
+
+**Описание:** Целият query builder използва `formatSql()` който прави **client-side string interpolation** (`?` → стойност). Това е уязвимост при злонамерен input.
+
+**Файл:** `clients/nim-allographer/src/allographer/query_builder/models/baradb/baradb_exec.nim`, процедура `formatSql()`.
+
+**Проблем:**
+```nim
+proc formatSql*(sql: string, args: seq[JsonNode]): string =
+  result = sql
+  for arg in args:
+    let pos = result.find("?")
+    result = result[0..<pos] & escapeSqlValue(arg) & result[pos+1..^1]
+```
+
+**Решение:** Query builder-ът трябва да изпраща заявките чрез **prepared statements** (`mkQueryParams` в wire protocol), вместо да интерполира стойностите в SQL string. Prepared statements вече са имплементирани (`preparedGet`, `preparedExec` в `baradb_exec.nim`), но query builder-ът ги заобикаля.
+
+**Обхват:** Всички `.where()`, `.insert()`, `.update()`, `.delete()` методи в query builder.
+
+---
+
+### 4. [КЛИЕНТ] Schema builder използва `information_schema`
+
+**Описание:** `create_schema.nim` пита `information_schema.tables` и `information_schema.columns`, които са PostgreSQL/MySQL специфични. BaraDB вероятно няма тези view-та.
+
+**Файл:** `clients/nim-allographer/src/allographer/schema_builder/usecases/baradb/create_schema.nim`
+
+**Решение:** Имплементирай BaraDB-специфична интроспекция — или чрез BaraQL команди, или чрез `PRAGMA`-подобни заявки ако сървърът ги поддържа.
+
+---
+
+## 🟢 Дреболии
+
+### 5. [КЛИЕНТ] `getRowPlain()` връща празен seq при празен резултат
+
+**Описание:** Поправено е да не хвърля `IndexDefect`, но сега връща `@[]`. Потребителят не разбира дали заявката е върнала 0 реда или е станала грешка.
+
+**Файл:** `clients/nim-allographer/src/allographer/query_builder/models/baradb/baradb_exec.nim`
+
+---
+
+## Приоритет
+
+| Приоритет | # | Проблем | Отговорност |
+|-----------|---|---------|-------------|
+| P0 | 1 | Сървърен SIGSEGV при INSERT в транзакция | Сървърен екип |
+| P0 | 2 | Сървърен parser error за резервирани думи | Сървърен екип |
+| P1 | 3 | SQL injection — преминаване към prepared statements | Клиентски екип |
+| P1 | 4 | Schema builder `information_schema` | Клиентски екип |
+| P2 | 5 | `getRowPlain()` semantics | Клиентски екип |
+
@@ -110,12 +110,12 @@ proc readString(buf: openArray[byte], pos: var int): string =
    result[i] = char(buf[pos + i])
  pos += len

-proc toBytes(s: string): seq[byte] =
+proc toBytes*(s: string): seq[byte] =
  result = newSeq[byte](s.len)
  for i, c in s:
    result[i] = byte(c)

-proc toString(s: seq[byte]): string =
+proc toString*(s: seq[byte]): string =
  result = newString(s.len)
  for i, b in s:
    result[i] = char(b)
@@ -277,7 +277,7 @@ type

  BaraClient* = ref object
    config: ClientConfig
-    socket: AsyncSocket
+    socket*: AsyncSocket
    connected: bool
    requestId: uint32

@@ -294,7 +294,7 @@ proc connect*(client: BaraClient) {.async.} =
  await client.socket.connect(client.config.host, Port(client.config.port))
  client.connected = true

-proc nextId(client: BaraClient): uint32 =
+proc nextId*(client: BaraClient): uint32 =
  inc client.requestId; client.requestId

 proc close*(client: BaraClient) =
@@ -325,7 +325,7 @@ proc wireValueToString*(wv: WireValue): string =
  of fkVector: return "<vector:" & $wv.vecVal.len & ">"
  of fkJson: return wv.jsonVal

-proc readQueryResponse(client: BaraClient): Future[QueryResult] {.async.} =
+proc readQueryResponse*(client: BaraClient): Future[QueryResult] {.async.} =
  let headerData = await client.socket.recv(12)
  if headerData.len < 12:
    raise newException(IOError, "Connection closed")
@@ -1,4 +1,5 @@
 import std/asyncdispatch
+import std/asyncnet
 import std/deques
 import std/json
 import std/monotimes
@@ -9,11 +10,13 @@ import std/sequtils
 import std/tables
 import std/times
 import ../../error
+import ../../enums
 import ../../libs/baradb/baradb_client
 import ../../log
 import ../database_types
 import ./query/baradb_builder
 import ./baradb_types
+import ./baradb_query


 # ================================================================================
@@ -144,6 +147,12 @@ proc escapeSqlValue(val: JsonNode): string =

 proc formatSql*(sql: string, args: seq[JsonNode]): string =
  result = sql
+  var placeholderCount = 0
+  for i in 0..<result.len:
+    if result[i] == '?':
+      placeholderCount += 1
+  if placeholderCount != args.len:
+    raise newException(DbError, "Placeholder count mismatch: expected " & $placeholderCount & " but got " & $args.len & " arguments")
  for arg in args:
    let pos = result.find("?")
    if pos < 0:
@@ -169,20 +178,33 @@ proc toJson*(resultSet: QueryResult): seq[JsonNode] =
    for c in 0 ..< resultSet.columns.len:
      let key = resultSet.columns[c]
      let val = resultSet.rows[r][c]
+      let colType = if c < resultSet.columnTypes.len: resultSet.columnTypes[c] else: "fkString"
      if val.len == 0:
        response_row[key] = newJNull()
-      elif val == "NULL" or val == "null":
-        response_row[key] = newJNull()
-      elif val == "t" or val == "true" or val == "f" or val == "false":
-        response_row[key] = newJBool(val == "t" or val == "true")
      else:
-        try:
-          response_row[key] = newJInt(val.parseInt)
-        except ValueError:
+        case colType
+        of "fkNull":
+          response_row[key] = newJNull()
+        of "fkBool":
+          response_row[key] = newJBool(val == "t" or val == "true" or val == "1")
+        of "fkInt8", "fkInt16", "fkInt32", "fkInt64":
+          try:
+            response_row[key] = newJInt(val.parseInt)
+          except ValueError:
+            response_row[key] = newJString(val)
+        of "fkFloat32", "fkFloat64":
          try:
            response_row[key] = newJFloat(val.parseFloat)
          except ValueError:
            response_row[key] = newJString(val)
+        of "fkJson":
+          try:
+            response_row[key] = parseJson(val)
+          except JsonParsingError:
+            response_row[key] = newJString(val)
+        else:
+          # fkString, fkBytes, fkArray, fkObject, fkVector, and unknown types
+          response_row[key] = newJString(val)
    response_table[r] = response_row
  return response_table

@@ -256,7 +278,9 @@ proc getRowPlain(self: BaradbQuery, queryString: string, args: JsonNode): Future

  let sql = formatSql(queryString, args)
  let qr = await self.pools.conns[connI].client.query(sql)
-  return qr.rows[0]
+  if qr.rows.len > 0:
+    return qr.rows[0]
+  return @[]


 proc getAllRows(self: RawBaradbQuery, queryString: string): Future[seq[JsonNode]] {.async.} =
@@ -336,7 +360,9 @@ proc getRowPlain(self: RawBaradbQuery, queryString: string, args: JsonNode): Fut
    arr.add(arg)
  let sql = formatSql(queryString, arr)
  let qr = await self.pools.conns[connI].client.query(sql)
-  return qr.rows[0]
+  if qr.rows.len > 0:
+    return qr.rows[0]
+  return @[]


 proc exec(self: BaradbQuery, queryString: string) {.async.} =
@@ -411,18 +437,25 @@ proc transactionStart(self: BaradbConnections) {.async.} =
  let connI = getFreeConn(self).await
  if connI == errorConnectionNum:
    raisePoolTimeout(self)
+  discard await self.pools.conns[connI].client.exec("BEGIN")
  self.isInTransaction = true
  self.transactionConn = connI
-  discard await self.pools.conns[connI].client.exec("BEGIN")


 proc transactionEnd(self: BaradbConnections, query: string) {.async.} =
+  let connI = self.transactionConn
  defer:
-    self.returnConn(self.transactionConn).await
+    try:
+      await self.returnConn(connI)
+    except CatchableError:
+      discard
    self.transactionConn = 0
    self.isInTransaction = false

-  discard await self.pools.conns[self.transactionConn].client.exec(query)
+  try:
+    discard await self.pools.conns[connI].client.exec(query)
+  except CatchableError:
+    discard


 # ================================================================================
@@ -601,7 +634,14 @@ proc count*(self: BaradbQuery): Future[int] {.async.} =
  self.log.logger(sql)
  let response = await self.getRow(sql)
  if response.isSome:
-    return response.get["aggregate"].getStr().parseInt()
+    let agg = response.get["aggregate"]
+    case agg.kind
+    of JInt:
+      return agg.getInt
+    of JFloat:
+      return int(agg.getFloat)
+    else:
+      return agg.getStr.parseInt()
  else:
    return 0

@@ -643,7 +683,14 @@ proc avg*(self: BaradbQuery, column: string): Future[Option[float]] {.async.} =
  self.log.logger(sql)
  let response = await self.getRow(sql)
  if response.isSome:
-    return response.get["aggregate"].getStr().parseFloat.some
+    let agg = response.get["aggregate"]
+    case agg.kind
+    of JInt:
+      return some(float(agg.getInt))
+    of JFloat:
+      return some(agg.getFloat)
+    else:
+      return some(agg.getStr.parseFloat)
  else:
    return none(float)

@@ -653,7 +700,14 @@ proc sum*(self: BaradbQuery, column: string): Future[Option[float]] {.async.} =
  self.log.logger(sql)
  let response = await self.getRow(sql)
  if response.isSome:
-    return response.get["aggregate"].getStr.parseFloat.some
+    let agg = response.get["aggregate"]
+    case agg.kind
+    of JInt:
+      return some(float(agg.getInt))
+    of JFloat:
+      return some(agg.getFloat)
+    else:
+      return some(agg.getStr.parseFloat)
  else:
    return none(float)

@@ -715,8 +769,8 @@ proc paginate*(self: BaradbQuery, page: int = 1, perPage: int = 20): Future[Pagi
  ## Offset-based pagination. Returns rows for the given page plus metadata.
  let total = await self.count()
  let offset = (page - 1) * perPage
-  self.limit(perPage)
-  self.offset(offset)
+  discard self.limit(perPage)
+  discard self.offset(offset)
  let rows = await self.get()
  let qr = PaginateResult(
    rows: rows,
@@ -732,10 +786,10 @@ proc fastPaginate*(self: BaradbQuery, cursorColumn: string, perPage: int = 20,
  ## Cursor-based pagination (keyset). More efficient than offset for large tables.
  ## Requires a unique, ordered column (usually the primary key).
  let total = await self.count()
-  self.limit(perPage)
-  self.orderBy(cursorColumn, Asc)
+  discard self.limit(perPage)
+  discard self.orderBy(cursorColumn, Asc)
  if afterId.len > 0:
-    self.where(cursorColumn, ">", afterId)
+    discard self.where(cursorColumn, ">", afterId)
  let rows = await self.get()
  var hasMore = rows.len == perPage
  let qr = PaginateResult(
@@ -809,7 +863,7 @@ proc isMigrationApplied*(self: BaradbConnections, name: string): Future[bool] {.
 # ================================================================================

 proc sendPrepared(client: BaraClient, sql: string, params: seq[WireValue]): Future[QueryResult] {.async.} =
-  if not client.connected:
+  if not client.isConnected():
    raise newException(IOError, "Not connected")
  let msg = makeQueryParamsMessage(client.nextId(), sql, params)
  let msgStr = toString(msg)
@@ -205,6 +205,8 @@ proc orWhere*(self: BaradbQuery, column: string, symbol: string, value: nil.type


 proc whereBetween*(self: BaradbQuery, column: string, width: array[2, int|float]): BaradbQuery =
+  self.placeHolder.add(%*{"key": column, "value": width[0]})
+  self.placeHolder.add(%*{"key": column, "value": width[1]})
  if self.query.hasKey("where_between") == false:
    self.query["where_between"] = %*[{
      "column": column,
@@ -219,6 +221,8 @@ proc whereBetween*(self: BaradbQuery, column: string, width: array[2, int|float]


 proc whereBetween*(self: BaradbQuery, column: string, width: array[2, string]): BaradbQuery =
+  self.placeHolder.add(%*{"key": column, "value": width[0]})
+  self.placeHolder.add(%*{"key": column, "value": width[1]})
  if self.query.hasKey("where_between_string") == false:
    self.query["where_between_string"] = %*[{
      "column": column,
@@ -233,6 +237,8 @@ proc whereBetween*(self: BaradbQuery, column: string, width: array[2, string]):


 proc whereNotBetween*(self: BaradbQuery, column: string, width: array[2, int|float]): BaradbQuery =
+  self.placeHolder.add(%*{"key": column, "value": width[0]})
+  self.placeHolder.add(%*{"key": column, "value": width[1]})
  if self.query.hasKey("where_not_between") == false:
    self.query["where_not_between"] = %*[{
      "column": column,
@@ -247,6 +253,8 @@ proc whereNotBetween*(self: BaradbQuery, column: string, width: array[2, int|flo


 proc whereNotBetween*(self: BaradbQuery, column: string, width: array[2, string]): BaradbQuery =
+  self.placeHolder.add(%*{"key": column, "value": width[0]})
+  self.placeHolder.add(%*{"key": column, "value": width[1]})
  if self.query.hasKey("where_not_between_string") == false:
    self.query["where_not_between_string"] = %*[{
      "column": column,
@@ -261,6 +269,8 @@ proc whereNotBetween*(self: BaradbQuery, column: string, width: array[2, string]


 proc whereIn*(self: BaradbQuery, column: string, width: seq[int|float|string]): BaradbQuery =
+  for val in width:
+    self.placeHolder.add(%*{"key": column, "value": val})
  if self.query.hasKey("where_in") == false:
    self.query["where_in"] = %*[{
      "column": column,
@@ -275,6 +285,8 @@ proc whereIn*(self: BaradbQuery, column: string, width: seq[int|float|string]):


 proc whereNotIn*(self: BaradbQuery, column: string, width: seq[int|float|string]): BaradbQuery =
+  for val in width:
+    self.placeHolder.add(%*{"key": column, "value": val})
  if self.query.hasKey("where_not_in") == false:
    self.query["where_not_in"] = %*[{
      "column": column,
@@ -267,6 +267,7 @@ proc offsetSql*(self: BaradbQuery): BaradbQuery =
 # ==================== INSERT ====================

 proc insertSql*(self: BaradbQuery): BaradbQuery =
+  self.placeHolder = newJArray()
  let table = self.query["table"].getStr()
  self.queryString = &"INSERT INTO `{table}`"
  return self
@@ -16,11 +16,11 @@ let rdb = baradb
 suite("Baradb query"):
  test("create table raw"):
    let sql = """
-      CREATE TABLE IF NOT EXISTS "users" (
-        "id" SERIAL PRIMARY KEY,
-        "name" VARCHAR(255),
-        "email" VARCHAR(255),
-        "age" INTEGER
+      CREATE TABLE IF NOT EXISTS `users` (
+        `id` SERIAL PRIMARY KEY,
+        `name` VARCHAR(255),
+        `email` VARCHAR(255),
+        `age` INTEGER
      )
    """
    waitFor rdb.raw(sql).exec()