Alon Bar-Lev has posted comments on this change.
Change subject: engine : Validate parameter in canDoAction
......................................................................
Patch Set 1:
(7 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java
Line 2164: }
Line 2165:
Line 2166: protected void setRequiredParameters(VdcParameter[] params) {
Line 2167: this.requiredParameters = Arrays.asList(params);
Line 2168: }
this is not good for inheritance. we should allow appending.
C2 -> C1 -> CommandBase
[p1, p2, p3] [p1, p2] []
Line 2169:
Line 2170: protected void setOptionalParameters(VdcParameter[] params) {
Line 2171: this.optionalParameters = Arrays.asList(params);
Line 2172: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VdcParametersValidator.java
Line 16: import java.util.Set;
Line 17:
Line 18: public class VdcParametersValidator {
Line 19: private static final Log log =
LogFactory.getLog(VdcParametersValidator.class);
Line 20: private static final VdcParameter[] commonParametersArray = {
you can drop this temp variable
Line 21: CoreVdcParameters.GUID_ID,
Line 22: CoreVdcParameters.SESSION_ID,
Line 23: CoreVdcParameters.PARAMETERS_CURRENT_USER,
Line 24: CoreVdcParameters.SHOULD_BE_LOGGED,
Line 42: CoreVdcParameters.FILTERED,
Line 43: CoreVdcParameters.REFRESH};
Line 44: private static final List<VdcParameter> commonParameters =
Arrays.asList(commonParametersArray);
Line 45:
Line 46: public static <T extends VdcParametersMap> boolean
validateParameters(
why is this function a template?
Line 47: T parameters,
Line 48: List<VdcParameter> requiredParameters,
Line 49: List<VdcParameter> optionalParameters) {
Line 50: Set<Map.Entry<VdcParameter, Serializable>> paramsSet =
parameters.entrySet();
Line 51: List<VdcParameter> paramNames = new ArrayList<VdcParameter>();
Line 52: for (Map.Entry<VdcParameter, Serializable> param : paramsSet) {
Line 53: paramNames.add(param.getKey());
Line 54: try {
Line 55: if
(!Class.forName(param.getKey().getJavaType()).isAssignableFrom(param.getValue().getClass()))
{
I hope this will be required only for the canDoAction, and if it is, then after
we validate parameters existence (required, optional) we should have another
function to check the types and call it just from canDoAction.
Line 56: throw new
InvalidParameterValueException(param.getKey().getName(),
param.getKey().getJavaType(), param.getValue().getClass().getName());
Line 57: }
Line 58: } catch (ClassNotFoundException e) {
Line 59: throw new
InvalidParameterValueException(param.getKey().getName(),
param.getKey().getJavaType());
Line 59: throw new
InvalidParameterValueException(param.getKey().getName(),
param.getKey().getJavaType());
Line 60: }
Line 61: }
Line 62: if (requiredParameters.isEmpty()) {
Line 63: return true;
please avoid return at middle of functions, and I am unsure it is required as
the flow will handle this case as well.
Line 64: }
Line 65: if (!paramNames.containsAll(requiredParameters)) {
Line 66: List<VdcParameter> copy = new
ArrayList(requiredParameters);
Line 67: copy.removeAll(paramNames);
Line 77: buf.append(" ,");
Line 78: }
Line 79: buf.append(param.getName());
Line 80: }
Line 81: log.warnFormat("Unknown parameters {0} found in command.",
buf.toString());
I am unsure that warning is appropriate compared to debug, but for now during
development it is usable.
Line 82: }
Line 83: return true;
Line 84: }
Line 85:
Line 79: buf.append(param.getName());
Line 80: }
Line 81: log.warnFormat("Unknown parameters {0} found in command.",
buf.toString());
Line 82: }
Line 83: return true;
where do you return false?
Line 84: }
Line 85:
--
To view, visit http://gerrit.ovirt.org/21485
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I15566c9177da28b2d47bbb6018fbfb61defcf3da
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ravi Nori <rn...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
