exceptionfactory commented on code in PR #8484:
URL: https://github.com/apache/nifi/pull/8484#discussion_r1546313115
##########
nifi-nar-bundles/nifi-standard-services/nifi-lookup-services-bundle/nifi-lookup-services/src/main/java/org/apache/nifi/lookup/RestLookupService.java:
##########
@@ -251,6 +264,8 @@ public void onEnabled(final ConfigurationContext context) {
buildHeaders(context);
urlTemplate = context.getProperty(URL);
+
+ responseHandler = context.getProperty(PROP_RESPONSE_CODE_HANDLING);
Review Comment:
After changing the member variable type, the `asAllowableValue()` method can
be used to resolve the `enum` value from the property value.
##########
nifi-nar-bundles/nifi-standard-services/nifi-lookup-services-bundle/nifi-lookup-services/src/main/java/org/apache/nifi/lookup/RestLookupService.java:
##########
@@ -214,6 +226,7 @@ protected List<PropertyDescriptor>
getSupportedPropertyDescriptors() {
private volatile String basicUser;
private volatile String basicPass;
private volatile boolean isDigest;
+ private volatile PropertyValue responseHandler;
Review Comment:
This can be changed to use the `ResponseHandlingStrategy` enum:
```suggestion
private volatile ResponseHandlingStrategy responseHandlingStrategy;
```
##########
nifi-nar-bundles/nifi-standard-services/nifi-lookup-services-bundle/nifi-lookup-services/src/main/java/org/apache/nifi/lookup/RestLookupService.java:
##########
@@ -320,13 +335,20 @@ public Optional<Record> lookup(Map<String, Object>
coordinates, Map<String, Stri
Request request = buildRequest(mimeType, method, body, endpoint,
context);
try {
Response response = executeRequest(request);
+ final ResponseBody responseBody = response.body();
if (getLogger().isDebugEnabled()) {
getLogger().debug("Response code {} was returned for
coordinate {}",
new Object[]{response.code(), coordinates});
}
- final ResponseBody responseBody = response.body();
+ if (!response.isSuccessful()
+ &&
ResponseHandlingStrategy.EVALUATED.equals(responseHandler.asAllowableValue(ResponseHandlingStrategy.class)))
{
+ final String responseText = responseBody == null ? "<No
Message Received from Server>" : responseBody.string();
+ throw new IOException("Failed to download content from URL " +
request.url() +
+ ": Response code was " + response.code() + ": " +
responseText);
Review Comment:
Recommend using String formatting instead of concatenation for readability:
```suggestion
throw new IOException("Request failed with HTTP %d for [%s]:
%s".formatted(response.code(), request.url(), responseText));
```
##########
nifi-nar-bundles/nifi-standard-services/nifi-lookup-services-bundle/nifi-lookup-services/src/main/java/org/apache/nifi/lookup/RestLookupService.java:
##########
@@ -320,13 +335,20 @@ public Optional<Record> lookup(Map<String, Object>
coordinates, Map<String, Stri
Request request = buildRequest(mimeType, method, body, endpoint,
context);
try {
Response response = executeRequest(request);
+ final ResponseBody responseBody = response.body();
if (getLogger().isDebugEnabled()) {
getLogger().debug("Response code {} was returned for
coordinate {}",
new Object[]{response.code(), coordinates});
}
- final ResponseBody responseBody = response.body();
+ if (!response.isSuccessful()
+ &&
ResponseHandlingStrategy.EVALUATED.equals(responseHandler.asAllowableValue(ResponseHandlingStrategy.class)))
{
Review Comment:
After changing the member variable, this comparison can be simplified.
##########
nifi-nar-bundles/nifi-standard-services/nifi-lookup-services-bundle/nifi-lookup-services/src/main/java/org/apache/nifi/lookup/RestLookupService.java:
##########
@@ -172,6 +172,17 @@ public class RestLookupService extends
AbstractControllerService implements Reco
.addValidator(StandardValidators.TIME_PERIOD_VALIDATOR)
.build();
+
+ public static final PropertyDescriptor PROP_RESPONSE_CODE_HANDLING = new
PropertyDescriptor.Builder()
Review Comment:
Recommend removing the `PROP_` prefix:
```suggestion
public static final PropertyDescriptor RESPONSE_HANDLING_STRATEGY = new
PropertyDescriptor.Builder()
```
##########
nifi-nar-bundles/nifi-standard-services/nifi-lookup-services-bundle/nifi-lookup-services/src/main/java/org/apache/nifi/lookup/RestLookupService.java:
##########
@@ -320,13 +335,20 @@ public Optional<Record> lookup(Map<String, Object>
coordinates, Map<String, Stri
Request request = buildRequest(mimeType, method, body, endpoint,
context);
try {
Response response = executeRequest(request);
+ final ResponseBody responseBody = response.body();
if (getLogger().isDebugEnabled()) {
getLogger().debug("Response code {} was returned for
coordinate {}",
new Object[]{response.code(), coordinates});
}
- final ResponseBody responseBody = response.body();
+ if (!response.isSuccessful()
+ &&
ResponseHandlingStrategy.EVALUATED.equals(responseHandler.asAllowableValue(ResponseHandlingStrategy.class)))
{
+ final String responseText = responseBody == null ? "<No
Message Received from Server>" : responseBody.string();
Review Comment:
Recommend replacing angle brackets with square brackets in the placeholder
message:
```suggestion
final String responseText = responseBody == null ? "[No
Message Received]" : responseBody.string();
```
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
