You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "adamdebreceni (via GitHub)" <gi...@apache.org> on 2023/03/03 10:36:20 UTC
[GitHub] [nifi-minifi-cpp] adamdebreceni opened a new pull request, #1522: MINIFICPP-2065 - Only accept configuration format the agent can handle
adamdebreceni opened a new pull request, #1522:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1522
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:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] 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.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] 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)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] 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] adamdebreceni commented on a diff in pull request #1522: MINIFICPP-2065 - Only accept configuration format the agent can handle
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1522:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1522#discussion_r1124336355
##########
extensions/http-curl/protocols/RESTSender.cpp:
##########
@@ -162,7 +163,9 @@ C2Payload RESTSender::sendPayload(const std::string& url, const Direction direct
if (payload.getOperation() == Operation::TRANSFER) {
auto read = std::make_unique<utils::HTTPReadCallback>(std::numeric_limits<size_t>::max());
client.setReadCallback(std::move(read));
- client.setRequestHeader("Accept", "application/vnd.minifi-c2+json;version=1, text/yml");
+ if (accepted_formats) {
Review Comment:
yes, we should not set the accept header explicitly if the `accepted_formats` is empty
--
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] adamdebreceni commented on a diff in pull request #1522: MINIFICPP-2065 - Only accept configuration format the agent can handle
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1522:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1522#discussion_r1124332857
##########
extensions/http-curl/protocols/RESTSender.cpp:
##########
@@ -181,7 +184,7 @@ C2Payload RESTSender::sendPayload(const std::string& url, const Direction direct
const auto response_body_bytes = gsl::make_span(client.getResponseBody()).as_span<const std::byte>();
logger_->log_trace("Received response: \"%s\"", [&] {return utils::StringUtils::escapeUnprintableBytes(response_body_bytes);});
if (isOkay && !clientError && !serverError) {
- if (payload.isRaw()) {
+ if (accepted_formats) {
Review Comment:
depending on the outgoing payload's `isRaw` to process the response seems to be a workaround, so they do not really correlate
--
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 #1522: MINIFICPP-2065 - Only accept configuration format the agent can handle
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1522:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1522#discussion_r1124409139
##########
libminifi/src/c2/C2Agent.cpp:
##########
@@ -842,8 +842,7 @@ std::optional<std::string> C2Agent::fetchFlow(const std::string& uri) const {
return std::nullopt;
}
- C2Payload payload(Operation::TRANSFER, true);
- C2Payload &&response = protocol_.load()->consumePayload(resolved_url.value(), payload, RECEIVE, false);
+ C2Payload &&response = protocol_.load()->fetch(resolved_url.value(), update_sink_->getSupportedConfigurationFormats());
Review Comment:
Would you mind removing the `&&` from here? It doesn't do anything other than confusing readers.
```suggestion
C2Payload response = protocol_.load()->fetch(resolved_url.value(), update_sink_->getSupportedConfigurationFormats());
```
--
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 a diff in pull request #1522: MINIFICPP-2065 - Only accept configuration format the agent can handle
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1522:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1522#discussion_r1124321339
##########
extensions/http-curl/protocols/RESTSender.cpp:
##########
@@ -181,7 +184,7 @@ C2Payload RESTSender::sendPayload(const std::string& url, const Direction direct
const auto response_body_bytes = gsl::make_span(client.getResponseBody()).as_span<const std::byte>();
logger_->log_trace("Received response: \"%s\"", [&] {return utils::StringUtils::escapeUnprintableBytes(response_body_bytes);});
if (isOkay && !clientError && !serverError) {
- if (payload.isRaw()) {
+ if (accepted_formats) {
Review Comment:
How does the accepted_formats correlate if the payload is raw? Could you add a comment for this, I don't think it's trivial.
##########
extensions/http-curl/protocols/RESTSender.cpp:
##########
@@ -162,7 +163,9 @@ C2Payload RESTSender::sendPayload(const std::string& url, const Direction direct
if (payload.getOperation() == Operation::TRANSFER) {
auto read = std::make_unique<utils::HTTPReadCallback>(std::numeric_limits<size_t>::max());
client.setReadCallback(std::move(read));
- client.setRequestHeader("Accept", "application/vnd.minifi-c2+json;version=1, text/yml");
+ if (accepted_formats) {
Review Comment:
I think we should also check if this vector is empty, I'm not sure how empty accept header would be interpreted.
--
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 a diff in pull request #1522: MINIFICPP-2065 - Only accept configuration format the agent can handle
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1522:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1522#discussion_r1124353151
##########
extensions/http-curl/protocols/RESTSender.cpp:
##########
@@ -181,7 +184,7 @@ C2Payload RESTSender::sendPayload(const std::string& url, const Direction direct
const auto response_body_bytes = gsl::make_span(client.getResponseBody()).as_span<const std::byte>();
logger_->log_trace("Received response: \"%s\"", [&] {return utils::StringUtils::escapeUnprintableBytes(response_body_bytes);});
if (isOkay && !clientError && !serverError) {
- if (payload.isRaw()) {
+ if (accepted_formats) {
Review Comment:
So why do we check if `accepted_formats` is set here (also maybe check if it is empty here as well)? If `accepted_formats` is not set the format is interpreted as `*/*` on the C2 server side, which means it can return either yaml or json, how does the processing of the response differ depending on the `accepted_formats`?
--
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] adamdebreceni commented on a diff in pull request #1522: MINIFICPP-2065 - Only accept configuration format the agent can handle
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1522:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1522#discussion_r1124373053
##########
extensions/http-curl/protocols/RESTSender.cpp:
##########
@@ -181,7 +184,7 @@ C2Payload RESTSender::sendPayload(const std::string& url, const Direction direct
const auto response_body_bytes = gsl::make_span(client.getResponseBody()).as_span<const std::byte>();
logger_->log_trace("Received response: \"%s\"", [&] {return utils::StringUtils::escapeUnprintableBytes(response_body_bytes);});
if (isOkay && !clientError && !serverError) {
- if (payload.isRaw()) {
+ if (accepted_formats) {
Review Comment:
as I understand the direct download feature (e.g. a GET to the flow url) is outside the scope of the c2 protocol, even though it was done through the `consumePayload` method, the newly added `fetch` allows us to be explicit about this, implementation-wise the `RESTSender` uses its `sendPayload` for both except if the `accepted_formats` is set (not `nullopt`) we do not try to parse the response as JSON expecting a command, but forward the response as-is
--
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 #1522: MINIFICPP-2065 - Only accept configuration format the agent can handle
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm closed pull request #1522: MINIFICPP-2065 - Only accept configuration format the agent can handle
URL: https://github.com/apache/nifi-minifi-cpp/pull/1522
--
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