Juan Hernandez has posted comments on this change. Change subject: restapi : ints and shorts overflow ......................................................................
Patch Set 1: Code-Review+1 (7 comments) Looks good. Some minor comments inside. Can we add at least one test to check that we actually generate the exception when a invalid value is provided? http://gerrit.ovirt.org/#/c/23856/1/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/utils/IntegerParser.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/utils/IntegerParser.java: Line 8: public static int parseIntegerToShort(String value) { Line 9: BigInteger result = Line 10: DatatypeConverter.parseInteger(value); Line 11: if (result.longValue() > Short.MAX_VALUE) { Line 12: throw new InvalidValueException("Value " +value+ " greater than Short.MAX_VALUE"); As we are going to display this to the user, I think it is better if we don't use Java concepts, like Short.MAX_VALUE. Can we use something like this? "Value " + value + " greater than maximum " + Short.MAX_VALUE + " Line 13: } Line 14: return result.intValue(); Line 15: } Line 16: Line 17: public static int parseIntegerToInt(String value) { Line 18: BigInteger result = Line 19: DatatypeConverter.parseInteger(value); Line 20: if (result.longValue() > Integer.MAX_VALUE) { Line 21: throw new InvalidValueException("Value " +value+ " greater than Integer.MAX_VALUE"); Same. Line 22: } Line 23: return result.intValue(); Line 24: } Line 25: http://gerrit.ovirt.org/#/c/23856/1/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 6: Line 7: Line 8: <xs:annotation> Line 9: <xs:appinfo> Line 10: <jaxb:globalBindings generateIsSetMethod="true"> Can you add a comment here explaining why this is needed? Line 11: <jaxb:javaType name="int" xmlType="xs:int" Line 12: parseMethod="org.ovirt.engine.api.utils.IntegerParser.parseIntegerToInt" Line 13: printMethod="org.ovirt.engine.api.utils.IntegerParser.printIntToInteger" Line 14: /> http://gerrit.ovirt.org/#/c/23856/1/backend/manager/modules/restapi/jaxrs/pom.xml File backend/manager/modules/restapi/jaxrs/pom.xml: Line 110: <dependency> Line 111: <groupId>org.jboss.resteasy</groupId> Line 112: <artifactId>resteasy-jaxb-provider</artifactId> Line 113: <version>2.3.2.Final</version> Line 114: </dependency> Please indent this like the rest of the code. Line 115: Line 116: </dependencies> Line 117: Line 118: <profiles> http://gerrit.ovirt.org/#/c/23856/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/BackendApplication.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/BackendApplication.java: Line 162: singletons.add(new RequestVerbLogger()); Line 163: singletons.add(new RequestPayloadLogger()); Line 164: singletons.add(new ResponseStatusLogger()); Line 165: singletons.add(new ResponsePayloadLogger()); Line 166: singletons.add(new XmlMessageBodyReader()); This isn't a logger. Can you add a blank line and a comment explaining that what this is? Line 167: Line 168: // Intercepter that maps exceptions cause by illegal guid string to 400 status (BAD_REQUEST). Line 169: singletons.add(new MalformedIdExceptionMapper()); Line 170: singletons.add(new JaxbExceptionMapper()); http://gerrit.ovirt.org/#/c/23856/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/validation/InvalidValueExceptionMapper.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/validation/InvalidValueExceptionMapper.java: Line 10: @Provider Line 11: public class InvalidValueExceptionMapper implements ExceptionMapper<InvalidValueException> { Line 12: Line 13: @Override Line 14: public javax.ws.rs.core.Response toResponse(InvalidValueException e) { Do you need the fully qualified class name here? Line 15: Fault fault = new Fault(); Line 16: fault.setReason("Invalid Value"); Line 17: fault.setDetail(e.getMessage()); Line 18: return Response.status(Response.Status.BAD_REQUEST).entity(fault).build(); http://gerrit.ovirt.org/#/c/23856/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/validation/XmlMessageBodyReader.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/validation/XmlMessageBodyReader.java: Line 26: import java.lang.reflect.Type; Line 27: Line 28: @Provider Line 29: @Consumes({ MediaType.APPLICATION_XML }) Line 30: public class XmlMessageBodyReader implements MessageBodyReader<Object> { It is my understanding that you are replacing a default reader used by RESTEasy with this one, and that you probably copied most of the content from there. Is that correct? Isn't it possible to extend that reader and add only the custom error handler? Line 31: Line 32: /** XML parsers */ Line 33: @Context Line 34: protected Providers providers; -- To view, visit http://gerrit.ovirt.org/23856 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9d0be6f3f220d7a5f58bb47906a8565d80e0ebf0 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@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
