Hi Prabath, I don't think this is correct. Parameter validation is something outside > the scope of PaginationRequest/SearchCriteria. SearchCriteria is something > that accepts a set of "valid rules". So, the validation of the "structure" > of the rules should be determined and acted upon, before the real searching > logic begins to execute. In other words, obean validation and filtering of > data should be handled as two separate tasks. > +1, Still we can move those validations into a Util class and make the API look much cleaner. For instance RequestValidationUtil can do the validation. WDYT?
> One improvement we can possibly implement upon the above pointed out > logic, however, is that, we can effectively re-use "device-search" > functionality to replace the hard-coded filtering done. That'd make a good > case for us to re-use the existing functionalities to the fullest to > replace all similar device filtering logic that has already been done using > multiple different approaches. +1. Thanks On Mon, Jun 27, 2016 at 2:09 PM, Prabath Abeysekera <[email protected]> wrote: > Hi Rasika, > > Thanks for the improvements suggested. Please find my comments below. > > On Mon, Jun 27, 2016 at 12:53 PM, Rasika Perera <[email protected]> wrote: > >> Hi All, >> >> I have some suggestions for improving REST APIs in CDMF. >> >> *1. Why have we implemented custom exception classes for JAX-RSs in many >> places and why we wrap ErrorResponseBuilder in them?* >> >> throw new ConflictException( >> new >> ErrorResponse.ErrorResponseBuilder().setCode(409l).setMessage("User by >> username: " + >> userWrapper.getUsername() + " already exists. Therefore, >> request made to add user " + >> "was refused.").build()); >> >> >> For instance >> [1][2].UnexpectedServerErrorException, UnknownApplicationTypeException, >> ConflictException we >> don't need them. >> >> *on success* >> >> return Response.ok().entity(result).build(); >> >> *on errors(4xx-5xx);* >> >> return new >> ErrorResponse.ErrorResponseBuilder().setCode(409l).setMessage("User >> by username: " + >> >> userWrapper.getUsername() + " already exists. Therefore, request made to >> add user " + >> "was refused.").build() >> >> +1. > > Further, what's suggested above needs to change as below (depending on the > type of error). > > return Response.serverError().entity( > ErrorResponse.ErrorResponseBuilder().setCode(409l).setMessage("User by > username: " + userWrapper.getUsername() + " already exists. Therefore, > request made to add user was refused.").build()).build(); > > JAX-RSs are at the top most layer, we generally use our own custom >> exception classes when we have different logics to handle exceptions. >> >> For instance when BackEnd returning IOException, we don't know for what >> reason this have raised. Hence BE wrap it with their own custom exception >> classes to differentiate them for the *upper* layer. I don't think we need >> custom exception classes at REST APIs level, *status* of the resource >> should always represented with HTTP Status. >> >> *2. Why are we keeping owner,device-type,device-name,ownership...etc in >> PaginationRequest?* >> >> IMO Pagination needs *only* limit and offset. Others are belongs to >> SearchCriteria. Can we introduce/rename it into SearchCriteria? >> > > +1. > > >> >> *3. Move null check validations into PaginationRequest / SearchCriteria.* >> >> Have a look on this method. All these null checks can be moved to the >> PaginationRequest / SearchCriteria. For instance; SearchCriteria.validate() >> >> try { >> RequestValidationUtil.validateSelectionCriteria(type, user, roleName, >> ownership, status); >> >> DeviceManagementProviderService dms = >> DeviceMgtAPIUtils.getDeviceManagementService(); >> PaginationRequest request = new PaginationRequest(offset, limit); >> PaginationResult result; >> >> if (type != null) { >> request.setDeviceType(type); >> } >> if (user != null) { >> request.setOwner(user); >> } >> if (ownership != null) { >> RequestValidationUtil.validateOwnershipType(ownership); >> request.setOwnership(ownership); >> } >> if (status != null) { >> RequestValidationUtil.validateStatus(status); >> request.setStatus(status); >> } >> >> if (ifModifiedSince != null) { >> Date sinceDate; >> SimpleDateFormat format = new SimpleDateFormat("EEE, d MMM yyyy >> HH:mm:ss Z"); >> try { >> sinceDate = format.parse(ifModifiedSince); >> } catch (ParseException e) { >> throw new InputValidationException( >> new >> ErrorResponse.ErrorResponseBuilder().setCode(400l).setMessage("Invalid date >> " + >> "string is provided in 'If-Modified-Since' >> header").build()); >> } >> request.setSince(sinceDate); >> result = dms.getAllDevices(request); >> if (result == null || result.getData() == null || >> result.getData().size() <= 0) { >> return Response.status(Response.Status.NOT_MODIFIED).entity("No >> device is modified " + >> "after the timestamp provided in 'If-Modified-Since' >> header").build(); >> } >> } else if (since != null) { >> Date sinceDate; >> SimpleDateFormat format = new SimpleDateFormat("EEE, d MMM yyyy >> HH:mm:ss Z"); >> try { >> sinceDate = format.parse(since); >> } catch (ParseException e) { >> throw new InputValidationException( >> new >> ErrorResponse.ErrorResponseBuilder().setCode(400l).setMessage("Invalid date >> " + >> "string is provided in 'since' filter").build()); >> } >> request.setSince(sinceDate); >> result = dms.getAllDevices(request); >> if (result == null || result.getData() == null || >> result.getData().size() <= 0) { >> return Response.status(Response.Status.OK).entity("No device is >> modified " + >> "after the timestamp provided in 'since' >> filter").build(); >> } >> } else { >> >> *-------------------------------our real logic starts >> here---------------------------------------------* >> result = dms.getAllDevices(request); >> int resultCount = result.getRecordsTotal(); >> if (resultCount == 0) { >> throw new NotFoundException( >> new >> ErrorResponse.ErrorResponseBuilder().setCode(404l).setMessage("No device is >> currently" + >> " enrolled with the server").build()); >> } >> } >> >> return Response.ok().entity(result).build(); >> >> DeviceList devices = new DeviceList(); >> devices.setList((List<Device>) result.getData()); >> devices.setCount(result.getRecordsTotal()); >> return Response.status(Response.Status.OK).entity(devices).build(); >> } catch (DeviceManagementException e) { >> String msg = "Error occurred while fetching all enrolled devices"; >> log.error(msg, e); >> throw new UnexpectedServerErrorException( >> new >> ErrorResponse.ErrorResponseBuilder().setCode(500l).setMessage(msg).build()); >> } >> >> I don't think this is correct. Parameter validation is something outside > the scope of PaginationRequest/SearchCriteria. SearchCriteria is something > that accepts a set of "valid rules". So, the validation of the "structure" > of the rules should be determined and acted upon, before the real searching > logic begins to execute. In other words, obean validation and filtering of > data should be handled as two separate tasks. > > One improvement we can possibly implement upon the above pointed out > logic, however, is that, we can effectively re-use "device-search" > functionality to replace the hard-coded filtering done. That'd make a good > case for us to re-use the existing functionalities to the fullest to > replace all similar device filtering logic that has already been done using > multiple different approaches. > > Cheers, > Prabath > > WDYT? >> >> [1] >> https://github.com/wso2/carbon-device-mgt/tree/aeb4ad329528e34191378a1ea13e753c37e5d5d9/components/certificate-mgt/org.wso2.carbon.certificate.mgt.api/src/main/java/org/wso2/carbon/certificate/mgt/jaxrs/exception >> [2] >> https://github.com/wso2/carbon-device-mgt/tree/master/components/device-mgt/org.wso2.carbon.device.mgt.api/src/main/java/org/wso2/carbon/device/mgt/jaxrs/exception >> >> >> -- >> With Regards, >> >> *Rasika Perera* >> Software Engineer >> M: +94 71 680 9060 E: [email protected] >> LinkedIn: http://lk.linkedin.com/in/rasika90 >> >> WSO2 Inc. www.wso2.com >> lean.enterprise.middleware >> >> >> >> -- >> With Regards, >> >> *Rasika Perera* >> Software Engineer >> M: +94 71 680 9060 E: [email protected] >> LinkedIn: http://lk.linkedin.com/in/rasika90 >> >> WSO2 Inc. www.wso2.com >> lean.enterprise.middleware >> > > > > -- > Prabath Abeysekara > Technical Lead > WSO2 Inc. > Email: [email protected] > Mobile: +94774171471 > -- With Regards, *Rasika Perera* Software Engineer M: +94 71 680 9060 E: [email protected] LinkedIn: http://lk.linkedin.com/in/rasika90 WSO2 Inc. www.wso2.com lean.enterprise.middleware
_______________________________________________ Dev mailing list [email protected] http://wso2.org/cgi-bin/mailman/listinfo/dev
