You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by anshul1886 <gi...@git.apache.org> on 2014/11/18 06:34:38 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
GitHub user anshul1886 opened a pull request:
https://github.com/apache/cloudstack/pull/41
CLOUDSTACK-7930, CLOUDSTACK-7931: Do not allow to set invalid values for global settings which are of type integer and float
Do not allow to set invalid values for global settings which are of type integer and float
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anshul1886/cloudstack-1 CLOUDSTACK-7930
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/41.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 #41
----
commit b7de1ecbe470365c30bc6afcea5f5c560bc9c8c2
Author: Anshul Gangwar <an...@citrix.com>
Date: 2014-11-17T09:57:22Z
CLOUDSTACK-7930, CLOUDSTACK-7931: Do not allow to set invalid values for global settings which are of type integer and float
----
---
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] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/41#discussion_r20492860
--- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java ---
@@ -725,6 +725,21 @@ private String validateConfigurationValue(String name, String value, String scop
type = c.getType();
}
+ String errMsg = null;
+ try {
+ if (type.equals(Integer.class)) {
+ errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid integer value for parameter " + name;
+ Integer.parseInt(value);
+ } else if (type.equals(Float.class)) {
+ errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid float value for parameter " + name;
+ Float.parseFloat(value);
+ }
+ } catch (Exception e) {
--- End diff --
catching top level exception and swallowing the stacktrace makes it really difficult during debugging.
For end user, we anyway wont show the exception but only the 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.
---
[GitHub] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/41
---
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] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/41#discussion_r20489652
--- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java ---
@@ -725,6 +725,21 @@ private String validateConfigurationValue(String name, String value, String scop
type = c.getType();
}
+ String errMsg = null;
+ try {
+ if (type.equals(Integer.class)) {
+ errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid integer value for parameter " + name;
+ Integer.parseInt(value);
+ } else if (type.equals(Float.class)) {
+ errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid float value for parameter " + name;
+ Float.parseFloat(value);
--- End diff --
No. Because they will fail later in parsing.
---
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] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/41#discussion_r20492616
--- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java ---
@@ -725,6 +725,21 @@ private String validateConfigurationValue(String name, String value, String scop
type = c.getType();
}
+ String errMsg = null;
+ try {
+ if (type.equals(Integer.class)) {
+ errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid integer value for parameter " + name;
+ Integer.parseInt(value);
+ } else if (type.equals(Float.class)) {
+ errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid float value for parameter " + name;
+ Float.parseFloat(value);
--- End diff --
I understand that it will fail during parsing. But, we can string replace "," with ""
---
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] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/41#issuecomment-63430386
Can you add unittests for this?
---
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] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/41#discussion_r20489860
--- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java ---
@@ -725,6 +725,21 @@ private String validateConfigurationValue(String name, String value, String scop
type = c.getType();
}
+ String errMsg = null;
+ try {
+ if (type.equals(Integer.class)) {
+ errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid integer value for parameter " + name;
+ Integer.parseInt(value);
+ } else if (type.equals(Float.class)) {
+ errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid float value for parameter " + name;
+ Float.parseFloat(value);
+ }
+ } catch (Exception e) {
--- End diff --
What value will be added by catching specific exceptions?
In the end user only wants to know whether the value is valid or not. he/she is not concerned with what type of exception he is getting.
---
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] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/41#discussion_r20488370
--- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java ---
@@ -725,6 +725,21 @@ private String validateConfigurationValue(String name, String value, String scop
type = c.getType();
}
+ String errMsg = null;
+ try {
+ if (type.equals(Integer.class)) {
+ errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid integer value for parameter " + name;
+ Integer.parseInt(value);
+ } else if (type.equals(Float.class)) {
+ errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid float value for parameter " + name;
+ Float.parseFloat(value);
--- End diff --
should we allow values like "3,400.5" ?
---
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] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/41#discussion_r20488404
--- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java ---
@@ -725,6 +725,21 @@ private String validateConfigurationValue(String name, String value, String scop
type = c.getType();
}
+ String errMsg = null;
+ try {
+ if (type.equals(Integer.class)) {
+ errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid integer value for parameter " + name;
+ Integer.parseInt(value);
+ } else if (type.equals(Float.class)) {
+ errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid float value for parameter " + name;
+ Float.parseFloat(value);
+ }
+ } catch (Exception e) {
--- End diff --
Can you catch specific expected exceptions NPE and NFE?
---
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.
---