moresandeep commented on a change in pull request #423:
URL: https://github.com/apache/knox/pull/423#discussion_r605672939
##########
File path:
gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/AbstractGatewayDispatch.java
##########
@@ -132,12 +134,22 @@ public void copyRequestHeaderFields(HttpUriRequest
outboundRequest,
outboundRequest.addHeader( name, value );
}
}
+
+ //If there are some headers to be appended, append them
+ Map<String, String> extraHeaders = getOutboundRequestAppendHeaders();
+ if(null != extraHeaders){
+ extraHeaders.keySet().forEach(header ->
outboundRequest.addHeader(header, extraHeaders.get(header)));
Review comment:
you can simplify this
`extraHeaders.forEach(header, value -> outboundRequest.addHeader(header,
value));`
or better
`outboundRequest.putAll(extraHeaders)`
##########
File path:
gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/AbstractGatewayDispatch.java
##########
@@ -132,12 +134,22 @@ public void copyRequestHeaderFields(HttpUriRequest
outboundRequest,
outboundRequest.addHeader( name, value );
}
}
+
+ //If there are some headers to be appended, append them
+ Map<String, String> extraHeaders = getOutboundRequestAppendHeaders();
Review comment:
Can we pull this logic down to ConfigurableDispatch class. Makes sense
to keep configurations in ConfigurableDispatch and keep abstract dispatch bare
min.
##########
File path: gateway-spi/pom.xml
##########
@@ -170,5 +170,14 @@
<artifactId>velocity</artifactId>
<scope>test</scope>
</dependency>
+
+ <dependency>
Review comment:
nit: can we move this above, on line #167, the unspoken format is to
have test dependencies at the end.
##########
File path:
gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/ConfigurableDispatch.java
##########
@@ -41,13 +47,26 @@
public class ConfigurableDispatch extends DefaultDispatch {
private Set<String> requestExcludeHeaders =
super.getOutboundRequestExcludeHeaders();
private Set<String> responseExcludeHeaders =
super.getOutboundResponseExcludeHeaders();
+ private Map<String, String> requestAppendHeaders = new HashMap<>();
private Set<String> responseExcludeSetCookieHeaderDirectives =
super.getOutboundResponseExcludedSetCookieHeaderDirectives();
private Boolean removeUrlEncoding = false;
private Set<String> convertCommaDelimitedHeadersToSet(String headers) {
return headers == null ? Collections.emptySet(): new
HashSet<>(Arrays.asList(headers.split("\\s*,\\s*")));
}
+ private Map<String, String> convertJSONHeadersToMap(String headers) {
Review comment:
Putting JSON in XML looks wrong, we should follow the same pattern here
as we follow elsewhere to extract parameters from service.xml file. I think
NiFi dispatch does something like that with twoWaySSL.
##########
File path:
gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/ConfigurableDispatch.java
##########
@@ -41,13 +47,26 @@
public class ConfigurableDispatch extends DefaultDispatch {
private Set<String> requestExcludeHeaders =
super.getOutboundRequestExcludeHeaders();
private Set<String> responseExcludeHeaders =
super.getOutboundResponseExcludeHeaders();
+ private Map<String, String> requestAppendHeaders = new HashMap<>();
Review comment:
Collections.emptyMap()?
##########
File path:
gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/AbstractGatewayDispatch.java
##########
@@ -132,12 +134,22 @@ public void copyRequestHeaderFields(HttpUriRequest
outboundRequest,
outboundRequest.addHeader( name, value );
}
}
+
+ //If there are some headers to be appended, append them
+ Map<String, String> extraHeaders = getOutboundRequestAppendHeaders();
+ if(null != extraHeaders){
Review comment:
We should check for empty as well, something like
[CollectionUtils.isEmpty
](https://commons.apache.org/proper/commons-collections/apidocs/org/apache/commons/collections4/CollectionUtils.html#isEmpty-java.util.Collection-)
--
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:
[email protected]
