Skip to content

Commit ff5df59

Browse files
xlai20xlai20
committed
Fix data structure and add integration test
1 parent 3569d65 commit ff5df59

File tree

7 files changed

+261
-78
lines changed

7 files changed

+261
-78
lines changed

‎google/cloud/storage/internal/object_metadata_parser.cc‎

Lines changed: 35 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -52,19 +52,25 @@ void SetJsonContextsIfNotEmpty(nlohmann::json& json,
5252
if (!meta.has_contexts()) {
5353
return;
5454
}
55-
56-
nlohmann::json custom_json;
57-
for (auto const& kv : meta.contexts().custom) {
58-
custom_json[kv.first] = nlohmann::json{
59-
{"value", kv.second.value},
60-
{"createTime",
61-
google::cloud::internal::FormatRfc3339(kv.second.create_time)},
62-
{"updateTime",
63-
google::cloud::internal::FormatRfc3339(kv.second.update_time)},
64-
};
55+
if (meta.contexts().has_custom()) {
56+
nlohmann::json custom_json;
57+
for (auto const& kv : meta.contexts().custom()) {
58+
if (kv.second.has_value()) {
59+
nlohmann::json item;
60+
item["value"] = kv.second.value().value;
61+
item["createTime"] = google::cloud::internal::FormatRfc3339(
62+
kv.second.value().create_time);
63+
item["updateTime"] = google::cloud::internal::FormatRfc3339(
64+
kv.second.value().update_time);
65+
custom_json[kv.first] = std::move(item);
66+
} else {
67+
custom_json[kv.first] = nullptr;
68+
}
69+
}
70+
json["contexts"] = nlohmann::json{{"custom", std::move(custom_json)}};
71+
} else {
72+
json["contexts"] = nlohmann::json{{"custom", nullptr}};
6573
}
66-
67-
json["contexts"] = nlohmann::json{{"custom", std::move(custom_json)}};
6874
}
6975

7076
Status ParseAcl(ObjectMetadata& meta, nlohmann::json const& json) {
@@ -192,19 +198,26 @@ Status ParseContexts(ObjectMetadata& meta, nlohmann::json const& json) {
192198

193199
ObjectContexts contexts;
194200
for (auto const& kv : f_custom->items()) {
195-
ObjectCustomContextPayload payload;
196-
auto value = kv.value().value("value", "");
197-
payload.value = value;
201+
if (kv.value().is_null()) {
202+
contexts.upsert_custom_context(kv.key(), absl::nullopt);
203+
204+
} else {
205+
ObjectCustomContextPayload payload;
206+
auto value = kv.value().value("value", "");
207+
payload.value = value;
198208

199-
auto create_time = internal::ParseTimestampField(kv.value(), "createTime");
200-
if (!create_time) return std::move(create_time).status();
201-
payload.create_time = *create_time;
209+
auto create_time =
210+
internal::ParseTimestampField(kv.value(), "createTime");
211+
if (!create_time) return std::move(create_time).status();
212+
payload.create_time = *create_time;
202213

203-
auto update_time = internal::ParseTimestampField(kv.value(), "updateTime");
204-
if (!update_time) return std::move(update_time).status();
205-
payload.update_time = *update_time;
214+
auto update_time =
215+
internal::ParseTimestampField(kv.value(), "updateTime");
216+
if (!update_time) return std::move(update_time).status();
217+
payload.update_time = *update_time;
206218

207-
contexts.custom.emplace(kv.key(), std::move(payload));
219+
contexts.upsert_custom_context(kv.key(), std::move(payload));
220+
}
208221
}
209222
meta.set_contexts(std::move(contexts));
210223
return Status{};

‎google/cloud/storage/object_contexts.cc‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,19 @@ std::ostream& operator<<(std::ostream& os,
3232

3333
std::ostream& operator<<(std::ostream& os, ObjectContexts const& rhs) {
3434
os << "ObjectContexts={custom={";
35-
char const* sep = "";
36-
for (auto const& kv : rhs.custom) {
37-
os << sep << kv.first << "=" << kv.second;
38-
sep = ",\n";
35+
if (rhs.has_custom()) {
36+
char const* sep = "";
37+
for (auto const& kv : rhs.custom()) {
38+
os << sep << kv.first << "=";
39+
if (kv.second.has_value()) {
40+
os << kv.second.value();
41+
} else {
42+
os << "null";
43+
}
44+
sep = ",\n";
45+
}
46+
} else {
47+
os << "null";
3948
}
4049
return os << "}}";
4150
}

‎google/cloud/storage/object_contexts.h‎

Lines changed: 51 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_OBJECT_CONTEXTS_H
1717

1818
#include "google/cloud/storage/version.h"
19+
#include "absl/types/optional.h"
1920
#include <chrono>
2021
#include <map>
2122
#include <string>
@@ -54,36 +55,67 @@ std::ostream& operator<<(std::ostream& os,
5455
* Specifies the custom contexts of an object.
5556
*/
5657
struct ObjectContexts {
58+
public:
59+
// Returns true if the map itself exists.
60+
bool has_custom() const { return custom_map_.has_value(); }
61+
5762
/**
58-
* Represents the map of user-defined object contexts, keyed by a string
59-
* value.
63+
* Returns true if the map exists AND the key is present AND the value is
64+
* a valid value.
6065
*/
61-
std::map<std::string, ObjectCustomContextPayload> custom;
66+
bool has_custom(std::string const& key) const {
67+
if (!has_custom()) {
68+
return false;
69+
}
70+
return custom_map_->find(key) != custom_map_->end() &&
71+
custom_map_->at(key).has_value();
72+
}
6273

6374
/**
64-
* A set of helper functions to handle the custom.
75+
* The `custom` attribute of the object contexts.
76+
* Values are now absl::optional.
77+
*
78+
* It is undefined behavior to call this member function if
79+
* `has_custom() == false`.
6580
*/
66-
bool has_custom(std::string const& key) const {
67-
return custom.end() != custom.find(key);
81+
std::map<std::string, absl::optional<ObjectCustomContextPayload>> const&
82+
custom() const {
83+
return *custom_map_;
84+
}
85+
86+
/**
87+
* Upserts a context. Passing absl::nullopt for the value
88+
* represents a "null" entry in the map.
89+
*/
90+
void upsert_custom_context(std::string const& key,
91+
absl::optional<ObjectCustomContextPayload> value) {
92+
if (!has_custom()) {
93+
custom_map_.emplace();
94+
}
95+
96+
(*custom_map_)[key] = std::move(value);
6897
}
69-
ObjectCustomContextPayload const& get_custom(std::string const& key) const {
70-
return custom.at(key);
98+
99+
void reset_custom() { custom_map_.reset(); }
100+
101+
bool operator==(ObjectContexts const& other) const {
102+
return custom_map_ == other.custom_map_;
71103
}
72-
void upsert_custom(std::string const& key,
73-
ObjectCustomContextPayload const& value) {
74-
custom[key] = value;
104+
105+
bool operator!=(ObjectContexts const& other) const {
106+
return !(*this == other);
75107
}
76-
void delete_custom(std::string const& key) { custom.erase(key); }
77-
};
78108

79-
inline bool operator==(ObjectContexts const& lhs, ObjectContexts const& rhs) {
80-
return lhs.custom == rhs.custom;
109+
private:
110+
/**
111+
* Represents the map of user-defined object contexts.
112+
* Inner optional allows keys to point to a "null" value.
113+
*/
114+
absl::optional<
115+
std::map<std::string, absl::optional<ObjectCustomContextPayload>>>
116+
custom_map_;
81117
};
82118

83-
inline bool operator!=(ObjectContexts const& lhs, ObjectContexts const& rhs) {
84-
return !(lhs == rhs);
85-
}
86-
87119
std::ostream& operator<<(std::ostream& os, ObjectContexts const& rhs);
88120

89121
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END

‎google/cloud/storage/object_metadata.cc‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,11 +134,9 @@ std::ostream& operator<<(std::ostream& os, ObjectMetadata const& rhs) {
134134
if (rhs.has_hard_delete_time()) {
135135
os << ", hard_delete_time=" << FormatRfc3339(rhs.hard_delete_time());
136136
}
137-
138137
if (rhs.has_contexts()) {
139138
os << ", contexts=" << rhs.contexts();
140139
}
141-
142140
return os << "}";
143141
}
144142

‎google/cloud/storage/object_metadata.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -451,7 +451,7 @@ class ObjectMetadata {
451451
return *this;
452452
}
453453

454-
/// Returns `true` if the object has user-defined contexts.
454+
/// Returns `true` if the object has custom contexts.
455455
bool has_contexts() const { return contexts_.has_value(); }
456456

457457
/**
@@ -468,7 +468,7 @@ class ObjectMetadata {
468468
return *this;
469469
}
470470

471-
/// Reset the object's custom contexts.
471+
/// Reset the object contexts.
472472
ObjectMetadata& reset_contexts() {
473473
contexts_.reset();
474474
return *this;

‎google/cloud/storage/object_metadata_test.cc‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -216,9 +216,9 @@ TEST(ObjectMetadataTest, Parse) {
216216
EXPECT_EQ(actual.hard_delete_time(),
217217
std::chrono::system_clock::from_time_t(1710160496L) +
218218
std::chrono::milliseconds(789));
219-
ASSERT_TRUE(actual.has_contexts());
219+
ASSERT_TRUE(actual.has_contexts() && actual.contexts().has_custom());
220220
EXPECT_EQ(
221-
actual.contexts().custom.at("environment"),
221+
actual.contexts().custom().at("environment"),
222222
(ObjectCustomContextPayload{
223223
"prod",
224224
google::cloud::internal::ParseRfc3339("2024-07-18T00:00:00Z").value(),
@@ -692,19 +692,19 @@ TEST(ObjectMetadataTest, ResetRetention) {
692692
TEST(ObjectMetadataTest, SetContexts) {
693693
auto const expected = CreateObjectMetadataForTest();
694694
auto copy = expected;
695-
auto const context_payload = ObjectCustomContextPayload{"engineering", {}, {}};
696-
std::map<std::string, ObjectCustomContextPayload> custom{
697-
{"department", context_payload}};
698-
auto const contexts = ObjectContexts{custom};
695+
auto const context_payload =
696+
ObjectCustomContextPayload{"engineering", {}, {}};
697+
ObjectContexts contexts;
698+
contexts.upsert_custom_context("department", context_payload);
699699
copy.set_contexts(contexts);
700-
EXPECT_TRUE(expected.has_contexts());
701-
EXPECT_TRUE(copy.has_contexts());
700+
EXPECT_TRUE(expected.contexts().has_custom());
701+
EXPECT_TRUE(copy.contexts().has_custom());
702702
EXPECT_EQ(contexts, copy.contexts());
703703
EXPECT_NE(expected, copy);
704704
}
705705

706706
/// @test Verify we can reset the `contexts` field.
707-
TEST(ObjectMetadataTest, ResetContexts) {
707+
TEST(ObjectMetadataTest, DeleteContexts) {
708708
auto const expected = CreateObjectMetadataForTest();
709709
auto copy = expected;
710710
copy.reset_contexts();

0 commit comments

Comments
 (0)