smolnar82 commented on code in PR #594:
URL: https://github.com/apache/knox/pull/594#discussion_r898334503
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -56,17 +56,23 @@ public void contributeFilter( DeploymentContext context,
Provider provider, Serv
Map<String, String> providerParams = provider.getParams();
String providerNames = providerParams.get("composite.provider.names");
+ if (!providerNames.isEmpty()) {
Review Comment:
I'd use StringUtils over String here because the first one is `null` safe.
Even better, I think `parseProviderNames` should return a collection and not a
String array.
##########
gateway-provider-security-authz-composite/src/test/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzProviderTest.java:
##########
@@ -64,4 +64,16 @@ public void testParsingProviderNames() throws Exception {
assertEquals(providerNames[1], "SomeOther");
assertEquals(providerNames[2], "TheOtherOne");
}
+
+ @Test
+ public void testingParsingProviderNames() throws Exception {
+ String testnames = " AclsAuthz , SomeOther , TheOtherOne ,";
Review Comment:
I'd modify this sample string to cover all cases:
- whitespace just before the name
- whitespace just after the name
- whitespace before and after the name
- more whitespaces
- no whitespace at all
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -56,17 +56,23 @@ public void contributeFilter( DeploymentContext context,
Provider provider, Serv
Map<String, String> providerParams = provider.getParams();
String providerNames = providerParams.get("composite.provider.names");
+ if (!providerNames.isEmpty()) {
String[] names = parseProviderNames(providerNames);
for (String name : names) {
getProviderSpecificParams(resource, params, providerParams, name);
DeploymentFactory.getProviderContributor("authorization", name)
- .contributeFilter(context, provider, service, resource, params);
+ .contributeFilter(context, provider, service, resource, params);
params.clear();
}
+ }
}
String[] parseProviderNames(String providerNames) {
- return providerNames.split(",\\s*");
+ String[] b = providerNames.split("\\s*,\\s*");
Review Comment:
Please avoid using parameter names like `b`. It's just against clean code
principles.
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -56,17 +56,23 @@ public void contributeFilter( DeploymentContext context,
Provider provider, Serv
Map<String, String> providerParams = provider.getParams();
String providerNames = providerParams.get("composite.provider.names");
+ if (!providerNames.isEmpty()) {
String[] names = parseProviderNames(providerNames);
for (String name : names) {
getProviderSpecificParams(resource, params, providerParams, name);
DeploymentFactory.getProviderContributor("authorization", name)
- .contributeFilter(context, provider, service, resource, params);
+ .contributeFilter(context, provider, service, resource, params);
params.clear();
}
+ }
Review Comment:
This additional `if` statement modifies the indentation here so that content
within that condition should be intended more.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
