googleapis / google-cloud-cpp

C++ Client Libraries for Google Cloud Services
https://cloud.google.com/
Apache License 2.0
554 stars 374 forks source link

error: call to deleted constructor of 'spanner::Client' #13380

Closed ericel closed 10 months ago

ericel commented 10 months ago

I think of this as a bug because one will have to keep doing database connections more than once.

Spanner

Describe the bug : Spanner default client constructor is deleted. This means there is no way of having a spanner client as a class variable.

class Client {
 public:
  /**
   * Constructs a `Client` object using the specified @p conn and @p opts.
   *
   * See `MakeConnection()` for how to create a connection to Spanner. To help
   * with unit testing, callers may create fake/mock `Connection` objects that
   * are injected into the `Client`.
   */
  explicit Client(std::shared_ptr<Connection> conn, Options opts = {})
      : conn_(std::move(conn)),
        opts_(internal::MergeOptions(std::move(opts), conn_->options())) {}

  /// No default construction.
  Client() = delete;

  ///@{
  /// @name Copy and move support
  Client(Client const&) = default;
  Client& operator=(Client const&) = default;
  Client(Client&&) = default;
  Client& operator=(Client&&) = default;

To Reproduce Steps to reproduce the behavior:

I have a class call Spanner in my project, in my .h file I have something like this:

class Spanner {
public:
    // Constructor
    Spanner();

    void insert();

private:
    google::cloud::spanner::Client client_;
    std::string instance_id;
    std::string project_id;
    std::string database_id;
};

#endif

and the implementation file .cc is

#include "Spanner.h"

Spanner::Spanner() {
    auto &app = drogon::app();
    auto customConfig = app.getCustomConfig();

    project_id = customConfig.get("gcloud_project_id", "default-project").asString();
    instance_id = customConfig.get("gcloud_spanner_instance_id", "default-instance").asString();
    database_id = customConfig.get("gcloud_spanner_database_id", "default-database").asString();

     // Initialize the Spanner client with the necessary connection
    client_(google::cloud::spanner::MakeConnection(google::cloud::spanner::Database(project_id, instance_id, database_id)));
}...

But that has an error of:

[ 2%] Building CXX object CMakeFiles/wahalao.dir/utils/Spanner.cc.o /Applications/dev/cplusplus/wahalao/utils/Spanner.cc:3:10: error: call to deleted constructor of 'spanner::Client' Spanner::Spanner() { ^ /Applications/dev/cplusplus/wahalao/build/vcpkg_installed/x64-osx/include/google/cloud/spanner/client.h:138:3: note: 'Client' has been explicitly marked deleted here Client() = delete; ^

devbww commented 10 months ago

Hi @ericel,

one will have to keep doing database connections more than once.

I don't think that is true, but please allow to expand on that by directly addressing your example.

First off, I don't believe your "Initialize the Spanner client" snippet compiles. But I'll assume you meant to do an assignment to client_ within Spanner::Spanner(), rather than the misplaced member initialization.

there is no way of having a spanner client as a class variable.

I also don't think that is true, as long as you don't require the (deleted) default constructor. Here are three possible ways to do that. The choice between them probably depends on how you plan to use class Spanner.

  1. Don't hold the spanner::Client by value

Consider ...

class Spanner {
 public:
  Spanner();

 private:
  std::unique_ptr<google::cloud::spanner::Client> client_;
  std::string instance_id;
  std::string project_id;
  std::string database_id;
};

Spanner::Spanner() {
  auto& app = drogon::app();
  auto customConfig = app.getCustomConfig();

  project_id =
      customConfig.get("gcloud_project_id", "default-project").asString();
  instance_id =
      customConfig.get("gcloud_spanner_instance_id", "default-instance")
          .asString();
  database_id =
      customConfig.get("gcloud_spanner_database_id", "default-database")
          .asString();

  // Initialize the Spanner client with the necessary connection
  client_ = std::make_unique<google::cloud::spanner::Client>(
      google::cloud::spanner::MakeConnection(google::cloud::spanner::Database(
          project_id, instance_id, database_id)));
}

Here we use nullptr to represent the "unconnected client" at the beginning of the constructor body. If you want Spanner itself to be copyable, you could use std::shared_ptr instead.

  1. Initialize the spanner::Client during class initialization using a helper function

We don't need the spanner::Client default constructor if we set client_ only once. For example, ...

class Spanner {
 public:
  Spanner() : client_(MakeConnection()) {}

 private:
  static std::shared_ptr<google::cloud::spanner::Connection> MakeConnection();

  google::cloud::spanner::Client client_;
};

std::shared_ptr<google::cloud::spanner::Connection> Spanner::MakeConnection() {
  auto customConfig = drogon::app().getCustomConfig();
  return google::cloud::spanner::MakeConnection(
      google::cloud::spanner::Database(
          customConfig.get("gcloud_project_id", "default-project").asString(),
          customConfig.get("gcloud_spanner_instance_id", "default-instance")
              .asString(),
          customConfig.get("gcloud_spanner_database_id", "default-database")
              .asString()));
}
  1. Initialize the spanner::Client during class initialization by giving Spanner a construction argument

We also don't need the default constructor if we simply move all the connection making outside the Spanner class. For example, ...

class Spanner {
 public:
  Spanner(std::shared_ptr<google::cloud::spanner::Connection> conn)
      : client_(std::move(conn)) {}

 private:
  google::cloud::spanner::Client client_;
};

Then you would construct Spanner values directly from a connection.

  auto conn = google::cloud::spanner::MakeConnection(google::cloud::spanner::Database(...));
  Spanner s(conn);

And you could also then construct additional Spanner values without having "to keep doing database connections".

  Spanner s2(conn);
  Spanner s3(conn);

Anyway, I hope this helps to demonstrate that there are ways to avoid needing an "unconnected" google::cloud::spanner::Client state, and therefore avoid needing a default constructor.

ericel commented 10 months ago

Thank you @devbww I missed that!!