Ori Liel has posted comments on this change.
Change subject: restapi: #854479 Attach Disk To VM - Return Disk In Response
......................................................................
Patch Set 3: (1 inline comment)
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmDisksResource.java
Line 56: if (disk.isSetId()) {
Line 57: return Response.fromResponse(attachDiskToVm(disk))
Line 58:
.entity(resolveCreated(Guid.createGuidFromString(disk.getId()),
Line 59: getEntityIdResolver(null),
Line 60: VM.class)).build();
Actually there's a reason why I didn't do that.
performCreation() is meant for creation flows: flows where an object is created
and Backend returns its ID in the response. REST-API then resolves the object
using this ID. When we attach a disk to a VM this is not a creation operation,
it's just a regular action, and the Backend does not return the ID of the disk
in the response. Trying to resolve using this non-existent ID would fail:
==========================================================
from performCreation():
---------------------------------
...
R model = resolveCreated(createResult, entityResolver, suggestedParentType);
...
from resolveCreated():
-------------------------------
...
Q created = entityResolver.resolve(result.getActionReturnValue());
...
(**notice how the ID is supplied to the resolver from the Backend response**)
==========================================================
In order to resolve this disk in this flow, we need to use the ID that was
supplied by the user. I could change performCreation() in a way that receives a
flag which tells it how to resolve the object (from response, or using the
user-supplied object), or I can could the behavior to trying to resolve the
object using the response, and if that fails - checking if the ID exists in the
object that the user supplied, and then trying to resolve using that. But I
feel that would contaminate and over-complicate performCreation(), which is
meant - as its name suggests - for creation flows.
What we need is a new flow for performing an action and returning an object,
and we have an open RFE for this. In my opinion, either we do this RFE now, or
we push the change as is and revisit it when we have the proper support - but
abusing performCreation() to serve non-creation flows doesn't seem to me like
the right path to take.
Line 61: }else {
Line 62: return super.add(disk);
Line 63: }
Line 64: }
--
To view, visit http://gerrit.ovirt.org/8335
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1a50077886734f73842141262f5c338979dcae5c
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ori Liel <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Ori Liel <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
