Allon Mureinik has posted comments on this change.
Change subject: core: add validation to AddStorageServerConnection
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(8 inline comments)
Something seems to be off with your formatter. In the project's style, we add
spaces around brackets. E.g.: "if(someCondition){" should be changed to "if
(someCondition) {". This repeats throughout the patch.
Also, see specific issues inline.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommand.java
Line 37: isValidConnection = result.getFirst();
Line 38:
Line 39: // Add storage connection to the database.
Line 40: if (isValidConnection) {
Line 41: //if there was no id sent by client, or if there was sent
id, but this id already exists in db - allocate a new id.
Formatting: please start comments with a space
Line 42: //since canDoAction already checks if there is a same
connection in db (same by content, not by id),
Line 43: // we can assume here that if the id received is already
taken - it can be overriden by a new one
Line 44: if(StringUtils.isEmpty(connection.getid()) ||
getConnectionFromDbById(connection.getid()) !=null) {
Line 45: connection.setid(Guid.NewGuid().toString());
Line 39: // Add storage connection to the database.
Line 40: if (isValidConnection) {
Line 41: //if there was no id sent by client, or if there was sent
id, but this id already exists in db - allocate a new id.
Line 42: //since canDoAction already checks if there is a same
connection in db (same by content, not by id),
Line 43: // we can assume here that if the id received is already
taken - it can be overriden by a new one
Why is overriding a connection allowed?
Shouldn't we just fail in this case?
Line 44: if(StringUtils.isEmpty(connection.getid()) ||
getConnectionFromDbById(connection.getid()) !=null) {
Line 45: connection.setid(Guid.NewGuid().toString());
Line 46: }
Line 47: saveConnection(connection);
Line 59: protected StorageServerConnections getConnectionFromDbById(String
connectionId) {
Line 60: return
getDbFacade().getStorageServerConnectionDao().get(connectionId);
Line 61: }
Line 62:
Line 63: protected Pair<Boolean, Integer> connectToStorage() {
Please document this method signature - it's not too intuitive
Line 64: Guid vdsId = getVds().getId();
Line 65: Pair<Boolean, Integer> result = connect(vdsId);
Line 66: return result;
Line 67: }
Line 71: }
Line 72:
Line 73: protected boolean isConnWithSameDetailsExists() {
Line 74: String connection =
getParameters().getStorageServerConnection().getconnection();
Line 75: if
((getDbFacade().getStorageServerConnectionDao().getAllForStorage(connection)).size()
!= 0) {
Just return
(getDbFacade().getStorageServerConnectionDao().getAllForStorage(connection)).size()
!= 0
Line 76: return true;
Line 77: }
Line 78: return false;
Line 79: }
Line 90: if (paramConnection.getstorage_type() == StorageType.POSIXFS
&& (StringUtils.isEmpty(paramConnection.getVfsType()))) {
Line 91: return
failCanDoAction(VdcBllMessages.VALIDATION_STORAGE_CONNECTION_EMPTY_VFSTYPE);
Line 92: }
Line 93: if(isConnWithSameDetailsExists()) {
Line 94: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS);
Don't know if this is just a gerrit foobar, but the indentation seems fishy
here.
Line 95: }
Line 96:
Line 97: if (getParameters().getVdsId().equals(Guid.Empty)) {
Line 98: returnValue = InitializeVds();
....................................................
Commit Message
Line 5: CommitDate: 2013-06-05 15:06:32 +0300
Line 6:
Line 7: core: add validation to AddStorageServerConnection
Line 8:
Line 9: Work in progress...
Please mark the subject with WIP, so this is not merged by mistake.
Line 10: Prevent addition of a new connection if another one with same
Line 11: connection details already exists.
Line 12: Added more unitests for the AddStorageServerConnection command.
Line 13: Also added VAR__TYPE__STORAGE__CONNECTION to VdcBllMessages, so the
connection related
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java
Line 778: }
Line 779:
Line 780: private void cleanConnection(StorageServerConnections connection,
Guid hostId) {
Line 781: //if create connection command was the one to fail and didn't
create a connection
Line 782: //then the id of connection will be empty, and there's
nothing to delete.
Formatting - please start comment with a space
Line 783: if(connection.getid()!=null &&
!connection.getid().equalsIgnoreCase("")) {
Line 784:
Frontend.RunAction(VdcActionType.RemoveStorageServerConnection, new
StorageServerConnectionParametersBase(connection, hostId),
Line 785: null, this);
Line 786: }
Line 779:
Line 780: private void cleanConnection(StorageServerConnections connection,
Guid hostId) {
Line 781: //if create connection command was the one to fail and didn't
create a connection
Line 782: //then the id of connection will be empty, and there's
nothing to delete.
Line 783: if(connection.getid()!=null &&
!connection.getid().equalsIgnoreCase("")) {
why is the case interesting when comparing to an empty string?
Line 784:
Frontend.RunAction(VdcActionType.RemoveStorageServerConnection, new
StorageServerConnectionParametersBase(connection, hostId),
Line 785: null, this);
Line 786: }
Line 787:
--
To view, visit http://gerrit.ovirt.org/15388
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If9bcef8a413589654d36db8d878c844550ae6066
Gerrit-PatchSet: 2
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>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
