[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
GitHub user pvillard31 opened a pull request: https://github.com/apache/nifi/pull/272 NIFI-1620 Allow empty Content-Type in InvokeHTTP processor You can merge this pull request into a Git repository by running: $ git pull https://github.com/pvillard31/nifi NIFI-1620 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/nifi/pull/272.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #272 commit 490278016a2df854c2cece9047a0e1e3e10f213f Author: Pierre Villard Date: 2016-03-12T16:13:04Z NIFI-1620 Allow empty Content-Type in InvokeHTTP processor --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user pvillard31 commented on the pull request: https://github.com/apache/nifi/pull/272#issuecomment-195793923 Due to JIRA unavailability, I think the PR has not been linked to issue in JIRA. Just adding a comment to mention the PR in JIRA. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user taftster commented on the pull request: https://github.com/apache/nifi/pull/272#issuecomment-196079414 I'm not entirely sure if this is a good idea. Any web service which _disallows_ a standard HTTP header is arguably broken. Quoting RFC 2616: > Any HTTP/1.1 message containing an entity-body SHOULD include a > Content-Type header field defining the media type of that body. If > and only if the media type is not given by a Content-Type field, the > recipient MAY attempt to guess the media type via inspection of its > content and/or the name extension(s) of the URI used to identify the > resource. If the media type remains unknown, the recipient SHOULD > treat it as type "application/octet-stream". It seems hard to believe given the above that a web service would reject a response with Content-Type. The current behavior of InvokeHTTP is possibly the most consistent with the HTTP specification. A custom processor designed to specifically interact with the remote service in question should be considered as an alternative to modifying InvokeHTTP. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user joewitt commented on the pull request: https://github.com/apache/nifi/pull/272#issuecomment-196089886 i was reviewing this earlier today and frankly had a similar concern to this as Adam. I didn't reply because I hadn't really figured out what to think. First, I agree that a service which rejects that header is arguably broken. Second, as the patch is right now I am curious how it works when the value is empty string because there is a static call to MediaType which seems like it would have trouble (still need to verify the logic there though). However, having said this Pierre can you clarify if the intent is only for the case where there is no entity body or is it also for when there is an entity body in the request? If the idea is that this is only necessary when there is no entity body we should tighten the code for that case and if it is for either scenario then I think i'm of similar mind to Adam here. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user pvillard31 commented on the pull request: https://github.com/apache/nifi/pull/272#issuecomment-196200486 Definitely agree with you that this API should not act this way. Anyway, just to clarify, this is only in the case there is no body/content sent with the request. That's why, in a message on the mailing list, I suggested to add a "body" property. If this property is set (possibly set to empty), the possible (incoming relationship is not mandatory with this processor) incoming flow file is ignored to set content and the request body is constructed with the property. This way we can validate the empty content type if and only if body is set to empty. Let me know if that makes sense. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user joewitt commented on the pull request: https://github.com/apache/nifi/pull/272#issuecomment-196284703 Thank you @pvillard31 . That does make sense and in the case that there is no request body being sent over I am supportive of the notion of not sending the content type header. This also seems in line with Adam's rfc reference. You want to tweak this PR then to retain existing content header logic but rather have a 'send body' property or something which when false will not send the body and will not set that header? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user pvillard31 commented on the pull request: https://github.com/apache/nifi/pull/272#issuecomment-196309847 @joewitt yes I can have a look this afternoon. I'll try to propose something before the end of the day. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user pvillard31 commented on the pull request: https://github.com/apache/nifi/pull/272#issuecomment-196786056 Not sure to be able to propose something today, I don't have as much free time as expected. I believe this should be moved to 0.7.0. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user pvillard31 commented on the pull request: https://github.com/apache/nifi/pull/272#issuecomment-198776846 Should be OK now : - Added body property - Reverted default Content-Type and original unit tests - Authorized no incoming connection with POST/PUT - Added check : if content-type empty, body must be empty - Added check : if body empty, content-type must be empty - Added unit tests - Tested against the API at the origin of the JIRA --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user taftster commented on the pull request: https://github.com/apache/nifi/pull/272#issuecomment-199086081 This pull request is not strictly dealing with NIFI-1620. The edge case of suppressing the Content-Type header for an empty message-body makes sense to fix and address. NIFI-1620 is dealing with this edge case. However, this PR is adding the ability to specify an HTTP request message-body directly as a property of InvokeHTTP. In my opinion, this is not a desired feature, and I am -1 on this. A static message-body can always be provided in a dataflow using a tool like ReplaceText; keeping the semantics that InvokeHTTP always reads from flowfile payloads has a good consistency. Recommend that this PR focus solely on the problem dealing with Content-Type of an empty payload for a POST/PUT operation. Another JIRA can be opened to discuss the merits of how to provide static content to an HTTP transaction. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user joewitt commented on the pull request: https://github.com/apache/nifi/pull/272#issuecomment-199086549 @pvillard31 If you stay focused on the goal of NIFI-1620 here and avoid the body manipulation this seems like a good step. Clearly you found a case worthy of support (null body means unset or empty strong content type). But, i do share Adam's view that it is best to avoid the content manipulation here. Do you agree or do you feel like for the use case(s) you envisioned this would be problematic? Thanks --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user pvillard31 commented on the pull request: https://github.com/apache/nifi/pull/272#issuecomment-199144952 I updated the PR with a true/false "send body" property, should be in line with last comments. Let me know if not. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user taftster commented on a diff in the pull request:
https://github.com/apache/nifi/pull/272#discussion_r56928547
--- Diff:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
---
@@ -215,14 +215,24 @@
.build();
public static final PropertyDescriptor PROP_CONTENT_TYPE = new
PropertyDescriptor.Builder()
-.name("Content-Type")
-.description("The Content-Type to specify for when content is
being transmitted through a PUT or POST. "
-+ "In the case of an empty value after evaluating an
expression language expression, Content-Type defaults to " +
DEFAULT_CONTENT_TYPE)
-.required(true)
-.expressionLanguageSupported(true)
-.defaultValue("${" + CoreAttributes.MIME_TYPE.key() + "}")
-.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
-.build();
+.name("Content-Type")
+.description("The Content-Type to specify for when content is
being transmitted through a PUT or POST. "
++ "In the case of an empty value after evaluating an
expression language expression, Content-Type defaults to " +
DEFAULT_CONTENT_TYPE + "."
++ "If and only if body is not sent, Content-Type must
be set to empty")
+.required(false)
+.expressionLanguageSupported(true)
+.defaultValue("${" + CoreAttributes.MIME_TYPE.key() + "}")
+
.addValidator(StandardValidators.createAttributeExpressionLanguageValidator(AttributeExpression.ResultType.STRING))
+.build();
+
+public static final PropertyDescriptor PROP_SEND_BODY = new
PropertyDescriptor.Builder()
+.name("Send body")
+.description("True if the body content should be sent, false
otherwise")
+.defaultValue("true")
--- End diff --
We should refer to this as the HTTP spec refers to it. It's called the
"message-body" in the spec, so I think we should use that here as well. i.e.
```
.name("send-message-body")
.displayName("Send Message Body")
.description("If true, sends the HTTP message body on POST/PUT requests
(default). If false, suppresses the message body and content-type header for
these requests.")
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user taftster commented on a diff in the pull request:
https://github.com/apache/nifi/pull/272#discussion_r56928549
--- Diff:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
---
@@ -444,14 +455,27 @@ public void onPropertyModified(final
PropertyDescriptor descriptor, final String
@Override
protected Collection customValidate(final
ValidationContext validationContext) {
-final List results = new ArrayList<>(1);
+final List results = new
ArrayList(3);
+
+// proxy host and port validation
final boolean proxyHostSet =
validationContext.getProperty(PROP_PROXY_HOST).isSet();
final boolean proxyPortSet =
validationContext.getProperty(PROP_PROXY_PORT).isSet();
-
if ((proxyHostSet && !proxyPortSet) || (!proxyHostSet &&
proxyPortSet)) {
results.add(new ValidationResult.Builder().subject("Proxy Host
and Port").valid(false).explanation("If Proxy Host or Proxy Port is set, both
must be set").build());
}
+// body and content-type validation
+final boolean contentTypeSet =
validationContext.getProperty(PROP_CONTENT_TYPE).isSet();
+final boolean sendBody =
validationContext.getProperty(PROP_SEND_BODY).asBoolean();
+final String contentType =
validationContext.getProperty(PROP_CONTENT_TYPE).getValue();
+if(contentTypeSet && contentType.isEmpty() && sendBody) {
+results.add(new
ValidationResult.Builder().subject("Content-Type and Send body")
+.valid(false).explanation("If Content-Type is set to
empty, Send body property must be set to false").build());
+}
+if(!sendBody && !contentType.isEmpty()) {
+results.add(new
ValidationResult.Builder().subject("Content-Type and Send
body").valid(false).explanation("If body is not sent, Content-Type must be set
to empty").build());
+}
+
return results;
--- End diff --
This custom validation rule can be removed if the content-type property is
left alone.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user taftster commented on a diff in the pull request:
https://github.com/apache/nifi/pull/272#discussion_r56928543
--- Diff:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
---
@@ -215,14 +215,24 @@
.build();
public static final PropertyDescriptor PROP_CONTENT_TYPE = new
PropertyDescriptor.Builder()
-.name("Content-Type")
-.description("The Content-Type to specify for when content is
being transmitted through a PUT or POST. "
-+ "In the case of an empty value after evaluating an
expression language expression, Content-Type defaults to " +
DEFAULT_CONTENT_TYPE)
-.required(true)
-.expressionLanguageSupported(true)
-.defaultValue("${" + CoreAttributes.MIME_TYPE.key() + "}")
-.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
-.build();
+.name("Content-Type")
+.description("The Content-Type to specify for when content is
being transmitted through a PUT or POST. "
++ "In the case of an empty value after evaluating an
expression language expression, Content-Type defaults to " +
DEFAULT_CONTENT_TYPE + "."
++ "If and only if body is not sent, Content-Type must
be set to empty")
+.required(false)
+.expressionLanguageSupported(true)
+.defaultValue("${" + CoreAttributes.MIME_TYPE.key() + "}")
+
.addValidator(StandardValidators.createAttributeExpressionLanguageValidator(AttributeExpression.ResultType.STRING))
+.build();
+
--- End diff --
I'm not sure if we even need this change. In the case that the
PROP_SEND_BODY is false, we should always suppress setting the Content-Type
header. Therefore, I would leave this as-is a required field, with
documentation in the PROP_SEND_FIELD property describing the behavior for empty
message bodies.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1620 Allow empty Content-Type in InvokeHTT...
Github user pvillard31 commented on the pull request: https://github.com/apache/nifi/pull/272#issuecomment-199714780 @taftster Thanks Adam. I updated the PR regarding your comments. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
