Re: [Dev] Fwd: Improvements to the RESTful APIs in CDMF
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
wrote:
> Hi Rasika,
>
> Thanks for the improvements suggested. Please find my comments below.
>
> On Mon, Jun 27, 2016 at 12:53 PM, Rasika Perera 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
>> 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 || res
Re: [Dev] Fwd: Improvements to the RESTful APIs in CDMF
Hi Rasika,
Thanks for the improvements suggested. Please find my comments below.
On Mon, Jun 27, 2016 at 12:53 PM, Rasika Perera 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
> 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
> 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 logi
[Dev] Fwd: Improvements to the RESTful APIs in CDMF
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
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
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) result.getData());
devices.setCount(result.getRecordsTotal());
return Response.status(Response.Status.OK).entity(devices).build();
} catch (DeviceManagementException e) {
Str
