From 75ab9f8826d79d2051b396b57cb1fdf21f4ed632 Mon Sep 17 00:00:00 2001 From: Paulo Coghi Date: Sat, 11 Jul 2026 08:29:11 +0200 Subject: [PATCH 1/3] chore: multiple efficiency and security improvements --- src/alumna-sqlite.cr | 48 ++++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/src/alumna-sqlite.cr b/src/alumna-sqlite.cr index c98d9cb..1e85d9f 100644 --- a/src/alumna-sqlite.cr +++ b/src/alumna-sqlite.cr @@ -174,7 +174,8 @@ module Alumna end when .time? case val - when Time then val + when Time then val + # Prevent unhandled 500s from corrupt or legacy dates in the database when String then Time::Format::RFC_3339.parse(val) rescue nil else nil end @@ -381,15 +382,15 @@ module Alumna filters = q.typed_filters(@sch) return filters if filters.is_a?(ServiceError) - args = Array(DB::Any).new(initial_capacity: filters.size * 4) + # Increased heuristic to accommodate typical $in conditions safely + args = Array(DB::Any).new(initial_capacity: filters.size * 8) - # Declared before String.build so the q.select branch can update them - # via closure capture; map_row reads the final values after the block. col_names = @fetch_col_names col_fds = @fetch_col_fds id_idx = 0 - query = String.build(capacity: 256) do |sql| + # Increased capacity to 512 to prevent buffer doubling on medium-to-large queries + query = String.build(capacity: 512) do |sql| sql << "SELECT " if fields = q.select @@ -406,11 +407,14 @@ module Alumna built_names << "id" built_fds << nil sql << "id" - elsif fd = @sch.find_field(c) + elsif @columns_to_update.includes?(c) + fd = @sch.find_field(c) sql << ", " if built_names.size > 0 built_names << c built_fds << fd sql << c + else + return ServiceError.bad_request("Invalid select field: #{c}") end end @@ -435,7 +439,9 @@ module Alumna filters.each do |field, conditions| next if conditions.empty? fd = conditions.first.fd - next unless fd + + # Strictly prevent silently dropping filters for unknown fields + return ServiceError.bad_request("Unknown query field: #{field}") unless fd dot_idx = field.index('.') is_array = fd.type.array? @@ -486,18 +492,18 @@ module Alumna first_sort = true sort.each do |(f, dir)| dot_idx = f.index('.') - valid_col = dot_idx ? !@sch.find_field(f).nil? : (f == "id" || !@sch.find_field(f).nil?) - - if valid_col - sql << (first_sort ? " ORDER BY " : ", ") - if dot_idx - write_db_col(sql, f, dot_idx) - else - sql << f - end - sql << (dir == 1 ? " ASC" : " DESC") - first_sort = false + valid_col = dot_idx ? !@sch.find_field(f).nil? : (f == "id" || @columns_to_update.includes?(f)) + + return ServiceError.bad_request("Unknown sort field: #{f}") unless valid_col + + sql << (first_sort ? " ORDER BY " : ", ") + if dot_idx + write_db_col(sql, f, dot_idx) + else + sql << f end + sql << (dir == 1 ? " ASC" : " DESC") + first_sort = false end end @@ -558,7 +564,9 @@ module Alumna columns = Array(String).new(initial_capacity: ctx.data.size) ctx.data.each do |k, v| next if k == "id" - return ServiceError.bad_request("Unknown column: #{k}") unless @sch.find_field(k) + # Validate solely against top-level columns without triggering dot-notation scans + return ServiceError.bad_request("Unknown column: #{k}") unless @columns_to_update.includes?(k) + existing[k] = v columns << k end @@ -571,6 +579,8 @@ module Alumna args = Array(DB::Any).new(initial_capacity: columns.size + 1) + # If the column count matches, we can safely reuse the pre-compiled full UPDATE statement + # because we already verified all columns exist in @columns_to_update if columns.size == @columns_to_update.size query = @update_sql @columns_to_update.each { |c| args << cast_to_db(existing[c]) } From 9a6560b8b761dcfa137a4187e56ba9e4b365e414 Mon Sep 17 00:00:00 2001 From: Paulo Coghi Date: Sat, 11 Jul 2026 21:06:22 +0200 Subject: [PATCH 2/3] feat/perf: updated @unique_fields and added @indexed_fields for faster performance --- src/alumna-sqlite.cr | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/alumna-sqlite.cr b/src/alumna-sqlite.cr index 1e85d9f..4a6d965 100644 --- a/src/alumna-sqlite.cr +++ b/src/alumna-sqlite.cr @@ -22,7 +22,9 @@ module Alumna @fetch_col_fds : Array(FieldDescriptor?) # Pre-compiled SQL strings and keys for uniqueness checks - @unique_fields : Array(FieldDescriptor) + @unique_fields : Array({String, FieldDescriptor}) + @indexed_fields : Array({String, FieldDescriptor}) + @unique_sqls_no_skip : Array(String) @unique_sqls_with_skip : Array(String) @@ -66,12 +68,14 @@ module Alumna end @insert_sql = "INSERT INTO #{@table_name} (#{@columns_to_update.join(", ")}) VALUES (#{placeholders})" - @unique_fields = @sch.fields.select(&.unique) + @unique_fields = @sch.unique_fields + @indexed_fields = @sch.indexed_fields + @unique_sqls_no_skip = Array(String).new(initial_capacity: @unique_fields.size) @unique_sqls_with_skip = Array(String).new(initial_capacity: @unique_fields.size) - @unique_fields.each do |fd| - db_col = extract_db_col(fd.name) + @unique_fields.each do |(path, fd)| + db_col = extract_db_col(path) @unique_sqls_no_skip << "SELECT 1 FROM #{@table_name} WHERE #{db_col} = ? LIMIT 1" @unique_sqls_with_skip << "SELECT 1 FROM #{@table_name} WHERE #{db_col} = ? AND id != ? LIMIT 1" end @@ -109,11 +113,10 @@ module Alumna # ------------------------------------------------------------------------- def create_indexes! : Nil - @sch.fields.each do |fd| - next unless fd.unique || fd.indexed - idx_name = "idx_#{@table_name}_#{fd.name.gsub(SAFE_NAME_RE, '_')}" + @indexed_fields.each do |(path, fd)| + idx_name = "idx_#{@table_name}_#{path.gsub(SAFE_NAME_RE, '_')}" unique_str = fd.unique ? "UNIQUE " : "" - db_col = extract_db_col(fd.name) + db_col = extract_db_col(path) @db.exec("CREATE #{unique_str}INDEX IF NOT EXISTS #{idx_name} ON #{@table_name}(#{db_col})") end @@ -290,9 +293,9 @@ module Alumna # ------------------------------------------------------------------------- private def check_unique(record : Hash(String, AnyData), skip_id : String? = nil) : ServiceError? - # 1. Single-field fast path - @unique_fields.each_with_index do |fd, i| - val = extract_value(record, fd.name) + # 1. Single-field fast path (now natively includes nested hashes) + @unique_fields.each_with_index do |(path, fd), i| + val = extract_value(record, path) next if val.nil? duplicate = if skip_id @@ -302,7 +305,7 @@ module Alumna end if duplicate - return ServiceError.unprocessable("Unique constraint violation", {fd.name => "already exists"} of String => AnyData) + return ServiceError.unprocessable("Unique constraint violation", {path => "already exists"} of String => AnyData) end end @@ -317,9 +320,7 @@ module Alumna idx.fields.each do |f| v = extract_value(record, f) if v.nil? - # LCOV_EXCL_START - kcov wrongly reports on this line, while reporting coverage for the `break` right after missing_val = true - # LCOV_EXCL_STOP break end args << cast_to_db(v) From 9513336dc099076033164c5186c70b8e9f45e448 Mon Sep 17 00:00:00 2001 From: Paulo Coghi Date: Sat, 11 Jul 2026 22:39:28 +0200 Subject: [PATCH 3/3] test: kcov fix --- src/alumna-sqlite.cr | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/alumna-sqlite.cr b/src/alumna-sqlite.cr index 4a6d965..bbd6098 100644 --- a/src/alumna-sqlite.cr +++ b/src/alumna-sqlite.cr @@ -320,7 +320,9 @@ module Alumna idx.fields.each do |f| v = extract_value(record, f) if v.nil? + # LCOV_EXCL_START - kcov wrongly reports this assignment as uncovered while reporting `break` as covered missing_val = true + # LCOV_EXCL_STOP break end args << cast_to_db(v)