You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@stratos.apache.org by anuruddhal <gi...@git.apache.org> on 2015/05/06 14:47:41 UTC
[GitHub] stratos pull request: Moving non api methods
GitHub user anuruddhal opened a pull request:
https://github.com/apache/stratos/pull/298
Moving non api methods
Improvements to exception handling and string validation
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anuruddhal/stratos moving_non_API_methods
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/stratos/pull/298.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 #298
----
commit 44e8933cc353467a6061606e2f9bc67645fb9a14
Author: anuruddhal <an...@gmail.com>
Date: 2015-05-06T12:33:36Z
Improving exception handling and validation
commit 03c6f349644e3df7bcba4cb1cb59d053d8ec0557
Author: anuruddhal <an...@gmail.com>
Date: 2015-05-06T12:42:17Z
Logging the UserStoreException
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by swgkg <gi...@git.apache.org>.
Github user swgkg commented on a diff in the pull request:
https://github.com/apache/stratos/pull/298#discussion_r30002483
--- Diff: components/org.apache.stratos.rest.endpoint/src/main/java/org/apache/stratos/rest/endpoint/api/StratosApiV41Utils.java ---
@@ -2836,52 +2838,58 @@ public static void updateExistingTenant(org.apache.stratos.common.beans.TenantIn
} catch (UserStoreException e) {
String msg = "Error in retrieving the tenant id for the tenant domain: " + tenantDomain + ".";
log.error(msg, e);
- throw new Exception(msg, e);
+ throw new RestAPIException(msg, e);
}
Tenant tenant;
try {
tenant = (Tenant) tenantManager.getTenant(tenantId);
} catch (UserStoreException e) {
- String msg = "Error in retrieving the tenant id for the tenant domain: " +
- tenantDomain + ".";
+ String msg = "Error in retrieving the tenant from tenant id: " +
+ tenantId + ".";
log.error(msg, e);
- throw new TenantNotFoundException(msg, e);
+ throw new RestAPIException(msg, e);
}
// filling the first and last name values
- if (tenantInfoBean.getFirstname() != null && !tenantInfoBean.getFirstname().trim().equals("")) {
+ if (StringUtils.isBlank(tenantInfoBean.getFirstname())) {
try {
CommonUtil.validateName(tenantInfoBean.getFirstname(), "First Name");
} catch (Exception e) {
String msg = "Invalid first name is provided.";
log.error(msg, e);
- throw new Exception(msg, e);
+ throw new RestAPIException(msg, e);
}
}
- if (tenantInfoBean.getLastname() != null && !tenantInfoBean.getLastname().trim().equals("")) {
+ if (StringUtils.isBlank(tenantInfoBean.getLastname())) {
try {
CommonUtil.validateName(tenantInfoBean.getLastname(), "Last Name");
} catch (Exception e) {
--- End diff --
Better to catch with the specific exceptions rather than catching whole exception
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by swgkg <gi...@git.apache.org>.
Github user swgkg commented on a diff in the pull request:
https://github.com/apache/stratos/pull/298#discussion_r30002496
--- Diff: components/org.apache.stratos.rest.endpoint/src/main/java/org/apache/stratos/rest/endpoint/api/StratosApiV41Utils.java ---
@@ -3097,7 +3113,9 @@ public static void deactivateTenant(String tenantDomain) throws RestAPIException
try {
TenantMgtUtil.deactivateTenant(tenantDomain, tenantManager, tenantId);
} catch (Exception e) {
--- End diff --
Same as above.Please use the specific exceptions.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by anuruddhal <gi...@git.apache.org>.
Github user anuruddhal commented on a diff in the pull request:
https://github.com/apache/stratos/pull/298#discussion_r29824484
--- Diff: components/org.apache.stratos.rest.endpoint/src/main/java/org/apache/stratos/rest/endpoint/api/StratosApiV41Utils.java ---
@@ -2969,7 +2983,7 @@ public static void updateExistingTenant(org.apache.stratos.common.beans.TenantIn
try {
bean = ObjectConverter
.convertCarbonTenantInfoBeanToTenantInfoBean(TenantMgtUtil.initializeTenantInfoBean(tenantId, tenant));
- } catch (NullPointerException e) {
+ } catch (Exception e) {
log.error(String.format("Couldn't find tenant for provided tenant domain. [Tenant Domain] %s", tenantDomain));
--- End diff --
Fixed with commit 881329fcb5d0f1d11c08923430020fac333aab9b
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by lahirus <gi...@git.apache.org>.
Github user lahirus commented on a diff in the pull request:
https://github.com/apache/stratos/pull/298#discussion_r29998288
--- Diff: components/org.apache.stratos.rest.endpoint/src/main/java/org/apache/stratos/rest/endpoint/api/StratosApiV41Utils.java ---
@@ -2932,7 +2946,7 @@ public static void updateExistingTenant(org.apache.stratos.common.beans.TenantIn
} catch (StratosException e) {
String msg = "Error in notifying tenant update.";
log.error(msg, e);
- throw new Exception(msg, e);
+ throw new RestAPIException(msg, e);
--- End diff --
This shold be handled with a specific exception, not with RestAPIException
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/stratos/pull/298
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by imesh <gi...@git.apache.org>.
Github user imesh commented on a diff in the pull request:
https://github.com/apache/stratos/pull/298#discussion_r29761603
--- Diff: components/org.apache.stratos.rest.endpoint/src/main/java/org/apache/stratos/rest/endpoint/api/StratosApiV41Utils.java ---
@@ -2969,7 +2983,7 @@ public static void updateExistingTenant(org.apache.stratos.common.beans.TenantIn
try {
bean = ObjectConverter
.convertCarbonTenantInfoBeanToTenantInfoBean(TenantMgtUtil.initializeTenantInfoBean(tenantId, tenant));
- } catch (NullPointerException e) {
+ } catch (Exception e) {
log.error(String.format("Couldn't find tenant for provided tenant domain. [Tenant Domain] %s", tenantDomain));
--- End diff --
The log.error() method call has not included the exception object 'e'.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by anuruddhal <gi...@git.apache.org>.
Github user anuruddhal commented on a diff in the pull request:
https://github.com/apache/stratos/pull/298#discussion_r29824479
--- Diff: components/org.apache.stratos.rest.endpoint/src/main/java/org/apache/stratos/rest/endpoint/api/StratosApiV41Utils.java ---
@@ -2751,15 +2751,16 @@ public static void addTenant(org.apache.stratos.common.beans.TenantInfoBean tena
UserRegistry userRegistry = (UserRegistry) PrivilegedCarbonContext.getThreadLocalCarbonContext().
getRegistry(RegistryType.USER_GOVERNANCE);
if (userRegistry == null) {
- log.error("Security alert! User registry is null. A user is trying create a tenant "
- + " without an authenticated session.");
- throw new RestAPIException("Security alert! User registry is null. A user is trying create a tenant "
- + " without an authenticated session.");
+ String msg="Security alert! User registry is null. A user is trying create a tenant "
--- End diff --
Fixed with commit 881329fcb5d0f1d11c08923430020fac333aab9b
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by swgkg <gi...@git.apache.org>.
Github user swgkg commented on a diff in the pull request:
https://github.com/apache/stratos/pull/298#discussion_r30002470
--- Diff: components/org.apache.stratos.rest.endpoint/src/main/java/org/apache/stratos/rest/endpoint/api/StratosApiV41Utils.java ---
@@ -2836,52 +2838,58 @@ public static void updateExistingTenant(org.apache.stratos.common.beans.TenantIn
} catch (UserStoreException e) {
String msg = "Error in retrieving the tenant id for the tenant domain: " + tenantDomain + ".";
log.error(msg, e);
- throw new Exception(msg, e);
+ throw new RestAPIException(msg, e);
}
Tenant tenant;
try {
tenant = (Tenant) tenantManager.getTenant(tenantId);
} catch (UserStoreException e) {
- String msg = "Error in retrieving the tenant id for the tenant domain: " +
- tenantDomain + ".";
+ String msg = "Error in retrieving the tenant from tenant id: " +
+ tenantId + ".";
log.error(msg, e);
- throw new TenantNotFoundException(msg, e);
+ throw new RestAPIException(msg, e);
}
// filling the first and last name values
- if (tenantInfoBean.getFirstname() != null && !tenantInfoBean.getFirstname().trim().equals("")) {
+ if (StringUtils.isBlank(tenantInfoBean.getFirstname())) {
try {
CommonUtil.validateName(tenantInfoBean.getFirstname(), "First Name");
} catch (Exception e) {
--- End diff --
Better to catch with the specific exceptions rather than catching whole exception
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by lahirus <gi...@git.apache.org>.
Github user lahirus commented on a diff in the pull request:
https://github.com/apache/stratos/pull/298#discussion_r29998276
--- Diff: components/org.apache.stratos.rest.endpoint/src/main/java/org/apache/stratos/rest/endpoint/api/StratosApiV41Utils.java ---
@@ -2836,52 +2838,58 @@ public static void updateExistingTenant(org.apache.stratos.common.beans.TenantIn
} catch (UserStoreException e) {
String msg = "Error in retrieving the tenant id for the tenant domain: " + tenantDomain + ".";
log.error(msg, e);
- throw new Exception(msg, e);
+ throw new RestAPIException(msg, e);
}
Tenant tenant;
try {
tenant = (Tenant) tenantManager.getTenant(tenantId);
} catch (UserStoreException e) {
- String msg = "Error in retrieving the tenant id for the tenant domain: " +
- tenantDomain + ".";
+ String msg = "Error in retrieving the tenant from tenant id: " +
+ tenantId + ".";
log.error(msg, e);
- throw new TenantNotFoundException(msg, e);
+ throw new RestAPIException(msg, e);
}
// filling the first and last name values
- if (tenantInfoBean.getFirstname() != null && !tenantInfoBean.getFirstname().trim().equals("")) {
+ if (StringUtils.isBlank(tenantInfoBean.getFirstname())) {
try {
CommonUtil.validateName(tenantInfoBean.getFirstname(), "First Name");
} catch (Exception e) {
String msg = "Invalid first name is provided.";
log.error(msg, e);
- throw new Exception(msg, e);
+ throw new RestAPIException(msg, e);
}
}
- if (tenantInfoBean.getLastname() != null && !tenantInfoBean.getLastname().trim().equals("")) {
+ if (StringUtils.isBlank(tenantInfoBean.getLastname())) {
try {
CommonUtil.validateName(tenantInfoBean.getLastname(), "Last Name");
} catch (Exception e) {
String msg = "Invalid last name is provided.";
log.error(msg, e);
- throw new Exception(msg, e);
+ throw new RestAPIException(msg, e);
--- End diff --
Thold be handled with a specific exception, not with RestAPIException
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by lahirus <gi...@git.apache.org>.
Github user lahirus commented on a diff in the pull request:
https://github.com/apache/stratos/pull/298#discussion_r29998263
--- Diff: components/org.apache.stratos.rest.endpoint/src/main/java/org/apache/stratos/rest/endpoint/api/StratosApiV41Utils.java ---
@@ -2751,15 +2751,16 @@ public static void addTenant(org.apache.stratos.common.beans.TenantInfoBean tena
UserRegistry userRegistry = (UserRegistry) PrivilegedCarbonContext.getThreadLocalCarbonContext().
getRegistry(RegistryType.USER_GOVERNANCE);
if (userRegistry == null) {
- log.error("Security alert! User registry is null. A user is trying create a tenant "
- + " without an authenticated session.");
- throw new RestAPIException("Security alert! User registry is null. A user is trying create a tenant "
- + " without an authenticated session.");
+ String msg="Security alert! User registry is null. A user is trying create a tenant "
--- End diff --
Formatting is not correct in equal sign.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by swgkg <gi...@git.apache.org>.
Github user swgkg commented on a diff in the pull request:
https://github.com/apache/stratos/pull/298#discussion_r30002527
--- Diff: components/org.apache.stratos.rest.endpoint/src/main/java/org/apache/stratos/rest/endpoint/api/StratosApiV41Utils.java ---
@@ -3179,7 +3201,9 @@ public static void updateUser(UserInfoBean userInfoBean) throws RestAPIException
StratosUserManagerUtils.updateUser(getTenantUserStoreManager(), userInfoBean);
} catch (UserManagerException e) {
- throw new RestAPIException(e.getMessage());
+ String msg = "Error in updating user";
+ log.error(msg, e);
+ throw new RestAPIException(msg,e);
}
--- End diff --
Better to have more descriptive error message for updating user.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by imesh <gi...@git.apache.org>.
Github user imesh commented on a diff in the pull request:
https://github.com/apache/stratos/pull/298#discussion_r29761431
--- Diff: components/org.apache.stratos.rest.endpoint/src/main/java/org/apache/stratos/rest/endpoint/api/StratosApiV41Utils.java ---
@@ -2751,15 +2751,16 @@ public static void addTenant(org.apache.stratos.common.beans.TenantInfoBean tena
UserRegistry userRegistry = (UserRegistry) PrivilegedCarbonContext.getThreadLocalCarbonContext().
getRegistry(RegistryType.USER_GOVERNANCE);
if (userRegistry == null) {
- log.error("Security alert! User registry is null. A user is trying create a tenant "
- + " without an authenticated session.");
- throw new RestAPIException("Security alert! User registry is null. A user is trying create a tenant "
- + " without an authenticated session.");
+ String msg="Security alert! User registry is null. A user is trying create a tenant "
--- End diff --
If this is a security alert, IMO we should not raise it to the client. It can be logged in the backend and raise a different error message to the client.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by lahirus <gi...@git.apache.org>.
Github user lahirus commented on a diff in the pull request:
https://github.com/apache/stratos/pull/298#discussion_r29998268
--- Diff: components/org.apache.stratos.rest.endpoint/src/main/java/org/apache/stratos/rest/endpoint/api/StratosApiV41Utils.java ---
@@ -2781,7 +2782,7 @@ public static void addTenant(org.apache.stratos.common.beans.TenantInfoBean tena
try {
TenantMgtUtil.addClaimsToUserStoreManager(tenant);
} catch (Exception e) {
- String msg = "Error in granting permissions for tenant " + tenantDomain;
+ String msg = "Error in granting permissions for tenant " + tenantDomain + e.getLocalizedMessage();
--- End diff --
No need of using "e.getLocalizedMessage()" as we do not support localization IMO.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] stratos pull request: Moving non api methods
Posted by swgkg <gi...@git.apache.org>.
Github user swgkg commented on a diff in the pull request:
https://github.com/apache/stratos/pull/298#discussion_r30002518
--- Diff: components/org.apache.stratos.rest.endpoint/src/main/java/org/apache/stratos/rest/endpoint/api/StratosApiV41Utils.java ---
@@ -3123,7 +3141,9 @@ public static void addUser(UserInfoBean userInfoBean) throws RestAPIException {
try {
StratosUserManagerUtils.addUser(getTenantUserStoreManager(), userInfoBean);
} catch (UserManagerException e) {
- throw new RestAPIException(e.getMessage());
+ String msg = "Error in adding User";
+ log.error(msg, e);
+ throw new RestAPIException(msg, e);
}
--- End diff --
Better to have more descriptive error message.You can use some user specific details with 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---