code review notes

close / do_close in result_set
avoid closing statements
remove named arguments
refactor positioned arguments query

spec/std/db/db_spec.cr

@@ -23,7 +23,7 @@ describe DB do
     cnn.not_nil!.closed?.should be_true
   end
 
-  it "query should close statement" do
+  it "query should close result_set" do
     with_witness do |w|
       with_dummy do |db|
         db.query "1,2" do
@@ -31,7 +31,7 @@ describe DB do
         end
 
         w.check
-        db.connection.last_statement.closed?.should be_true
+        DummyDriver::DummyResultSet.last_result_set.closed?.should be_true
       end
     end
   end
@@ -39,14 +39,14 @@ describe DB do
   it "exec should close statement" do
     with_dummy do |db|
       db.exec ""
-      db.connection.last_statement.closed?.should be_true
+      DummyDriver::DummyResultSet.last_result_set.closed?.should be_true
     end
   end
 
   it "scalar should close statement" do
     with_dummy do |db|
       db.scalar "1"
-      db.connection.last_statement.closed?.should be_true
+      DummyDriver::DummyResultSet.last_result_set.closed?.should be_true
     end
   end
 
@@ -54,7 +54,6 @@ describe DB do
     with_dummy do |db|
       db.exec ""
       DummyDriver::DummyResultSet.last_result_set.executed?.should be_true
-      db.connection.last_statement.closed?.should be_true
     end
   end
 end

spec/std/db/dummy_driver.cr

@@ -6,10 +6,8 @@ class DummyDriver < DB::Driver
   end
 
   class DummyConnection < DB::Connection
-    getter! last_statement
-
     def prepare(query)
-      @last_statement = DummyStatement.new(self, query.split.map { |r| r.split ',' })
+      DummyStatement.new(self, query)
     end
 
     def last_insert_id : Int64
@@ -21,32 +19,27 @@ class DummyDriver < DB::Driver
   end
 
   class DummyStatement < DB::Statement
-    property! params
+    property params
 
-    def initialize(driver, @items)
+    def initialize(driver, @query)
+      @params = Hash(Int32 | String, DB::Any).new
       super(driver)
     end
 
-    protected def begin_parameters
-      @params = Hash(Int32 | String, DB::Any?).new
-    end
-
-    protected def add_parameter(index : Int32, value)
-      params[index] = value
-    end
-
-    protected def add_parameter(name : String, value)
-      params[":#{name}"] = value
-    end
-
-    protected def perform
-      DummyResultSet.new self, @items.each
+    protected def perform(args : Slice(DB::Any))
+      @params.clear
+      args.each_with_index do |arg, index|
+        @params[index] = arg
+      end
+      DummyResultSet.new self, @query
     end
   end
 
   class DummyResultSet < DB::ResultSet
-    def initialize(statement, @iterator)
+    def initialize(statement, query)
       super(statement)
+      @iterator = query.split.map { |r| r.split(',') }.to_a.each
+
       @executed = false
       @@last_result_set = self
     end
@@ -89,10 +82,6 @@ class DummyDriver < DB::Driver
         return @statement.params[0]
       end
 
-      if n.starts_with?(":")
-        return @statement.params[n]
-      end
-
       return n
     end
 

spec/std/db/dummy_driver_spec.cr

@@ -39,7 +39,7 @@ describe DummyDriver do
       end
     end
 
-    it "should query with block shuold executes always" do
+    it "should query with block should executes always" do
       with_witness do |w|
         with_dummy do |db|
           db.query "" do |rs|
@@ -47,7 +47,9 @@ describe DummyDriver do
           end
         end
       end
+    end
 
+    it "should query with block should executes always" do
       with_witness do |w|
         with_dummy do |db|
           db.query "lorem ipsum" do |rs|
@@ -136,28 +138,6 @@ describe DummyDriver do
           db.scalar(typeof({{value}}), "?", {{value}}).should eq({{value}})
         end
       end
-
-      it "should set arguments by symbol for {{value.id}}" do
-        with_dummy do |db|
-          db.query ":once :twice", {once: {{value}}, twice: {{value + value}} } do |rs|
-            rs.move_next.should be_true
-            rs.read(typeof({{value}})).should eq({{value}})
-            rs.move_next.should be_true
-            rs.read(typeof({{value}})).should eq({{value + value}})
-          end
-        end
-      end
-
-      it "should set arguments by string for {{value.id}}" do
-        with_dummy do |db|
-          db.query ":once :twice", {"once": {{value}}, "twice": {{value + value}} } do |rs|
-            rs.move_next.should be_true
-            rs.read(typeof({{value}})).should eq({{value}})
-            rs.move_next.should be_true
-            rs.read(typeof({{value}})).should eq({{value + value}})
-          end
-        end
-      end
     {% end %}
 
     it "executes and selects blob" do

spec/std/db/statement_spec.cr

@@ -19,26 +19,6 @@ describe DB::Statement do
     end
   end
 
-  it "should initialize symbol named params in query" do
-    with_dummy do |db|
-      stmt = db.prepare("the query")
-      stmt.query({a: "a", b: 1, c: nil})
-      stmt.params[":a"].should eq("a")
-      stmt.params[":b"].should eq(1)
-      stmt.params[":c"].should eq(nil)
-    end
-  end
-
-  it "should initialize string named params in query" do
-    with_dummy do |db|
-      stmt = db.prepare("the query")
-      stmt.query({"a": "a", "b": 1, "c": nil})
-      stmt.params[":a"].should eq("a")
-      stmt.params[":b"].should eq(1)
-      stmt.params[":c"].should eq(nil)
-    end
-  end
-
   it "should initialize positional params in exec" do
     with_dummy do |db|
       stmt = db.prepare("the query")
@@ -49,26 +29,6 @@ describe DB::Statement do
     end
   end
 
-  it "should initialize symbol named params in exec" do
-    with_dummy do |db|
-      stmt = db.prepare("the query")
-      stmt.exec({a: "a", b: 1, c: nil})
-      stmt.params[":a"].should eq("a")
-      stmt.params[":b"].should eq(1)
-      stmt.params[":c"].should eq(nil)
-    end
-  end
-
-  it "should initialize string named params in exec" do
-    with_dummy do |db|
-      stmt = db.prepare("the query")
-      stmt.exec({"a": "a", "b": 1, "c": nil})
-      stmt.params[":a"].should eq("a")
-      stmt.params[":b"].should eq(1)
-      stmt.params[":c"].should eq(nil)
-    end
-  end
-
   it "should initialize positional params in scalar" do
     with_dummy do |db|
       stmt = db.prepare("the query")
@@ -79,26 +39,6 @@ describe DB::Statement do
     end
   end
 
-  it "should initialize symbol named params in scalar" do
-    with_dummy do |db|
-      stmt = db.prepare("the query")
-      stmt.scalar(String, {a: "a", b: 1, c: nil})
-      stmt.params[":a"].should eq("a")
-      stmt.params[":b"].should eq(1)
-      stmt.params[":c"].should eq(nil)
-    end
-  end
-
-  it "should initialize string named params in scalar" do
-    with_dummy do |db|
-      stmt = db.prepare("the query")
-      stmt.scalar(String, {"a": "a", "b": 1, "c": nil})
-      stmt.params[":a"].should eq("a")
-      stmt.params[":b"].should eq(1)
-      stmt.params[":c"].should eq(nil)
-    end
-  end
-
   it "should initialize positional params in scalar?" do
     with_dummy do |db|
       stmt = db.prepare("the query")
@@ -109,26 +49,6 @@ describe DB::Statement do
     end
   end
 
-  it "should initialize symbol named params in scalar?" do
-    with_dummy do |db|
-      stmt = db.prepare("the query")
-      stmt.scalar?(String, {a: "a", b: 1, c: nil})
-      stmt.params[":a"].should eq("a")
-      stmt.params[":b"].should eq(1)
-      stmt.params[":c"].should eq(nil)
-    end
-  end
-
-  it "should initialize string named params in scalar?" do
-    with_dummy do |db|
-      stmt = db.prepare("the query")
-      stmt.scalar?(String, {"a": "a", "b": 1, "c": nil})
-      stmt.params[":a"].should eq("a")
-      stmt.params[":b"].should eq(1)
-      stmt.params[":c"].should eq(nil)
-    end
-  end
-
   it "query with block should not close statement" do
     with_dummy do |db|
       stmt = db.prepare "3,4 1,2"
@@ -137,45 +57,45 @@ describe DB::Statement do
     end
   end
 
-  it "query with block should close statement" do
+  it "query with block should not close statement" do
     with_dummy do |db|
       stmt = db.prepare "3,4 1,2"
       stmt.query do |rs|
       end
-      stmt.closed?.should be_true
+      stmt.closed?.should be_false
     end
   end
 
-  it "query should close statement" do
+  it "query should not close statement" do
     with_dummy do |db|
       stmt = db.prepare "3,4 1,2"
       stmt.query do |rs|
       end
-      stmt.closed?.should be_true
+      stmt.closed?.should be_false
     end
   end
 
-  it "scalar should close statement" do
+  it "scalar should not close statement" do
     with_dummy do |db|
       stmt = db.prepare "3,4 1,2"
       stmt.scalar
-      stmt.closed?.should be_true
+      stmt.closed?.should be_false
     end
   end
 
-  it "scalar should close statement" do
+  it "scalar should not close statement" do
     with_dummy do |db|
       stmt = db.prepare "3,4 1,2"
       stmt.scalar?
-      stmt.closed?.should be_true
+      stmt.closed?.should be_false
     end
   end
 
-  it "exec should close statement" do
+  it "exec should not close statement" do
     with_dummy do |db|
       stmt = db.prepare "3,4 1,2"
       stmt.exec
-      stmt.closed?.should be_true
+      stmt.closed?.should be_false
     end
   end
 end

src/db/connection.cr

@@ -15,15 +15,17 @@ module DB
   # Also override `#last_insert_id` to allow safe access to the last inserted id through this connection.
   #
   abstract class Connection
-    getter connection_string
+    # TODO add IDLE status, for connection ppool management.
 
-    def initialize(@connection_string)
-      @closed = false
+    getter connection_string # TODO Remove
+    @closed = false
+
+    def initialize(@connection_string) # TODO Remove
     end
 
     # Closes this connection.
     def close
-      raise "Connection already closed" if @closed
+      raise "Connection already closed" if @closed # TODO make it no fail if closed
       @closed = true
       perform_close
     end
@@ -44,8 +46,9 @@ module DB
     include QueryMethods
 
     # Returns the last inserted id through this connection.
-    abstract def last_insert_id : Int64
+    abstract def last_insert_id : Int64 # TODO move to ExecResult record. plano. with last_rows. eagerly askit.
 
-    protected abstract def perform_close
+
+    protected abstract def perform_close # TODO do_close
   end
 end

src/db/database.cr

@@ -24,8 +24,7 @@ module DB
       @connection.close
     end
 
-    # Returns a `Connection` to the database.
-    # Useful if you need to ensure the statements are executed in the connection.
+    # :nodoc:
     def connection
       @connection
     end

src/db/db.cr

@@ -64,10 +64,11 @@ module DB
   # method to be used as query parameters
   TYPES = [String, Int32, Int64, Float32, Float64, Slice(UInt8)]
 
-  # See `DB::TYPES` in `DB`
-  alias Any = String | Int32 | Int64 | Float32 | Float64 | Slice(UInt8)
+  # See `DB::TYPES` in `DB`. `Any` is a nillable version of the union of all types in `DB::TYPES`
+  alias Any = Nil | String | Int32 | Int64 | Float32 | Float64 | Slice(UInt8)
 
   # :nodoc:
+
   def self.driver_class(driver_name) # : Driver.class
     @@drivers.not_nil![driver_name]
   end

src/db/query_methods.cr

@@ -45,6 +45,8 @@ module DB
       prepare(query).scalar(*args)
     end
 
+    # TODO remove scalar?  make it always nillable. raise if 0-rows   raise +1-rows
+
     # Performs the `query` and returns a single scalar value of type `t`.
     # `t` must be any of the allowed `DB::Any` types.
     #

src/db/result_set.cr

@@ -13,12 +13,13 @@ module DB
   # 4. Override `#column_count`, `#column_name`.
   # 5. Override `#column_type`. It must return a type in `DB::TYPES`.
   abstract class ResultSet
-    # :nodoc:
     getter statement
 
     def initialize(@statement : Statement)
     end
 
+    # TODO add_next_result_set : Bool
+
     # Iterates over all the rows
     def each
       while move_next
@@ -26,9 +27,24 @@ module DB
       end
     end
 
-    # Closes the result set.
+    # Closes this result set.
     def close
-      @statement.close
+      return if @closed
+      @closed = true
+      do_close
+    end
+
+    # Returns `true` if this result set is closed. See `#close`.
+    def closed?
+      @closed
+    end
+
+    # :nodoc:
+    def finalize
+      close
+    end
+
+    protected def do_close
     end
 
     # Ensures it executes the query
@@ -65,5 +81,13 @@ module DB
         read?({{t}}).not_nil!
       end
     {% end %}
+
+    # def read_blob
+    #   yield ... io ....
+    # end
+
+    # def read_text
+    #   yield ... io ....
+    # end
   end
 end

src/db/statement.cr

@@ -72,35 +72,20 @@ module DB
       end
     end
 
+    private def execute : ResultSet
+      perform(Slice(Any).new(0))
+    end
+
     private def execute(*args) : ResultSet
-      execute args
-    end
-
-    private def execute(arg : Slice(UInt8))
-      begin_parameters
-      add_parameter 0, arg
-      perform
-    end
-
-    private def execute(args : Enumerable)
-      begin_parameters
-      args.each_with_index do |arg, index|
-        if arg.is_a?(Hash)
-          arg.each do |key, value|
-            add_parameter key.to_s, value
-          end
-        else
-          add_parameter index, arg
-        end
-      end
-      perform
+      # TODO better way to do it
+      perform(args.to_a.to_unsafe.to_slice(args.size))
     end
 
     # Closes this statement.
     def close
-      raise "Statement already closed" if @closed
+      return if @closed # make it work if closed
       @closed = true
-      on_close
+      do_close
     end
 
     # Returns `true` if this statement is closed. See `#close`.
@@ -108,19 +93,14 @@ module DB
       @closed
     end
 
-    # # :nodoc:
-    # def finalize
-    #   close unless closed?
-    # end
-
-    # 0-based positional arguments
-    protected def begin_parameters
+    # :nodoc:
+    def finalize
+      close
     end
-    protected abstract def add_parameter(index : Int32, value)
-    protected abstract def add_parameter(name : String, value)
 
-    protected abstract def perform : ResultSet
-    protected def on_close
+    protected abstract def perform(args : Slice(Any)) : ResultSet
+
+    protected def do_close
     end
   end
 end