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 2021/03/12 04:48:48 UTC
[GitHub] [nifi] exceptionfactory opened a new pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
exceptionfactory opened a new pull request #4892:
URL: https://github.com/apache/nifi/pull/4892
#### Description of PR
NIFI-8304 Refactors unit tests for `InvokeHTTP` to a single class implemented using OkHttp MockWebServer. This approach removes the duplicative execution of multiple unit test classes for TLS communication and includes both HTTP and HTTPS unit tests in a single class. MockWebServer replaces the test instance of Jetty Server, reducing the amount of code needed to prepare various types of HTTP responses. Changes to `InvokeHTTP` include code cleanup for several unused properties as well as replacing Joda Time with `java.time` for date formatting and replacing Guava `Files` with `java.nio.file.Files` for creating the ETag cache directory. Additional changes include updates to `PutTCP` unit tests, lengthening the timeout for testing the connection per FlowFile property and also closing connection sockets per message for that particular unit test method.
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 **NIFI-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? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [X] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [X] Have you written or updated unit tests to verify your changes?
- [X] Have you verified that the full build is successful on JDK 8?
- [X] Have you verified that the full build is successful on JDK 11?
- [ ] 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, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### 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 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#discussion_r593394000
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -839,22 +822,14 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
final int maxAttributeSize = context.getProperty(PROP_PUT_ATTRIBUTE_MAX_LENGTH).asInteger();
final ComponentLog logger = getLogger();
- // log ETag cache metrics
- final boolean eTagEnabled = context.getProperty(PROP_USE_ETAG).asBoolean();
- if (eTagEnabled && logger.isDebugEnabled()) {
- final Cache cache = okHttpClient.cache();
- logger.debug("OkHttp ETag cache metrics :: Request Count: {} | Network Count: {} | Hit Count: {}",
- new Object[]{cache.requestCount(), cache.networkCount(), cache.hitCount()});
- }
-
Review comment:
This block of code is difficult to unit test since it relies on enabling debug logging. Although it might be interesting for some specific scenarios, it seems like it would be better to rely on enabling logging for OkHttp itself as opposed to logging cache metrics using this approach.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#discussion_r593391659
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -558,24 +553,19 @@
public static final Set<Relationship> RELATIONSHIPS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
REL_SUCCESS_REQ, REL_RESPONSE, REL_RETRY, REL_NO_RETRY, REL_FAILURE)));
- private volatile Set<String> dynamicPropertyNames = new HashSet<>();
+ // RFC 2616 Date Time Formatter with hard-coded GMT Zone
+ private static final DateTimeFormatter RFC_2616_DATE_TIME = DateTimeFormatter.ofPattern("EEE, dd MMM yyyy HH:mm:ss 'GMT'");
- /**
- * Pattern used to compute RFC 2616 Dates (#sec3.3.1). This format is used by the HTTP Date header and is optionally sent by the processor. This date is effectively an RFC 822/1123 date
- * string, but HTTP requires it to be in GMT (preferring the literal 'GMT' string).
- */
- private static final String RFC_1123 = "EEE, dd MMM yyyy HH:mm:ss 'GMT'";
- private static final DateTimeFormatter DATE_FORMAT = DateTimeFormat.forPattern(RFC_1123).withLocale(Locale.US).withZoneUTC();
+ // Multiple Header Delimiter
+ private static final String MULTIPLE_HEADER_DELIMITER = ", ";
- private final AtomicReference<OkHttpClient> okHttpClientAtomicReference = new AtomicReference<>();
+ private volatile Set<String> dynamicPropertyNames = new HashSet<>();
- @Override
- protected void init(ProcessorInitializationContext context) {
- excludedHeaders.put("Trusted Hostname", "HTTP request header '{}' excluded. " +
- "Update processor to use the SSLContextService instead. " +
- "See the Access Policies section in the System Administrator's Guide.");
+ private volatile Pattern regexAttributesToSend = null;
- }
Review comment:
Understanding that this was intended to catch previous uses, sending an HTTP Header named `Trusted Hostname` will have no impact on the received HTTP server unless that server was explicitly configured to look for that property. Since it is a non-standard HTTP Header, and it is up to the receiving system to do something with it, performing the check in this processor seems unnecessary.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#discussion_r593397403
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -1132,31 +1097,25 @@ public long contentLength() {
} else if (sendBody) {
return requestBody;
}
- return RequestBody.create(null, new byte[0]);
+ return RequestBody.create(new byte[0], null);
}
- private Request.Builder setHeaderProperties(final ProcessContext context, Request.Builder requestBuilder, final FlowFile requestFlowFile) {
+ private void setHeaderProperties(final ProcessContext context, final Request.Builder requestBuilder, final FlowFile requestFlowFile) {
// check if we should send the a Date header with the request
if (context.getProperty(PROP_DATE_HEADER).asBoolean()) {
- requestBuilder = requestBuilder.addHeader("Date", DATE_FORMAT.print(System.currentTimeMillis()));
+ final ZonedDateTime universalCoordinatedTimeNow = ZonedDateTime.now(ZoneOffset.UTC);
+ requestBuilder.addHeader("Date", RFC_2616_DATE_TIME.format(universalCoordinatedTimeNow));
}
- final ComponentLog logger = getLogger();
for (String headerKey : dynamicPropertyNames) {
String headerValue = context.getProperty(headerKey).evaluateAttributeExpressions(requestFlowFile).getValue();
- // don't include any of the excluded headers, log instead
- if (excludedHeaders.containsKey(headerKey)) {
- logger.warn(excludedHeaders.get(headerKey), new Object[]{headerKey});
- continue;
- }
-
Review comment:
This goes along with the comment regarding `Trusted Hostname`. The log message does not contain much useful details, and as any use of the custom `Trusted Hostname` header depends on the handling of the receiving HTTP server, I recommend removing this block.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#issuecomment-797705226
> Left a few comments after very quickly going through it. I'd definitely like to have a few people looking into these changes as this processor is one of the most widely used.
Thanks for the detailed feedback and questions @pvillard31. I am open to reverting some of the changes to `InvokeHTTP`, but hopefully the answers on particular changes explain why it seemed to make sense to include the adjustments. The new `InvokeHTTPTest` attempts to cover a wide array of use cases, which also helped expose some of the limited use and dead code lines in `InvokeHTTP`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#discussion_r593391659
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -558,24 +553,19 @@
public static final Set<Relationship> RELATIONSHIPS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
REL_SUCCESS_REQ, REL_RESPONSE, REL_RETRY, REL_NO_RETRY, REL_FAILURE)));
- private volatile Set<String> dynamicPropertyNames = new HashSet<>();
+ // RFC 2616 Date Time Formatter with hard-coded GMT Zone
+ private static final DateTimeFormatter RFC_2616_DATE_TIME = DateTimeFormatter.ofPattern("EEE, dd MMM yyyy HH:mm:ss 'GMT'");
- /**
- * Pattern used to compute RFC 2616 Dates (#sec3.3.1). This format is used by the HTTP Date header and is optionally sent by the processor. This date is effectively an RFC 822/1123 date
- * string, but HTTP requires it to be in GMT (preferring the literal 'GMT' string).
- */
- private static final String RFC_1123 = "EEE, dd MMM yyyy HH:mm:ss 'GMT'";
- private static final DateTimeFormatter DATE_FORMAT = DateTimeFormat.forPattern(RFC_1123).withLocale(Locale.US).withZoneUTC();
+ // Multiple Header Delimiter
+ private static final String MULTIPLE_HEADER_DELIMITER = ", ";
- private final AtomicReference<OkHttpClient> okHttpClientAtomicReference = new AtomicReference<>();
+ private volatile Set<String> dynamicPropertyNames = new HashSet<>();
- @Override
- protected void init(ProcessorInitializationContext context) {
- excludedHeaders.put("Trusted Hostname", "HTTP request header '{}' excluded. " +
- "Update processor to use the SSLContextService instead. " +
- "See the Access Policies section in the System Administrator's Guide.");
+ private volatile Pattern regexAttributesToSend = null;
- }
Review comment:
Understanding that this was intended to catch previous uses, sending an HTTP Header named `Trusted Hostname` will no impact on the received HTTP server unless that server was explicitly configured to look for that property. Since it is a non-standard HTTP Header, and it is up to the receiving system to do something with it, performing the check in this processor seems unnecessary.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#discussion_r593395390
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -876,10 +851,6 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
int statusCode = responseHttp.code();
String statusMessage = responseHttp.message();
- if (statusCode == 0) {
- throw new IllegalStateException("Status code unknown, connection hasn't been attempted.");
- }
-
Review comment:
The OkHttp client Response Builder class sets the default value to `-1` so a value of `0` should not be possible. Perhaps this reflected an issue with earlier versions of the OkHttp client, but with the version 4 series, this should not happen.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#discussion_r593392978
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -674,20 +661,11 @@ public void onPropertyModified(final PropertyDescriptor descriptor, final String
ProxyConfiguration.validateProxySpec(validationContext, results, PROXY_SPECS);
- for (String headerKey : validationContext.getProperties().values()) {
- if (excludedHeaders.containsKey(headerKey)) {
- // We're not using the header message format string here, just this
- // static validation message string:
- results.add(new ValidationResult.Builder().subject(headerKey).valid(false).explanation("Matches excluded HTTP header name").build());
- }
- }
-
Review comment:
This validation block was related to the `excludedHeaders` check. However, the way this was written, it was not actually working properly since `getProperties().values()` returns property _values_ instead of property _names_. If the check for `Trusted Hostname` is absolutely necessary, this could be corrected, but I recommend removing all references.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#issuecomment-797507254
> Thanks for the refactoring @exceptionfactory - I still see some flaky tests with Java 11 build, is that something that should be addressed by this PR or would it be a follow up work?
Thanks for taking a look at this @pvillard31! The two tests updated in this PR were ones that were failing more regularly, but I see TestListenHTTP failed here for JDK 11. I will take a look at that test as well to see if I can improve that one as well to get a clean build.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#discussion_r593398895
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -1339,26 +1261,7 @@ private Charset getCharsetFromMediaType(MediaType contentType) {
*
* @return the directory in which the ETag cache should be written
*/
- private static File getETagCacheDir() {
- return Files.createTempDir();
- }
-
- private static class OverrideHostnameVerifier implements HostnameVerifier {
-
- private final String trustedHostname;
- private final HostnameVerifier delegate;
-
- private OverrideHostnameVerifier(String trustedHostname, HostnameVerifier delegate) {
- this.trustedHostname = trustedHostname;
- this.delegate = delegate;
- }
-
- @Override
- public boolean verify(String hostname, SSLSession session) {
- if (trustedHostname.equalsIgnoreCase(hostname)) {
- return true;
- }
- return delegate.verify(hostname, session);
- }
+ private static File getETagCacheDir() throws IOException {
+ return Files.createTempDirectory(InvokeHTTP.class.getSimpleName()).toFile();
Review comment:
This method is called in the `setUpClient()` method which is annotated with `OnScheduled`. This previous implementation did the same thing using Guava's Files class. The approach seems acceptable since it is called when the OkHttpClient is being configured.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#issuecomment-799579067
Changes look good to me. I think it does a good job of simplifying the unit tests and cleans up some code that I agree is not really needed. +1 from me.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#issuecomment-799605228
Merged to main, thanks @exceptionfactory
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on a change in pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on a change in pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#discussion_r593387260
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -1012,63 +981,59 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
// cleanup response flowfile, if applicable
- try {
- if (responseFlowFile != null) {
- session.remove(responseFlowFile);
- }
- } catch (final Exception e1) {
- logger.error("Could not cleanup response flowfile due to exception: {}", new Object[]{e1}, e1);
Review comment:
Is it safe to remove the try/catch?
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -839,22 +822,14 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
final int maxAttributeSize = context.getProperty(PROP_PUT_ATTRIBUTE_MAX_LENGTH).asInteger();
final ComponentLog logger = getLogger();
- // log ETag cache metrics
- final boolean eTagEnabled = context.getProperty(PROP_USE_ETAG).asBoolean();
- if (eTagEnabled && logger.isDebugEnabled()) {
- final Cache cache = okHttpClient.cache();
- logger.debug("OkHttp ETag cache metrics :: Request Count: {} | Network Count: {} | Hit Count: {}",
- new Object[]{cache.requestCount(), cache.networkCount(), cache.hitCount()});
- }
-
Review comment:
Not a strong opinion on this, but shouldn't we keep this?
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -876,10 +851,6 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
int statusCode = responseHttp.code();
String statusMessage = responseHttp.message();
- if (statusCode == 0) {
- throw new IllegalStateException("Status code unknown, connection hasn't been attempted.");
- }
-
Review comment:
Not sure why but I imagine this was here for some reasons. Do we think this is safe to remove?
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -1132,31 +1097,25 @@ public long contentLength() {
} else if (sendBody) {
return requestBody;
}
- return RequestBody.create(null, new byte[0]);
+ return RequestBody.create(new byte[0], null);
}
- private Request.Builder setHeaderProperties(final ProcessContext context, Request.Builder requestBuilder, final FlowFile requestFlowFile) {
+ private void setHeaderProperties(final ProcessContext context, final Request.Builder requestBuilder, final FlowFile requestFlowFile) {
// check if we should send the a Date header with the request
if (context.getProperty(PROP_DATE_HEADER).asBoolean()) {
- requestBuilder = requestBuilder.addHeader("Date", DATE_FORMAT.print(System.currentTimeMillis()));
+ final ZonedDateTime universalCoordinatedTimeNow = ZonedDateTime.now(ZoneOffset.UTC);
+ requestBuilder.addHeader("Date", RFC_2616_DATE_TIME.format(universalCoordinatedTimeNow));
}
- final ComponentLog logger = getLogger();
for (String headerKey : dynamicPropertyNames) {
String headerValue = context.getProperty(headerKey).evaluateAttributeExpressions(requestFlowFile).getValue();
- // don't include any of the excluded headers, log instead
- if (excludedHeaders.containsKey(headerKey)) {
- logger.warn(excludedHeaders.get(headerKey), new Object[]{headerKey});
- continue;
- }
-
Review comment:
do we need/want to remove this?
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -558,24 +553,19 @@
public static final Set<Relationship> RELATIONSHIPS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
REL_SUCCESS_REQ, REL_RESPONSE, REL_RETRY, REL_NO_RETRY, REL_FAILURE)));
- private volatile Set<String> dynamicPropertyNames = new HashSet<>();
+ // RFC 2616 Date Time Formatter with hard-coded GMT Zone
+ private static final DateTimeFormatter RFC_2616_DATE_TIME = DateTimeFormatter.ofPattern("EEE, dd MMM yyyy HH:mm:ss 'GMT'");
- /**
- * Pattern used to compute RFC 2616 Dates (#sec3.3.1). This format is used by the HTTP Date header and is optionally sent by the processor. This date is effectively an RFC 822/1123 date
- * string, but HTTP requires it to be in GMT (preferring the literal 'GMT' string).
- */
- private static final String RFC_1123 = "EEE, dd MMM yyyy HH:mm:ss 'GMT'";
- private static final DateTimeFormatter DATE_FORMAT = DateTimeFormat.forPattern(RFC_1123).withLocale(Locale.US).withZoneUTC();
+ // Multiple Header Delimiter
+ private static final String MULTIPLE_HEADER_DELIMITER = ", ";
- private final AtomicReference<OkHttpClient> okHttpClientAtomicReference = new AtomicReference<>();
+ private volatile Set<String> dynamicPropertyNames = new HashSet<>();
- @Override
- protected void init(ProcessorInitializationContext context) {
- excludedHeaders.put("Trusted Hostname", "HTTP request header '{}' excluded. " +
- "Update processor to use the SSLContextService instead. " +
- "See the Access Policies section in the System Administrator's Guide.");
+ private volatile Pattern regexAttributesToSend = null;
- }
Review comment:
I think this was on purpose to block the use of a previous existing property in the processor
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -558,24 +553,19 @@
public static final Set<Relationship> RELATIONSHIPS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
REL_SUCCESS_REQ, REL_RESPONSE, REL_RETRY, REL_NO_RETRY, REL_FAILURE)));
- private volatile Set<String> dynamicPropertyNames = new HashSet<>();
+ // RFC 2616 Date Time Formatter with hard-coded GMT Zone
+ private static final DateTimeFormatter RFC_2616_DATE_TIME = DateTimeFormatter.ofPattern("EEE, dd MMM yyyy HH:mm:ss 'GMT'");
- /**
- * Pattern used to compute RFC 2616 Dates (#sec3.3.1). This format is used by the HTTP Date header and is optionally sent by the processor. This date is effectively an RFC 822/1123 date
- * string, but HTTP requires it to be in GMT (preferring the literal 'GMT' string).
- */
- private static final String RFC_1123 = "EEE, dd MMM yyyy HH:mm:ss 'GMT'";
- private static final DateTimeFormatter DATE_FORMAT = DateTimeFormat.forPattern(RFC_1123).withLocale(Locale.US).withZoneUTC();
+ // Multiple Header Delimiter
+ private static final String MULTIPLE_HEADER_DELIMITER = ", ";
- private final AtomicReference<OkHttpClient> okHttpClientAtomicReference = new AtomicReference<>();
+ private volatile Set<String> dynamicPropertyNames = new HashSet<>();
- @Override
- protected void init(ProcessorInitializationContext context) {
- excludedHeaders.put("Trusted Hostname", "HTTP request header '{}' excluded. " +
- "Update processor to use the SSLContextService instead. " +
- "See the Access Policies section in the System Administrator's Guide.");
+ private volatile Pattern regexAttributesToSend = null;
- }
Review comment:
I think this was on purpose to block the use of a previous existing property in the processor
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -674,20 +661,11 @@ public void onPropertyModified(final PropertyDescriptor descriptor, final String
ProxyConfiguration.validateProxySpec(validationContext, results, PROXY_SPECS);
- for (String headerKey : validationContext.getProperties().values()) {
- if (excludedHeaders.containsKey(headerKey)) {
- // We're not using the header message format string here, just this
- // static validation message string:
- results.add(new ValidationResult.Builder().subject(headerKey).valid(false).explanation("Matches excluded HTTP header name").build());
- }
- }
-
Review comment:
is it some dead code?
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -1339,26 +1261,7 @@ private Charset getCharsetFromMediaType(MediaType contentType) {
*
* @return the directory in which the ETag cache should be written
*/
- private static File getETagCacheDir() {
- return Files.createTempDir();
- }
-
- private static class OverrideHostnameVerifier implements HostnameVerifier {
-
- private final String trustedHostname;
- private final HostnameVerifier delegate;
-
- private OverrideHostnameVerifier(String trustedHostname, HostnameVerifier delegate) {
- this.trustedHostname = trustedHostname;
- this.delegate = delegate;
- }
-
- @Override
- public boolean verify(String hostname, SSLSession session) {
- if (trustedHostname.equalsIgnoreCase(hostname)) {
- return true;
- }
- return delegate.verify(hostname, session);
- }
+ private static File getETagCacheDir() throws IOException {
+ return Files.createTempDirectory(InvokeHTTP.class.getSimpleName()).toFile();
Review comment:
Could it cause an issue in case of concurrent tasks?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#discussion_r593396421
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -1012,63 +981,59 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
// cleanup response flowfile, if applicable
- try {
- if (responseFlowFile != null) {
- session.remove(responseFlowFile);
- }
- } catch (final Exception e1) {
- logger.error("Could not cleanup response flowfile due to exception: {}", new Object[]{e1}, e1);
Review comment:
Since this try-catch block is wrapping a framework-level call to `session.remove()`, it seems unnecessary. The `responseFlowFile` can be null, which is being checked. However, if there is some other problem with the value of `responseFlowFile` at this point, that more likely points to a problem in the processor itself, should be corrected instead of just logged as an error.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#issuecomment-797316796
Thanks for the refactoring @exceptionfactory - I still see some flaky tests with Java 11 build, is that something that should be addressed by this PR or would it be a follow up work?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] asfgit closed pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4892:
URL: https://github.com/apache/nifi/pull/4892
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#issuecomment-799581475
> Changes look good to me. I think it does a good job of simplifying the unit tests and cleans up some code that I agree is not really needed. +1 from me.
Thanks for the feedback @markap14!
Please let me know if you have any additional questions @pvillard31.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #4892: NIFI-8304 Refactored InvokeHTTP unit tests using OkHttp MockWebServer
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #4892:
URL: https://github.com/apache/nifi/pull/4892#issuecomment-797646699
@pvillard31 That last two GitHub CI runs completed successfully for JDK 11 as well as other versions, so this is ready for another review.
----------------------------------------------------------------
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:
users@infra.apache.org