fgerlits commented on code in PR #1331:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1331#discussion_r873840546
##########
extensions/http-curl/processors/InvokeHTTP.cpp:
##########
@@ -264,18 +269,36 @@ bool InvokeHTTP::shouldEmitFlowFile() const {
return ("POST" == method_ || "PUT" == method_ || "PATCH" == method_);
}
-std::optional<std::map<std::string, std::string>>
InvokeHTTP::validateAttributesAgainstHTTPHeaderRules(const
std::map<std::string, std::string>& attributes) const {
- std::map<std::string, std::string> result;
- for (const auto& [attribute_name, attribute_value] : attributes) {
- if (utils::HTTPClient::isValidHttpHeaderField(attribute_name)) {
- result.emplace(attribute_name, attribute_value);
- } else if (invalid_http_header_field_handling_strategy_ ==
InvalidHTTPHeaderFieldHandlingOption::TRANSFORM) {
-
result.emplace(utils::HTTPClient::replaceInvalidCharactersInHttpHeaderFieldName(attribute_name),
attribute_value);
- } else if (invalid_http_header_field_handling_strategy_ ==
InvalidHTTPHeaderFieldHandlingOption::FAIL) {
- return std::nullopt;
- }
+/**
+ * Calls append_header with valid HTTP header keys, based on
attributes_to_send_. Returns whether the flow file should be routed to Failure.
+ * @param flow_file
+ * @param append_header Callback to append HTTP header to the request
+ * @return Whether the flow file should be routed to failure
+ */
+bool InvokeHTTP::appendHeaders(const core::FlowFile& flow_file,
/*std::invocable<std::string, std::string>*/ auto append_header) {
+ if (!attributes_to_send_) return true;
+ const auto key_fn = [](const std::pair<std::string, std::string>& pair) {
return pair.first; };
+ const auto original_attributes = flow_file.getAttributes();
+ // non-const views, because otherwise it doesn't satisfy viewable_range, and
transform would fail
+ ranges::viewable_range auto matching_attributes = original_attributes
+ | ranges::views::filter([this](const auto& key) { return
utils::regexMatch(key, *attributes_to_send_); }, key_fn);
+ switch (invalid_http_header_field_handling_strategy_.value()) {
+ case InvalidHTTPHeaderFieldHandlingOption::FAIL:
+ if (ranges::any_of(original_attributes,
std::not_fn(&utils::HTTPClient::isValidHttpHeaderField), key_fn)) return false;
Review Comment:
Why is it a problem if some non-matching attribute is invalid?
```suggestion
if (ranges::any_of(matching_attributes,
std::not_fn(&utils::HTTPClient::isValidHttpHeaderField), key_fn)) return false;
```
I can see that this is how it worked in the old code, too, but it seems
user-unfriendly.
##########
extensions/http-curl/processors/InvokeHTTP.cpp:
##########
@@ -264,18 +269,36 @@ bool InvokeHTTP::shouldEmitFlowFile() const {
return ("POST" == method_ || "PUT" == method_ || "PATCH" == method_);
}
-std::optional<std::map<std::string, std::string>>
InvokeHTTP::validateAttributesAgainstHTTPHeaderRules(const
std::map<std::string, std::string>& attributes) const {
- std::map<std::string, std::string> result;
- for (const auto& [attribute_name, attribute_value] : attributes) {
- if (utils::HTTPClient::isValidHttpHeaderField(attribute_name)) {
- result.emplace(attribute_name, attribute_value);
- } else if (invalid_http_header_field_handling_strategy_ ==
InvalidHTTPHeaderFieldHandlingOption::TRANSFORM) {
-
result.emplace(utils::HTTPClient::replaceInvalidCharactersInHttpHeaderFieldName(attribute_name),
attribute_value);
- } else if (invalid_http_header_field_handling_strategy_ ==
InvalidHTTPHeaderFieldHandlingOption::FAIL) {
- return std::nullopt;
- }
+/**
+ * Calls append_header with valid HTTP header keys, based on
attributes_to_send_. Returns whether the flow file should be routed to Failure.
+ * @param flow_file
+ * @param append_header Callback to append HTTP header to the request
+ * @return Whether the flow file should be routed to failure
Review Comment:
To me this suggests that a return value of `true` means "route to failure",
which is the opposite of what happens. Something like "@return whether the
headers were added successfully" would be better.
Also, I would remove the second sentence from line 273, as saying something
twice is asking for trouble: at some point, someone will update one of them but
not the other.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
