[GitHub] cloudstack pull request: Dereference NULL return value
Github user kansal closed the pull request at: https://github.com/apache/cloudstack/pull/618 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user kansal commented on the pull request: https://github.com/apache/cloudstack/pull/618#issuecomment-125552998 Closing this pull request. Will open one for each issue. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user kansal commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/618#discussion_r35524999
--- Diff:
plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java
---
@@ -358,7 +363,8 @@ private boolean setup() {
}
_timer = new Timer();
final HttpClient client = new HttpClient();
-final String idpMetaDataUrl =
SAMLIdentityProviderMetadataURL.value();
+//final String idpMetaDataUrl =
SAMLIdentityProviderMetadataURL.value();
--- End diff --
@bhaisaab Sure. Removing the comment. The function call is needed for
mocking at the time of unit testing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user kansal commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/618#discussion_r35524889
--- Diff:
plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java
---
@@ -119,12 +123,12 @@
@Override
public boolean start() {
if (isSAMLPluginEnabled()) {
-setup();
s_logger.info(SAML auth plugin loaded);
+return setup();
} else {
--- End diff --
@bhaisaab I feel we can go with the if-else style because it anyhow is
doing the same thing plus it give better code readability and logging. Your
take?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/618#discussion_r35521326
--- Diff:
plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java
---
@@ -119,12 +123,12 @@
@Override
public boolean start() {
if (isSAMLPluginEnabled()) {
-setup();
s_logger.info(SAML auth plugin loaded);
+return setup();
} else {
--- End diff --
we can remove else {} here just keep;
if (condition){
return stuff();
}
return otherstuff();
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/618#discussion_r35521408
--- Diff:
plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java
---
@@ -358,7 +363,8 @@ private boolean setup() {
}
_timer = new Timer();
final HttpClient client = new HttpClient();
-final String idpMetaDataUrl =
SAMLIdentityProviderMetadataURL.value();
+//final String idpMetaDataUrl =
SAMLIdentityProviderMetadataURL.value();
--- End diff --
Remove commented deadcode, or simply keep final String idpMetaDataUrl =
SAMLIdentityProviderMetadataURL.value(); as not many consumers of the new
getSAMLIdentityProviderMetadataURL() method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/618#issuecomment-125154356 @kansal Please see my comments. While it's okay, please squash the commits into two separate commits; one targeting the saml related stuff and second one targeting the other component, so as to allow us to port the commits to other branches. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user kansal commented on the pull request: https://github.com/apache/cloudstack/pull/618#issuecomment-125106632 @DaanHoogland Added the test file. Checking only the false cases. @wilderrodrigues Got away with the IF statements and replaced with the api @karuturi suggested. Please review the changes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user wilderrodrigues commented on the pull request: https://github.com/apache/cloudstack/pull/618#issuecomment-125150991 LGTM :+1: Thanks for the improvements! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user kishankavala commented on the pull request: https://github.com/apache/cloudstack/pull/618#issuecomment-125136388 LGTM. @bhaisaab can you also take a look at the changes? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user karuturi commented on the pull request: https://github.com/apache/cloudstack/pull/618#issuecomment-124330473 instead of a notnull check everywhere, can you use a Non Nulllable ArrayList for tags or use CollectionUtils.addIgnoreNull() api? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user kansal commented on the pull request: https://github.com/apache/cloudstack/pull/618#issuecomment-124034693 @kishankavala: The message has been changed. @karuturi: Added the path. Please check I have also made another change with respect to solving this problem of dereferencing of null pointer. Kindly review the changes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/618#issuecomment-124034602 thanks @kansal Can you add some test cases (for null values found for instance)? LGTM --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Dereference NULL return value
GitHub user kansal opened a pull request: https://github.com/apache/cloudstack/pull/618 Dereference NULL return value No check on the null value of metadatafile. If null, the following operations failed. Added null check for the metadatafile. You can merge this pull request into a Git repository by running: $ git pull https://github.com/kansal/cloudstack master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/618.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #618 commit aa0bd53162bd3f4450e5e7915e3b1f9e898fe55d Author: Kshitij Kansal kansal@kshitijs-macbook-pro.local Date: 2015-07-23T15:55:17Z Dereference NULL return value --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user kishankavala commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/618#discussion_r35290636
--- Diff:
plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java
---
@@ -368,8 +368,14 @@ private boolean setup() {
_idpMetaDataProvider = new HTTPMetadataProvider(_timer,
client, idpMetaDataUrl);
} else {
File metadataFile =
PropertiesUtil.findConfigFile(idpMetaDataUrl);
-s_logger.debug(Provided Metadata is not a URL, trying to
read metadata file from local path: + metadataFile.getAbsolutePath());
-_idpMetaDataProvider = new
FilesystemMetadataProvider(_timer, metadataFile);
+if (metadataFile == null) {
+s_logger.error(Metadata file returned null);
--- End diff --
Can you please update the error message: Provided Metadata is not a URL,
Unable to locate metadata file from local path
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/618#discussion_r35291257
--- Diff:
plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java
---
@@ -368,8 +368,14 @@ private boolean setup() {
_idpMetaDataProvider = new HTTPMetadataProvider(_timer,
client, idpMetaDataUrl);
} else {
File metadataFile =
PropertiesUtil.findConfigFile(idpMetaDataUrl);
-s_logger.debug(Provided Metadata is not a URL, trying to
read metadata file from local path: + metadataFile.getAbsolutePath());
-_idpMetaDataProvider = new
FilesystemMetadataProvider(_timer, metadataFile);
+if (metadataFile == null) {
+s_logger.error(Metadata file returned null);
--- End diff --
Can you also add the file path its trying to fing (idpMetaDataUrl) to the
error message?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
