Michael Pasternak has posted comments on this change.
Change subject: restapi: add storage server connections resource
......................................................................
Patch Set 25: I would prefer that you didn't submit this
(10 inline comments)
few comments inline, but overall well done Alissa!
....................................................
File
backend/manager/modules/restapi/interface/common/jaxrs/src/main/java/org/ovirt/engine/api/common/util/LinkHelper.java
Line 271: map = new ParentToCollectionMap(SnapshotResource.class,
SnapshotsResource.class, VM.class);
Line 272: TYPES.put(Snapshot.class, map);
Line 273:
Line 274: map = new
ParentToCollectionMap(StorageServerConnectionResource.class,
StorageServerConnectionsResource.class);
Line 275: map.add(StorageResource.class, HostStorageResource.class,
Host.class);
why do you replace a old one instead of adding the new relation?
Line 276: TYPES.put(Storage.class, map);
Line 277:
Line 278: map = new ParentToCollectionMap(StorageDomainResource.class,
StorageDomainsResource.class);
Line 279: map.add(AttachedStorageDomainResource.class,
AttachedStorageDomainsResource.class, DataCenter.class);
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 1974: <xs:complexType name="Storage">
Line 1975: <xs:complexContent>
Line 1976: <xs:extension base="BaseResource">
Line 1977: <xs:sequence>
Line 1978: <xs:group ref="BaseStorageConnection"/>
if it's relevant for all connections, worth doing:
1. BaseStorageConnection::BaseResource
2. Storage::BaseStorageConnection
Line 1979: <xs:choice>
Line 1980: <xs:group ref="NfsStorage"/>
Line 1981: <xs:group ref="IscsiStorage"/>
Line 1982: <xs:group ref="IscsiStorageConnection"/>
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageServerConnectionResource.java
Line 23: }
Line 24:
Line 25: @Override
Line 26: public Storage get() {
Line 27: return performGet(VdcQueryType.GetStorageServerConnectionById,
new StorageServerConnectionQueryParametersBase(guid.toString()));
not sure StorageServerConnectionQueryParametersBase should accept string rather
than UUID, if you don't have any use case, please consider replacing it with
UUID
Line 28: }
Line 29:
Line 30: @Override
Line 31: public Storage update(Storage connection) {
Line 28: }
Line 29:
Line 30: @Override
Line 31: public Storage update(Storage connection) {
Line 32: validateParameters(connection, "address", "type", "host");
why update require these fields? it should not have any restrictions at all.
just think of someone want to update 'description', he will have to supply
"address", "type", "host" as well ...
in api we using 'PATCH over PUT' concept for update, i.e on update pass only
what you need to change, all other data reside on the server side and can be
completed at runtime (this why you have in all mapper methods 'entity' and
'template' arguments)
Line 33: validateEnums(Storage.class, connection);
Line 34: return performUpdate(connection,
Line 35: new
QueryIdResolver<String>(VdcQueryType.GetStorageServerConnectionById,
StorageServerConnectionQueryParametersBase.class),
Line 36:
VdcActionType.UpdateStorageServerConnection,
Line 56: return new
StorageServerConnectionParametersBase(connection,hostId);
Line 57: }
Line 58:
Line 59: private Guid getHostId(Host host) {
Line 60: // presence of host ID or name already validated
1. as mentioned, do not demand it, but fetch from the existent
StorageServerConnections (if not specified in the 'incoming')
2. also AFAIR AbstractBackendActionableResource has same method,
worth finding a way to share it between all consumers
Line 61: return host.isSetId()
Line 62: ? new Guid(host.getId())
Line 63: : host.isSetName()
Line 64: ? getEntity(VDS.class,
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageServerConnectionsResource.java
Line 45: StorageConnections collection = new StorageConnections();
Line 46: for
(org.ovirt.engine.core.common.businessentities.StorageServerConnections entity
: entities) {
Line 47: Storage connection = map(entity);
Line 48: if (connection != null) {
Line 49: collection.getStorageConnections().add(connection);
you should be calling 'addLinks(populate(' on the entity you're adding,
otherwise it won't have any links and won't call doPopulate() when All-Content
header on the request path.
Line 50: }
Line 51: }
Line 52: return collection;
Line 53: }
Line 53: }
Line 54:
Line 55: @Override
Line 56: public Response add(Storage storage) {
Line 57: validateParameters(storage, "address", "type", "host");
1. you should be checking not just host existence, but id or name specified,
i.e "host.id|name"
2. does "address" relevant for all storage types? ...
Line 58: // map to backend object
Line 59: StorageServerConnections storageConnection =
Line 60: getMapper(Storage.class,
StorageServerConnections.class).map(storage, null);
Line 61: Guid hostId = getHostId(storage.getHost());
Line 57: validateParameters(storage, "address", "type", "host");
Line 58: // map to backend object
Line 59: StorageServerConnections storageConnection =
Line 60: getMapper(Storage.class,
StorageServerConnections.class).map(storage, null);
Line 61: Guid hostId = getHostId(storage.getHost());
in the way it implemented right now, if no id/name in the host specified, will
be triggered ""Hosts: name=" + storageDomain.getHost().getName()" i.e will be
listed all hosts in the system and as result will be taken first host in the
list instead of host being specified by the user
Line 62: switch (storageConnection.getstorage_type()) {
Line 63: case ISCSI:
Line 64: validateParameters(storage, "iqn", "port");
Line 65: break;
Line 78: getAddParams(storageConnection, hostId),
Line 79: ENTITY_RETRIEVER);
Line 80: }
Line 81:
Line 82: private Guid getHostId(Host host) {
same comment as in BackendStorageServerConnectionResource
Line 83: // presence of host ID or name already validated
Line 84: return host.isSetId()
Line 85: ? new Guid(host.getId())
Line 86: : host.isSetName()
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageServerConnectionResourceTest.java
Line 101: protected void update(boolean canDo, boolean
executeCommandResult, int getConnectionExecTimes) throws Exception {
Line 102: // the below method is called several times because
Line 103: // the mocked behavior must be recorded twice
Line 104: // since the getConnectionById query is executed
Line 105: // twice during a successful update operation.
the reasons for it to be called twice are:
1. combine PATH over PUT missing params from existent entity
2. fetch updated resource for return
Line 106: // not calling it twice causes the test to fail with NPE.
Line 107: for (int i = 0; i < getConnectionExecTimes; i++) {
Line 108: setUpGetEntityExpectations();
Line 109: }
--
To view, visit http://gerrit.ovirt.org/16617
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If6bc32ead098390723825872f6fb292097d52835
Gerrit-PatchSet: 25
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Ori Liel <ol...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
