smolnar82 commented on code in PR #841:
URL: https://github.com/apache/knox/pull/841#discussion_r1495847968
##########
gateway-spi/src/test/java/org/apache/knox/gateway/dispatch/DefaultHttpClientFactoryTest.java:
##########
@@ -209,6 +210,7 @@ public void testHttpClientPathNormalization() {
GatewayConfig gatewayConfig = createMock(GatewayConfig.class);
expect(gatewayConfig.getHttpClientConnectionTimeout()).andReturn(20000).once();
expect(gatewayConfig.getHttpClientSocketTimeout()).andReturn(20000).once();
+
expect(gatewayConfig.getHttpClientCookieSpec()).andReturn("standard").anyTimes();
Review Comment:
I'd use org.apache.http.client.config.CookieSpecs.STANDARD here.
##########
gateway-spi/src/test/java/org/apache/knox/gateway/dispatch/DefaultHttpClientFactoryTest.java:
##########
@@ -61,6 +61,7 @@ public void testCreateHttpClientSSLContextDefaults() throws
Exception {
expect(gatewayConfig.getHttpClientMaxConnections()).andReturn(32).once();
expect(gatewayConfig.getHttpClientConnectionTimeout()).andReturn(20000).once();
expect(gatewayConfig.getHttpClientSocketTimeout()).andReturn(20000).once();
+
expect(gatewayConfig.getHttpClientCookieSpec()).andReturn("standard").anyTimes();
Review Comment:
I'd use `org.apache.http.client.config.CookieSpecs.STANDARD` here.
##########
gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/DefaultHttpClientFactory.java:
##########
@@ -369,4 +374,12 @@ private static long parseTimeout( String s ) {
Period p = Period.parse( s, f );
return p.toStandardDuration().getMillis();
}
+
+ private static String getCookieSpec(FilterConfig filterConfig) {
+ GatewayConfig globalConfig =
+
(GatewayConfig)filterConfig.getServletContext().getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE);
+ return globalConfig != null
Review Comment:
This will always return use the gateway-level value (very likely `null`)
even if `httpclient.cookieSpec` is set on the topology level. Based on the
other helper methods here (`getMaxConnections`, `getConnectionTimeout`,
`getSocketTimeout`,...) you should change this to check if
`httpclient.cookieSpec` is set in the topology and default to the GatewayConfig
value otherwise.
You may also want to refactor that code out to some private method to avoid
boilerplate private methods.
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -1565,4 +1566,9 @@ public int getTokenMigrationProgressCount() {
return getInt(TOKEN_MIGRATION_PROGRESS_COUNT, 10);
}
+ @Override
+ public String getHttpClientCookieSpec() {
+ return get(HTTP_CLIENT_COOKIE_SPEC, null);
Review Comment:
The default value `null` is redundant here.
##########
gateway-spi/src/test/java/org/apache/knox/gateway/dispatch/DefaultHttpClientFactoryTest.java:
##########
@@ -246,6 +248,7 @@ public void testHttpRetriesValues() throws Exception {
expect(gatewayConfig.getHttpClientMaxConnections()).andReturn(32).anyTimes();
expect(gatewayConfig.getHttpClientConnectionTimeout()).andReturn(20000).anyTimes();
expect(gatewayConfig.getHttpClientSocketTimeout()).andReturn(20000).anyTimes();
+
expect(gatewayConfig.getHttpClientCookieSpec()).andReturn("standard").anyTimes();
Review Comment:
I'd use org.apache.http.client.config.CookieSpecs.STANDARD here.
--
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: dev-unsubscr...@knox.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
