Skip to content

Commit c1e8cb5

Browse files
committed
feat: Add error collection pattern to PendingUpdate
Implement error vector collection pattern in PendingUpdateTyped to allow builder methods to accumulate validation errors instead of failing fast. This enables users to see all validation issues at once rather than fixing them one by one. Changes: - Add protected error collection methods (AddError, CheckErrors, HasErrors, ClearErrors) to PendingUpdateTyped base class - Add std::vector<Error> errors_ member to collect validation errors - Update PendingUpdate documentation with usage examples - Add 6 comprehensive unit tests demonstrating error collection Pattern follows TableMetadataBuilder implementation. All tests pass (11/11).
1 parent dbc9c1c commit c1e8cb5

File tree

2 files changed

+179
-2
lines changed

2 files changed

+179
-2
lines changed

src/iceberg/pending_update.h

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@
2222
/// \file iceberg/pending_update.h
2323
/// API for table changes using builder pattern
2424

25+
#include <string>
26+
#include <vector>
27+
2528
#include "iceberg/iceberg_export.h"
2629
#include "iceberg/result.h"
2730
#include "iceberg/type_fwd.h"
@@ -72,6 +75,30 @@ class ICEBERG_EXPORT PendingUpdate {
7275
/// Apply() can be used to validate and inspect the uncommitted changes before
7376
/// committing. Commit() applies the changes and commits them to the table.
7477
///
78+
/// Error Collection Pattern:
79+
/// Builder methods in subclasses should use AddError() to collect validation
80+
/// errors instead of returning immediately. The accumulated errors will be
81+
/// returned by Apply() or Commit() when they are called. This allows users
82+
/// to see all validation errors at once rather than fixing them one by one.
83+
///
84+
/// Example usage in a subclass:
85+
/// \code
86+
/// MyUpdate& SetProperty(std::string_view key, std::string_view value) {
87+
/// if (key.empty()) {
88+
/// AddError(ErrorKind::kInvalidArgument, "Property key cannot be empty");
89+
/// return *this;
90+
/// }
91+
/// // ... continue with normal logic
92+
/// return *this;
93+
/// }
94+
///
95+
/// Result<T> Apply() override {
96+
/// // Check for accumulated errors first
97+
/// ICEBERG_RETURN_IF_ERROR(CheckErrors());
98+
/// // ... proceed with apply logic
99+
/// }
100+
/// \endcode
101+
///
75102
/// \tparam T The type of result returned by Apply()
76103
template <typename T>
77104
class ICEBERG_EXPORT PendingUpdateTyped : public PendingUpdate {
@@ -89,6 +116,51 @@ class ICEBERG_EXPORT PendingUpdateTyped : public PendingUpdate {
89116

90117
protected:
91118
PendingUpdateTyped() = default;
119+
120+
/// \brief Add a validation error to be returned later
121+
///
122+
/// Errors are accumulated and will be returned by Apply() or Commit().
123+
/// This allows builder methods to continue and collect all errors rather
124+
/// than failing fast on the first error.
125+
///
126+
/// \param kind The kind of error
127+
/// \param message The error message
128+
void AddError(ErrorKind kind, std::string message) {
129+
errors_.emplace_back(kind, std::move(message));
130+
}
131+
132+
/// \brief Check if any errors have been collected
133+
///
134+
/// \return true if there are accumulated errors
135+
bool HasErrors() const { return !errors_.empty(); }
136+
137+
/// \brief Check for accumulated errors and return them if any exist
138+
///
139+
/// This should be called at the beginning of Apply() or Commit() to
140+
/// return all accumulated validation errors.
141+
///
142+
/// \return Status::OK if no errors, or a ValidationFailed error with
143+
/// all accumulated error messages
144+
Status CheckErrors() const {
145+
if (!errors_.empty()) {
146+
std::string error_msg = "Validation failed due to the following errors:\n";
147+
for (const auto& [kind, message] : errors_) {
148+
error_msg += " - " + message + "\n";
149+
}
150+
return ValidationFailed("{}", error_msg);
151+
}
152+
return {};
153+
}
154+
155+
/// \brief Clear all accumulated errors
156+
///
157+
/// This can be useful for resetting the error state in tests or
158+
/// when reusing a builder instance.
159+
void ClearErrors() { errors_.clear(); }
160+
161+
private:
162+
// Error collection (since builder methods return *this and cannot throw)
163+
std::vector<Error> errors_;
92164
};
93165

94166
} // namespace iceberg

src/iceberg/test/pending_update_test.cc

Lines changed: 107 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,21 +27,61 @@
2727
namespace iceberg {
2828

2929
// Mock implementation for testing the interface
30-
class MockSnapshot {};
30+
class MockSnapshot {
31+
public:
32+
std::string name;
33+
};
3134

3235
class MockPendingUpdate : public PendingUpdateTyped<MockSnapshot> {
3336
public:
3437
MockPendingUpdate() = default;
3538

39+
// Builder-style methods that use error collection
40+
MockPendingUpdate& SetName(std::string_view name) {
41+
if (name.empty()) {
42+
AddError(ErrorKind::kInvalidArgument, "Name cannot be empty");
43+
return *this;
44+
}
45+
if (name.length() > 100) {
46+
AddError(ErrorKind::kInvalidArgument, "Name cannot exceed 100 characters");
47+
return *this;
48+
}
49+
name_ = name;
50+
return *this;
51+
}
52+
53+
MockPendingUpdate& SetId(int64_t id) {
54+
if (id < 0) {
55+
AddError(ErrorKind::kInvalidArgument, "ID must be non-negative");
56+
return *this;
57+
}
58+
id_ = id;
59+
return *this;
60+
}
61+
3662
Result<MockSnapshot> Apply() override {
63+
// Check for accumulated errors first
64+
auto error_status = CheckErrors();
65+
if (!error_status.has_value()) {
66+
return std::unexpected(error_status.error());
67+
}
68+
3769
if (should_fail_) {
3870
return ValidationFailed("Mock validation failed");
3971
}
4072
apply_called_ = true;
41-
return MockSnapshot{};
73+
MockSnapshot snapshot;
74+
snapshot.name = name_;
75+
return snapshot;
4276
}
4377

4478
Status Commit() override {
79+
// Check for accumulated errors first
80+
auto error_status = CheckErrors();
81+
if (!error_status.has_value()) {
82+
return error_status;
83+
}
84+
4585
if (should_fail_commit_) {
4686
return CommitFailed("Mock commit failed");
4787
}
@@ -55,6 +95,8 @@ class MockPendingUpdate : public PendingUpdateTyped<MockSnapshot> {
5595
bool CommitCalled() const { return commit_called_; }
5696

5797
private:
98+
std::string name_;
99+
int64_t id_ = 0;
58100
bool should_fail_ = false;
59101
bool should_fail_commit_ = false;
60102
bool apply_called_ = false;
@@ -96,4 +138,67 @@ TEST(PendingUpdateTest, BaseClassPolymorphism) {
96138
EXPECT_THAT(status, IsOk());
97139
}
98140

141+
// Error collection tests
142+
TEST(PendingUpdateTest, ErrorCollectionSingleError) {
143+
MockPendingUpdate update;
144+
update.SetName(""); // This should add an error
145+
146+
auto result = update.Apply();
147+
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
148+
EXPECT_THAT(result, HasErrorMessage("Name cannot be empty"));
149+
}
150+
151+
TEST(PendingUpdateTest, ErrorCollectionMultipleErrors) {
152+
MockPendingUpdate update;
153+
update.SetName(""); // Error: empty name
154+
update.SetId(-5); // Error: negative ID
155+
156+
auto result = update.Apply();
157+
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
158+
// Should contain both error messages
159+
EXPECT_THAT(result, HasErrorMessage("Name cannot be empty"));
160+
EXPECT_THAT(result, HasErrorMessage("ID must be non-negative"));
161+
}
162+
163+
TEST(PendingUpdateTest, ErrorCollectionInCommit) {
164+
MockPendingUpdate update;
165+
update.SetName(""); // This should add an error
166+
167+
auto status = update.Commit();
168+
EXPECT_THAT(status, IsError(ErrorKind::kValidationFailed));
169+
EXPECT_THAT(status, HasErrorMessage("Name cannot be empty"));
170+
}
171+
172+
TEST(PendingUpdateTest, ErrorCollectionValidInputNoErrors) {
173+
MockPendingUpdate update;
174+
update.SetName("valid_name");
175+
update.SetId(42);
176+
177+
auto result = update.Apply();
178+
EXPECT_THAT(result, IsOk());
179+
EXPECT_EQ(result->name, "valid_name");
180+
}
181+
182+
TEST(PendingUpdateTest, ErrorCollectionBuilderChaining) {
183+
MockPendingUpdate update;
184+
// Test that builder methods can be chained even when errors occur
185+
update.SetName("").SetId(-1);
186+
187+
auto result = update.Apply();
188+
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
189+
// Should contain both errors
190+
EXPECT_THAT(result, HasErrorMessage("Name cannot be empty"));
191+
EXPECT_THAT(result, HasErrorMessage("ID must be non-negative"));
192+
}
193+
194+
TEST(PendingUpdateTest, ErrorCollectionPartialValidation) {
195+
MockPendingUpdate update;
196+
update.SetName("valid_name"); // Valid
197+
update.SetId(-1); // Error
198+
199+
auto result = update.Apply();
200+
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
201+
EXPECT_THAT(result, HasErrorMessage("ID must be non-negative"));
202+
}
203+
99204
} // namespace iceberg

0 commit comments

Comments
 (0)