You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/12/21 12:46:33 UTC
[GitHub] [nifi-minifi-cpp] martinzink opened a new pull request, #1482: MINIFICPP-2009 - CWEL should add resolved attributes with json output…
martinzink opened a new pull request, #1482:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1482
… as well
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [x] If applicable, have you updated the LICENSE file?
- [x] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [x] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1482: MINIFICPP-2009 - CWEL should add resolved attributes with json output…
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1482:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1482#discussion_r1097388376
##########
extensions/windows-event-log/tests/ConsumeWindowsEventLogTests.cpp:
##########
@@ -238,6 +238,18 @@ TEST_CASE("ConsumeWindowsEventLog extracts some attributes by default", "[onTrig
auto logger_processor = test_plan->addProcessor("LogAttribute", "logger", Success, true);
test_plan->setProperty(logger_processor, LogAttribute::FlowFilesToLog.getName(), "0");
+ SECTION("XML output") {
+ CHECK(test_plan->setProperty(cwel_processor, ConsumeWindowsEventLog ::OutputFormat.getName(), "XML"));
Review Comment:
very minor, but the space before the `::` looks weird
##########
extensions/windows-event-log/tests/ConsumeWindowsEventLogTests.cpp:
##########
@@ -238,6 +238,18 @@ TEST_CASE("ConsumeWindowsEventLog extracts some attributes by default", "[onTrig
auto logger_processor = test_plan->addProcessor("LogAttribute", "logger", Success, true);
test_plan->setProperty(logger_processor, LogAttribute::FlowFilesToLog.getName(), "0");
+ SECTION("XML output") {
+ CHECK(test_plan->setProperty(cwel_processor, ConsumeWindowsEventLog ::OutputFormat.getName(), "XML"));
Review Comment:
I think these three should be `REQUIRE`s instead of `CHECK`s, because it doesn't make sense to continue running the test if we could not set the property
##########
extensions/windows-event-log/ConsumeWindowsEventLog.cpp:
##########
@@ -699,42 +699,38 @@ void ConsumeWindowsEventLog::refreshTimeZoneData() {
}
void ConsumeWindowsEventLog::putEventRenderFlowFileToSession(const EventRender& eventRender, core::ProcessSession& session) const {
- auto commitFlowFile = [&] (const std::shared_ptr<core::FlowFile>& flowFile, const std::string& content, const std::string& mimeType) {
- session.writeBuffer(flowFile, content);
- session.putAttribute(flowFile, core::SpecialFlowAttribute::MIME_TYPE, mimeType);
- session.putAttribute(flowFile, "timezone.name", timezone_name_);
- session.putAttribute(flowFile, "timezone.offset", timezone_offset_);
- session.getProvenanceReporter()->receive(flowFile, provenanceUri_, getUUIDStr(), "Consume windows event logs", 0ms);
- session.transfer(flowFile, Success);
+ auto commitFlowFile = [&] (const std::string& content, const std::string& mimeType) {
+ auto flow_file = session.create();
+ addMatchedFieldsAsAttributes(eventRender, session, flow_file);
+ session.writeBuffer(flow_file, content);
+ session.putAttribute(flow_file, core::SpecialFlowAttribute::MIME_TYPE, mimeType);
+ session.putAttribute(flow_file, "timezone.name", timezone_name_);
+ session.putAttribute(flow_file, "timezone.offset", timezone_offset_);
+ session.getProvenanceReporter()->receive(flow_file, provenanceUri_, getUUIDStr(), "Consume windows event logs", 0ms);
+ session.transfer(flow_file, Success);
};
if (output_.xml) {
- auto flowFile = session.create();
logger_->log_trace("Writing rendered XML to a flow file");
-
- for (const auto &fieldMapping : eventRender.matched_fields) {
- if (!fieldMapping.second.empty()) {
- session.putAttribute(flowFile, fieldMapping.first, fieldMapping.second);
- }
- }
-
- commitFlowFile(flowFile, eventRender.xml, "application/xml");
+ commitFlowFile(eventRender.xml, "application/xml");
}
if (output_.plaintext) {
logger_->log_trace("Writing rendered plain text to a flow file");
- commitFlowFile(session.create(), eventRender.plaintext, "text/plain");
+ commitFlowFile(eventRender.plaintext, "text/plain");
}
- if (output_.json.type == JSONType::Raw) {
- logger_->log_trace("Writing rendered raw JSON to a flow file");
- commitFlowFile(session.create(), eventRender.json, "application/json");
- } else if (output_.json.type == JSONType::Simple) {
- logger_->log_trace("Writing rendered simple JSON to a flow file");
- commitFlowFile(session.create(), eventRender.json, "application/json");
- } else if (output_.json.type == JSONType::Flattened) {
- logger_->log_trace("Writing rendered flattened JSON to a flow file");
- commitFlowFile(session.create(), eventRender.json, "application/json");
+ if (output_.json.type != JSONType::None) {
Review Comment:
`OutputFormat::JSON` has an `operator bool` which does this, so this could be `if (output_.json) { ... }`
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1482: MINIFICPP-2009 - CWEL should add resolved attributes with json output…
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1482:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1482#discussion_r1054439110
##########
extensions/windows-event-log/tests/ConsumeWindowsEventLogTests.cpp:
##########
@@ -236,6 +236,14 @@ TEST_CASE("ConsumeWindowsEventLog extracts some attributes by default", "[onTrig
auto logger_processor = test_plan->addProcessor("LogAttribute", "logger", Success, true);
test_plan->setProperty(logger_processor, LogAttribute::FlowFilesToLog.getName(), "0");
+ SECTION("XML output") {
+ CHECK(test_plan->setProperty(cwel_processor, ConsumeWindowsEventLog ::OutputFormat.getName(), "XML"));
+ }
+
+ SECTION("Json output") {
+ CHECK(test_plan->setProperty(cwel_processor, ConsumeWindowsEventLog ::OutputFormat.getName(), "JSON"));
+ }
Review Comment:
Are there tests that cover the content replacement case? I'm hoping this fix doesn't break that.
##########
extensions/windows-event-log/ConsumeWindowsEventLog.cpp:
##########
@@ -696,32 +696,30 @@ void ConsumeWindowsEventLog::putEventRenderFlowFileToSession(const EventRender&
};
if (output_.xml) {
- auto flowFile = session.create();
logger_->log_trace("Writing rendered XML to a flow file");
-
- for (const auto &fieldMapping : eventRender.matched_fields) {
- if (!fieldMapping.second.empty()) {
- session.putAttribute(flowFile, fieldMapping.first, fieldMapping.second);
- }
- }
-
- commitFlowFile(flowFile, eventRender.xml, "application/xml");
+ auto flow_file = session.create();
+ addMatchedFieldsAsAttributes(eventRender, session, flow_file);
+ commitFlowFile(flow_file, eventRender.xml, "application/xml");
}
if (output_.plaintext) {
logger_->log_trace("Writing rendered plain text to a flow file");
commitFlowFile(session.create(), eventRender.plaintext, "text/plain");
}
- if (output_.json.type == JSONType::Raw) {
- logger_->log_trace("Writing rendered raw JSON to a flow file");
- commitFlowFile(session.create(), eventRender.json, "application/json");
- } else if (output_.json.type == JSONType::Simple) {
- logger_->log_trace("Writing rendered simple JSON to a flow file");
- commitFlowFile(session.create(), eventRender.json, "application/json");
- } else if (output_.json.type == JSONType::Flattened) {
- logger_->log_trace("Writing rendered flattened JSON to a flow file");
- commitFlowFile(session.create(), eventRender.json, "application/json");
+ if (output_.json.type != JSONType::None) {
+ logger_->log_trace("Writing rendered %s JSON to a flow file", output_.json.type.toString());
+ auto flow_file = session.create();
+ addMatchedFieldsAsAttributes(eventRender, session, flow_file);
Review Comment:
I think it would make sense to add the attributes in all cases, including plaintext.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1482: MINIFICPP-2009 - CWEL should add resolved attributes with json output…
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1482:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1482#discussion_r1098371820
##########
extensions/windows-event-log/tests/ConsumeWindowsEventLogTests.cpp:
##########
@@ -238,6 +238,18 @@ TEST_CASE("ConsumeWindowsEventLog extracts some attributes by default", "[onTrig
auto logger_processor = test_plan->addProcessor("LogAttribute", "logger", Success, true);
test_plan->setProperty(logger_processor, LogAttribute::FlowFilesToLog.getName(), "0");
+ SECTION("XML output") {
+ CHECK(test_plan->setProperty(cwel_processor, ConsumeWindowsEventLog ::OutputFormat.getName(), "XML"));
Review Comment:
agreed, don't why that was there, fixed it in https://github.com/apache/nifi-minifi-cpp/pull/1482/commits/efb169ad2b661a82f1608e80031dfcb7672eab06#diff-a2d991cd055149f79da7e62be4848113893ed6fc7104d9972c97cb671d4de7ceR242
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on pull request #1482: MINIFICPP-2009 - CWEL should add resolved attributes with json output…
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on PR #1482:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1482#issuecomment-1418739726
> Currently we have the options XML, Plaintext, Both, JSON for Output Format. The "Both" option is quite ambiguous now that we have 3 different output formats, should we revise this? Either have an "All" option for all 3 output formats, or rename it to "XML and Plaintext'.
Yeah thats a bit confusing I agree. I think we should address that. (But I dont see an easy fix without breaking compatibility with previous configurations and seems out of the scope of this PR)
I've created a jira for this https://issues.apache.org/jira/browse/MINIFICPP-2042
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm closed pull request #1482: MINIFICPP-2009 - CWEL should add resolved attributes with json output…
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm closed pull request #1482: MINIFICPP-2009 - CWEL should add resolved attributes with json output…
URL: https://github.com/apache/nifi-minifi-cpp/pull/1482
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on pull request #1482: MINIFICPP-2009 - CWEL should add resolved attributes with json output…
Posted by GitBox <gi...@apache.org>.
lordgamez commented on PR #1482:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1482#issuecomment-1385016182
Currently we have the options XML, Plaintext, Both, JSON for Output Format. The "Both" option is quite ambiguous now that we have 3 different output formats, should we revise this? Either have an "All" option for all 3 output formats, or rename it to "XML and Plaintext'.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1482: MINIFICPP-2009 - CWEL should add resolved attributes with json output…
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1482:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1482#discussion_r1098371050
##########
extensions/windows-event-log/ConsumeWindowsEventLog.cpp:
##########
@@ -699,42 +699,38 @@ void ConsumeWindowsEventLog::refreshTimeZoneData() {
}
void ConsumeWindowsEventLog::putEventRenderFlowFileToSession(const EventRender& eventRender, core::ProcessSession& session) const {
- auto commitFlowFile = [&] (const std::shared_ptr<core::FlowFile>& flowFile, const std::string& content, const std::string& mimeType) {
- session.writeBuffer(flowFile, content);
- session.putAttribute(flowFile, core::SpecialFlowAttribute::MIME_TYPE, mimeType);
- session.putAttribute(flowFile, "timezone.name", timezone_name_);
- session.putAttribute(flowFile, "timezone.offset", timezone_offset_);
- session.getProvenanceReporter()->receive(flowFile, provenanceUri_, getUUIDStr(), "Consume windows event logs", 0ms);
- session.transfer(flowFile, Success);
+ auto commitFlowFile = [&] (const std::string& content, const std::string& mimeType) {
+ auto flow_file = session.create();
+ addMatchedFieldsAsAttributes(eventRender, session, flow_file);
+ session.writeBuffer(flow_file, content);
+ session.putAttribute(flow_file, core::SpecialFlowAttribute::MIME_TYPE, mimeType);
+ session.putAttribute(flow_file, "timezone.name", timezone_name_);
+ session.putAttribute(flow_file, "timezone.offset", timezone_offset_);
+ session.getProvenanceReporter()->receive(flow_file, provenanceUri_, getUUIDStr(), "Consume windows event logs", 0ms);
+ session.transfer(flow_file, Success);
};
if (output_.xml) {
- auto flowFile = session.create();
logger_->log_trace("Writing rendered XML to a flow file");
-
- for (const auto &fieldMapping : eventRender.matched_fields) {
- if (!fieldMapping.second.empty()) {
- session.putAttribute(flowFile, fieldMapping.first, fieldMapping.second);
- }
- }
-
- commitFlowFile(flowFile, eventRender.xml, "application/xml");
+ commitFlowFile(eventRender.xml, "application/xml");
}
if (output_.plaintext) {
logger_->log_trace("Writing rendered plain text to a flow file");
- commitFlowFile(session.create(), eventRender.plaintext, "text/plain");
+ commitFlowFile(eventRender.plaintext, "text/plain");
}
- if (output_.json.type == JSONType::Raw) {
- logger_->log_trace("Writing rendered raw JSON to a flow file");
- commitFlowFile(session.create(), eventRender.json, "application/json");
- } else if (output_.json.type == JSONType::Simple) {
- logger_->log_trace("Writing rendered simple JSON to a flow file");
- commitFlowFile(session.create(), eventRender.json, "application/json");
- } else if (output_.json.type == JSONType::Flattened) {
- logger_->log_trace("Writing rendered flattened JSON to a flow file");
- commitFlowFile(session.create(), eventRender.json, "application/json");
+ if (output_.json.type != JSONType::None) {
Review Comment:
:+1: good idea, changed it in https://github.com/apache/nifi-minifi-cpp/pull/1482/commits/efb169ad2b661a82f1608e80031dfcb7672eab06#diff-0769252d38a0bd8b7b412439a306a061cd5da7b84419220a1aec6e100a853cecR723
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1482: MINIFICPP-2009 - CWEL should add resolved attributes with json output…
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1482:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1482#discussion_r1097100636
##########
extensions/windows-event-log/ConsumeWindowsEventLog.cpp:
##########
@@ -696,32 +696,30 @@ void ConsumeWindowsEventLog::putEventRenderFlowFileToSession(const EventRender&
};
if (output_.xml) {
- auto flowFile = session.create();
logger_->log_trace("Writing rendered XML to a flow file");
-
- for (const auto &fieldMapping : eventRender.matched_fields) {
- if (!fieldMapping.second.empty()) {
- session.putAttribute(flowFile, fieldMapping.first, fieldMapping.second);
- }
- }
-
- commitFlowFile(flowFile, eventRender.xml, "application/xml");
+ auto flow_file = session.create();
+ addMatchedFieldsAsAttributes(eventRender, session, flow_file);
+ commitFlowFile(flow_file, eventRender.xml, "application/xml");
}
if (output_.plaintext) {
logger_->log_trace("Writing rendered plain text to a flow file");
commitFlowFile(session.create(), eventRender.plaintext, "text/plain");
}
- if (output_.json.type == JSONType::Raw) {
- logger_->log_trace("Writing rendered raw JSON to a flow file");
- commitFlowFile(session.create(), eventRender.json, "application/json");
- } else if (output_.json.type == JSONType::Simple) {
- logger_->log_trace("Writing rendered simple JSON to a flow file");
- commitFlowFile(session.create(), eventRender.json, "application/json");
- } else if (output_.json.type == JSONType::Flattened) {
- logger_->log_trace("Writing rendered flattened JSON to a flow file");
- commitFlowFile(session.create(), eventRender.json, "application/json");
+ if (output_.json.type != JSONType::None) {
+ logger_->log_trace("Writing rendered %s JSON to a flow file", output_.json.type.toString());
+ auto flow_file = session.create();
+ addMatchedFieldsAsAttributes(eventRender, session, flow_file);
Review Comment:
Sry for the late reply.
I've added this for all cases(and a test for plaintext aswell) in https://github.com/apache/nifi-minifi-cpp/pull/1482/commits/76db15357c1f2efd8cf2fa1cec2a7ed370eb0bf7
##########
extensions/windows-event-log/tests/ConsumeWindowsEventLogTests.cpp:
##########
@@ -236,6 +236,14 @@ TEST_CASE("ConsumeWindowsEventLog extracts some attributes by default", "[onTrig
auto logger_processor = test_plan->addProcessor("LogAttribute", "logger", Success, true);
test_plan->setProperty(logger_processor, LogAttribute::FlowFilesToLog.getName(), "0");
+ SECTION("XML output") {
+ CHECK(test_plan->setProperty(cwel_processor, ConsumeWindowsEventLog ::OutputFormat.getName(), "XML"));
+ }
+
+ SECTION("Json output") {
+ CHECK(test_plan->setProperty(cwel_processor, ConsumeWindowsEventLog ::OutputFormat.getName(), "JSON"));
+ }
Review Comment:
Sorry for the late reply.
Yes there are some tests that cover those cases in [MetadataWalkerTests](https://github.com/apache/nifi-minifi-cpp/blob/main/extensions/windows-event-log/tests/MetadataWalkerTests.cpp)
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1482: MINIFICPP-2009 - CWEL should add resolved attributes with json output…
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1482:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1482#discussion_r1098372406
##########
extensions/windows-event-log/tests/ConsumeWindowsEventLogTests.cpp:
##########
@@ -238,6 +238,18 @@ TEST_CASE("ConsumeWindowsEventLog extracts some attributes by default", "[onTrig
auto logger_processor = test_plan->addProcessor("LogAttribute", "logger", Success, true);
test_plan->setProperty(logger_processor, LogAttribute::FlowFilesToLog.getName(), "0");
+ SECTION("XML output") {
+ CHECK(test_plan->setProperty(cwel_processor, ConsumeWindowsEventLog ::OutputFormat.getName(), "XML"));
Review Comment:
You are right. I've switched to `REQUIRE` in https://github.com/apache/nifi-minifi-cpp/pull/1482/commits/efb169ad2b661a82f1608e80031dfcb7672eab06#diff-a2d991cd055149f79da7e62be4848113893ed6fc7104d9972c97cb671d4de7ceR242
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org