Michael Pasternak has posted comments on this change.
Change subject: restapi: 'async' URL Parameter Malfunction (#957452)
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(5 inline comments)
....................................................
File
backend/manager/modules/restapi/interface/common/jaxrs/src/main/java/org/ovirt/engine/api/common/util/QueryHelper.java
Line 187: }
Line 188:
Line 189: public static String getMatrixConstraint(UriInfo uriInfo, String
constraint) {
Line 190: return getMatrixConstraints(uriInfo,
constraint).containsKey(constraint) ?
Line 191: getMatrixConstraints(uriInfo,
constraint).get(constraint) : null;
no need to fetch same param twice
Line 192: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendResource.java
Line 168: }
Line 169:
Line 170: protected boolean isAsync() {
Line 171: if (QueryHelper.hasMatrixParam(getUriInfo(),
ASYNC_CONSTRAINT)) {
Line 172: String value =
QueryHelper.getMatrixConstraint(getUriInfo(), ASYNC_CONSTRAINT);
you can fetch the value of ASYNC_CONSTRAINT only once and then compare != NULL
Line 173: if (value.equalsIgnoreCase(TRUE) || value.isEmpty()) {
Line 174: return true;
Line 175: } else if (value.equalsIgnoreCase(FALSE)) {
Line 176: return false;
Line 169:
Line 170: protected boolean isAsync() {
Line 171: if (QueryHelper.hasMatrixParam(getUriInfo(),
ASYNC_CONSTRAINT)) {
Line 172: String value =
QueryHelper.getMatrixConstraint(getUriInfo(), ASYNC_CONSTRAINT);
Line 173: if (value.equalsIgnoreCase(TRUE) || value.isEmpty()) {
potential NPE, your QueryHelper.getMatrixConstraint() returns NULL when not
prived
Line 174: return true;
Line 175: } else if (value.equalsIgnoreCase(FALSE)) {
Line 176: return false;
Line 177: } else {
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BaseBackendResource.java
Line 19: import org.ovirt.engine.api.common.util.EnumValidator;
Line 20: import org.ovirt.engine.api.model.Fault;
Line 21: import org.ovirt.engine.api.restapi.logging.MessageBundle;
Line 22: import org.ovirt.engine.api.restapi.logging.Messages;
Line 23: import
org.ovirt.engine.api.restapi.resource.exception.UrlParamException;
can't see the definition for the UrlParamException, are you missing a file?
Line 24: import
org.ovirt.engine.api.restapi.resource.validation.ValidatorLocator;
Line 25: import org.ovirt.engine.api.restapi.util.SessionHelper;
Line 26: import org.ovirt.engine.api.restapi.utils.MalformedIdException;
Line 27: import org.ovirt.engine.core.common.action.VdcActionParametersBase;
Line 218: throw new
WebApplicationException(Response.status(Response.Status.NOT_FOUND).build());
Line 219: } else if ((e instanceof BackendFailureException) &&
(!StringUtils.isEmpty(e.getMessage()))) {
Line 220:
LOG.errorFormat(localize(Messages.BACKEND_FAILED_TEMPLATE), e.getMessage(),
null);
Line 221: throw new WebFaultException(null, e.getMessage(),
Response.Status.BAD_REQUEST);
Line 222: } else if (e instanceof UrlParamException){
here i'd expect check for the WebFaultException, this way all permutations
of the WebFaultException will be in a handled same way.
Line 223: UrlParamException e2 = (UrlParamException) e;
Line 224: throw e2;
Line 225: } else {
Line 226:
LOG.errorFormat(localize(Messages.BACKEND_FAILED_TEMPLATE), detail(e), e);
--
To view, visit http://gerrit.ovirt.org/15094
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b8ac5d49823612300f1c7872c8d317a20e02797
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ori Liel <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
