Skip to content

Commit 7bfdfc1

Browse files
DatabeanDatabean
committed
Refactor DownloadFileWithBackup to a freestanding function
This makes the BuildApi interface smaller, reducing the burden on interface implementations. The extracted logic is also now shared between all implementations. - Remove DownloadFileWithBackup from BuildApi interface. - Implement it as a freestanding function accepting BuildApi& in build_api.cpp. - Remove implementations from AndroidBuildApi and CachingBuildApi. - Add unit tests for the new freestanding function. Assisted-by: Jetski:Gemini Next
1 parent ab19418 commit 7bfdfc1

8 files changed

Lines changed: 158 additions & 49 deletions

File tree

‎base/cvd/cuttlefish/host/libs/web/BUILD.bazel‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,19 @@ cf_cc_test(
9898
],
9999
)
100100

101+
cf_cc_test(
102+
name = "build_api_test",
103+
srcs = ["build_api_test.cpp"],
104+
deps = [
105+
"//cuttlefish/host/libs/web:android_build",
106+
"//cuttlefish/host/libs/web:android_build_string",
107+
"//cuttlefish/host/libs/web:build_api",
108+
"//cuttlefish/host/libs/zip/libzip_cc:seekable_source",
109+
"//cuttlefish/result",
110+
"//cuttlefish/result:result_matchers",
111+
],
112+
)
113+
101114
cf_cc_library(
102115
name = "android_build_url",
103116
srcs = ["android_build_url.cpp"],
@@ -112,6 +125,7 @@ cf_cc_library(
112125

113126
cf_cc_library(
114127
name = "build_api",
128+
srcs = ["build_api.cpp"],
115129
hdrs = ["build_api.h"],
116130
deps = [
117131
"//cuttlefish/host/libs/web:android_build",

‎base/cvd/cuttlefish/host/libs/web/android_build_api.cpp‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -148,17 +148,6 @@ Result<std::string> AndroidBuildApi::DownloadFile(
148148
return DownloadTargetFile(build, target_directory, artifact_name);
149149
}
150150

151-
Result<std::string> AndroidBuildApi::DownloadFileWithBackup(
152-
const Build& build, const std::string& target_directory,
153-
const std::string& artifact_name, const std::string& backup_artifact_name) {
154-
std::unordered_set<std::string> artifacts =
155-
CF_EXPECT(Artifacts(build, {artifact_name, backup_artifact_name}));
156-
std::string selected_artifact = artifact_name;
157-
if (!Contains(artifacts, artifact_name)) {
158-
selected_artifact = backup_artifact_name;
159-
}
160-
return DownloadTargetFile(build, target_directory, selected_artifact);
161-
}
162151

163152
Result<SeekableZipSource> AndroidBuildApi::FileReader(
164153
const Build& build, const std::string& artifact_name) {

‎base/cvd/cuttlefish/host/libs/web/android_build_api.h‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,6 @@ class AndroidBuildApi : public BuildApi {
5252
const std::string& target_directory,
5353
const std::string& artifact_name) override;
5454

55-
Result<std::string> DownloadFileWithBackup(
56-
const Build& build, const std::string& target_directory,
57-
const std::string& artifact_name,
58-
const std::string& backup_artifact_name) override;
5955

6056
Result<SeekableZipSource> FileReader(
6157
const Build&, const std::string& artifact_name) override;

‎base/cvd/cuttlefish/host/libs/web/build_api.cpp‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
//
2+
// Copyright (C) 2026 The Android Open Source Project
3+
//
4+
// Licensed under the Apache License, Version 2.0 (the "License");
5+
// you may not use this file except in compliance with the License.
6+
// You may obtain a copy of the License at
7+
//
8+
// http://www.apache.org/licenses/LICENSE-2.0
9+
//
10+
// Unless required by applicable law or agreed to in writing, software
11+
// distributed under the License is distributed on an "AS IS" BASIS,
12+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
// See the License for the specific language governing permissions and
14+
// limitations under the License.
15+
16+
#include "cuttlefish/host/libs/web/build_api.h"
17+
18+
#include <string>
19+
20+
#include "cuttlefish/host/libs/web/android_build.h"
21+
#include "cuttlefish/result/result.h"
22+
23+
namespace cuttlefish {
24+
25+
Result<std::string> DownloadFileWithBackup(
26+
BuildApi& build_api, const Build& build,
27+
const std::string& target_directory, const std::string& artifact_name,
28+
const std::string& backup_artifact_name) {
29+
Result<std::string> result =
30+
build_api.DownloadFile(build, target_directory, artifact_name);
31+
if (result.ok()) {
32+
return result;
33+
}
34+
return build_api.DownloadFile(build, target_directory, backup_artifact_name);
35+
}
36+
37+
} // namespace cuttlefish

‎base/cvd/cuttlefish/host/libs/web/build_api.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,13 @@ class BuildApi {
3333
const Build& build, const std::string& target_directory,
3434
const std::string& artifact_name) = 0;
3535

36-
virtual Result<std::string> DownloadFileWithBackup(
37-
const Build& build, const std::string& target_directory,
38-
const std::string& artifact_name,
39-
const std::string& backup_artifact_name) = 0;
40-
4136
virtual Result<SeekableZipSource> FileReader(
4237
const Build&, const std::string& artifact_name) = 0;
4338
};
4439

40+
Result<std::string> DownloadFileWithBackup(
41+
BuildApi& build_api, const Build& build,
42+
const std::string& target_directory, const std::string& artifact_name,
43+
const std::string& backup_artifact_name);
44+
4545
} // namespace cuttlefish

‎base/cvd/cuttlefish/host/libs/web/build_api_test.cpp‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
//
2+
// Copyright (C) 2026 The Android Open Source Project
3+
//
4+
// Licensed under the Apache License, Version 2.0 (the "License");
5+
// you may not use this file except in compliance with the License.
6+
// You may obtain a copy of the License at
7+
//
8+
// http://www.apache.org/licenses/LICENSE-2.0
9+
//
10+
// Unless required by applicable law or agreed to in writing, software
11+
// distributed under the License is distributed on an "AS IS" BASIS,
12+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
// See the License for the specific language governing permissions and
14+
// limitations under the License.
15+
16+
#include "cuttlefish/host/libs/web/build_api.h"
17+
18+
#include <string>
19+
20+
#include "gmock/gmock.h"
21+
#include "gtest/gtest.h"
22+
23+
#include "cuttlefish/host/libs/web/android_build.h"
24+
#include "cuttlefish/host/libs/web/android_build_string.h"
25+
#include "cuttlefish/host/libs/zip/libzip_cc/seekable_source.h"
26+
#include "cuttlefish/result/result.h"
27+
#include "cuttlefish/result/result_matchers.h"
28+
29+
namespace cuttlefish {
30+
namespace {
31+
32+
using ::testing::_;
33+
using ::testing::Return;
34+
35+
class MockBuildApi : public BuildApi {
36+
public:
37+
MOCK_METHOD(Result<Build>, GetBuild, (const BuildString&), (override));
38+
MOCK_METHOD(Result<std::string>, DownloadFile,
39+
(const Build&, const std::string&, const std::string&),
40+
(override));
41+
MOCK_METHOD(Result<SeekableZipSource>, FileReader,
42+
(const Build&, const std::string&), (override));
43+
};
44+
45+
TEST(BuildApiTest, DownloadFileWithBackupSuccessFirst) {
46+
MockBuildApi mock_api;
47+
Build build = DeviceBuild{.id = "123", .target = "test"};
48+
std::string target_dir = "/tmp";
49+
std::string artifact_name = "primary.zip";
50+
std::string backup_artifact_name = "backup.zip";
51+
std::string expected_path = "/tmp/primary.zip";
52+
53+
EXPECT_CALL(mock_api, DownloadFile(_, target_dir, artifact_name))
54+
.WillOnce(Return(expected_path));
55+
EXPECT_CALL(mock_api, DownloadFile(_, target_dir, backup_artifact_name))
56+
.Times(0);
57+
58+
Result<std::string> result = DownloadFileWithBackup(
59+
mock_api, build, target_dir, artifact_name, backup_artifact_name);
60+
61+
ASSERT_THAT(result, IsOkAndValue(expected_path));
62+
}
63+
64+
TEST(BuildApiTest, DownloadFileWithBackupFallback) {
65+
MockBuildApi mock_api;
66+
Build build = DeviceBuild{.id = "123", .target = "test"};
67+
std::string target_dir = "/tmp";
68+
std::string artifact_name = "primary.zip";
69+
std::string backup_artifact_name = "backup.zip";
70+
std::string expected_path = "/tmp/backup.zip";
71+
72+
EXPECT_CALL(mock_api, DownloadFile(_, target_dir, artifact_name))
73+
.WillOnce(Return(CF_ERR("File not found")));
74+
EXPECT_CALL(mock_api, DownloadFile(_, target_dir, backup_artifact_name))
75+
.WillOnce(Return(expected_path));
76+
77+
Result<std::string> result = DownloadFileWithBackup(
78+
mock_api, build, target_dir, artifact_name, backup_artifact_name);
79+
80+
ASSERT_THAT(result, IsOkAndValue(expected_path));
81+
}
82+
83+
TEST(BuildApiTest, DownloadFileWithBackupBothFail) {
84+
MockBuildApi mock_api;
85+
Build build = DeviceBuild{.id = "123", .target = "test"};
86+
std::string target_dir = "/tmp";
87+
std::string artifact_name = "primary.zip";
88+
std::string backup_artifact_name = "backup.zip";
89+
90+
EXPECT_CALL(mock_api, DownloadFile(_, target_dir, artifact_name))
91+
.WillOnce(Return(CF_ERR("Primary file not found")));
92+
EXPECT_CALL(mock_api, DownloadFile(_, target_dir, backup_artifact_name))
93+
.WillOnce(Return(CF_ERR("Backup file not found")));
94+
95+
Result<std::string> result = DownloadFileWithBackup(
96+
mock_api, build, target_dir, artifact_name, backup_artifact_name);
97+
98+
ASSERT_THAT(result, IsError());
99+
}
100+
101+
} // namespace
102+
} // namespace cuttlefish

‎base/cvd/cuttlefish/host/libs/web/caching_build_api.cpp‎

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
#include <fmt/core.h>
2323
#include <fmt/format.h>
2424
#include "absl/log/log.h"
25-
#include "absl/strings/match.h"
2625

2726
#include "cuttlefish/common/libs/utils/files.h"
2827
#include "cuttlefish/host/libs/web/android_build.h"
@@ -119,30 +118,6 @@ Result<std::string> CachingBuildApi::DownloadFile(
119118
kOverwriteExistingFile));
120119
}
121120

122-
Result<std::string> CachingBuildApi::DownloadFileWithBackup(
123-
const Build& build, const std::string& target_directory,
124-
const std::string& artifact_name, const std::string& backup_artifact_name) {
125-
const auto paths =
126-
CF_EXPECT(ConstructCachePaths(cache_base_path_, build, target_directory,
127-
artifact_name, backup_artifact_name));
128-
if (IsInCache(paths.cache_artifact)) {
129-
return CF_EXPECT(CreateHardLink(paths.cache_artifact, paths.target_artifact,
130-
kOverwriteExistingFile));
131-
}
132-
if (IsInCache(paths.cache_backup_artifact)) {
133-
return CF_EXPECT(CreateHardLink(paths.cache_backup_artifact,
134-
paths.target_backup_artifact,
135-
kOverwriteExistingFile));
136-
}
137-
const auto artifact_filepath = CF_EXPECT(build_api_.DownloadFileWithBackup(
138-
build, paths.build_cache, artifact_name, backup_artifact_name));
139-
if (absl::EndsWith(artifact_filepath, artifact_name)) {
140-
return CF_EXPECT(CreateHardLink(paths.cache_artifact, paths.target_artifact,
141-
kOverwriteExistingFile));
142-
}
143-
return CF_EXPECT(CreateHardLink(paths.cache_backup_artifact,
144-
paths.target_backup_artifact));
145-
}
146121

147122
Result<SeekableZipSource> CachingBuildApi::FileReader(
148123
const Build& build, const std::string& artifact) {

‎base/cvd/cuttlefish/host/libs/web/caching_build_api.h‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,6 @@ class CachingBuildApi : public BuildApi {
3535
Result<std::string> DownloadFile(const Build& build,
3636
const std::string& target_directory,
3737
const std::string& artifact_name) override;
38-
Result<std::string> DownloadFileWithBackup(
39-
const Build& build, const std::string& target_directory,
40-
const std::string& artifact_name,
41-
const std::string& backup_artifact_name) override;
4238

4339
Result<SeekableZipSource> FileReader(const Build&,
4440
const std::string& artifact) override;

0 commit comments

Comments
 (0)