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
_______________________________________________ Dev mailing list [email protected] http://wso2.org/cgi-bin/mailman/listinfo/dev
