Ori Liel has posted comments on this change.
Change subject: restapi: add VmPayload support
......................................................................
Patch Set 5: (10 inline comments)
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmResource.java
Line 79: setPayload(vm);
let's move 'setPayload(vm)' to populate() method. This will take care of both
get() and update() flows.
Line 103: }
I think we can avoid the code duplication. You have a reference to 'parent';
this is an instance of BackendVmsResource. Let's change setPayload() there to
protected, and then (since these two classes are in the same package) you can
simply call: parent.setPayload().
Line 126: return retVal;
Move call to setPayload() in populate() (takes care of get() flow as well).
Line 395: return params;
Let's add to VmMapper a method that maps:
(restapi) VmPayloads--> (backend) VmPayload
Then reference it from here and from BackendVmsResource
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmsResource.java
Line 103:
I know that since REST-API passes to Backend a VmStatic for create (why???)
then we can't map the payload in the mapper: VM-->VmStatic, because this field
isn't exposed on VmStatic.
But we can add to VmMapper a method that maps:
(restapi) VmPayloads--> (backend) VmPayload
And then reference it from here and from BackendVmResource
Line 317: setPayload(vm);
Better to put the call 'setPayload(vm)' inside the method populate().
That's what this method is for - it adds any information you want to the
collection objects after they have been mapped.
Line 324: try {
1) Please catch WebFaultException (not the generic 'Exception')
2) Please rephrase the TODO in a more informative way:
"'getEntity()' always throws an exception if the entity is not found. It should
be refactored to make it the programmer's decision. In this context it's legal
to not receive a payload for this VM, so the exception is caught and ignored."
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java
Line 837: model.setType(entity.getType().getName());
There's probably no reason why type would be null, but just to be safe I'd add
the validation (if entity.getType()!=null).
Line 851: model.setFileName(entity.getFile().getName());
I think we need to add a null validation here too. This code would crash if
user didn't supply <file> within <vm_payload>. If <file> is mandaory we will
validate for it elsewhere, but the mapper shouldn't be in risk of NPE.
Use entity.isSetFile() which was automatically generated by jaxb.
Line 853:
same here, verify for null
--
To view, visit http://gerrit.ovirt.org/3460
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I75d42a1963e537ff4e2f1fdd50ee7f2b427c6076
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <[email protected]>
Gerrit-Reviewer: Ori Liel <[email protected]>
Gerrit-Reviewer: Shahar Havivi <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
