From d01da912f7221e7e87bc0e6a5dd47f2a22a41642 Mon Sep 17 00:00:00 2001 From: "Brian J. Cardiff" Date: Mon, 1 Feb 2016 21:55:30 -0300 Subject: [PATCH] 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 | 9 ++- spec/std/db/dummy_driver.cr | 37 ++++-------- spec/std/db/dummy_driver_spec.cr | 26 +------- spec/std/db/statement_spec.cr | 100 ++++--------------------------- src/db/connection.cr | 15 +++-- src/db/database.cr | 3 +- src/db/db.cr | 5 +- src/db/query_methods.cr | 2 + src/db/result_set.cr | 30 +++++++++- src/db/statement.cr | 48 +++++---------- 10 files changed, 86 insertions(+), 189 deletions(-) diff --git a/spec/std/db/db_spec.cr b/spec/std/db/db_spec.cr index d57925b..d8a3e8b 100644 --- a/spec/std/db/db_spec.cr +++ b/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 diff --git a/spec/std/db/dummy_driver.cr b/spec/std/db/dummy_driver.cr index 83806c7..1d9e1ef 100644 --- a/spec/std/db/dummy_driver.cr +++ b/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 diff --git a/spec/std/db/dummy_driver_spec.cr b/spec/std/db/dummy_driver_spec.cr index 4454564..05301d6 100644 --- a/spec/std/db/dummy_driver_spec.cr +++ b/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 diff --git a/spec/std/db/statement_spec.cr b/spec/std/db/statement_spec.cr index 1bfcd60..c41dfe6 100644 --- a/spec/std/db/statement_spec.cr +++ b/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 diff --git a/src/db/connection.cr b/src/db/connection.cr index ad67283..9ac0900 100644 --- a/src/db/connection.cr +++ b/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 diff --git a/src/db/database.cr b/src/db/database.cr index b6b1dc7..d31fa72 100644 --- a/src/db/database.cr +++ b/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 diff --git a/src/db/db.cr b/src/db/db.cr index 4093c6c..0ad093e 100644 --- a/src/db/db.cr +++ b/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 diff --git a/src/db/query_methods.cr b/src/db/query_methods.cr index 9a50250..e154974 100644 --- a/src/db/query_methods.cr +++ b/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. # diff --git a/src/db/result_set.cr b/src/db/result_set.cr index 669e630..d6a680c 100644 --- a/src/db/result_set.cr +++ b/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 diff --git a/src/db/statement.cr b/src/db/statement.cr index c97d7f8..cfe5600 100644 --- a/src/db/statement.cr +++ b/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