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()
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?
*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());
}
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: rasi...@wso2.com
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: rasi...@wso2.com
LinkedIn: http://lk.linkedin.com/in/rasika90
WSO2 Inc. www.wso2.com
lean.enterprise.middleware
_______________________________________________
Dev mailing list
Dev@wso2.org
http://wso2.org/cgi-bin/mailman/listinfo/dev
