Ori Liel has posted comments on this change.
Change subject: [WIP] API: Adding Job mapper class
......................................................................
Patch Set 1: (9 inline comments)
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/JobMapper.java
Line 11: import org.ovirt.engine.core.compat.Guid;
Line 12:
Line 13: public class JobMapper {
Line 14:
Line 15: @Mapping(from = org.ovirt.engine.core.common.job.Job.class, to =
org.ovirt.engine.api.model.Job.class)
It's a petty comment, but could you please import one of the 'Job' classes?
Obviously you can't import both because they are both named 'Job', but
importing one would stilll save a lot of 'org.ovirt.engine...'
Line 16: public static org.ovirt.engine.api.model.Job
map(org.ovirt.engine.core.common.job.Job entity, org.ovirt.engine.api.model.Job
job) {
Line 17:
Line 18: org.ovirt.engine.api.model.Job model = job != null ? job : new
org.ovirt.engine.api.model.Job();
Line 19: model.setId(String.valueOf(entity.getId()));
Line 15: @Mapping(from = org.ovirt.engine.core.common.job.Job.class, to =
org.ovirt.engine.api.model.Job.class)
Line 16: public static org.ovirt.engine.api.model.Job
map(org.ovirt.engine.core.common.job.Job entity, org.ovirt.engine.api.model.Job
job) {
Line 17:
Line 18: org.ovirt.engine.api.model.Job model = job != null ? job : new
org.ovirt.engine.api.model.Job();
Line 19: model.setId(String.valueOf(entity.getId()));
For no other reason than internal consistency (with other mapper classes) could
you please use:
entity.getId().toString()
instead of:
String.valueOf(entity.getId())
?
(I know they do exactly the same)
Line 20: model.setActionType(entity.getActionType().name());
Line 21: model.setDescription(entity.getDescription());
Line 22: model.setStatus(map(entity.getStatus(), null));
Line 23: if (entity.getOwnerId() != null) {
Line 16: public static org.ovirt.engine.api.model.Job
map(org.ovirt.engine.core.common.job.Job entity, org.ovirt.engine.api.model.Job
job) {
Line 17:
Line 18: org.ovirt.engine.api.model.Job model = job != null ? job : new
org.ovirt.engine.api.model.Job();
Line 19: model.setId(String.valueOf(entity.getId()));
Line 20: model.setActionType(entity.getActionType().name());
We don't map enum classes this way, we use an explicit mapper.
Line 21: model.setDescription(entity.getDescription());
Line 22: model.setStatus(map(entity.getStatus(), null));
Line 23: if (entity.getOwnerId() != null) {
Line 24: User user = new User();
Line 21: model.setDescription(entity.getDescription());
Line 22: model.setStatus(map(entity.getStatus(), null));
Line 23: if (entity.getOwnerId() != null) {
Line 24: User user = new User();
Line 25: user.setId(String.valueOf(entity.getOwnerId()));
Again, for consistency, could you use entity.getOwnerId().toString()?
Line 26: model.setOwner(user);
Line 27: }
Line 28:
model.setStartTime(TypeConversionHelper.toXMLGregorianCalendar(entity.getStartTime(),
null));
Line 29: if (entity.getEndTime() != null) {
Line 24: User user = new User();
Line 25: user.setId(String.valueOf(entity.getOwnerId()));
Line 26: model.setOwner(user);
Line 27: }
Line 28:
model.setStartTime(TypeConversionHelper.toXMLGregorianCalendar(entity.getStartTime(),
null));
For the sake of consistency with other mappers, could you please use:
DateMapper.map(entity.getStartTime(), null)
?
(Same for similar lines below)
Line 29: if (entity.getEndTime() != null) {
Line 30:
model.setEndTime(TypeConversionHelper.toXMLGregorianCalendar(entity.getEndTime(),
null));
Line 31: }
Line 32: if (entity.getLastUpdateTime() != null) {
Line 38: return model;
Line 39: }
Line 40:
Line 41: @Mapping(from = org.ovirt.engine.api.model.Job.class, to =
org.ovirt.engine.core.common.job.Job.class)
Line 42: public static org.ovirt.engine.core.common.job.Job
map(org.ovirt.engine.api.model.Job job,
This method could be used for creating a new Job, or updating an existing Job.
In case of update - as in all mappers - we need to override only the fields
which the user has set. Every line in this method should be preceedded by and
'if' statement that checks if the value is not 'null'.
Line 43: org.ovirt.engine.core.common.job.Job entity) {
Line 44: org.ovirt.engine.core.common.job.Job target =
Line 45: entity != null ? entity : new
org.ovirt.engine.core.common.job.Job();
Line 46: target.setId(new Guid(job.getId()));
Line 43: org.ovirt.engine.core.common.job.Job entity) {
Line 44: org.ovirt.engine.core.common.job.Job target =
Line 45: entity != null ? entity : new
org.ovirt.engine.core.common.job.Job();
Line 46: target.setId(new Guid(job.getId()));
Line 47:
target.setActionType(VdcActionType.valueOf(job.getActionType()));
We don't map enums this way, we do explicit mapping
Line 48: target.setDescription(job.getDescription());
Line 49: target.setStatus(map(job.getStatus()));
Line 50: target.setOwnerId(new Guid(job.getOwner().getId()));
Line 51:
target.setStartTime(job.getStartTime().toGregorianCalendar().getTime());
Line 48: target.setDescription(job.getDescription());
Line 49: target.setStatus(map(job.getStatus()));
Line 50: target.setOwnerId(new Guid(job.getOwner().getId()));
Line 51:
target.setStartTime(job.getStartTime().toGregorianCalendar().getTime());
Line 52: target.setEndTime(job.isSetStartTime() ?
job.getStartTime().toGregorianCalendar().getTime()
It appears that you're mapping start time to end time, probably a bug?
Line 53: : new
Date((Calendar.getInstance().getTimeInMillis())));
Line 54:
Line 55: target.setLastUpdateTime(job.isSetLastUpdated() ?
job.getLastUpdated().toGregorianCalendar().getTime()
Line 56: : new
Date((Calendar.getInstance().getTimeInMillis())));
Line 59:
Line 60: return target;
Line 61: }
Line 62:
Line 63: @Mapping(from = org.ovirt.engine.api.model.Status.class,
'Status' and 'JobExecutionStatus' are imported, so why do you need the package
prefix?
Line 64: to =
org.ovirt.engine.core.common.job.JobExecutionStatus.class)
Line 65: public static org.ovirt.engine.core.common.job.JobExecutionStatus
map(org.ovirt.engine.api.model.Status status) {
Line 66: if
(JobExecutionStatus.STARTED.name().equals(status.getState().toUpperCase())) {
Line 67: return JobExecutionStatus.STARTED;
--
To view, visit http://gerrit.ovirt.org/16160
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1b95a094dc586e6ebbdacd44e0a034e91602163
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Eli Mesika <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Ori Liel <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
