You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2022/06/20 21:53:28 UTC
[GitHub] [knox] harshiljhaveri opened a new pull request, #597: KNOX-2762 Bug fixes for spaces around delimiters with all reviewed comments addressed
harshiljhaveri opened a new pull request, #597:
URL: https://github.com/apache/knox/pull/597
## What changes were proposed in this pull request?
Fix for NullPointer Exception arising due to whitespaces around delimiters in Composite Auth Provider Names. All review comments addressed.
## How was this patch tested?
Comprehensive Unit tests used to cover most possible test cases. Also manually tested.
[Knox Contributing Process](https://cwiki.apache.org/confluence/display/KNOX/Contribution+Process#ContributionProcess-GithubWorkflow) reviewed.
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] zeroflag commented on a diff in pull request #597: KNOX-2762 Bug fixes for spaces around delimiters with all reviewed comments addressed
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #597:
URL: https://github.com/apache/knox/pull/597#discussion_r905828331
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -58,7 +59,7 @@ public void contributeFilter( DeploymentContext context, Provider provider, Serv
Map<String, String> providerParams = provider.getParams();
String providerNames = providerParams.get("composite.provider.names");
- if (!providerNames.isEmpty()) {
+ if (!StringUtils.isEmpty(providerNames)) {
Review Comment:
Good job and thank you for the changes @harshiljhaveri. One last thing is that this conditional seems to be unnecessary. The `parseProviderNames` returns an empty list if the parameter is blank so the for loop won't do anything in that case. I would just remove this `if` statement and rely on that.
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep merged pull request #597: KNOX-2762 Bug fixes for spaces around delimiters with all reviewed comments addressed
Posted by GitBox <gi...@apache.org>.
moresandeep merged PR #597:
URL: https://github.com/apache/knox/pull/597
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a diff in pull request #597: KNOX-2762 Bug fixes for spaces around delimiters with all reviewed comments addressed
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #597:
URL: https://github.com/apache/knox/pull/597#discussion_r904699792
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -56,23 +59,24 @@ 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);
- params.clear();
- }
+ if (!StringUtils.isEmpty(providerNames)) {
+ List<String> names = parseProviderNames(providerNames);
+ for (String name : names) {
+ getProviderSpecificParams(resource, params, providerParams, name);
+ DeploymentFactory.getProviderContributor("authorization", name)
+ .contributeFilter(context, provider, service, resource, params);
+ params.clear();
+ }
}
}
- String[] parseProviderNames(String providerNames) {
- String[] b = providerNames.split("\\s*,\\s*");
- for (int i = 0; i < b.length; i++) {
- b[i] = b[i].trim();
+ List parseProviderNames(String providerNames) {
+ if (providerNames==null || StringUtils.isEmpty(providerNames) || StringUtils.isBlank(providerNames)){
Review Comment:
`StringUtils.isBlank(providerNames)` is enough here. It is `null safe` and will also cover `StringUtils.isEmpty(providerNames)`.
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a diff in pull request #597: KNOX-2762 Bug fixes for spaces around delimiters with all reviewed comments addressed
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #597:
URL: https://github.com/apache/knox/pull/597#discussion_r905326309
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -71,7 +71,7 @@ public void contributeFilter( DeploymentContext context, Provider provider, Serv
}
List parseProviderNames(String providerNames) {
- if (providerNames==null || StringUtils.isEmpty(providerNames) || StringUtils.isBlank(providerNames)){
+ if (providerNames==null || StringUtils.isBlank(providerNames)){
Review Comment:
As I explained earlier, `StringUtils.isBlank(providerNames)` is enough here. It's null-safe.
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a diff in pull request #597: KNOX-2762 Bug fixes for spaces around delimiters with all reviewed comments addressed
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #597:
URL: https://github.com/apache/knox/pull/597#discussion_r902941720
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -57,22 +59,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);
- params.clear();
- }
+ List<String> names = parseProviderNames(providerNames);
+ for (String name : names) {
+ getProviderSpecificParams(resource, params, providerParams, name);
+ DeploymentFactory.getProviderContributor("authorization", name)
+ .contributeFilter(context, provider, service, resource, params);
+ params.clear();
+ }
}
}
- String[] parseProviderNames(String providerNames) {
- String[] b = providerNames.split("\\s*,\\s*");
- for (int i = 0; i < b.length; i++) {
- b[i] = b[i].trim();
+ List parseProviderNames(String providerNames) {
+ if (providerNames==null){
Review Comment:
nit: formatting
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -57,22 +59,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);
- params.clear();
- }
+ List<String> names = parseProviderNames(providerNames);
+ for (String name : names) {
+ getProviderSpecificParams(resource, params, providerParams, name);
+ DeploymentFactory.getProviderContributor("authorization", name)
+ .contributeFilter(context, provider, service, resource, params);
+ params.clear();
+ }
}
}
- String[] parseProviderNames(String providerNames) {
- String[] b = providerNames.split("\\s*,\\s*");
- for (int i = 0; i < b.length; i++) {
- b[i] = b[i].trim();
+ List parseProviderNames(String providerNames) {
+ if (providerNames==null){
+ return Collections.singletonList("");
}
- return b;
+ List<String> providerNamesList = Arrays.asList(providerNames.split("\\s*,\\s*"));
+ providerNamesList.replaceAll(String::trim);
Review Comment:
Cool; I like this change.
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a diff in pull request #597: KNOX-2762 Bug fixes for spaces around delimiters with all reviewed comments addressed
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #597:
URL: https://github.com/apache/knox/pull/597#discussion_r902944796
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -57,22 +59,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);
- params.clear();
- }
+ List<String> names = parseProviderNames(providerNames);
+ for (String name : names) {
+ getProviderSpecificParams(resource, params, providerParams, name);
+ DeploymentFactory.getProviderContributor("authorization", name)
+ .contributeFilter(context, provider, service, resource, params);
+ params.clear();
+ }
}
}
- String[] parseProviderNames(String providerNames) {
- String[] b = providerNames.split("\\s*,\\s*");
- for (int i = 0; i < b.length; i++) {
- b[i] = b[i].trim();
+ List parseProviderNames(String providerNames) {
+ if (providerNames==null){
+ return Collections.singletonList("");
Review Comment:
This should be `Collections.emptyList()`
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a diff in pull request #597: KNOX-2762 Bug fixes for spaces around delimiters with all reviewed comments addressed
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #597:
URL: https://github.com/apache/knox/pull/597#discussion_r905831775
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -58,7 +59,7 @@ public void contributeFilter( DeploymentContext context, Provider provider, Serv
Map<String, String> providerParams = provider.getParams();
String providerNames = providerParams.get("composite.provider.names");
- if (!providerNames.isEmpty()) {
+ if (!StringUtils.isEmpty(providerNames)) {
Review Comment:
Nice catch Attila! I missed it previously. Thanks for spotting this out!
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a diff in pull request #597: KNOX-2762 Bug fixes for spaces around delimiters with all reviewed comments addressed
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #597:
URL: https://github.com/apache/knox/pull/597#discussion_r905328537
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -71,7 +71,7 @@ public void contributeFilter( DeploymentContext context, Provider provider, Serv
}
List parseProviderNames(String providerNames) {
- if (providerNames==null || StringUtils.isEmpty(providerNames) || StringUtils.isBlank(providerNames)){
+ if (providerNames==null || StringUtils.isBlank(providerNames)){
Review Comment:
See the JavaDoc here: https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html#isBlank-java.lang.CharSequence-
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] zeroflag commented on a diff in pull request #597: KNOX-2762 Bug fixes for spaces around delimiters with all reviewed comments addressed
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #597:
URL: https://github.com/apache/knox/pull/597#discussion_r905386669
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -71,7 +71,7 @@ public void contributeFilter( DeploymentContext context, Provider provider, Serv
}
List parseProviderNames(String providerNames) {
Review Comment:
What's the reason for not having a type parameter for the List? (E.g. List<String>)
##########
gateway-provider-security-authz-composite/src/test/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzProviderTest.java:
##########
@@ -83,13 +85,19 @@ public void testingNullandEmptyProviderNames() throws Exception {
String testnames = "";
CompositeAuthzDeploymentContributor c = new CompositeAuthzDeploymentContributor();
List providerNames = c.parseProviderNames(testnames);
- assertEquals(providerNames.size(), 1);
- assertEquals(providerNames.get(0), "");
+ assertSame(providerNames.size(),0);
Review Comment:
Here the order of parameters are reversed. The first is the expected and the second is the actual.
This is not a correctness issue, but the error message will misleading if the test fails.
For example if providerNames has a size of 3 it'll say: expected 3 got 0, but it is the other way around.
More importantly what's the reason for using assertSame instead of assertEquals? My understanding is that assertSame checks identity (`==`) not equality (`.equals`).
--
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-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] zeroflag commented on a diff in pull request #597: KNOX-2762 Bug fixes for spaces around delimiters with all reviewed comments addressed
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #597:
URL: https://github.com/apache/knox/pull/597#discussion_r905386669
##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -71,7 +71,7 @@ public void contributeFilter( DeploymentContext context, Provider provider, Serv
}
List parseProviderNames(String providerNames) {
Review Comment:
What's the reason for not having a type parameter for the List? (E.g. List<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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org