Skip to content

Commit 37045cb

Browse files
shangxinliwgtmac
authored andcommitted
feat: extract ErrorCollector utility and add AddError overload
Address review comments from wgtmac on PR #358: 1. Add AddError(Error err) overload to accept existing Error objects This allows propagating errors from other components without deconstructing and reconstructing them. 2. Extract error collection into reusable ErrorCollector utility class This eliminates code duplication between PendingUpdateTyped and TableMetadataBuilder, and provides a reusable pattern for other builders in the codebase. Changes: - Add src/iceberg/util/error_collector.h with ErrorCollector class - Update PendingUpdateTyped to use ErrorCollector internally - Update TableMetadataBuilder to use ErrorCollector instead of std::vector<Error> - Add test for AddError(Error err) overload - Add comprehensive documentation with usage examples All 12 tests pass.
1 parent 9e25214 commit 37045cb

File tree

3 files changed

+144
-26
lines changed

3 files changed

+144
-26
lines changed

src/iceberg/pending_update.h

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

25-
#include <string>
26-
#include <vector>
27-
2825
#include "iceberg/iceberg_export.h"
2926
#include "iceberg/result.h"
3027
#include "iceberg/type_fwd.h"
28+
#include "iceberg/util/error_collector.h"
3129

3230
namespace iceberg {
3331

@@ -126,13 +124,20 @@ class ICEBERG_EXPORT PendingUpdateTyped : public PendingUpdate {
126124
/// \param kind The kind of error
127125
/// \param message The error message
128126
void AddError(ErrorKind kind, std::string message) {
129-
errors_.emplace_back(kind, std::move(message));
127+
error_collector_.AddError(kind, std::move(message));
130128
}
131129

130+
/// \brief Add an existing error object to be returned later
131+
///
132+
/// Useful when propagating errors from other components.
133+
///
134+
/// \param err The error to add
135+
void AddError(Error err) { error_collector_.AddError(std::move(err)); }
136+
132137
/// \brief Check if any errors have been collected
133138
///
134139
/// \return true if there are accumulated errors
135-
bool HasErrors() const { return !errors_.empty(); }
140+
bool HasErrors() const { return error_collector_.HasErrors(); }
136141

137142
/// \brief Check for accumulated errors and return them if any exist
138143
///
@@ -141,26 +146,17 @@ class ICEBERG_EXPORT PendingUpdateTyped : public PendingUpdate {
141146
///
142147
/// \return Status::OK if no errors, or a ValidationFailed error with
143148
/// 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-
}
149+
Status CheckErrors() const { return error_collector_.CheckErrors(); }
154150

155151
/// \brief Clear all accumulated errors
156152
///
157153
/// This can be useful for resetting the error state in tests or
158154
/// when reusing a builder instance.
159-
void ClearErrors() { errors_.clear(); }
155+
void ClearErrors() { error_collector_.ClearErrors(); }
160156

161157
private:
162158
// Error collection (since builder methods return *this and cannot throw)
163-
std::vector<Error> errors_;
159+
ErrorCollector error_collector_;
164160
};
165161

166162
} // namespace iceberg

src/iceberg/table_metadata.cc

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
#include "iceberg/snapshot.h"
3737
#include "iceberg/sort_order.h"
3838
#include "iceberg/table_update.h"
39+
#include "iceberg/util/error_collector.h"
3940
#include "iceberg/util/gzip_internal.h"
4041
#include "iceberg/util/macros.h"
4142
#include "iceberg/util/uuid.h"
@@ -276,7 +277,7 @@ struct TableMetadataBuilder::Impl {
276277
std::vector<std::unique_ptr<TableUpdate>> changes;
277278

278279
// Error collection (since methods return *this and cannot throw)
279-
std::vector<Error> errors;
280+
ErrorCollector error_collector;
280281

281282
// Metadata location tracking
282283
std::optional<std::string> metadata_location;
@@ -348,7 +349,8 @@ TableMetadataBuilder& TableMetadataBuilder::AssignUUID(std::string_view uuid) {
348349

349350
// Validation: UUID cannot be empty
350351
if (uuid_str.empty()) {
351-
impl_->errors.emplace_back(ErrorKind::kInvalidArgument, "Cannot assign empty UUID");
352+
impl_->error_collector.AddError(ErrorKind::kInvalidArgument,
353+
"Cannot assign empty UUID");
352354
return *this;
353355
}
354356

@@ -499,13 +501,7 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveEncryptionKey(std::string_view
499501

500502
Result<std::unique_ptr<TableMetadata>> TableMetadataBuilder::Build() {
501503
// 1. Check for accumulated errors
502-
if (!impl_->errors.empty()) {
503-
std::string error_msg = "Failed to build TableMetadata due to validation errors:\n";
504-
for (const auto& [kind, message] : impl_->errors) {
505-
error_msg += " - " + message + "\n";
506-
}
507-
return CommitFailed("{}", error_msg);
508-
}
504+
ICEBERG_RETURN_UNEXPECTED(impl_->error_collector.CheckErrors());
509505

510506
// 2. Validate metadata consistency through TableMetadata#Validate
511507

src/iceberg/util/error_collector.h

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
#pragma once
21+
22+
/// \file iceberg/util/error_collector.h
23+
/// Utility for collecting validation errors in builder patterns
24+
25+
#include <string>
26+
#include <vector>
27+
28+
#include "iceberg/result.h"
29+
30+
namespace iceberg {
31+
32+
/// \brief Utility class for collecting validation errors in builder patterns
33+
///
34+
/// This class provides error accumulation functionality for builders that
35+
/// cannot throw exceptions. Builder methods can call AddError() to accumulate
36+
/// validation errors, and CheckErrors() returns all errors at once.
37+
///
38+
/// This allows users to see all validation errors at once rather than fixing
39+
/// them one by one (fail-slow instead of fail-fast).
40+
///
41+
/// Example usage:
42+
/// \code
43+
/// class MyBuilder {
44+
/// protected:
45+
/// ErrorCollector errors_;
46+
///
47+
/// MyBuilder& SetValue(int val) {
48+
/// if (val < 0) {
49+
/// errors_.AddError(ErrorKind::kInvalidArgument, "Value must be non-negative");
50+
/// return *this;
51+
/// }
52+
/// value_ = val;
53+
/// return *this;
54+
/// }
55+
///
56+
/// Result<MyObject> Build() {
57+
/// ICEBERG_RETURN_UNEXPECTED(errors_.CheckErrors());
58+
/// return MyObject{value_};
59+
/// }
60+
/// };
61+
/// \endcode
62+
class ErrorCollector {
63+
public:
64+
ErrorCollector() = default;
65+
66+
/// \brief Add a validation error
67+
///
68+
/// \param kind The kind of error
69+
/// \param message The error message
70+
void AddError(ErrorKind kind, std::string message) {
71+
errors_.emplace_back(kind, std::move(message));
72+
}
73+
74+
/// \brief Add an existing error object
75+
///
76+
/// Useful when propagating errors from other components or reusing
77+
/// error objects without deconstructing and reconstructing them.
78+
///
79+
/// \param err The error to add
80+
void AddError(Error err) { errors_.push_back(std::move(err)); }
81+
82+
/// \brief Check if any errors have been collected
83+
///
84+
/// \return true if there are accumulated errors
85+
bool HasErrors() const { return !errors_.empty(); }
86+
87+
/// \brief Get the number of errors collected
88+
///
89+
/// \return The count of accumulated errors
90+
size_t ErrorCount() const { return errors_.size(); }
91+
92+
/// \brief Check for accumulated errors and return them if any exist
93+
///
94+
/// This should be called before completing a builder operation (e.g.,
95+
/// in Build(), Apply(), or Commit() methods) to validate that no errors
96+
/// were accumulated during the builder method calls.
97+
///
98+
/// \return Status::OK if no errors, or a ValidationFailed error with
99+
/// all accumulated error messages
100+
Status CheckErrors() const {
101+
if (!errors_.empty()) {
102+
std::string error_msg = "Validation failed due to the following errors:\n";
103+
for (const auto& [kind, message] : errors_) {
104+
error_msg += " - " + message + "\n";
105+
}
106+
return ValidationFailed("{}", error_msg);
107+
}
108+
return {};
109+
}
110+
111+
/// \brief Clear all accumulated errors
112+
///
113+
/// This can be useful for resetting the error state in tests or
114+
/// when reusing a builder instance.
115+
void ClearErrors() { errors_.clear(); }
116+
117+
/// \brief Get read-only access to all collected errors
118+
///
119+
/// \return A const reference to the vector of errors
120+
const std::vector<Error>& Errors() const { return errors_; }
121+
122+
private:
123+
std::vector<Error> errors_;
124+
};
125+
126+
} // namespace iceberg

0 commit comments

Comments
 (0)