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:
us...@infra.apache.org
