ApiBundle: Use a creator method instead of constructor

This will allow for ApiBundle to be used together with the TestApi
implementation of IExecutionApi in tests.

Also rename CreateRemote method to MakeRemote in order to remove
any semantical confusion.

9 files changed, 95 insertions, 66 deletions

diff --git a/src/buildtool/execution_api/common/api_bundle.cpp b/src/buildtool/execution_api/common/api_bundle.cpp
index 15e84440..8808e5d8 100644
--- a/src/buildtool/execution_api/common/api_bundle.cpp
+++ b/src/buildtool/execution_api/common/api_bundle.cpp
@@ -18,16 +18,33 @@
 #include "src/buildtool/execution_api/local/local_api.hpp"
 #include "src/buildtool/execution_api/remote/bazel/bazel_api.hpp"
 
-ApiBundle::ApiBundle(gsl::not_null<LocalContext const*> const& local_context,
-                     gsl::not_null<RemoteContext const*> const& remote_context,
-                     RepositoryConfig const* repo_config)
-    : hash_function{local_context->storage_config->hash_function},
-      local{std::make_shared<LocalApi>(local_context, repo_config)},
-      remote{CreateRemote(remote_context->exec_config->remote_address,
-                          remote_context->auth,
-                          remote_context->retry_config)} {}
+/// \note Some logic from MakeRemote is duplicated here as that method cannot
+/// be used without the hash_function field being set prior to the call.
+auto ApiBundle::Create(
+    gsl::not_null<LocalContext const*> const& local_context,
+    gsl::not_null<RemoteContext const*> const& remote_context,
+    RepositoryConfig const* repo_config) -> ApiBundle {
+    auto const hash_fct = local_context->storage_config->hash_function;
+    IExecutionApi::Ptr local_api =
+        std::make_shared<LocalApi>(local_context, repo_config);
+    IExecutionApi::Ptr remote_api = local_api;
+    if (auto const address = remote_context->exec_config->remote_address) {
+        ExecutionConfiguration config;
+        config.skip_cache_lookup = false;
+        remote_api = std::make_shared<BazelApi>("remote-execution",
+                                                address->host,
+                                                address->port,
+                                                remote_context->auth,
+                                                remote_context->retry_config,
+                                                config,
+                                                hash_fct);
+    }
+    return ApiBundle{.hash_function = hash_fct,
+                     .local = std::move(local_api),
+                     .remote = std::move(remote_api)};
+}
 
-auto ApiBundle::CreateRemote(
+auto ApiBundle::MakeRemote(
     std::optional<ServerAddress> const& address,
     gsl::not_null<Auth const*> const& authentication,
     gsl::not_null<RetryConfig const*> const& retry_config) const
diff --git a/src/buildtool/execution_api/common/api_bundle.hpp b/src/buildtool/execution_api/common/api_bundle.hpp
index 17c64d1e..28595010 100644
--- a/src/buildtool/execution_api/common/api_bundle.hpp
+++ b/src/buildtool/execution_api/common/api_bundle.hpp
@@ -29,10 +29,13 @@
 /// same time. If the remote api cannot be instantiated, it falls back to
 /// exactly the same instance that local api is (&*remote == & *local).
 struct ApiBundle final {
-    explicit ApiBundle(
+    /// \brief Create an ApiBundle instance.
+    /// \note A creator method is used instead of a constructor to allow for
+    /// tests to instantiate ApiBundles with own implementations of the APIs.
+    [[nodiscard]] static auto Create(
         gsl::not_null<LocalContext const*> const& local_context,
         gsl::not_null<RemoteContext const*> const& remote_context,
-        RepositoryConfig const* repo_config);
+        RepositoryConfig const* repo_config) -> ApiBundle;
 
     /// \brief Create a Remote object based on the given arguments.
     /// \param address          The endpoint address.
@@ -40,13 +43,12 @@ struct ApiBundle final {
     /// \param retry_config     The retry strategy configuration.
     /// \returns A configured api: BazelApi if a remote address is given,
     /// otherwise fall back to the already configured LocalApi instance.
-    [[nodiscard]] auto CreateRemote(
+    [[nodiscard]] auto MakeRemote(
         std::optional<ServerAddress> const& address,
         gsl::not_null<Auth const*> const& authentication,
         gsl::not_null<RetryConfig const*> const& retry_config) const
         -> gsl::not_null<IExecutionApi::Ptr>;
 
-    // Needed to be set before creating the remote (via CreateRemote)
     HashFunction const hash_function;
     // 7 bytes of alignment.
 
diff --git a/src/buildtool/graph_traverser/graph_traverser.hpp b/src/buildtool/graph_traverser/graph_traverser.hpp
index a63f7d8c..301ef722 100644
--- a/src/buildtool/graph_traverser/graph_traverser.hpp
+++ b/src/buildtool/graph_traverser/graph_traverser.hpp
@@ -385,9 +385,9 @@ class GraphTraverser {
         std::vector<ArtifactIdentifier> const& artifact_ids) const -> bool {
         // setup rebuilder with api for cache endpoint
         auto api_cached =
-            apis_.CreateRemote(remote_context_.exec_config->cache_address,
-                               remote_context_.auth,
-                               remote_context_.retry_config);
+            apis_.MakeRemote(remote_context_.exec_config->cache_address,
+                             remote_context_.auth,
+                             remote_context_.retry_config);
         Rebuilder executor{repo_config_,
                            &*apis_.local,
                            &*apis_.remote,
diff --git a/src/buildtool/main/main.cpp b/src/buildtool/main/main.cpp
index 18caa871..e97b5a27 100644
--- a/src/buildtool/main/main.cpp
+++ b/src/buildtool/main/main.cpp
@@ -822,9 +822,10 @@ auto main(int argc, char* argv[]) -> int {
                     .retry_config = &retry_config,
                     .exec_config = &remote_exec_config};
 
-                ApiBundle const exec_apis{&local_context,
-                                          &remote_context,
-                                          /*repo_config=*/nullptr};
+                auto const exec_apis =
+                    ApiBundle::Create(&local_context,
+                                      &remote_context,
+                                      /*repo_config=*/nullptr);
 
                 return execution_server->Run(&local_context,
                                              &remote_context,
@@ -888,9 +889,10 @@ auto main(int argc, char* argv[]) -> int {
                     .retry_config = &*retry_config,
                     .exec_config = &*remote_exec_config};
 
-                ApiBundle const serve_apis{&local_context,
-                                           &remote_context,
-                                           /*repo_config=*/nullptr};
+                auto const serve_apis =
+                    ApiBundle::Create(&local_context,
+                                      &remote_context,
+                                      /*repo_config=*/nullptr);
                 auto serve = ServeApi::Create(*serve_config,
                                               &local_context,
                                               &remote_context,
@@ -988,8 +990,8 @@ auto main(int argc, char* argv[]) -> int {
                                            .retry_config = &*retry_config,
                                            .exec_config = &*remote_exec_config};
 
-        ApiBundle const main_apis{
-            &local_context, &remote_context, &repo_config};
+        auto const main_apis =
+            ApiBundle::Create(&local_context, &remote_context, &repo_config);
         GraphTraverser const traverser{
             {jobs,
              std::move(arguments.build),
diff --git a/src/buildtool/serve_api/serve_service/target.cpp b/src/buildtool/serve_api/serve_service/target.cpp
index ec8ff458..9ad1520b 100644
--- a/src/buildtool/serve_api/serve_service/target.cpp
+++ b/src/buildtool/serve_api/serve_service/target.cpp
@@ -501,8 +501,8 @@ auto TargetService::ServeTarget(
 
         // Use a new ApiBundle that knows about local repository config and
         // dispatch endpoint for traversing
-        ApiBundle const local_apis{
-            &local_context_, &dispatch_context, &repository_config};
+        auto const local_apis = ApiBundle::Create(
+            &local_context_, &dispatch_context, &repository_config);
 
         GraphTraverser const traverser{
             std::move(traverser_args),
diff --git a/src/other_tools/just_mr/fetch.cpp b/src/other_tools/just_mr/fetch.cpp
index 088c9ace..d236b7e8 100644
--- a/src/other_tools/just_mr/fetch.cpp
+++ b/src/other_tools/just_mr/fetch.cpp
@@ -440,9 +440,9 @@ auto MultiRepoFetch(std::shared_ptr<Configuration> const& config,
                                        .exec_config = &*remote_exec_config};
 
     // setup the APIs for archive fetches; only happens if in native mode
-    ApiBundle const apis{&local_context,
-                         &remote_context,
-                         /*repo_config=*/nullptr};
+    auto const apis = ApiBundle::Create(&local_context,
+                                        &remote_context,
+                                        /*repo_config=*/nullptr);
 
     bool const has_remote_api =
         apis.local != apis.remote and not common_args.compatible;
diff --git a/src/other_tools/just_mr/setup.cpp b/src/other_tools/just_mr/setup.cpp
index 9f9e09bd..ba5e39b4 100644
--- a/src/other_tools/just_mr/setup.cpp
+++ b/src/other_tools/just_mr/setup.cpp
@@ -161,9 +161,9 @@ auto MultiRepoSetup(std::shared_ptr<Configuration> const& config,
                                        .retry_config = &*retry_config,
                                        .exec_config = &*remote_exec_config};
 
-    ApiBundle const apis{&local_context,
-                         &remote_context,
-                         /*repo_config=*/nullptr};
+    auto const apis = ApiBundle::Create(&local_context,
+                                        &remote_context,
+                                        /*repo_config=*/nullptr);
 
     bool const has_remote_api =
         apis.local != apis.remote and not common_args.compatible;
diff --git a/test/buildtool/build_engine/target_map/target_map.test.cpp b/test/buildtool/build_engine/target_map/target_map.test.cpp
index ed859e31..ddcecdd0 100644
--- a/test/buildtool/build_engine/target_map/target_map.test.cpp
+++ b/test/buildtool/build_engine/target_map/target_map.test.cpp
@@ -126,9 +126,9 @@ TEST_CASE("simple targets", "[target_map]") {
                                        .retry_config = &retry_config,
                                        .exec_config = &remote_exec_config};
 
-    ApiBundle const apis{&local_context,
-                         &remote_context,
-                         /*repo_config=*/nullptr};
+    auto const apis = ApiBundle::Create(&local_context,
+                                        &remote_context,
+                                        /*repo_config=*/nullptr);
     auto serve =
         ServeApi::Create(*serve_config, &local_context, &remote_context, &apis);
     AnalyseContext ctx{.repo_config = &repo_config,
@@ -589,9 +589,9 @@ TEST_CASE("configuration deduplication", "[target_map]") {
                                        .retry_config = &retry_config,
                                        .exec_config = &remote_exec_config};
 
-    ApiBundle const apis{&local_context,
-                         &remote_context,
-                         /*repo_config=*/nullptr};
+    auto const apis = ApiBundle::Create(&local_context,
+                                        &remote_context,
+                                        /*repo_config=*/nullptr);
 
     auto serve =
         ServeApi::Create(*serve_config, &local_context, &remote_context, &apis);
@@ -699,9 +699,9 @@ TEST_CASE("generator functions in string arguments", "[target_map]") {
                                        .retry_config = &retry_config,
                                        .exec_config = &remote_exec_config};
 
-    ApiBundle const apis{&local_context,
-                         &remote_context,
-                         /*repo_config=*/nullptr};
+    auto const apis = ApiBundle::Create(&local_context,
+                                        &remote_context,
+                                        /*repo_config=*/nullptr);
 
     auto serve =
         ServeApi::Create(*serve_config, &local_context, &remote_context, &apis);
@@ -821,9 +821,9 @@ TEST_CASE("built-in rules", "[target_map]") {
                                        .retry_config = &retry_config,
                                        .exec_config = &remote_exec_config};
 
-    ApiBundle const apis{&local_context,
-                         &remote_context,
-                         /*repo_config=*/nullptr};
+    auto const apis = ApiBundle::Create(&local_context,
+                                        &remote_context,
+                                        /*repo_config=*/nullptr);
 
     auto serve =
         ServeApi::Create(*serve_config, &local_context, &remote_context, &apis);
@@ -1055,9 +1055,9 @@ TEST_CASE("target reference", "[target_map]") {
                                        .retry_config = &retry_config,
                                        .exec_config = &remote_exec_config};
 
-    ApiBundle const apis{&local_context,
-                         &remote_context,
-                         /*repo_config=*/nullptr};
+    auto const apis = ApiBundle::Create(&local_context,
+                                        &remote_context,
+                                        /*repo_config=*/nullptr);
 
     auto serve =
         ServeApi::Create(*serve_config, &local_context, &remote_context, &apis);
@@ -1220,9 +1220,9 @@ TEST_CASE("trees", "[target_map]") {
                                        .retry_config = &retry_config,
                                        .exec_config = &remote_exec_config};
 
-    ApiBundle const apis{&local_context,
-                         &remote_context,
-                         /*repo_config=*/nullptr};
+    auto const apis = ApiBundle::Create(&local_context,
+                                        &remote_context,
+                                        /*repo_config=*/nullptr);
 
     auto serve =
         ServeApi::Create(*serve_config, &local_context, &remote_context, &apis);
@@ -1349,9 +1349,9 @@ TEST_CASE("RESULT error reporting", "[target_map]") {
                                        .retry_config = &retry_config,
                                        .exec_config = &remote_exec_config};
 
-    ApiBundle const apis{&local_context,
-                         &remote_context,
-                         /*repo_config=*/nullptr};
+    auto const apis = ApiBundle::Create(&local_context,
+                                        &remote_context,
+                                        /*repo_config=*/nullptr);
 
     auto serve =
         ServeApi::Create(*serve_config, &local_context, &remote_context, &apis);
@@ -1537,9 +1537,9 @@ TEST_CASE("wrong arguments", "[target_map]") {
                                        .retry_config = &retry_config,
                                        .exec_config = &remote_exec_config};
 
-    ApiBundle const apis{&local_context,
-                         &remote_context,
-                         /*repo_config=*/nullptr};
+    auto const apis = ApiBundle::Create(&local_context,
+                                        &remote_context,
+                                        /*repo_config=*/nullptr);
 
     auto serve =
         ServeApi::Create(*serve_config, &local_context, &remote_context, &apis);
diff --git a/test/buildtool/graph_traverser/graph_traverser.test.hpp b/test/buildtool/graph_traverser/graph_traverser.test.hpp
index 314d7bd6..6618e5bf 100644
--- a/test/buildtool/graph_traverser/graph_traverser.test.hpp
+++ b/test/buildtool/graph_traverser/graph_traverser.test.hpp
@@ -186,7 +186,8 @@ class TestProject {
                                        .retry_config = &retry_config,
                                        .exec_config = remote_config};
 
-    ApiBundle const apis{&local_context, &remote_context, p.GetRepoConfig()};
+    auto const apis =
+        ApiBundle::Create(&local_context, &remote_context, p.GetRepoConfig());
 
     GraphTraverser const gt{clargs.gtargs,
                             p.GetRepoConfig(),
@@ -264,7 +265,8 @@ class TestProject {
                                        .retry_config = &retry_config,
                                        .exec_config = remote_config};
 
-    ApiBundle const apis{&local_context, &remote_context, p.GetRepoConfig()};
+    auto const apis =
+        ApiBundle::Create(&local_context, &remote_context, p.GetRepoConfig());
 
     GraphTraverser const gt{clargs.gtargs,
                             p.GetRepoConfig(),
@@ -312,7 +314,8 @@ class TestProject {
                                        .retry_config = &retry_config,
                                        .exec_config = remote_config};
 
-    ApiBundle const apis{&local_context, &remote_context, p.GetRepoConfig()};
+    auto const apis =
+        ApiBundle::Create(&local_context, &remote_context, p.GetRepoConfig());
 
     GraphTraverser const gt{clargs.gtargs,
                             p.GetRepoConfig(),
@@ -379,8 +382,8 @@ class TestProject {
                                        .retry_config = &retry_config,
                                        .exec_config = remote_config};
 
-    ApiBundle const apis{
-        &local_context, &remote_context, full_hello_world.GetRepoConfig()};
+    auto const apis = ApiBundle::Create(
+        &local_context, &remote_context, full_hello_world.GetRepoConfig());
 
     GraphTraverser const gt_upload{clargs_update_cpp.gtargs,
                                    full_hello_world.GetRepoConfig(),
@@ -453,7 +456,8 @@ static void TestBlobsUploadedAndUsed(
                                        .retry_config = &retry_config,
                                        .exec_config = remote_config};
 
-    ApiBundle const apis{&local_context, &remote_context, p.GetRepoConfig()};
+    auto const apis =
+        ApiBundle::Create(&local_context, &remote_context, p.GetRepoConfig());
 
     GraphTraverser gt{clargs.gtargs,
                       p.GetRepoConfig(),
@@ -509,7 +513,8 @@ static void TestEnvironmentVariablesSetAndUsed(
                                        .retry_config = &retry_config,
                                        .exec_config = remote_config};
 
-    ApiBundle const apis{&local_context, &remote_context, p.GetRepoConfig()};
+    auto const apis =
+        ApiBundle::Create(&local_context, &remote_context, p.GetRepoConfig());
 
     GraphTraverser gt{clargs.gtargs,
                       p.GetRepoConfig(),
@@ -565,7 +570,8 @@ static void TestTreesUsed(
                                        .retry_config = &retry_config,
                                        .exec_config = remote_config};
 
-    ApiBundle const apis{&local_context, &remote_context, p.GetRepoConfig()};
+    auto const apis =
+        ApiBundle::Create(&local_context, &remote_context, p.GetRepoConfig());
 
     GraphTraverser gt{clargs.gtargs,
                       p.GetRepoConfig(),
@@ -621,7 +627,8 @@ static void TestNestedTreesUsed(
                                        .retry_config = &retry_config,
                                        .exec_config = remote_config};
 
-    ApiBundle const apis{&local_context, &remote_context, p.GetRepoConfig()};
+    auto const apis =
+        ApiBundle::Create(&local_context, &remote_context, p.GetRepoConfig());
 
     GraphTraverser gt{clargs.gtargs,
                       p.GetRepoConfig(),
@@ -676,7 +683,8 @@ static void TestFlakyHelloWorldDetected(
                                        .retry_config = &retry_config,
                                        .exec_config = remote_config};
 
-    ApiBundle const apis{&local_context, &remote_context, p.GetRepoConfig()};
+    auto const apis =
+        ApiBundle::Create(&local_context, &remote_context, p.GetRepoConfig());
 
     {
         auto clargs = p.CmdLineArgs("_entry_points_ctimes");