insert query builder insert step improvements

include/matador/query/insert_query_builder.hpp

@@ -7,11 +7,11 @@
 
 #include "matador/query/basic_schema.hpp"
 #include "matador/query/intermediates/executable_query.hpp"
+#include "matador/query/insert_step.hpp"
 #include "matador/query/query.hpp"
 #include "matador/query/query_contexts.hpp"
 #include "matador/query/query_builder_exception.hpp"
 
-#include "matador/sql/execute_result.hpp"
 #include "matador/sql/statement.hpp"
 
 #include "matador/utils/primary_key_accessor.hpp"
@@ -56,157 +56,6 @@ private:
   std::string join_column_;
 };
 
-struct insert_step {
-  sql::query_context ctx;
-
-  utils::result<void, utils::error> acquire_and_bind(sql::statement &stmt) const;
-
-  // Session uses these to handle manual/sequence/table pre-insert PKs
-  utils::generator_type pk_generator{utils::generator_type::Manual};
-  utils::primary_key_accessor pk_accessor;
-
-  // Identity post-insert
-  std::function<void(const sql::record &)> apply_returning{};
-  std::function<void(const utils::identifier &)> apply_primary_key{};
-  std::function<void(sql::statement &)> bind_object{};
-  std::function<void()> make_object_persistent{};
-};
-
-class insert_step2 {
-public:
-  explicit insert_step2(sql::query_context ctx)
-  : ctx_(std::move(ctx)) {}
-  virtual ~insert_step2() = default;
-
-  virtual utils::result<void, utils::error> prepare(sql::executor &conn) const = 0;
-  virtual utils::result<void, utils::error> insert(sql::statement &stmt) const = 0;
-
-protected:
-  utils::primary_key_accessor pk_accessor_;
-  sql::query_context ctx_;
-};
-
-template <typename ObjectType>
-class insert_step_pk_generated : public insert_step2 {
-public:
-  insert_step_pk_generated(sql::query_context ctx, const object::object_ptr<ObjectType>& ptr, abstract_pk_generator& pk_generator)
-  : insert_step2(std::move(ctx))
-  , ptr_(ptr)
-  , pk_generator_(pk_generator){}
-
-  utils::result<void, utils::error> prepare(sql::executor& conn) const override {
-    auto result = pk_generator_.next_id(conn);
-    if (!result.is_ok()) {
-      return utils::failure(result.err());
-    }
-    pk_accessor_.set(*ptr_, *result);
-
-    return utils::ok<void>();
-  }
-
-  utils::result<void, utils::error> insert(sql::statement& stmt) const override {
-    stmt.bind(*ptr_);
-
-    if (const auto exec_result = stmt.execute(); !exec_result.is_ok()) {
-      return utils::failure(exec_result.err());
-    }
-
-    ptr_.change_state(object::object_state::Persistent);
-    return utils::ok<void>();
-  }
-
-private:
-  object::object_ptr<ObjectType> ptr_;
-  abstract_pk_generator& pk_generator_;
-};
-
-template <typename ObjectType>
-class insert_step_pk_identity : public insert_step2 {
-public:
-  insert_step_pk_identity(sql::query_context ctx, const object::object_ptr<ObjectType>& ptr, const std::string &pk_column_name)
-  : insert_step2(std::move(ctx))
-  , ptr_(ptr)
-  , pk_column_name_(pk_column_name){}
-
-  utils::result<void, utils::error> prepare(sql::executor&) const override {
-    return utils::ok<void>();
-  }
-
-  utils::result<void, utils::error> insert(sql::statement& stmt) const override {
-    stmt.bind(*ptr_);
-
-    auto result = stmt.fetch_one();
-    if (!result.is_ok()) {
-      return utils::failure(result.err());
-    }
-    if (!result.value().has_value()) {
-      return utils::failure(utils::error(error_code::FailedToFindObject, "Failed to insert object and retrieve identity."));
-    }
-
-    auto rec = result->value();
-    const auto& f = rec.at(pk_column_name_);
-    utils::identifier id;
-    if (auto res = id.assign(f.value()); !res.is_ok()) {
-      return utils::failure(res.err());
-    }
-    pk_accessor_.set(*ptr_, id);
-
-    ptr_.change_state(object::object_state::Persistent);
-    return utils::ok<void>();
-  }
-
-private:
-  object::object_ptr<ObjectType> ptr_;
-  std::string pk_column_name_;
-};
-
-template <typename ObjectType>
-class insert_step_pk_manual : public insert_step2 {
-public:
-  insert_step_pk_manual(sql::query_context ctx, const object::object_ptr<ObjectType>& ptr)
-  : insert_step2(std::move(ctx))
-  , ptr_(ptr) {}
-
-  utils::result<void, utils::error> prepare(sql::executor &) const override {
-    return utils::ok<void>();
-  }
-  utils::result<void, utils::error> insert(sql::statement &stmt) const override {
-    stmt.bind(*ptr_);
-    if (const auto exec_result = stmt.execute(); !exec_result.is_ok()) {
-      return utils::failure(exec_result.err());
-    }
-
-    ptr_.change_state(object::object_state::Persistent);
-    return utils::ok<void>();
-  }
-
-private:
-  object::object_ptr<ObjectType> ptr_;
-};
-
-template <typename ObjectType>
-class insert_step_relation : public insert_step2 {
-public:
-  insert_step_relation(sql::query_context ctx, const object::object_ptr<ObjectType>& ptr)
-  : insert_step2(std::move(ctx))
-  , ptr_(ptr) {}
-
-  utils::result<void, utils::error> prepare(sql::executor &) const override {
-    return utils::ok<void>();
-  }
-  utils::result<void, utils::error> insert(sql::statement &stmt) const override {
-    stmt.bind(*ptr_);
-    if (const auto exec_result = stmt.execute(); !exec_result.is_ok()) {
-      return utils::failure(exec_result.err());
-    }
-
-    return utils::ok<void>();
-  }
-
-private:
-  object::object_ptr<ObjectType> ptr_;
-};
-
 template<class ObjectType>
 class insert_query_builder {
 public:
@@ -215,7 +64,7 @@ public:
   , contexts_by_type_{contexts_by_type}
   {}
 
-  utils::result<std::vector<insert_step>, utils::error> build(const object::object_ptr<ObjectType> &ptr) {
+  utils::result<std::vector<std::unique_ptr<insert_step>>, utils::error> build(const object::object_ptr<ObjectType> &ptr) {
     if (const auto it = schema_.find(typeid(ObjectType)); it == schema_.end()) {
       return utils::failure(utils::error{error_code::UnknownType, "Unknown type for insert query"});
     }
@@ -236,7 +85,7 @@ public:
     }
     relation_steps_.clear();
 
-    return utils::ok(steps_);
+    return utils::ok(std::move(steps_));
   }
 
   template < class PrimaryKeyType >
@@ -303,7 +152,7 @@ public:
     }
 
     std::ignore = processing_many_to_many_relations_.insert(id);
-    std::vector<insert_step> rel_steps;
+    std::vector<std::unique_ptr<insert_step>> insert_relation_steps;
     for (auto &obj : objects) {
       if (!obj) {
         continue;
@@ -318,15 +167,9 @@ public:
 
       access::process(*this, *rel);
 
-      insert_step step{};
+      insert_relation_steps.push_back(std::make_unique<insert_step_relation<relation_value_type>>(cit->second.insert, rel));
-      step.pk_generator = utils::generator_type::None;
-      step.ctx = cit->second.insert;
-      step.bind_object = [rel](sql::statement &stmt) { stmt.bind(*rel); };
-      step.make_object_persistent = [] {};
-
-      rel_steps.push_back(std::move(step));
     }
-    relation_steps_.insert(relation_steps_.end(), rel_steps.begin(), rel_steps.end());
+    relation_steps_.insert(relation_steps_.end(), std::make_move_iterator(insert_relation_steps.begin()), std::make_move_iterator(insert_relation_steps.end()));
     processing_many_to_many_relations_.erase(id);
   }
 
@@ -360,7 +203,7 @@ public:
     }
 
     std::ignore = processing_many_to_many_relations_.insert(id);
-    std::vector<insert_step> rel_steps;
+    std::vector<std::unique_ptr<insert_step>> insert_relation_steps;
     for (auto &obj : objects) {
       if (!obj) {
         continue;
@@ -375,15 +218,9 @@ public:
 
       access::process(*this, *rel);
 
-      insert_step step{};
+      insert_relation_steps.push_back(std::make_unique<insert_step_relation<relation_value_type>>(cit->second.insert, rel));
-      step.pk_generator = utils::generator_type::None;
-      step.ctx = cit->second.insert;
-      step.bind_object = [rel](sql::statement &stmt) { stmt.bind(*rel); };
-      step.make_object_persistent = [] {};
-
-      rel_steps.push_back(std::move(step));
     }
-    relation_steps_.insert(relation_steps_.end(), rel_steps.begin(), rel_steps.end());
+    relation_steps_.insert(relation_steps_.end(), std::make_move_iterator(insert_relation_steps.begin()), std::make_move_iterator(insert_relation_steps.end()));
     processing_many_to_many_relations_.erase(id);
   }
 
@@ -403,7 +240,7 @@ private:
   };
 
   template<class EntityType>
-  utils::result<void, utils::error> build_for(const object::object_ptr<EntityType> &ptr, std::vector<insert_step> &steps) {
+  utils::result<void, utils::error> build_for(const object::object_ptr<EntityType> &ptr, std::vector<std::unique_ptr<insert_step>> &steps) {
     if (!ptr) {
       return utils::failure(utils::error{error_code::InvalidObject, "Object is null"});
     }
@@ -428,7 +265,6 @@ private:
 
     // 2) Build INSERT for this object
     const auto &info = it->second.node().info();
-    insert_step step{};
     if (!info.has_primary_key() || it->second.pk_generator().type() == utils::generator_type::None) {
       return utils::failure(utils::error{error_code::MissingPrimaryKey, "Type " + info.name() + " has no primary key"});
     }
@@ -438,34 +274,12 @@ private:
     }
 
     if (it->second.pk_generator().type() == utils::generator_type::Manual) {
-      steps2_.push_back(std::make_unique<insert_step_pk_manual<EntityType>>(cit->second.insert, ptr));
+      steps.push_back(std::make_unique<insert_step_pk_manual<EntityType>>(cit->second.insert, ptr));
     } else if (it->second.pk_generator().type() == utils::generator_type::Identity) {
-      steps2_.push_back(std::make_unique<insert_step_pk_identity<EntityType>>(cit->second.insert, ptr, info.primary_key_attribute()->name()));
+      steps.push_back(std::make_unique<insert_step_pk_identity<EntityType>>(cit->second.insert, ptr, info.primary_key_attribute()->name()));
     } else {
-      steps2_.push_back(std::make_unique<insert_step_pk_generated<EntityType>>(cit->second.insert, ptr));
+      steps.push_back(std::make_unique<insert_step_pk_generated<EntityType>>(cit->second.insert, ptr, it->second.pk_generator()));
     }
-    step.pk_generator = it->second.pk_generator().type();
-
-    step.ctx = cit->second.insert;
-    step.bind_object = [ptr](sql::statement &stmt) {
-      stmt.bind(*ptr);
-    };
-    if (info.has_primary_key() && step.pk_generator == utils::generator_type::Identity) {
-      const auto pk_name = info.primary_key_attribute()->name();
-      const table_column pk_col(&it->second.table(), pk_name);
-      step.apply_returning = [ptr, &step, pk_name = pk_name](const sql::record &rec) {
-        const auto& f = rec.at(pk_name);
-        utils::identifier id;
-        id.assign(f.value());
-        step.pk_accessor.set(*ptr, id);
-      };
-    } else if (info.has_primary_key() && (step.pk_generator == utils::generator_type::Sequence || step.pk_generator == utils::generator_type::Table)) {
-      step.apply_primary_key = [ptr, &step](const utils::identifier &id) {
-        step.pk_accessor.set(*ptr, id);
-      };
-    }
-    step.make_object_persistent = [ptr] { ptr.change_state(object::object_state::Persistent); };
-    steps.push_back(std::move(step));
 
     return utils::ok<void>();
   }
@@ -490,9 +304,8 @@ private:
 
   object::object_ptr<ObjectType> ptr_;
 
-  std::vector<std::unique_ptr<insert_step2>> steps2_;
+  std::vector<std::unique_ptr<insert_step>> steps_;
-  std::vector<insert_step> steps_;
+  std::vector<std::unique_ptr<insert_step>> relation_steps_;
-  std::vector<insert_step> relation_steps_;
   std::unordered_set<std::pair<std::type_index, const void *>, visit_key_hash> visited_;
   std::unordered_set<std::string> processing_many_to_many_relations_;
 };

include/matador/query/insert_step.hpp

@@ -0,0 +1,157 @@
+#ifndef MATADOR_INSERT_STEP_HPP
+#define MATADOR_INSERT_STEP_HPP
+
+#include <utility>
+
+#include "matador/utils/primary_key_accessor.hpp"
+#include "matador/utils/error.hpp"
+#include "matador/utils/result.hpp"
+
+#include "matador/object/object_ptr.hpp"
+
+#include "matador/sql/query_context.hpp"
+#include "matador/sql/executor.hpp"
+#include "matador/sql/execute_result.hpp"
+#include "matador/sql/statement.hpp"
+
+#include "matador/query/error_code.hpp"
+#include "matador/query/abstract_pk_generator.hpp"
+
+namespace matador::query {
+class insert_step {
+public:
+  explicit insert_step(sql::query_context ctx)
+  : ctx_(std::move(ctx)) {}
+  virtual ~insert_step() = default;
+
+  virtual utils::result<void, utils::error> prepare(sql::executor &conn) = 0;
+  virtual utils::result<void, utils::error> insert(sql::statement &stmt) = 0;
+
+  [[nodiscard]] const sql::query_context& ctx() const { return ctx_; }
+protected:
+  utils::primary_key_accessor pk_accessor_;
+  sql::query_context ctx_;
+};
+
+template <typename ObjectType>
+class insert_step_pk_generated : public insert_step {
+public:
+  insert_step_pk_generated(sql::query_context ctx, const object::object_ptr<ObjectType>& ptr, abstract_pk_generator& pk_generator)
+  : insert_step(std::move(ctx))
+  , ptr_(ptr)
+  , pk_generator_(pk_generator){}
+
+  utils::result<void, utils::error> prepare(sql::executor& conn) override {
+    auto result = pk_generator_.next_id(conn);
+    if (!result.is_ok()) {
+      return utils::failure(result.err());
+    }
+    pk_accessor_.set(*ptr_, utils::identifier{*result});
+
+    return utils::ok<void>();
+  }
+
+  utils::result<void, utils::error> insert(sql::statement& stmt) override {
+    stmt.bind(*ptr_);
+
+    if (const auto exec_result = stmt.execute(); !exec_result.is_ok()) {
+      return utils::failure(exec_result.err());
+    }
+
+    ptr_.change_state(object::object_state::Persistent);
+    return utils::ok<void>();
+  }
+
+private:
+  object::object_ptr<ObjectType> ptr_;
+  abstract_pk_generator& pk_generator_;
+};
+
+template <typename ObjectType>
+class insert_step_pk_identity : public insert_step {
+public:
+  insert_step_pk_identity(sql::query_context ctx, const object::object_ptr<ObjectType>& ptr, std::string pk_column_name)
+  : insert_step(std::move(ctx))
+  , ptr_(ptr)
+  , pk_column_name_(std::move(pk_column_name)){}
+
+  utils::result<void, utils::error> prepare(sql::executor&) override {
+    return utils::ok<void>();
+  }
+
+  utils::result<void, utils::error> insert(sql::statement& stmt) override {
+    stmt.bind(*ptr_);
+
+    auto result = stmt.fetch_one();
+    if (!result.is_ok()) {
+      return utils::failure(result.err());
+    }
+    if (!result.value().has_value()) {
+      return utils::failure(utils::error(error_code::FailedToFindObject, "Failed to insert object and retrieve identity."));
+    }
+
+    auto rec = result->value();
+    const auto& f = rec.at(pk_column_name_);
+    utils::identifier id;
+    if (auto res = id.assign(f.value()); !res.is_ok()) {
+      return utils::failure(res.err());
+    }
+    pk_accessor_.set(*ptr_, id);
+
+    ptr_.change_state(object::object_state::Persistent);
+    return utils::ok<void>();
+  }
+
+private:
+  object::object_ptr<ObjectType> ptr_;
+  std::string pk_column_name_;
+};
+
+template <typename ObjectType>
+class insert_step_pk_manual : public insert_step {
+public:
+  insert_step_pk_manual(sql::query_context ctx, const object::object_ptr<ObjectType>& ptr)
+  : insert_step(std::move(ctx))
+  , ptr_(ptr) {}
+
+  utils::result<void, utils::error> prepare(sql::executor &) override {
+    return utils::ok<void>();
+  }
+  utils::result<void, utils::error> insert(sql::statement &stmt) override {
+    stmt.bind(*ptr_);
+    if (const auto exec_result = stmt.execute(); !exec_result.is_ok()) {
+      return utils::failure(exec_result.err());
+    }
+
+    ptr_.change_state(object::object_state::Persistent);
+    return utils::ok<void>();
+  }
+
+private:
+  object::object_ptr<ObjectType> ptr_;
+};
+
+template <typename ObjectType>
+class insert_step_relation : public insert_step {
+public:
+  insert_step_relation(sql::query_context ctx, const object::object_ptr<ObjectType>& ptr)
+  : insert_step(std::move(ctx))
+  , ptr_(ptr) {}
+
+  utils::result<void, utils::error> prepare(sql::executor &) override {
+    return utils::ok<void>();
+  }
+  utils::result<void, utils::error> insert(sql::statement &stmt) override {
+    stmt.bind(*ptr_);
+    if (const auto exec_result = stmt.execute(); !exec_result.is_ok()) {
+      return utils::failure(exec_result.err());
+    }
+
+    return utils::ok<void>();
+  }
+
+private:
+  object::object_ptr<ObjectType> ptr_;
+};
+}
+#endif //MATADOR_INSERT_STEP_HPP

include/matador/query/session.hpp

@@ -81,8 +81,7 @@ utils::result<object::object_ptr<Type>, utils::error> session::insert(object::ob
     return utils::ok(obj);
   }
 
-  const auto it = schema_.find(typeid(Type));
+  if (const auto it = schema_.find(typeid(Type)); it == schema_.end()) {
-  if (it == schema_.end()) {
     return utils::failure(make_error(error_code::UnknownType, "Failed to determine requested type."));
   }
 
@@ -94,42 +93,25 @@ utils::result<object::object_ptr<Type>, utils::error> session::insert(object::ob
   }
 
   // Execute all steps; for Identity steps read RETURNING and write pk back into the object
-  for (insert_step &step : *steps) {
+  for (auto &step : *steps) {
-    if (step.pk_generator == utils::generator_type::Sequence || step.pk_generator == utils::generator_type::Table) {
+    const auto conn = pool_.acquire();
-      const auto conn = pool_.acquire();
+    if (!conn.valid()) {
-      if (!conn.valid()) {
+      return utils::failure(make_error(error_code::FailedToAcquirePool, "Failed to acquire connection pool for primary key generation."));
-        return utils::failure(make_error(error_code::FailedToAcquirePool, "Failed to acquire connection pool for primary key generation."));
-      }
-      auto result = it->second.pk_generator().next_id(*conn);
-      if (!result.is_ok()) {
-        return utils::failure(result.err());
-      }
-      step.apply_primary_key(utils::identifier{*result});
     }
 
-    auto stmt = cache_.acquire(step.ctx);
+    if (const auto result = step->prepare(*conn); !result.is_ok()) {
+      return utils::failure(result.err());
+    }
+    conn.release();
+
+    auto stmt = cache_.acquire(step->ctx());
     if (!stmt.is_ok()) {
       return utils::failure(stmt.err());
     }
 
-    if (auto result = step.acquire_and_bind(*stmt); !result.is_ok()) {
+    if (const auto result = step->insert(*stmt); !result.is_ok()) {
       return utils::failure(result.err());
     }
-
-    if (step.pk_generator == utils::generator_type::Identity) {
-      // insert and read RETURNING
-      auto record = stmt->fetch_one();
-      if (!record.is_ok()) {
-        return utils::failure(record.err());
-      }
-      if (!record.value().has_value()) {
-        return utils::failure(make_error(error_code::FailedToFindObject, "Failed to insert object and retrieve identity."));
-      }
-      step.apply_returning(*record.value());
-    } else if (const auto exec_result = stmt->execute(); !exec_result.is_ok()) {
-      return utils::failure(exec_result.err());
-    }
-    step.make_object_persistent();
   }
 
   obj.change_state(object::object_state::Persistent);

include/matador/sql/connection_pool.hpp

@@ -36,6 +36,8 @@ public:
   [[nodiscard]] std::optional<size_t> id() const;
   [[nodiscard]] bool valid() const;
 
+  void release() const;
+
 private:
   friend class connection_pool;
 

source/orm/CMakeLists.txt

@@ -1,5 +1,4 @@
 add_library(matador-orm STATIC
-  ../../include/matador/query/session.hpp
   ../../include/matador/query/abstract_pk_generator.hpp
   ../../include/matador/query/attribute_string_writer.hpp
   ../../include/matador/query/basic_schema.hpp
@@ -29,8 +28,10 @@ add_library(matador-orm STATIC
   ../../include/matador/query/expression_evaluator.hpp
   ../../include/matador/query/fk_value_extractor.hpp
   ../../include/matador/query/generator.hpp
+  ../../include/matador/query/identity_pk_generator.hpp
   ../../include/matador/query/insert_query_builder.hpp
   ../../include/matador/query/insert_query_builder.hpp
+  ../../include/matador/query/insert_step.hpp
   ../../include/matador/query/intermediates/executable_query.hpp
   ../../include/matador/query/intermediates/fetchable_query.hpp
   ../../include/matador/query/intermediates/query_alter_intermediate.hpp
@@ -81,6 +82,7 @@ add_library(matador-orm STATIC
   ../../include/matador/query/schema.hpp
   ../../include/matador/query/select_query_builder.hpp
   ../../include/matador/query/sequence_pk_generator.hpp
+  ../../include/matador/query/session.hpp
   ../../include/matador/query/table.hpp
   ../../include/matador/query/table_column.hpp
   ../../include/matador/query/table_constraint.hpp
@@ -119,7 +121,6 @@ add_library(matador-orm STATIC
   ../../include/matador/sql/sql_functions.hpp
   ../../include/matador/sql/statement.hpp
   ../../include/matador/sql/statement_cache.hpp
-  query/session.cpp
   query/abstract_pk_generator.cpp
   query/attribute_string_writer.cpp
   query/basic_schema.cpp
@@ -143,6 +144,7 @@ add_library(matador-orm STATIC
   query/expression/value_expression.cpp
   query/expression_evaluator.cpp
   query/generator.cpp
+  query/identity_pk_generator.cpp
   query/insert_query_builder.cpp
   query/intermediates/executable_query.cpp
   query/intermediates/fetchable_query.cpp
@@ -188,6 +190,7 @@ add_library(matador-orm STATIC
   query/schema.cpp
   query/select_query_builder.cpp
   query/sequence_pk_generator.cpp
+  query/session.cpp
   query/table.cpp
   query/table_column.cpp
   query/table_constraint.cpp
@@ -218,8 +221,6 @@ add_library(matador-orm STATIC
   sql/resolver_service.cpp
   sql/statement.cpp
   sql/statement_cache.cpp
-  ../../include/matador/query/identity_pk_generator.hpp
-  query/identity_pk_generator.cpp
 )
 
 target_include_directories(matador-orm

source/orm/query/insert_query_builder.cpp

@@ -3,9 +3,9 @@
 #include "matador/sql/statement_cache.hpp"
 
 namespace matador::query {
-utils::result<void, utils::error> insert_step::acquire_and_bind(sql::statement &stmt) const {
+// utils::result<void, utils::error> insert_step::acquire_and_bind(sql::statement &stmt) const {
-  bind_object(stmt);
+  // bind_object(stmt);
 
-  return utils::ok<void>();
+  // return utils::ok<void>();
-}
+// }
 }  // namespace matador::query

source/orm/sql/connection_pool.cpp

@@ -9,7 +9,7 @@ identifiable_connection::identifiable_connection(const size_t id, connection con
 , conn(std::move(conn)){}
 
 connection_ptr::~connection_ptr() {
-  pool_->release(connection_);
+  release();
 }
 
 connection_ptr::connection_ptr(identifiable_connection* c, connection_pool* pool)
@@ -53,6 +53,10 @@ bool connection_ptr::valid() const {
   return connection_ != nullptr;
 }
 
+void connection_ptr::release() const {
+  pool_->release(connection_);
+}
+
 connection_pool::connection_pool(const std::string& dns, const size_t count)
 : connection_pool(dns, count, [](const connection_info &info) { return connection{info}; })
 {}