From fe0ed55ef9096f5b829d5bff5fa5f3e8aab0b27e Mon Sep 17 00:00:00 2001 From: "Brian J. Cardiff" Date: Tue, 29 Nov 2016 20:14:07 -0300 Subject: [PATCH 1/8] introduce unprepared statements * rename QueryMethods#prepare to QueryMethods#build * rename Connection#build_statement to Connection#build_prepared_statement * add Connection#build_unprepared_statement * add Connection #prepared and #unprepared dsl methods --- spec/custom_drivers_types_spec.cr | 12 +++++- spec/database_spec.cr | 14 +++---- spec/dummy_driver.cr | 14 +++++-- spec/statement_spec.cr | 40 ++++++++++++-------- src/db/connection.cr | 61 +++++++++++++++++++++++++++++-- src/db/database.cr | 2 +- src/db/pool_statement.cr | 2 +- src/db/query_methods.cr | 12 +++--- 8 files changed, 118 insertions(+), 39 deletions(-) diff --git a/spec/custom_drivers_types_spec.cr b/spec/custom_drivers_types_spec.cr index f0fc8f1..8f4a0ca 100644 --- a/spec/custom_drivers_types_spec.cr +++ b/spec/custom_drivers_types_spec.cr @@ -48,9 +48,13 @@ class FooDriver < DB::Driver end class FooConnection < DB::Connection - def build_statement(query) + def build_prepared_statement(query) FooStatement.new(self) end + + def build_unprepared_statement(query) + raise "not implemented" + end end class FooStatement < DB::Statement @@ -107,9 +111,13 @@ class BarDriver < DB::Driver end class BarConnection < DB::Connection - def build_statement(query) + def build_prepared_statement(query) BarStatement.new(self) end + + def build_unprepared_statement(query) + raise "not implemented" + end end class BarStatement < DB::Statement diff --git a/spec/database_spec.cr b/spec/database_spec.cr index 9a42425..fe4f140 100644 --- a/spec/database_spec.cr +++ b/spec/database_spec.cr @@ -42,24 +42,24 @@ describe DB::Database do it "should allow creation of more statements than pool connections" do DB.open "dummy://localhost:1027?initial_pool_size=1&max_pool_size=2" do |db| - db.prepare("query1").should be_a(DB::PoolStatement) - db.prepare("query2").should be_a(DB::PoolStatement) - db.prepare("query3").should be_a(DB::PoolStatement) + db.build("query1").should be_a(DB::PoolStatement) + db.build("query2").should be_a(DB::PoolStatement) + db.build("query3").should be_a(DB::PoolStatement) end end it "should return same statement in pool per query" do with_dummy do |db| - stmt = db.prepare("query1") - db.prepare("query2").should_not eq(stmt) - db.prepare("query1").should eq(stmt) + stmt = db.build("query1") + db.build("query2").should_not eq(stmt) + db.build("query1").should eq(stmt) end end it "should close pool statements when closing db" do stmt = uninitialized DB::PoolStatement with_dummy do |db| - stmt = db.prepare("query1") + stmt = db.build("query1") end stmt.closed?.should be_true end diff --git a/spec/dummy_driver.cr b/spec/dummy_driver.cr index fbd02e0..d7e4cdd 100644 --- a/spec/dummy_driver.cr +++ b/spec/dummy_driver.cr @@ -22,8 +22,12 @@ class DummyDriver < DB::Driver @@connections.try &.clear end - def build_statement(query) - DummyStatement.new(self, query) + def build_prepared_statement(query) + DummyStatement.new(self, query, true) + end + + def build_unprepared_statement(query) + DummyStatement.new(self, query, false) end def last_insert_id : Int64 @@ -46,7 +50,7 @@ class DummyDriver < DB::Driver class DummyStatement < DB::Statement property params - def initialize(connection, @query : String) + def initialize(connection, @query : String, @prepared : Bool) @params = Hash(Int32 | String, DB::Any).new super(connection) end @@ -79,6 +83,10 @@ class DummyDriver < DB::Driver raise "not implemented for #{value.class}" end + def prepared? + @prepared + end + protected def do_close super end diff --git a/spec/statement_spec.cr b/spec/statement_spec.cr index ad7b4b7..063bd07 100644 --- a/spec/statement_spec.cr +++ b/spec/statement_spec.cr @@ -1,15 +1,25 @@ require "./spec_helper" describe DB::Statement do - it "should prepare statements" do + it "should build prepared statements" do with_dummy_connection do |cnn| - cnn.prepare("the query").should be_a(DB::Statement) + prepared = cnn.prepared("the query") + prepared.should be_a(DB::Statement) + prepared.as(DummyDriver::DummyStatement).prepared?.should be_true + end + end + + it "should build unprepared statements" do + with_dummy_connection do |cnn| + prepared = cnn.unprepared("the query") + prepared.should be_a(DB::Statement) + prepared.as(DummyDriver::DummyStatement).prepared?.should be_false end end it "should initialize positional params in query" do with_dummy_connection do |cnn| - stmt = cnn.prepare("the query").as(DummyDriver::DummyStatement) + stmt = cnn.prepared("the query").as(DummyDriver::DummyStatement) stmt.query "a", 1, nil stmt.params[0].should eq("a") stmt.params[1].should eq(1) @@ -19,7 +29,7 @@ describe DB::Statement do it "should initialize positional params in query with array" do with_dummy_connection do |cnn| - stmt = cnn.prepare("the query").as(DummyDriver::DummyStatement) + stmt = cnn.prepared("the query").as(DummyDriver::DummyStatement) stmt.query ["a", 1, nil] stmt.params[0].should eq("a") stmt.params[1].should eq(1) @@ -29,7 +39,7 @@ describe DB::Statement do it "should initialize positional params in exec" do with_dummy_connection do |cnn| - stmt = cnn.prepare("the query").as(DummyDriver::DummyStatement) + stmt = cnn.prepared("the query").as(DummyDriver::DummyStatement) stmt.exec "a", 1, nil stmt.params[0].should eq("a") stmt.params[1].should eq(1) @@ -39,7 +49,7 @@ describe DB::Statement do it "should initialize positional params in exec with array" do with_dummy_connection do |cnn| - stmt = cnn.prepare("the query").as(DummyDriver::DummyStatement) + stmt = cnn.prepared("the query").as(DummyDriver::DummyStatement) stmt.exec ["a", 1, nil] stmt.params[0].should eq("a") stmt.params[1].should eq(1) @@ -49,7 +59,7 @@ describe DB::Statement do it "should initialize positional params in scalar" do with_dummy_connection do |cnn| - stmt = cnn.prepare("the query").as(DummyDriver::DummyStatement) + stmt = cnn.prepared("the query").as(DummyDriver::DummyStatement) stmt.scalar "a", 1, nil stmt.params[0].should eq("a") stmt.params[1].should eq(1) @@ -59,7 +69,7 @@ describe DB::Statement do it "query with block should not close statement" do with_dummy_connection do |cnn| - stmt = cnn.prepare "3,4 1,2" + stmt = cnn.prepared "3,4 1,2" stmt.query stmt.closed?.should be_false end @@ -68,7 +78,7 @@ describe DB::Statement do it "closing connection should close statement" do stmt = uninitialized DB::Statement with_dummy_connection do |cnn| - stmt = cnn.prepare "3,4 1,2" + stmt = cnn.prepared "3,4 1,2" stmt.query end stmt.closed?.should be_true @@ -76,7 +86,7 @@ describe DB::Statement do it "query with block should not close statement" do with_dummy_connection do |cnn| - stmt = cnn.prepare "3,4 1,2" + stmt = cnn.prepared "3,4 1,2" stmt.query do |rs| end stmt.closed?.should be_false @@ -85,7 +95,7 @@ describe DB::Statement do it "query should not close statement" do with_dummy_connection do |cnn| - stmt = cnn.prepare "3,4 1,2" + stmt = cnn.prepared "3,4 1,2" stmt.query do |rs| end stmt.closed?.should be_false @@ -94,7 +104,7 @@ describe DB::Statement do it "scalar should not close statement" do with_dummy_connection do |cnn| - stmt = cnn.prepare "3,4 1,2" + stmt = cnn.prepared "3,4 1,2" stmt.scalar stmt.closed?.should be_false end @@ -102,7 +112,7 @@ describe DB::Statement do it "exec should not close statement" do with_dummy_connection do |cnn| - stmt = cnn.prepare "3,4 1,2" + stmt = cnn.prepared "3,4 1,2" stmt.exec stmt.closed?.should be_false end @@ -110,11 +120,11 @@ describe DB::Statement do it "connection should cache statements by query" do with_dummy_connection do |cnn| - rs = cnn.query "1, ?", 2 + rs = cnn.prepared.query "1, ?", 2 stmt = rs.statement rs.close - rs = cnn.query "1, ?", 4 + rs = cnn.prepared.query "1, ?", 4 rs.statement.should be(stmt) end end diff --git a/src/db/connection.cr b/src/db/connection.cr index d6a8ee2..42edf8d 100644 --- a/src/db/connection.cr +++ b/src/db/connection.cr @@ -11,7 +11,7 @@ module DB # # The connection must be initialized in `#initialize` and closed in `#do_close`. # - # Override `#build_statement` method in order to return a prepared `Statement` to allow querying. + # Override `#build_prepared_statement` method in order to return a prepared `Statement` to allow querying. # See also `Statement` to define how the statements are executed. # # If at any give moment the connection is lost a DB::ConnectionLost should be raised. This will @@ -29,16 +29,69 @@ module DB end # :nodoc: - def prepare(query) : Statement - @statements_cache.fetch(query) { build_statement(query) } + def build(query) : Statement + # TODO add flag for default statements kind. + # configured in database overridable by connection + fetch_or_build_prepared_statement(query) end - abstract def build_statement(query) : Statement + # :nodoc: + def fetch_or_build_prepared_statement(query) + @statements_cache.fetch(query) { build_prepared_statement(query) } + end + + abstract def build_prepared_statement(query) : Statement + + abstract def build_unprepared_statement(query) : Statement protected def do_close @statements_cache.each_value &.close @statements_cache.clear @database.pool.delete self end + + # dsl helper to build prepared statements + # returns a value that includes `QueryMethods` + def prepared + PreparedQuery.new(self) + end + + # Returns a prepared `Statement` that has not been executed yet. + def prepared(query) + prepared.build(query) + end + + # dsl helper to build unprepared statements + # returns a value that includes `QueryMethods` + def unprepared + UnpreparedQuery.new(self) + end + + # Returns an unprepared `Statement` that has not been executed yet. + def unprepared(query) + unprepared.build(query) + end + + struct PreparedQuery + include QueryMethods + + def initialize(@connection : Connection) + end + + def build(query) + @connection.fetch_or_build_prepared_statement(query) + end + end + + struct UnpreparedQuery + include QueryMethods + + def initialize(@connection : Connection) + end + + def build(query) + @connection.build_unprepared_statement(query) + end + end end end diff --git a/src/db/database.cr b/src/db/database.cr index 668f176..681ff57 100644 --- a/src/db/database.cr +++ b/src/db/database.cr @@ -59,7 +59,7 @@ module DB end # :nodoc: - def prepare(query) + def build(query) @statements_cache.fetch(query) { PoolStatement.new(self, query) } end diff --git a/src/db/pool_statement.cr b/src/db/pool_statement.cr index 6e2354e..237dfd9 100644 --- a/src/db/pool_statement.cr +++ b/src/db/pool_statement.cr @@ -64,7 +64,7 @@ module DB clean_connections conn, existing = @db.checkout_some(@connections) @connections << WeakRef.new(conn) unless existing - conn.prepare(@query) + conn.build(@query) end private def clean_connections diff --git a/src/db/query_methods.cr b/src/db/query_methods.cr index f41596b..802a6ad 100644 --- a/src/db/query_methods.cr +++ b/src/db/query_methods.cr @@ -15,11 +15,11 @@ module DB # # Convention of mapping how arguments are mapped to the query depends on each driver. # - # Including `QueryMethods` requires a `prepare(query) : Statement` method that is not expected + # Including `QueryMethods` requires a `build(query) : Statement` method that is not expected # to be called directly. module QueryMethods # :nodoc: - abstract def prepare(query) : Statement + abstract def build(query) : Statement # Executes a *query* and returns a `ResultSet` with the results. # The `ResultSet` must be closed manually. @@ -35,7 +35,7 @@ module DB # end # ``` def query(query, *args) - prepare(query).query(*args) + build(query).query(*args) end # Executes a *query* and yields a `ResultSet` with the results. @@ -49,7 +49,7 @@ module DB # end # ``` def query(query, *args) - # CHECK prepare(query).query(*args, &block) + # CHECK build(query).query(*args, &block) rs = query(query, *args) yield rs ensure rs.close end @@ -200,13 +200,13 @@ module DB # Performs the `query` and returns an `ExecResult` def exec(query, *args) - prepare(query).exec(*args) + build(query).exec(*args) end # Performs the `query` and returns a single scalar value # puts db.scalar("SELECT MAX(name)").as(String) # => (a String) def scalar(query, *args) - prepare(query).scalar(*args) + build(query).scalar(*args) end end end From 9ef9d19d1da7cd6c50ddb66daaf30da2fbad49c5 Mon Sep 17 00:00:00 2001 From: "Brian J. Cardiff" Date: Fri, 2 Dec 2016 22:09:27 -0300 Subject: [PATCH 2/8] add prepared_statements option to database * use ?prepared_statements=true|false on connection string (default: true) * make inmutable state in database * make mutable state in connection * change Connection#build to use the current prepared_statements flag to build prepared or unprepared statements. --- spec/database_spec.cr | 32 ++++++++++++++++++++++++++++++++ spec/db_spec.cr | 11 +++++++++++ spec/statement_spec.cr | 18 ++++++++++++++++++ src/db.cr | 9 +++++++++ src/db/connection.cr | 10 +++++++--- src/db/database.cr | 3 +++ 6 files changed, 80 insertions(+), 3 deletions(-) diff --git a/spec/database_spec.cr b/spec/database_spec.cr index fe4f140..35df12a 100644 --- a/spec/database_spec.cr +++ b/spec/database_spec.cr @@ -97,4 +97,36 @@ describe DB::Database do DummyDriver::DummyConnection.connections.size.should eq(2) end end + + describe "prepared_statements connection option" do + it "defaults to true" do + with_dummy "dummy://localhost:1027" do |db| + db.prepared_statements?.should be_true + end + end + + it "can be set to false" do + with_dummy "dummy://localhost:1027?prepared_statements=false" do |db| + db.prepared_statements?.should be_false + end + end + + it "is copied to connections and can be changed (false)" do + with_dummy "dummy://localhost:1027?prepared_statements=false&initial_pool_size=1" do |db| + connection = DummyDriver::DummyConnection.connections.first + connection.prepared_statements?.should be_false + connection.prepared_statements = true + connection.prepared_statements?.should be_true + end + end + + it "is copied to connections and can be changed (true)" do + with_dummy "dummy://localhost:1027?prepared_statements=true&initial_pool_size=1" do |db| + connection = DummyDriver::DummyConnection.connections.first + connection.prepared_statements?.should be_true + connection.prepared_statements = false + connection.prepared_statements?.should be_false + end + end + end end diff --git a/spec/db_spec.cr b/spec/db_spec.cr index 0ba8f29..ec42f30 100644 --- a/spec/db_spec.cr +++ b/spec/db_spec.cr @@ -97,4 +97,15 @@ describe DB do DB.open "foobar://baz" end end + + it "should parse boolean query string params" do + DB.fetch_bool(HTTP::Params.parse("foo=true"), "foo", false).should be_true + DB.fetch_bool(HTTP::Params.parse("foo=True"), "foo", false).should be_true + + DB.fetch_bool(HTTP::Params.parse("foo=false"), "foo", true).should be_false + DB.fetch_bool(HTTP::Params.parse("foo=False"), "foo", true).should be_false + + DB.fetch_bool(HTTP::Params.parse("bar=true"), "foo", false).should be_false + DB.fetch_bool(HTTP::Params.parse("bar=true"), "foo", true).should be_true + end end diff --git a/spec/statement_spec.cr b/spec/statement_spec.cr index 063bd07..0e410da 100644 --- a/spec/statement_spec.cr +++ b/spec/statement_spec.cr @@ -17,6 +17,24 @@ describe DB::Statement do end end + describe "prepared_statements flag" do + it "should build prepared statements if true" do + with_dummy_connection do |cnn| + cnn.prepared_statements = true + stmt = cnn.query("the query").statement + stmt.as(DummyDriver::DummyStatement).prepared?.should be_true + end + end + + it "should build unprepared statements if false" do + with_dummy_connection do |cnn| + cnn.prepared_statements = false + stmt = cnn.query("the query").statement + stmt.as(DummyDriver::DummyStatement).prepared?.should be_false + end + end + end + it "should initialize positional params in query" do with_dummy_connection do |cnn| stmt = cnn.prepared("the query").as(DummyDriver::DummyStatement) diff --git a/src/db.cr b/src/db.cr index 9e2979c..bd18035 100644 --- a/src/db.cr +++ b/src/db.cr @@ -120,6 +120,15 @@ module DB private def self.build_database(uri : URI) Database.new(driver_class(uri.scheme).new, uri) end + + # :nodoc: + def self.fetch_bool(params : HTTP::Params, name, default : Bool) + if value = params[name]? + value.underscore == "true" + else + default + end + end end require "./db/pool" diff --git a/src/db/connection.cr b/src/db/connection.cr index 42edf8d..1b1aeb2 100644 --- a/src/db/connection.cr +++ b/src/db/connection.cr @@ -24,15 +24,19 @@ module DB # :nodoc: getter database @statements_cache = StringKeyCache(Statement).new + property? prepared_statements : Bool def initialize(@database : Database) + @prepared_statements = @database.prepared_statements? end # :nodoc: def build(query) : Statement - # TODO add flag for default statements kind. - # configured in database overridable by connection - fetch_or_build_prepared_statement(query) + if prepared_statements? + fetch_or_build_prepared_statement(query) + else + build_unprepared_statement(query) + end end # :nodoc: diff --git a/src/db/database.cr b/src/db/database.cr index 681ff57..31c011c 100644 --- a/src/db/database.cr +++ b/src/db/database.cr @@ -25,6 +25,8 @@ module DB # Returns the uri with the connection settings to the database getter uri + getter? prepared_statements : Bool + @pool : Pool(Connection) @setup_connection : Connection -> Nil @statements_cache = StringKeyCache(PoolStatement).new @@ -32,6 +34,7 @@ module DB # :nodoc: def initialize(@driver : Driver, @uri : URI) params = HTTP::Params.parse(uri.query || "") + @prepared_statements = DB.fetch_bool(params, "prepared_statements", true) pool_options = @driver.connection_pool_options(params) @setup_connection = ->(conn : Connection) {} From 0593f63dbbe44a3c4bc11b8b9403aeff82593488 Mon Sep 17 00:00:00 2001 From: "Brian J. Cardiff" Date: Sat, 3 Dec 2016 15:56:03 -0300 Subject: [PATCH 3/8] add pool unprepared statements unprepared statements are executed in any free connection of the pool at the moment of executing them --- spec/database_spec.cr | 45 ++++++++++++++++++- src/db.cr | 1 + src/db/database.cr | 67 ++++++++++++++++++++++++++++- src/db/pool_unprepared_statement.cr | 58 +++++++++++++++++++++++++ 4 files changed, 168 insertions(+), 3 deletions(-) create mode 100644 src/db/pool_unprepared_statement.cr diff --git a/spec/database_spec.cr b/spec/database_spec.cr index 35df12a..88de311 100644 --- a/spec/database_spec.cr +++ b/spec/database_spec.cr @@ -59,7 +59,8 @@ describe DB::Database do it "should close pool statements when closing db" do stmt = uninitialized DB::PoolStatement with_dummy do |db| - stmt = db.build("query1") + # TODO remove cast + stmt = db.build("query1").as(DB::PoolStatement) end stmt.closed?.should be_true end @@ -128,5 +129,47 @@ describe DB::Database do connection.prepared_statements?.should be_false end end + + it "should build prepared statements if true" do + with_dummy "dummy://localhost:1027?prepared_statements=true" do |db| + db.build("the query").should be_a(DB::PoolStatement) + end + end + + it "should build unprepared statements if false" do + with_dummy "dummy://localhost:1027?prepared_statements=false" do |db| + db.build("the query").should be_a(DB::PoolUnpreparedStatement) + end + end + end + + describe "unprepared statements in pool" do + it "creating statements should not create new connections" do + with_dummy "dummy://localhost:1027?initial_pool_size=1" do |db| + stmt1 = db.unprepared.build("query1") + stmt2 = db.unprepared.build("query2") + DummyDriver::DummyConnection.connections.size.should eq(1) + end + end + + it "simultaneous statements should go to different connections" do + with_dummy "dummy://localhost:1027?initial_pool_size=1" do |db| + rs1 = db.unprepared.query("query1") + rs2 = db.unprepared.query("query2") + rs1.statement.connection.should_not eq(rs2.statement.connection) + DummyDriver::DummyConnection.connections.size.should eq(2) + end + end + + it "sequential statements should go to different connections" do + with_dummy "dummy://localhost:1027?initial_pool_size=1" do |db| + rs1 = db.unprepared.query("query1") + rs1.close + rs2 = db.unprepared.query("query2") + rs2.close + rs1.statement.connection.should eq(rs2.statement.connection) + DummyDriver::DummyConnection.connections.size.should eq(1) + end + end end end diff --git a/src/db.cr b/src/db.cr index bd18035..faffaa4 100644 --- a/src/db.cr +++ b/src/db.cr @@ -140,6 +140,7 @@ require "./db/driver" require "./db/connection" require "./db/statement" require "./db/pool_statement" +require "./db/pool_unprepared_statement" require "./db/result_set" require "./db/error" require "./db/mapping" diff --git a/src/db/database.cr b/src/db/database.cr index 31c011c..799f276 100644 --- a/src/db/database.cr +++ b/src/db/database.cr @@ -17,6 +17,8 @@ module DB # # Refer to `QueryMethods` for documentation about querying the database. class Database + include QueryMethods + # :nodoc: getter driver # :nodoc: @@ -63,7 +65,26 @@ module DB # :nodoc: def build(query) - @statements_cache.fetch(query) { PoolStatement.new(self, query) } + if prepared_statements? + fetch_or_build_prepared_statement(query) + else + build_unprepared_statement(query) + end + end + + # :nodoc: + def fetch_or_build_prepared_statement(query) + @statements_cache.fetch(query) { build_prepared_statement(query) } + end + + # :nodoc: + def build_prepared_statement(query) + PoolStatement.new(self, query) + end + + # :nodoc: + def build_unprepared_statement(query) + PoolUnpreparedStatement.new(self, query) end # :nodoc: @@ -95,6 +116,48 @@ module DB end end - include QueryMethods + # dsl helper to build prepared statements + # returns a value that includes `QueryMethods` + def prepared + PreparedQuery.new(self) + end + + # Returns a prepared `Statement` that has not been executed yet. + def prepared(query) + prepared.build(query) + end + + # dsl helper to build unprepared statements + # returns a value that includes `QueryMethods` + def unprepared + UnpreparedQuery.new(self) + end + + # Returns an unprepared `Statement` that has not been executed yet. + def unprepared(query) + unprepared.build(query) + end + + struct PreparedQuery + include QueryMethods + + def initialize(@db : Database) + end + + def build(query) + @db.fetch_or_build_prepared_statement(query) + end + end + + struct UnpreparedQuery + include QueryMethods + + def initialize(@db : Database) + end + + def build(query) + @db.build_unprepared_statement(query) + end + end end end diff --git a/src/db/pool_unprepared_statement.cr b/src/db/pool_unprepared_statement.cr new file mode 100644 index 0000000..de0c517 --- /dev/null +++ b/src/db/pool_unprepared_statement.cr @@ -0,0 +1,58 @@ +module DB + # Represents a statement to be executed in any of the connections + # of the pool. The statement is not be executed in a non prepared fashion. + # The execution of the statement is retried according to the pool configuration. + # + # See `PoolStatement` + class PoolUnpreparedStatement + include StatementMethods + + def initialize(@db : Database, @query : String) + end + + protected def do_close + # unprepared statements do not need to be release in each connection + end + + # See `QueryMethods#exec` + def exec : ExecResult + statement_with_retry &.exec + end + + # See `QueryMethods#exec` + def exec(*args) : ExecResult + statement_with_retry &.exec(*args) + end + + # See `QueryMethods#exec` + def exec(args : Array) : ExecResult + statement_with_retry &.exec(args) + end + + # See `QueryMethods#query` + def query : ResultSet + statement_with_retry &.query + end + + # See `QueryMethods#query` + def query(*args) : ResultSet + statement_with_retry &.query(*args) + end + + # See `QueryMethods#query` + def query(args : Array) : ResultSet + statement_with_retry &.query(args) + end + + # builds a statement over a real connection + private def build_statement + @db.pool.checkout.unprepared.build(@query) + end + + private def statement_with_retry + @db.retry do + return yield build_statement + end + end + end +end From 2cab0b37f5a6bca1be0b7aebd4ae20d589df43b3 Mon Sep 17 00:00:00 2001 From: "Brian J. Cardiff" Date: Sat, 3 Dec 2016 16:03:50 -0300 Subject: [PATCH 4/8] refactor prepared and unprepared pool statements --- spec/database_spec.cr | 11 +++---- src/db.cr | 1 + src/db/database.cr | 6 ++-- src/db/pool_prepared_statement.cr | 50 +++++++++++++++++++++++++++++ src/db/pool_statement.cr | 38 ++-------------------- src/db/pool_unprepared_statement.cr | 43 ++----------------------- 6 files changed, 64 insertions(+), 85 deletions(-) create mode 100644 src/db/pool_prepared_statement.cr diff --git a/spec/database_spec.cr b/spec/database_spec.cr index 88de311..608c7e6 100644 --- a/spec/database_spec.cr +++ b/spec/database_spec.cr @@ -42,9 +42,9 @@ describe DB::Database do it "should allow creation of more statements than pool connections" do DB.open "dummy://localhost:1027?initial_pool_size=1&max_pool_size=2" do |db| - db.build("query1").should be_a(DB::PoolStatement) - db.build("query2").should be_a(DB::PoolStatement) - db.build("query3").should be_a(DB::PoolStatement) + db.build("query1").should be_a(DB::PoolPreparedStatement) + db.build("query2").should be_a(DB::PoolPreparedStatement) + db.build("query3").should be_a(DB::PoolPreparedStatement) end end @@ -59,8 +59,7 @@ describe DB::Database do it "should close pool statements when closing db" do stmt = uninitialized DB::PoolStatement with_dummy do |db| - # TODO remove cast - stmt = db.build("query1").as(DB::PoolStatement) + stmt = db.build("query1") end stmt.closed?.should be_true end @@ -132,7 +131,7 @@ describe DB::Database do it "should build prepared statements if true" do with_dummy "dummy://localhost:1027?prepared_statements=true" do |db| - db.build("the query").should be_a(DB::PoolStatement) + db.build("the query").should be_a(DB::PoolPreparedStatement) end end diff --git a/src/db.cr b/src/db.cr index faffaa4..71d64c6 100644 --- a/src/db.cr +++ b/src/db.cr @@ -140,6 +140,7 @@ require "./db/driver" require "./db/connection" require "./db/statement" require "./db/pool_statement" +require "./db/pool_prepared_statement" require "./db/pool_unprepared_statement" require "./db/result_set" require "./db/error" diff --git a/src/db/database.cr b/src/db/database.cr index 799f276..95d2ecd 100644 --- a/src/db/database.cr +++ b/src/db/database.cr @@ -31,7 +31,7 @@ module DB @pool : Pool(Connection) @setup_connection : Connection -> Nil - @statements_cache = StringKeyCache(PoolStatement).new + @statements_cache = StringKeyCache(PoolPreparedStatement).new # :nodoc: def initialize(@driver : Driver, @uri : URI) @@ -64,7 +64,7 @@ module DB end # :nodoc: - def build(query) + def build(query) : PoolStatement if prepared_statements? fetch_or_build_prepared_statement(query) else @@ -79,7 +79,7 @@ module DB # :nodoc: def build_prepared_statement(query) - PoolStatement.new(self, query) + PoolPreparedStatement.new(self, query) end # :nodoc: diff --git a/src/db/pool_prepared_statement.cr b/src/db/pool_prepared_statement.cr new file mode 100644 index 0000000..9fe8f5d --- /dev/null +++ b/src/db/pool_prepared_statement.cr @@ -0,0 +1,50 @@ +module DB + # Represents a statement to be executed in any of the connections + # of the pool. The statement is not be executed in a prepared fashion. + # The execution of the statement is retried according to the pool configuration. + # + # See `PoolStatement` + class PoolPreparedStatement < PoolStatement + # connections where the statement was prepared + @connections = Set(WeakRef(Connection)).new + + def initialize(db : Database, query : String) + super + # Prepares a statement on some connection + # otherwise the preparation is delayed until the first execution. + # After the first initialization the connection must be released + # it will be checked out when executing it. + statement_with_retry &.release_connection + # TODO use a round-robin selection in the pool so multiple sequentially + # initialized statements are assigned to different connections. + end + + protected def do_close + # TODO close all statements on all connections. + # currently statements are closed when the connection is closed. + + # WHAT-IF the connection is busy? Should each statement be able to + # deallocate itself when the connection is free. + @connections.clear + end + + # builds a statement over a real connection + # the conneciton is registered in `@connections` + private def build_statement + clean_connections + conn, existing = @db.checkout_some(@connections) + @connections << WeakRef.new(conn) unless existing + conn.build(@query) + end + + private def clean_connections + # remove disposed or closed connections + @connections.each do |ref| + conn = ref.target + if !conn || conn.closed? + @connections.delete ref + end + end + end + end +end diff --git a/src/db/pool_statement.cr b/src/db/pool_statement.cr index 237dfd9..5a7b51f 100644 --- a/src/db/pool_statement.cr +++ b/src/db/pool_statement.cr @@ -3,29 +3,10 @@ module DB # a statement from the DB needs to be able to represent a statement in any # of the connections of the pool. Otherwise the user will need to deal with # actual connections in some point. - class PoolStatement + abstract class PoolStatement include StatementMethods - # connections where the statement was prepared - @connections = Set(WeakRef(Connection)).new - def initialize(@db : Database, @query : String) - # Prepares a statement on some connection - # otherwise the preparation is delayed until the first execution. - # After the first initialization the connection must be released - # it will be checked out when executing it. - statement_with_retry &.release_connection - # TODO use a round-robin selection in the pool so multiple sequentially - # initialized statements are assigned to different connections. - end - - protected def do_close - # TODO close all statements on all connections. - # currently statements are closed when the connection is closed. - - # WHAT-IF the connection is busy? Should each statement be able to - # deallocate itself when the connection is free. - @connections.clear end # See `QueryMethods#exec` @@ -60,22 +41,7 @@ module DB # builds a statement over a real connection # the conneciton is registered in `@connections` - private def build_statement - clean_connections - conn, existing = @db.checkout_some(@connections) - @connections << WeakRef.new(conn) unless existing - conn.build(@query) - end - - private def clean_connections - # remove disposed or closed connections - @connections.each do |ref| - conn = ref.target - if !conn || conn.closed? - @connections.delete ref - end - end - end + private abstract def build_statement : Statement private def statement_with_retry @db.retry do diff --git a/src/db/pool_unprepared_statement.cr b/src/db/pool_unprepared_statement.cr index de0c517..1a120e5 100644 --- a/src/db/pool_unprepared_statement.cr +++ b/src/db/pool_unprepared_statement.cr @@ -4,55 +4,18 @@ module DB # The execution of the statement is retried according to the pool configuration. # # See `PoolStatement` - class PoolUnpreparedStatement - include StatementMethods - - def initialize(@db : Database, @query : String) + class PoolUnpreparedStatement < PoolStatement + def initialize(db : Database, query : String) + super end protected def do_close # unprepared statements do not need to be release in each connection end - # See `QueryMethods#exec` - def exec : ExecResult - statement_with_retry &.exec - end - - # See `QueryMethods#exec` - def exec(*args) : ExecResult - statement_with_retry &.exec(*args) - end - - # See `QueryMethods#exec` - def exec(args : Array) : ExecResult - statement_with_retry &.exec(args) - end - - # See `QueryMethods#query` - def query : ResultSet - statement_with_retry &.query - end - - # See `QueryMethods#query` - def query(*args) : ResultSet - statement_with_retry &.query(*args) - end - - # See `QueryMethods#query` - def query(args : Array) : ResultSet - statement_with_retry &.query(args) - end - # builds a statement over a real connection private def build_statement @db.pool.checkout.unprepared.build(@query) end - - private def statement_with_retry - @db.retry do - return yield build_statement - end - end end end From 543592a337a3bcf9f6779ce64dc62aeb037d3baf Mon Sep 17 00:00:00 2001 From: "Brian J. Cardiff" Date: Sat, 3 Dec 2016 20:59:14 -0300 Subject: [PATCH 5/8] make pool_prepared_statements always prepare statement in connection --- spec/database_spec.cr | 12 ++++++++++++ src/db/pool_prepared_statement.cr | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/spec/database_spec.cr b/spec/database_spec.cr index 608c7e6..f20aac1 100644 --- a/spec/database_spec.cr +++ b/spec/database_spec.cr @@ -140,6 +140,18 @@ describe DB::Database do db.build("the query").should be_a(DB::PoolUnpreparedStatement) end end + + it "should be overrided by dsl" do + with_dummy "dummy://localhost:1027?prepared_statements=true" do |db| + stmt = db.unprepared.query("the query").statement.as(DummyDriver::DummyStatement) + stmt.prepared?.should be_false + end + + with_dummy "dummy://localhost:1027?prepared_statements=false" do |db| + stmt = db.prepared.query("the query").statement.as(DummyDriver::DummyStatement) + stmt.prepared?.should be_true + end + end end describe "unprepared statements in pool" do diff --git a/src/db/pool_prepared_statement.cr b/src/db/pool_prepared_statement.cr index 9fe8f5d..63ed49b 100644 --- a/src/db/pool_prepared_statement.cr +++ b/src/db/pool_prepared_statement.cr @@ -34,7 +34,7 @@ module DB clean_connections conn, existing = @db.checkout_some(@connections) @connections << WeakRef.new(conn) unless existing - conn.build(@query) + conn.prepared.build(@query) end private def clean_connections From d55b088216ed47cee9c06d7153273dcad29ac677 Mon Sep 17 00:00:00 2001 From: "Brian J. Cardiff" Date: Sun, 4 Dec 2016 15:14:43 -0300 Subject: [PATCH 6/8] Refactor Database and Connection dsl methods. Update docs. --- src/db.cr | 5 +-- src/db/connection.cr | 58 +++---------------------------- src/db/database.cr | 62 ++++----------------------------- src/db/session_methods.cr | 73 +++++++++++++++++++++++++++++++++++++++ src/db/statement.cr | 2 +- 5 files changed, 88 insertions(+), 112 deletions(-) create mode 100644 src/db/session_methods.cr diff --git a/src/db.cr b/src/db.cr index 71d64c6..43a90f4 100644 --- a/src/db.cr +++ b/src/db.cr @@ -134,12 +134,13 @@ end require "./db/pool" require "./db/string_key_cache" require "./db/query_methods" +require "./db/session_methods" require "./db/disposable" -require "./db/database" require "./db/driver" -require "./db/connection" require "./db/statement" +require "./db/connection" require "./db/pool_statement" +require "./db/database" require "./db/pool_prepared_statement" require "./db/pool_unprepared_statement" require "./db/result_set" diff --git a/src/db/connection.cr b/src/db/connection.cr index 1b1aeb2..74ac257 100644 --- a/src/db/connection.cr +++ b/src/db/connection.cr @@ -12,6 +12,7 @@ module DB # The connection must be initialized in `#initialize` and closed in `#do_close`. # # Override `#build_prepared_statement` method in order to return a prepared `Statement` to allow querying. + # Override `#build_unprepared_statement` method in order to return a unprepared `Statement` to allow querying. # See also `Statement` to define how the statements are executed. # # If at any give moment the connection is lost a DB::ConnectionLost should be raised. This will @@ -19,7 +20,7 @@ module DB # abstract class Connection include Disposable - include QueryMethods + include SessionMethods(Connection, Statement) # :nodoc: getter database @@ -30,22 +31,15 @@ module DB @prepared_statements = @database.prepared_statements? end - # :nodoc: - def build(query) : Statement - if prepared_statements? - fetch_or_build_prepared_statement(query) - else - build_unprepared_statement(query) - end - end - # :nodoc: def fetch_or_build_prepared_statement(query) @statements_cache.fetch(query) { build_prepared_statement(query) } end + # :nodoc: abstract def build_prepared_statement(query) : Statement + # :nodoc: abstract def build_unprepared_statement(query) : Statement protected def do_close @@ -53,49 +47,5 @@ module DB @statements_cache.clear @database.pool.delete self end - - # dsl helper to build prepared statements - # returns a value that includes `QueryMethods` - def prepared - PreparedQuery.new(self) - end - - # Returns a prepared `Statement` that has not been executed yet. - def prepared(query) - prepared.build(query) - end - - # dsl helper to build unprepared statements - # returns a value that includes `QueryMethods` - def unprepared - UnpreparedQuery.new(self) - end - - # Returns an unprepared `Statement` that has not been executed yet. - def unprepared(query) - unprepared.build(query) - end - - struct PreparedQuery - include QueryMethods - - def initialize(@connection : Connection) - end - - def build(query) - @connection.fetch_or_build_prepared_statement(query) - end - end - - struct UnpreparedQuery - include QueryMethods - - def initialize(@connection : Connection) - end - - def build(query) - @connection.build_unprepared_statement(query) - end - end end end diff --git a/src/db/database.cr b/src/db/database.cr index 95d2ecd..f80e6b8 100644 --- a/src/db/database.cr +++ b/src/db/database.cr @@ -13,11 +13,16 @@ module DB # - retry_attempts (default 1) # - retry_delay (in seconds, default 1.0) # + # When querying a database prepared statements are used by default. + # This can be changed from the `prepared_statements` URI parameter: + # + # - prepared_statements = `true`|`false` (default `true`) + # # It should be created from DB module. See `DB#open`. # - # Refer to `QueryMethods` for documentation about querying the database. + # Refer to `QueryMethods` and `SessionMethods` for documentation about querying the database. class Database - include QueryMethods + include SessionMethods(Database, PoolStatement) # :nodoc: getter driver @@ -63,15 +68,6 @@ module DB @pool.close end - # :nodoc: - def build(query) : PoolStatement - if prepared_statements? - fetch_or_build_prepared_statement(query) - else - build_unprepared_statement(query) - end - end - # :nodoc: def fetch_or_build_prepared_statement(query) @statements_cache.fetch(query) { build_prepared_statement(query) } @@ -115,49 +111,5 @@ module DB yield end end - - # dsl helper to build prepared statements - # returns a value that includes `QueryMethods` - def prepared - PreparedQuery.new(self) - end - - # Returns a prepared `Statement` that has not been executed yet. - def prepared(query) - prepared.build(query) - end - - # dsl helper to build unprepared statements - # returns a value that includes `QueryMethods` - def unprepared - UnpreparedQuery.new(self) - end - - # Returns an unprepared `Statement` that has not been executed yet. - def unprepared(query) - unprepared.build(query) - end - - struct PreparedQuery - include QueryMethods - - def initialize(@db : Database) - end - - def build(query) - @db.fetch_or_build_prepared_statement(query) - end - end - - struct UnpreparedQuery - include QueryMethods - - def initialize(@db : Database) - end - - def build(query) - @db.build_unprepared_statement(query) - end - end end end diff --git a/src/db/session_methods.cr b/src/db/session_methods.cr new file mode 100644 index 0000000..1bc189c --- /dev/null +++ b/src/db/session_methods.cr @@ -0,0 +1,73 @@ +module DB + # Methods that are shared accross session like objects: + # - Database + # - Connection + # + # Classes that includes this module are able to execute + # queries and statements in both prepared and unprepared fashion. + # + # This module serves for dsl reuse over session like objects. + module SessionMethods(Session, Stmt) + include QueryMethods + + # Returns whether by default the statements should + # be prepared or not. + abstract def prepared_statements? : Bool + + abstract def fetch_or_build_prepared_statement(query) : Stmt + + abstract def build_unprepared_statement(query) : Stmt + + def build(query) : Stmt + if prepared_statements? + fetch_or_build_prepared_statement(query) + else + build_unprepared_statement(query) + end + end + + # dsl helper to build prepared statements + # returns a value that includes `QueryMethods` + def prepared + PreparedQuery(Session, Stmt).new(self) + end + + # Returns a prepared `Statement` that has not been executed yet. + def prepared(query) + prepared.build(query) + end + + # dsl helper to build unprepared statements + # returns a value that includes `QueryMethods` + def unprepared + UnpreparedQuery(Session, Stmt).new(self) + end + + # Returns an unprepared `Statement` that has not been executed yet. + def unprepared(query) + unprepared.build(query) + end + + struct PreparedQuery(Session, Stmt) + include QueryMethods + + def initialize(@session : Session) + end + + def build(query) : Stmt + @session.fetch_or_build_prepared_statement(query) + end + end + + struct UnpreparedQuery(Session, Stmt) + include QueryMethods + + def initialize(@session : Session) + end + + def build(query) : Stmt + @session.build_unprepared_statement(query) + end + end + end +end diff --git a/src/db/statement.cr b/src/db/statement.cr index 3f8058b..01eba35 100644 --- a/src/db/statement.cr +++ b/src/db/statement.cr @@ -39,7 +39,7 @@ module DB abstract def query(args : Array) : ResultSet end - # Represents a prepared query in a `Connection`. + # Represents a query in a `Connection`. # It should be created by `QueryMethods`. # # ### Note to implementors From 4721ecbf6b4e33bd4e7cc2ec70e0eefdb6a20de4 Mon Sep 17 00:00:00 2001 From: "Brian J. Cardiff" Date: Sun, 4 Dec 2016 15:19:15 -0300 Subject: [PATCH 7/8] make Connection#prepared_statements? readonly --- spec/database_spec.cr | 8 ++------ spec/dummy_driver.cr | 4 ++-- spec/statement_spec.cr | 8 +++----- src/db/connection.cr | 2 +- 4 files changed, 8 insertions(+), 14 deletions(-) diff --git a/spec/database_spec.cr b/spec/database_spec.cr index f20aac1..3915a02 100644 --- a/spec/database_spec.cr +++ b/spec/database_spec.cr @@ -111,21 +111,17 @@ describe DB::Database do end end - it "is copied to connections and can be changed (false)" do + it "is copied to connections (false)" do with_dummy "dummy://localhost:1027?prepared_statements=false&initial_pool_size=1" do |db| connection = DummyDriver::DummyConnection.connections.first connection.prepared_statements?.should be_false - connection.prepared_statements = true - connection.prepared_statements?.should be_true end end - it "is copied to connections and can be changed (true)" do + it "is copied to connections (true)" do with_dummy "dummy://localhost:1027?prepared_statements=true&initial_pool_size=1" do |db| connection = DummyDriver::DummyConnection.connections.first connection.prepared_statements?.should be_true - connection.prepared_statements = false - connection.prepared_statements?.should be_false end end diff --git a/spec/dummy_driver.cr b/spec/dummy_driver.cr index d7e4cdd..426fe74 100644 --- a/spec/dummy_driver.cr +++ b/spec/dummy_driver.cr @@ -212,8 +212,8 @@ def with_dummy(uri : String = "dummy://host?checkout_timeout=0.5") end end -def with_dummy_connection - with_dummy do |db| +def with_dummy_connection(options = "") + with_dummy("dummy://host?checkout_timeout=0.5&#{options}") do |db| db.using_connection do |cnn| yield cnn.as(DummyDriver::DummyConnection) end diff --git a/spec/statement_spec.cr b/spec/statement_spec.cr index 0e410da..70c3385 100644 --- a/spec/statement_spec.cr +++ b/spec/statement_spec.cr @@ -10,7 +10,7 @@ describe DB::Statement do end it "should build unprepared statements" do - with_dummy_connection do |cnn| + with_dummy_connection("prepared_statements=false") do |cnn| prepared = cnn.unprepared("the query") prepared.should be_a(DB::Statement) prepared.as(DummyDriver::DummyStatement).prepared?.should be_false @@ -19,16 +19,14 @@ describe DB::Statement do describe "prepared_statements flag" do it "should build prepared statements if true" do - with_dummy_connection do |cnn| - cnn.prepared_statements = true + with_dummy_connection("prepared_statements=true") do |cnn| stmt = cnn.query("the query").statement stmt.as(DummyDriver::DummyStatement).prepared?.should be_true end end it "should build unprepared statements if false" do - with_dummy_connection do |cnn| - cnn.prepared_statements = false + with_dummy_connection("prepared_statements=false") do |cnn| stmt = cnn.query("the query").statement stmt.as(DummyDriver::DummyStatement).prepared?.should be_false end diff --git a/src/db/connection.cr b/src/db/connection.cr index 74ac257..803ebd8 100644 --- a/src/db/connection.cr +++ b/src/db/connection.cr @@ -25,7 +25,7 @@ module DB # :nodoc: getter database @statements_cache = StringKeyCache(Statement).new - property? prepared_statements : Bool + getter? prepared_statements : Bool def initialize(@database : Database) @prepared_statements = @database.prepared_statements? From da2831c17d627fa51ed300238ab92eb0923e318c Mon Sep 17 00:00:00 2001 From: "Brian J. Cardiff" Date: Mon, 5 Dec 2016 10:38:46 -0300 Subject: [PATCH 8/8] validate boolean option in connection string. proper downcase. --- spec/db_spec.cr | 4 ++++ src/db.cr | 11 ++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/spec/db_spec.cr b/spec/db_spec.cr index ec42f30..c619c9d 100644 --- a/spec/db_spec.cr +++ b/spec/db_spec.cr @@ -107,5 +107,9 @@ describe DB do DB.fetch_bool(HTTP::Params.parse("bar=true"), "foo", false).should be_false DB.fetch_bool(HTTP::Params.parse("bar=true"), "foo", true).should be_true + + expect_raises(ArgumentError, %(invalid "other" value for option "foo")) do + DB.fetch_bool(HTTP::Params.parse("foo=other"), "foo", true) + end end end diff --git a/src/db.cr b/src/db.cr index 43a90f4..74e3d98 100644 --- a/src/db.cr +++ b/src/db.cr @@ -123,10 +123,15 @@ module DB # :nodoc: def self.fetch_bool(params : HTTP::Params, name, default : Bool) - if value = params[name]? - value.underscore == "true" - else + case (value = params[name]?).try &.downcase + when nil default + when "true" + true + when "false" + false + else + raise ArgumentError.new(%(invalid "#{value}" value for option "#{name}")) end end end