fgerlits commented on a change in pull request #1057: URL: https://github.com/apache/nifi-minifi-cpp/pull/1057#discussion_r633553516
########## File path: libminifi/include/c2/HeartbeatJsonSerializer.h ########## @@ -0,0 +1,59 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include <string> + +#include "HeartbeatReporter.h" +#include "C2Payload.h" + +#ifdef RAPIDJSON_ASSERT +#undef RAPIDJSON_ASSERT +#endif +#define RAPIDJSON_ASSERT(x) if(!(x)) throw std::logic_error("rapidjson exception"); //NOLINT + +#ifdef RAPIDJSON_HAS_STDSTRING +#undef RAPIDJSON_HAS_STDSTRING +#endif +#define RAPIDJSON_HAS_STDSTRING 1 Review comment: Why is this needed / why was it not needed before? ########## File path: extensions/http-curl/tests/C2LogHeartbeatTest.cpp ########## @@ -0,0 +1,58 @@ +/** + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#undef NDEBUG +#include "TestBase.h" +#include "c2/C2Agent.h" +#include "protocols/RESTProtocol.h" +#include "protocols/RESTSender.h" +#include "HTTPIntegrationBase.h" +#include "HTTPHandlers.h" +#include "utils/IntegrationTestUtils.h" +#include "c2/HeartbeatLogger.h" + +class VerifyLogC2Heartbeat : public VerifyC2Base { + public: + void testSetup() override { + LogTestController::getInstance().setTrace<minifi::c2::C2Agent>(); + LogTestController::getInstance().setDebug<minifi::c2::RESTSender>(); + LogTestController::getInstance().setDebug<minifi::c2::RESTProtocol>(); + // the heartbeat is logged at TRACE level + LogTestController::getInstance().setTrace<minifi::c2::HeartbeatLogger>(); + VerifyC2Base::testSetup(); + } + + void runAssertions() override { + using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime; + assert(verifyLogLinePresenceInPollTime( + std::chrono::milliseconds(wait_time_), + "\"operation\": \"heartbeat\"")); + } + + void configureC2() override { + VerifyC2Base::configureC2(); + configuration->set("c2.agent.heartbeat.reporter.classes", "HeartbeatLogger"); Review comment: minor, but we normally use nifi-prefixed variables in new code, ie. `"nifi.c2.agent.heartbeat.reporter.classes"` ########## File path: libminifi/src/c2/HeartbeatJsonSerializer.cpp ########## @@ -0,0 +1,333 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "c2/HeartbeatJsonSerializer.h" +#include "rapidjson/document.h" +#include "rapidjson/writer.h" +#include "rapidjson/stringbuffer.h" +#include "rapidjson/prettywriter.h" + +namespace org { +namespace apache { +namespace nifi { +namespace minifi { +namespace c2 { + +std::string HeartbeatJsonSerializer::getOperation(const C2Payload& payload) { + switch (payload.getOperation()) { + case Operation::ACKNOWLEDGE: + return "acknowledge"; + case Operation::HEARTBEAT: + return "heartbeat"; + case Operation::RESTART: + return "restart"; + case Operation::DESCRIBE: + return "describe"; + case Operation::STOP: + return "stop"; + case Operation::START: + return "start"; + case Operation::UPDATE: + return "update"; + case Operation::PAUSE: + return "pause"; + case Operation::RESUME: + return "resume"; + default: + return "heartbeat"; + } Review comment: I know this is old code and you just moved it, but this function converts some enumeration values like CLEAR and TRANSFER into "heartbeat" -- is this OK? I would expect the switch statement to * cover all possible enum values * list values in the same order as in the enum definition * not have a default, or have a default like "UNKNOWN". If this behavior is intentional, it's probably worth adding a comment to explain why. ########## File path: libminifi/src/c2/protocols/RESTProtocol.cpp ########## @@ -318,133 +178,7 @@ bool RESTProtocol::containsPayload(const C2Payload &o) { return false; } -rapidjson::Value RESTProtocol::serializeConnectionQueues(const C2Payload &payload, std::string &label, rapidjson::Document::AllocatorType &alloc) { - rapidjson::Value json_payload(payload.isContainer() ? rapidjson::kArrayType : rapidjson::kObjectType); - - C2Payload adjusted(payload.getOperation(), payload.getIdentifier(), payload.isRaw()); - - auto name = payload.getLabel(); - std::string uuid; - C2ContentResponse updatedContent(payload.getOperation()); - for (const C2ContentResponse &content : payload.getContent()) { - for (const auto& op_arg : content.operation_arguments) { - if (op_arg.first == "uuid") { - uuid = op_arg.second.to_string(); - } - updatedContent.operation_arguments.insert(op_arg); - } - } - updatedContent.name = uuid; - adjusted.setLabel(uuid); - adjusted.setIdentifier(uuid); - c2::AnnotatedValue nd; - // name should be what was previously the TLN ( top level node ) - nd = name; - updatedContent.operation_arguments.insert(std::make_pair("name", nd)); - // the rvalue reference is an unfortunate side effect of the underlying API decision. - adjusted.addContent(std::move(updatedContent), true); - mergePayloadContent(json_payload, adjusted, alloc); - label = uuid; - return json_payload; -} - -rapidjson::Value RESTProtocol::serializeJsonPayload(const C2Payload &payload, rapidjson::Document::AllocatorType &alloc) { - // get the name from the content - rapidjson::Value json_payload(payload.isContainer() ? rapidjson::kArrayType : rapidjson::kObjectType); - - std::vector<ValueObject> children; - - bool isQueue = payload.getLabel() == "queues"; - - for (const auto &nested_payload : payload.getNestedPayloads()) { - std::string label = nested_payload.getLabel(); - rapidjson::Value* child_payload = new rapidjson::Value(isQueue ? serializeConnectionQueues(nested_payload, label, alloc) : serializeJsonPayload(nested_payload, alloc)); - - if (nested_payload.isCollapsible()) { - bool combine = false; - for (auto &subordinate : children) { - if (subordinate.name == label) { - subordinate.values.push_back(child_payload); - combine = true; - break; - } - } - if (!combine) { - ValueObject obj; - obj.name = label; - obj.values.push_back(child_payload); - children.push_back(obj); - } - } else { - ValueObject obj; - obj.name = label; - obj.values.push_back(child_payload); - children.push_back(obj); - } - } - - for (auto child_vector : children) { - rapidjson::Value children_json; - rapidjson::Value newMemberKey = getStringValue(child_vector.name, alloc); - if (child_vector.values.size() > 1) { - children_json.SetArray(); - for (auto child : child_vector.values) { - if (json_payload.IsArray()) - json_payload.PushBack(child->Move(), alloc); - else - children_json.PushBack(child->Move(), alloc); - } - if (!json_payload.IsArray()) - json_payload.AddMember(newMemberKey, children_json, alloc); - } else if (child_vector.values.size() == 1) { - rapidjson::Value* first = child_vector.values.front(); - if (first->IsObject() && first->HasMember(newMemberKey)) { - if (json_payload.IsArray()) - json_payload.PushBack((*first)[newMemberKey].Move(), alloc); - else - json_payload.AddMember(newMemberKey, (*first)[newMemberKey].Move(), alloc); - } else { - if (json_payload.IsArray()) { - json_payload.PushBack(first->Move(), alloc); - } else { - json_payload.AddMember(newMemberKey, first->Move(), alloc); - } - } - } - for (rapidjson::Value* child : child_vector.values) - delete child; - } - - mergePayloadContent(json_payload, payload, alloc); - return json_payload; -} - -std::string RESTProtocol::getOperation(const C2Payload &payload) { - switch (payload.getOperation()) { - case Operation::ACKNOWLEDGE: - return "acknowledge"; - case Operation::HEARTBEAT: - return "heartbeat"; - case Operation::RESTART: - return "restart"; - case Operation::DESCRIBE: - return "describe"; - case Operation::STOP: - return "stop"; - case Operation::START: - return "start"; - case Operation::UPDATE: - return "update"; - case Operation::PAUSE: - return "pause"; - case Operation::RESUME: - return "resume"; - default: - return "heartbeat"; - } -} - -Operation RESTProtocol::stringToOperation(const std::string str) { +Operation RESTProtocol::stringToOperation(const std::string& str) { Review comment: I think `stringToOperation()` should be in the same class as `getOperation()` [and cover the same values, in the same order]. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
