Ori Liel has posted comments on this change.
Change subject: [WIP] API: Adding Step mapping class
......................................................................
Patch Set 1: (8 inline comments)
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/StepMapper.java
Line 9: import org.ovirt.engine.core.common.job.StepEnum;
Line 10: import org.ovirt.engine.core.compat.Guid;
Line 11: import org.ovirt.engine.core.compat.NGuid;
Line 12:
Line 13: public class StepMapper {
Please import as many classes and possible and lose package prefixes where
possible
Line 14:
Line 15: @Mapping(from = org.ovirt.engine.core.common.job.Step.class, to =
org.ovirt.engine.api.model.Step.class)
Line 16: public static org.ovirt.engine.api.model.Step
map(org.ovirt.engine.core.common.job.Step entity,
Line 17: org.ovirt.engine.api.model.Step step) {
Line 16: public static org.ovirt.engine.api.model.Step
map(org.ovirt.engine.core.common.job.Step entity,
Line 17: org.ovirt.engine.api.model.Step step) {
Line 18:
Line 19: org.ovirt.engine.api.model.Step model = step != null ? step :
new org.ovirt.engine.api.model.Step();
Line 20: model.setId(String.valueOf(entity.getId()));
If possible, here and in other places, for consistency, use
entity.getId().toString() (instead of String.valueOf())
Line 21: if (entity.getParentStepId() != null) {
Line 22: org.ovirt.engine.api.model.Step parentStep = new
org.ovirt.engine.api.model.Step();
Line 23: parentStep.setId(String.valueOf(entity.getParentStepId()));
Line 24: model.setParentStep(parentStep);
Line 29: model.setStepType(map(entity.getStepType()));
Line 30: model.setDescription(entity.getDescription());
Line 31: model.setNumber(entity.getStepNumber());
Line 32: model.setStatus(map(entity.getStatus(), null));
Line 33:
model.setStartTime(TypeConversionHelper.toXMLGregorianCalendar(entity.getStartTime(),
null));
For consistency, use DateMapper
Line 34: if (entity.getEndTime() != null) {
Line 35:
model.setEndTime(TypeConversionHelper.toXMLGregorianCalendar(entity.getEndTime(),
null));
Line 36: }
Line 37: model.setExternal(entity.isExternal());
Line 46: target.setId(new Guid(step.getId()));
Line 47: if (step.isSetParentStep()) {
Line 48:
target.setParentStepId(NGuid.createGuidFromString(step.getParentStep().getId()));
Line 49: }
Line 50:
target.setJobId(Guid.createGuidFromString(step.getJob().getId()));
Need an if statement the checks if 'null' before each line
Line 51: target.setStepType(map(step.getStepType()));
Line 52: target.setDescription(step.getDescription());
Line 53: target.setStepNumber(step.getNumber());
Line 54: target.setStatus(map(step.getStatus(), null));
Line 52: target.setDescription(step.getDescription());
Line 53: target.setStepNumber(step.getNumber());
Line 54: target.setStatus(map(step.getStatus(), null));
Line 55:
target.setStartTime(step.getStartTime().toGregorianCalendar().getTime());
Line 56: target.setEndTime(step.isSetStartTime() ?
step.getStartTime().toGregorianCalendar().getTime()
Mapping start time to end time. Is this on purpose?
Line 57: : new
Date((Calendar.getInstance().getTimeInMillis())));
Line 58: target.setExternal(step.isExternal());
Line 59: return target;
Line 60: }
Line 58: target.setExternal(step.isExternal());
Line 59: return target;
Line 60: }
Line 61:
Line 62: @Mapping(from = org.ovirt.engine.api.model.Status.class,
I saw the mapping between Status and JobExecutionStatus in JobMapper. There is
no need to repeat the code twice. You can either take it out to a new mapper
class, or leave one occurrence and use it from both places.
Line 63: to =
org.ovirt.engine.core.common.job.JobExecutionStatus.class)
Line 64: public static org.ovirt.engine.core.common.job.JobExecutionStatus
map(org.ovirt.engine.api.model.Status status,
Line 65: org.ovirt.engine.core.common.job.JobExecutionStatus
incoming) {
Line 66: if
(JobExecutionStatus.STARTED.name().equals(status.getState().toUpperCase())) {
Line 102: st.setState(JobExecutionStatus.UNKNOWN.name());
Line 103: return st;
Line 104: }
Line 105:
Line 106: @Mapping(from = String.class,
We need an equivalent StepEnum class in rest-api, and the mapping should be
between the two enums, not between an enum and a String.
Line 107: to = org.ovirt.engine.core.common.job.StepEnum.class)
Line 108: public static org.ovirt.engine.core.common.job.StepEnum
map(String type) {
Line 109: if (StepEnum.VALIDATING.name().equals(type.toUpperCase())) {
Line 110: return StepEnum.VALIDATING;
Line 117: }
Line 118: return StepEnum.UNKNOWN;
Line 119: }
Line 120:
Line 121: @Mapping(from = org.ovirt.engine.core.common.job.StepEnum.class,
We need an equivalent StepEnum class in rest-api, and the mapping should be
between the two enums, not between an enum and a String.
Line 122: to = String.class)
Line 123: public static String
map(org.ovirt.engine.core.common.job.StepEnum type) {
Line 124: if (StepEnum.VALIDATING == type) {
Line 125: return StepEnum.VALIDATING.toString();
--
To view, visit http://gerrit.ovirt.org/16161
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1b95a094dc586e6ebbdacd44e0a034e91603274
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
