diff --git a/spec/alumna-sqlite_spec.cr b/spec/alumna-sqlite_spec.cr index 8b2e4eb..211c325 100644 --- a/spec/alumna-sqlite_spec.cr +++ b/spec/alumna-sqlite_spec.cr @@ -30,11 +30,113 @@ Alumna::Testing::AdapterSuite.run("Alumna::SqliteAdapter") do .int("score").float("price").int("order_index").str("category").bool("is_published").bytes("blob") .str("title", required: false).str("sequence", required: false) .str("first_name", required: false).str("last_name", required: false) - .int("view_count", required: false).nullable("metadata", required: false) + .int("view_count", required: false).any("metadata", nullable: true, required: false) Alumna::SqliteAdapter.new(SHARED_DB, "adapter_test", schema) end +describe "SqliteAdapter Specific Scenarios" do + it "handles unique constraints, nested JSON indexes, and edge cases natively" do + # 1. Setup a specific table for these coverage tests + SHARED_DB.exec("DROP TABLE IF EXISTS specific_scenarios") + SHARED_DB.exec(" + CREATE TABLE specific_scenarios ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + email TEXT, + profile TEXT, + tenant_id INTEGER, + role TEXT + ) + ") + + # 2. Define a schema that triggers all the uncovered initialization blocks + schema = Alumna::Schema.new(strict: false) + .str("email", unique: true) + .hash("profile") do |p| + p.str("handle") + end + .int("tenant_id") + .str("role") + # Define the unique constraint on the nested field at the schema level + .index("profile.handle", unique: true) + .index(["tenant_id", "role"], unique: true) + .index("email") # Simple non-unique index to trigger create_indexes! coverage + + adapter = Alumna::SqliteAdapter.new(SHARED_DB, "specific_scenarios", schema) + + # COVERAGE: create_indexes! + adapter.create_indexes! + + # 3. Create a valid record + # COVERAGE: db_exists?(query, arg) (1-arg variant) + # COVERAGE: db_exists?(query, args Array) (Array variant) + ctx1 = Alumna::Testing.build_ctx(service: adapter, method: Alumna::ServiceMethod::Create, data: { + "email" => "test@test.com", + "profile" => {"handle" => "tester"} of String => Alumna::AnyData, + "tenant_id" => 1_i64, + "role" => "admin", + } of String => Alumna::AnyData) + + res1 = adapter.create(ctx1).as(Hash(String, Alumna::AnyData)) + id = res1["id"].as(String) + + # 4. Trigger duplicate single field (email) + ctx_dup1 = Alumna::Testing.build_ctx(service: adapter, method: Alumna::ServiceMethod::Create, data: { + "email" => "test@test.com", + "profile" => {"handle" => "other"} of String => Alumna::AnyData, + "tenant_id" => 2_i64, + "role" => "user", + } of String => Alumna::AnyData) + adapter.create(ctx_dup1).should be_a(Alumna::ServiceError) + + # 5. Trigger duplicate nested JSON field (profile.handle) + ctx_dup2 = Alumna::Testing.build_ctx(service: adapter, method: Alumna::ServiceMethod::Create, data: { + "email" => "other@test.com", + "profile" => {"handle" => "tester"} of String => Alumna::AnyData, + "tenant_id" => 2_i64, + "role" => "user", + } of String => Alumna::AnyData) + adapter.create(ctx_dup2).should be_a(Alumna::ServiceError) + + # 6. Trigger duplicate compound (tenant_id + role) + ctx_dup3 = Alumna::Testing.build_ctx(service: adapter, method: Alumna::ServiceMethod::Create, data: { + "email" => "other2@test.com", + "profile" => {"handle" => "tester3"} of String => Alumna::AnyData, + "tenant_id" => 1_i64, + "role" => "admin", + } of String => Alumna::AnyData) + adapter.create(ctx_dup3).should be_a(Alumna::ServiceError) + + # 7. Update with skip_id (updating itself with same data should succeed) + # COVERAGE: db_exists?(query, arg1, arg2) (2-arg variant for skip_id) + ctx_upd = Alumna::Testing.build_ctx(service: adapter, method: Alumna::ServiceMethod::Update, id: id, data: { + "email" => "test@test.com", + "profile" => {"handle" => "tester"} of String => Alumna::AnyData, + "tenant_id" => 1_i64, + "role" => "admin", + } of String => Alumna::AnyData) + adapter.update(ctx_upd).should be_a(Hash(String, Alumna::AnyData)) + + # 8. Patch fast-path (sending exactly all 4 updatable columns) + # COVERAGE: if columns.size == @columns_to_update.size branch in patch + ctx_patch_all = Alumna::Testing.build_ctx(service: adapter, method: Alumna::ServiceMethod::Patch, id: id, data: { + "email" => "patch@test.com", + "profile" => {"handle" => "patcher"} of String => Alumna::AnyData, + "tenant_id" => 1_i64, + "role" => "superadmin", + } of String => Alumna::AnyData) + adapter.patch(ctx_patch_all).should be_a(Hash(String, Alumna::AnyData)) + + # 9. Float64 to Int64 coercion in cast_from_db + # COVERAGE: when Float64 then val.to_i64 + # Manually insert a float into an int column by bypassing the adapter + SHARED_DB.exec("INSERT INTO specific_scenarios (email, tenant_id) VALUES ('float@test.com', 99.9)") + ctx_get = Alumna::Testing.build_ctx(service: adapter, method: Alumna::ServiceMethod::Find, params: {"email" => "float@test.com"}) + float_rec = adapter.find(ctx_get).as(Array(Hash(String, Alumna::AnyData))).first + float_rec["tenant_id"].should eq(99_i64) # 99.9 safely truncated to 99 + end +end + Spec.after_suite do File.delete(FILE_PATH) if File.exists?(FILE_PATH) end diff --git a/src/alumna-sqlite.cr b/src/alumna-sqlite.cr index 79f831f..c98d9cb 100644 --- a/src/alumna-sqlite.cr +++ b/src/alumna-sqlite.cr @@ -4,6 +4,8 @@ require "json" module Alumna class SqliteAdapter < Service + SAFE_NAME_RE = /[^a-zA-Z0-9_]/ + getter db : DB::Database getter table_name : String @@ -12,56 +14,142 @@ module Alumna @fetch_by_id_sql : String @delete_sql : String @update_sql : String + @insert_sql : String @columns_to_update : Array(String) + # Pre-computed fetch metadata + @fetch_col_names : Array(String) + @fetch_col_fds : Array(FieldDescriptor?) + + # Pre-compiled SQL strings and keys for uniqueness checks + @unique_fields : Array(FieldDescriptor) + @unique_sqls_no_skip : Array(String) + @unique_sqls_with_skip : Array(String) + + @unique_compound_indexes : Array(IndexDef) + @compound_sqls_no_skip : Array(String) + @compound_sqls_with_skip : Array(String) + @compound_key_names : Array(String) + @max_compound_fields : Int32 + def initialize(@db : DB::Database, @table_name : String, schema : Schema? = nil) raise ArgumentError.new("SqliteAdapter requires a Schema to safely map types") unless schema @sch = schema - # Pre-compute static SQL components at boot to completely eliminate - # string allocations on standard GET, UPDATE, and DELETE requests. - cols = @sch.field_names.to_a - cols.unshift("id") unless cols.includes?("id") + cols = Array(String).new(initial_capacity: @sch.fields.size + 1) + cols << "id" + @sch.fields.each { |fd| cols << fd.name unless fd.name == "id" } + @select_all_sql = cols.join(", ") + @fetch_col_names = cols + @fetch_col_fds = cols.map { |c| c == "id" ? nil : @sch.find_field(c) } + @fetch_by_id_sql = "SELECT #{@select_all_sql} FROM #{@table_name} WHERE id = ? LIMIT 1" @delete_sql = "DELETE FROM #{@table_name} WHERE id = ?" - @columns_to_update = @sch.field_names.reject { |f| f == "id" }.to_a - set_clause = @columns_to_update.map { |c| "#{c} = ?" }.join(", ") + @columns_to_update = cols[1..] + + set_clause = String.build do |io| + @columns_to_update.each_with_index do |c, i| + io << ", " if i > 0 + io << c << " = ?" + end + end @update_sql = "UPDATE #{@table_name} SET #{set_clause} WHERE id = ?" + placeholders = String.build do |io| + @columns_to_update.size.times do |i| + io << ", " if i > 0 + io << "?" + end + end + @insert_sql = "INSERT INTO #{@table_name} (#{@columns_to_update.join(", ")}) VALUES (#{placeholders})" + + @unique_fields = @sch.fields.select(&.unique) + @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_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 + + @unique_compound_indexes = @sch.schema_indexes.select(&.unique) + @compound_key_names = @unique_compound_indexes.map { |idx| idx.fields.join("_") } + @max_compound_fields = @unique_compound_indexes.max_of?(&.fields.size) || 0 + + @compound_sqls_no_skip = @unique_compound_indexes.map do |idx| + where = String.build do |io| + idx.fields.each_with_index do |f, i| + io << " AND " if i > 0 + io << extract_db_col(f) << " = ?" + end + end + "SELECT 1 FROM #{@table_name} WHERE #{where} LIMIT 1" + end + + @compound_sqls_with_skip = @unique_compound_indexes.map do |idx| + where = String.build do |io| + idx.fields.each_with_index do |f, i| + io << " AND " if i > 0 + io << extract_db_col(f) << " = ?" + end + io << " AND id != ?" + end + "SELECT 1 FROM #{@table_name} WHERE #{where} LIMIT 1" + end + super(schema) end + # ------------------------------------------------------------------------- + # Schema Synchronization Helper + # ------------------------------------------------------------------------- + + 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, '_')}" + unique_str = fd.unique ? "UNIQUE " : "" + db_col = extract_db_col(fd.name) + @db.exec("CREATE #{unique_str}INDEX IF NOT EXISTS #{idx_name} ON #{@table_name}(#{db_col})") + end + + @sch.schema_indexes.each do |idx| + idx_name = "idx_#{@table_name}_#{idx.fields.join("_").gsub(SAFE_NAME_RE, '_')}" + unique_str = idx.unique ? "UNIQUE " : "" + db_cols = idx.fields.map { |f| extract_db_col(f) }.join(", ") + @db.exec("CREATE #{unique_str}INDEX IF NOT EXISTS #{idx_name} ON #{@table_name}(#{db_cols})") + end + end + # ------------------------------------------------------------------------- # Mappers: Bridging SQLite (DB::Any) to Alumna (AnyData) # ------------------------------------------------------------------------- - # Reads a DB::ResultSet into Alumna's AnyData hash, using the schema for coercion - private def row_to_hash(rs : DB::ResultSet) : Hash(String, AnyData) - record = {} of String => AnyData + # Shared row mapper to prevent code duplication + private def map_row(rs : DB::ResultSet, col_names : Array(String), col_fds : Array(FieldDescriptor?), id_idx : Int32) : Hash(String, AnyData) + record = Hash(String, AnyData).new(initial_capacity: col_names.size) - rs.column_names.each do |col_name| + col_names.each_with_index do |col_name, i| val = rs.read(DB::Any) - next if val.nil? - # Explicitly preserve the ID as a string, as Alumna expects - if col_name == "id" - record[col_name] = val.to_s - next + if i == id_idx + record[col_name] = val.nil? ? nil : val.to_s + elsif fd = col_fds[i] + record[col_name] = val.nil? ? nil : cast_from_db(val, fd.type) end - - fd = @sch.find_field(col_name) - next unless fd # Ignore columns not defined in the schema - - record[col_name] = cast_from_db(val, fd.type) end record end - # Coerces raw SQLite types into the native Crystal types Alumna expects + private def row_to_hash(rs : DB::ResultSet) : Hash(String, AnyData) + map_row(rs, @fetch_col_names, @fetch_col_fds, 0) + end + private def cast_from_db(val : DB::Any, type : FieldType) : AnyData case type when .bool? @@ -72,44 +160,50 @@ module Alumna else false end when .int? - val.is_a?(Int) ? val.to_i64 : val.to_s.to_i64 + case val + when Int then val.to_i64 + when Float64 then val.to_i64 + when String then val.to_i64? || 0_i64 + else 0_i64 + end when .float? case val when Float64, Int then val.to_f64 - else val.to_s.to_f64 + when String then val.to_f64? || 0.0 + else 0.0 end when .time? - val.is_a?(Time) ? val : Time::Format::RFC_3339.parse(val.to_s) + case val + when Time then val + when String then Time::Format::RFC_3339.parse(val) rescue nil + else nil + end when .str? val.to_s when .array?, .hash? - # Re-inflate complex structures stored as JSON text using the core helper - val.is_a?(String) ? JsonHelper.from_string(val) : val.as(AnyData) - when .nullable? - # If the value is a string that looks like JSON, inflate it + val.is_a?(String) ? JsonHelper.from_string(val) : val.as?(AnyData) || val.to_s + when .any? if val.is_a?(String) && (val.starts_with?('[') || val.starts_with?('{')) JsonHelper.from_string(val) else - val.as(AnyData) + val.as?(AnyData) || val.to_s end else - # Bytes, etc. - val.as(AnyData) + val.as?(AnyData) || val.to_s end end - # Prepares AnyData for SQLite insertion private def cast_to_db(val : AnyData) : DB::Any case val + when Nil + nil when Bool val ? 1 : 0 when Time Time::Format::RFC_3339.format(val) when Array, Hash - # Flatten nested data into JSON text using the core helper - JsonHelper.to_string(val.as(AnyData)) + JsonHelper.to_string(val) else - # Valid DB::Any primitives val.as(DB::Any) end end @@ -125,6 +219,124 @@ module Alumna nil end + private def db_exists?(query : String, args : Array(DB::Any)) : Bool + @db.query(query, args: args) { |rs| return true if rs.move_next } + false + end + + private def db_exists?(query : String, arg : DB::Any) : Bool + @db.query(query, arg) { |rs| return true if rs.move_next } + false + end + + private def db_exists?(query : String, arg1 : DB::Any, arg2 : DB::Any) : Bool + @db.query(query, arg1, arg2) { |rs| return true if rs.move_next } + false + end + + private def extract_db_col(field : String, dot_idx : Int32? = nil) : String + dot = dot_idx || field.index('.') + if dot + "json_extract(#{field[0...dot]}, '$.#{field[dot + 1..]}')" + else + field + end + end + + # Zero-allocation version for when we are already writing to a String.build IO + private def write_db_col(io : IO, field : String, dot_idx : Int32) : Nil + slice = field.to_slice + io << "json_extract(" + io.write_string(slice[0, dot_idx]) + io << ", '$." + io.write_string(slice[dot_idx + 1, field.bytesize - dot_idx - 1]) + io << "')" + end + + private def extract_value(rec : Hash(String, AnyData), field : String) : AnyData + # Fast-path optimization: avoids the loop setup on the 99% of fields that don't have dots. + # For fields that have, reusing `first_dot` index avoids a full O(n) scan. + first_dot = field.index('.') + return rec[field]? unless first_dot + + current : AnyData = rec + start = 0 + dot : Int32? = first_dot + + loop do + part = field[start...(dot || field.size)] + return nil unless current.is_a?(Hash(String, AnyData)) + current = current[part]? + break unless dot + return nil if current.nil? + start = dot + 1 + dot = field.index('.', start) + end + current + end + + @[AlwaysInline] + private def write_col_ref(io : IO, field : String, dot_idx : Int32?) : Nil + if dot_idx + write_db_col(io, field, dot_idx) + else + io << field + end + end + + # ------------------------------------------------------------------------- + # Uniqueness Constraints + # ------------------------------------------------------------------------- + + 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) + next if val.nil? + + duplicate = if skip_id + db_exists?(@unique_sqls_with_skip[i], cast_to_db(val), skip_id) + else + db_exists?(@unique_sqls_no_skip[i], cast_to_db(val)) + end + + if duplicate + return ServiceError.unprocessable("Unique constraint violation", {fd.name => "already exists"} of String => AnyData) + end + end + + # 2. Compound unique constraints + unless @unique_compound_indexes.empty? + args = Array(DB::Any).new(initial_capacity: @max_compound_fields + 1) + + @unique_compound_indexes.each_with_index do |idx, i| + args.clear + missing_val = false + + 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) + end + next if missing_val + + query = skip_id ? @compound_sqls_with_skip[i] : @compound_sqls_no_skip[i] + args << skip_id.as(DB::Any) if skip_id + + if db_exists?(query, args) + return ServiceError.unprocessable("Unique constraint violation", {@compound_key_names[i] => "already exists"} of String => AnyData) + end + end + end + + nil + end + # ------------------------------------------------------------------------- # Core Service Methods # ------------------------------------------------------------------------- @@ -136,30 +348,20 @@ module Alumna end def create(ctx : RuleContext) : Hash(String, AnyData) | ServiceError - record = {} of String => AnyData + if err = check_unique(ctx.data) + return err + end - # Only process keys that actually exist in our Schema (prevent SQL Injection) - valid_keys = ctx.data.keys.select { |k| k != "id" && @sch.find_field(k) } + args = Array(DB::Any).new(initial_capacity: @columns_to_update.size) + record = Hash(String, AnyData).new(initial_capacity: @columns_to_update.size + 1) - if valid_keys.empty? - # SQLite syntax for an empty insert - result = @db.exec("INSERT INTO #{@table_name} DEFAULT VALUES") - else - placeholders = Array.new(valid_keys.size, "?").join(", ") - col_names = valid_keys.join(", ") - args = [] of DB::Any - - valid_keys.each do |k| - val = ctx.data[k] - args << cast_to_db(val) - record[k] = val # Populate the returning record - end - - query = "INSERT INTO #{@table_name} (#{col_names}) VALUES (#{placeholders})" - result = @db.exec(query, args: args) + @columns_to_update.each do |col| + val = ctx.data[col]? + args << cast_to_db(val) + record[col] = val end - # Inject the auto-generated ID back into our returning record + result = @db.exec(@insert_sql, args: args) record["id"] = result.last_insert_id.to_s record end @@ -176,96 +378,133 @@ module Alumna def find(ctx : RuleContext) : Array(Hash(String, AnyData)) | ServiceError q = ctx.query - filters = q.typed_filters(schema) + filters = q.typed_filters(@sch) return filters if filters.is_a?(ServiceError) - where_clauses = [] of String - args = [] of DB::Any + args = Array(DB::Any).new(initial_capacity: filters.size * 4) - # 1. Build WHERE clauses - filters.each do |field, conditions| - # Security: Schema#find_field natively parses dot-notation ("user.age"). - # This guarantees the ENTIRE nested path exists, completely preventing SQL injection. - fd = @sch.find_field(field) - next unless fd + # 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 - if field.includes?('.') - parts = field.split('.', 2) - db_field = "json_extract(#{parts[0]}, '$.#{parts[1]}')" - else - db_field = field - end + query = String.build(capacity: 256) do |sql| + sql << "SELECT " - is_array = fd.type.array? - - conditions.each do |cond| - op_sql = case cond.op - when .eq? then "=" - when .ne? then "!=" - when .gt? then ">" - when .gte? then ">=" - when .lt? then "<" - when .lte? then "<=" - when .in? then "IN" - when .nin? then "NOT IN" - else "=" - end - - if cond.op.in? || cond.op.nin? - arr = cond.value.as(Array(AnyData)) - placeholders = Array.new(arr.size, "?").join(", ") - where_clauses << "#{db_field} #{op_sql} (#{placeholders})" - arr.each { |v| args << cast_to_db(v) } - elsif is_array && cond.op.eq? - # Check if value exists inside the JSON array - where_clauses << "EXISTS (SELECT 1 FROM json_each(#{db_field}) WHERE value = ?)" - args << cast_to_db(cond.value) - elsif is_array && cond.op.ne? - where_clauses << "NOT EXISTS (SELECT 1 FROM json_each(#{db_field}) WHERE value = ?)" - args << cast_to_db(cond.value) - else - where_clauses << "#{db_field} #{op_sql} ?" - args << cast_to_db(cond.value) + if fields = q.select + built_names = Array(String).new(initial_capacity: fields.size + 1) + built_fds = Array(FieldDescriptor?).new(initial_capacity: fields.size + 1) + has_id = false + + fields.each do |c| + if c == "id" + next if has_id + has_id = true + id_idx = built_names.size + sql << ", " if built_names.size > 0 + built_names << "id" + built_fds << nil + sql << "id" + elsif fd = @sch.find_field(c) + sql << ", " if built_names.size > 0 + built_names << c + built_fds << fd + sql << c + end end - end - end - # 2. Build the final SQL string safely - query = String.build do |sql| - # SELECT - if fields = q.select - base = fields.includes?("id") ? fields : ["id"] + fields - valid_selects = base.select { |c| c == "id" || @sch.find_field(c) } - sql << "SELECT " << valid_selects.join(", ") + # Contract: Adapters must always fetch the ID so records can be uniquely identified + unless has_id + id_idx = built_names.size + sql << ", " if built_names.size > 0 + built_names << "id" + built_fds << nil + sql << "id" + end + + col_names = built_names + col_fds = built_fds else - sql << "SELECT " << @select_all_sql + sql << @select_all_sql end sql << " FROM " << @table_name - sql << " WHERE " << where_clauses.join(" AND ") unless where_clauses.empty? + first_where = true + filters.each do |field, conditions| + next if conditions.empty? + fd = conditions.first.fd + next unless fd + + dot_idx = field.index('.') + is_array = fd.type.array? + + conditions.each do |cond| + sql << (first_where ? " WHERE " : " AND ") + first_where = false + + if cond.op.in? || cond.op.nin? + op_sql = cond.op.in? ? "IN" : "NOT IN" + arr = cond.value.as(Array(AnyData)) + write_col_ref(sql, field, dot_idx) + sql << " " << op_sql << " (" + arr.each_with_index do |v, i| + sql << ", " if i > 0 + sql << "?" + args << cast_to_db(v) + end + sql << ")" + elsif is_array && cond.op.eq? + sql << "EXISTS (SELECT 1 FROM json_each(" + write_col_ref(sql, field, dot_idx) + sql << ") WHERE value = ?)" + args << cast_to_db(cond.value) + elsif is_array && cond.op.ne? + sql << "NOT EXISTS (SELECT 1 FROM json_each(" + write_col_ref(sql, field, dot_idx) + sql << ") WHERE value = ?)" + args << cast_to_db(cond.value) + else + op_sql = case cond.op + when .eq? then "=" + when .ne? then "!=" + when .gt? then ">" + when .gte? then ">=" + when .lt? then "<" + when .lte? then "<=" + else "=" + end + write_col_ref(sql, field, dot_idx) + sql << " " << op_sql << " ?" + args << cast_to_db(cond.value) + end + end + end - # ORDER BY if sort = q.sort - sort_clauses = sort.compact_map do |(f, dir)| - dir_sql = dir == 1 ? "ASC" : "DESC" - if f.includes?('.') - next unless @sch.find_field(f) # Full-path schema validation! - parts = f.split('.', 2) - "json_extract(#{parts[0]}, '$.#{parts[1]}') #{dir_sql}" - else - next unless f == "id" || @sch.find_field(f) - "#{f} #{dir_sql}" + 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 end end - sql << " ORDER BY " << sort_clauses.join(", ") unless sort_clauses.empty? end - # LIMIT and OFFSET if limit = q.limit sql << " LIMIT " << limit elsif q.skip - sql << " LIMIT -1" # SQLite trick: requires LIMIT to use OFFSET + sql << " LIMIT -1" end if skip = q.skip @@ -273,10 +512,12 @@ module Alumna end end - # 3. Execute and map results - results = [] of Hash(String, AnyData) + results = Array(Hash(String, AnyData)).new(initial_capacity: q.limit || 16) + @db.query(query, args: args) do |rs| - rs.each { results << row_to_hash(rs) } + rs.each do + results << map_row(rs, col_names, col_fds, id_idx) + end end results @@ -286,22 +527,24 @@ module Alumna id = ctx.id return ServiceError.bad_request("ID required for update") unless id - args = [] of DB::Any - record = ctx.data.dup + if err = check_unique(ctx.data, skip_id: id) + return err + end + + args = Array(DB::Any).new(initial_capacity: @columns_to_update.size + 1) + record = Hash(String, AnyData).new(initial_capacity: @columns_to_update.size + 1) + record["id"] = id.as(AnyData) - # Iterate strictly over the pre-compiled schema fields. If the client omitted it, - # we intentionally set it to nil to replace the whole row. @columns_to_update.each do |col| - val = record[col]? - args << (val.nil? ? nil : cast_to_db(val)) + val = ctx.data[col]? + args << cast_to_db(val) + record[col] = val end args << id result = @db.exec(@update_sql, args: args) - return ServiceError.not_found if result.rows_affected == 0 - record["id"] = id record end @@ -309,29 +552,46 @@ module Alumna id = ctx.id return ServiceError.bad_request("ID required for patch") unless id - record = ctx.data.dup - record.delete("id") + existing = fetch_by_id(id) + return ServiceError.not_found unless existing + + 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) + existing[k] = v + columns << k + end + + return existing if columns.empty? - columns = record.keys - return ServiceError.bad_request("No data to patch") if columns.empty? + if err = check_unique(existing, skip_id: id) + return err + end + + args = Array(DB::Any).new(initial_capacity: columns.size + 1) - # Ensure we only patch columns that exist in the schema to prevent SQL Injection - columns.each do |col| - return ServiceError.bad_request("Unknown column: #{col}") unless @sch.find_field(col) + if columns.size == @columns_to_update.size + query = @update_sql + @columns_to_update.each { |c| args << cast_to_db(existing[c]) } + else + query = String.build(capacity: 128) do |sql| + sql << "UPDATE " << @table_name << " SET " + columns.each_with_index do |col, i| + sql << ", " if i > 0 + sql << col << " = ?" + args << cast_to_db(existing[col]) + end + sql << " WHERE id = ?" + end end - set_clause = columns.map { |c| "#{c} = ?" }.join(", ") - args = [] of DB::Any - columns.each { |col| args << cast_to_db(record[col]) } args << id - query = "UPDATE #{@table_name} SET #{set_clause} WHERE id = ?" result = @db.exec(query, args: args) + return ServiceError.new("Record modified concurrently", 409) if result.rows_affected == 0 - return ServiceError.not_found if result.rows_affected == 0 - - # Patch needs to return the *fully merged* record, not just the partial data - fetch_by_id(id) || ServiceError.internal("Record not found after patch") + existing end end end