Liron Aravot has uploaded a new change for review. Change subject: core: avoid saving duplicate records when saving LUN connections ......................................................................
core: avoid saving duplicate records when saving LUN connections When adding a LUN to the db, StorageHandlingCommandBase.proceedLUNInDb() is executed. Among its other operations, this method checks if connections with the same details as the LUN connections already exists by executing Getstorage_server_connectionsByKey and if not a new connection record is being saved, otherwise the existing one is used. Getstorage_server_connectionsByKey currently checks if connection with the same details exists according to the iqn, connection, port, portal username and password fields - the problem is that those fields could be set and persisted with with empty string or null, which will lead to duplicate records. *This patch only avoid having empty string in those fields - further patch will fix the current state in the db and will add proper contraints. *Further fix that's done in this patch is to remove the password field from Getstorage_server_connectionsByKey - each time we save/fetch a connection from the db by it's details we encrypt the password - as the encrypted password is different each time, we'll never be able to fetch an existing record. Change-Id: I997ba0fbc6e984a9e6782502c6b36b1f9a1f3d8c Bug-Url: https://bugzilla.redhat.com/1173951 Signed-off-by: Liron Aravot <[email protected]> --- M backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StorageServerConnections.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageServerConnectionDAODbFacadeImpl.java M backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/StorageServerConnectionDAOTest.java M backend/manager/modules/dal/src/test/resources/fixtures.xml M packaging/dbscripts/storages_san_sp.sql 5 files changed, 107 insertions(+), 37 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/36/36236/1 diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StorageServerConnections.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StorageServerConnections.java index 3564203..dcca1e9 100644 --- a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StorageServerConnections.java +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StorageServerConnections.java @@ -26,19 +26,19 @@ NfsVersion nfsVersion, Short nfsRetrans, Short nfsTimeo) { - this.connection = connection; - this.id = id; - this.iqn = iqn; - this.password = password; - this.storageType = storageType; - this.username = userName; - this.port = port; - this.portal = portal; - this.vfsType = vfsType; - this.mountOptions = mountOptions; - this.nfsVersion = nfsVersion; - this.nfsRetrans = nfsRetrans; - this.nfsTimeo = nfsTimeo; + setconnection(connection); + setid(id); + setiqn(iqn); + setpassword(password); + setstorage_type(storageType); + setuser_name(userName); + setport(port); + setportal(portal); + setVfsType(vfsType); + setMountOptions(mountOptions); + setNfsVersion(nfsVersion); + setNfsRetrans(nfsRetrans); + setNfsTimeo(nfsTimeo); } public StorageServerConnections(String connection, @@ -80,7 +80,7 @@ } public void setiqn(String value) { - this.iqn = value; + this.iqn = getStringValueToSet(value); } private String port; @@ -90,7 +90,7 @@ } public void setport(String value) { - this.port = value; + this.port = getStringValueToSet(value); } private String portal = DEFAULT_TPGT; @@ -100,7 +100,7 @@ } public void setportal(String value) { - this.portal = value; + this.portal = getStringValueToSet(value); } private String password; @@ -110,7 +110,7 @@ } public void setpassword(String value) { - this.password = value; + this.password = getStringValueToSet(value); } private StorageType storageType; @@ -130,7 +130,7 @@ } public void setuser_name(String value) { - this.username = value; + this.username = getStringValueToSet(value); } private String mountOptions; @@ -260,4 +260,11 @@ this.nfsRetrans = nfsRetrans; } + private static String getStringValueToSet(String value) { + if (value != null && value.isEmpty()) { + return null; + } + + return value; + } } diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageServerConnectionDAODbFacadeImpl.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageServerConnectionDAODbFacadeImpl.java index 8303281..780ec8e 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageServerConnectionDAODbFacadeImpl.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageServerConnectionDAODbFacadeImpl.java @@ -103,6 +103,9 @@ @Override public List<StorageServerConnections> getAllForConnection( StorageServerConnections connection) { + // NOTE - any change to this stored procedure parameters should require a change in + // the StorageServerConnections class, as those fields can be set only with null or a + // actual value (empty string can't be used). return getCallsHandler().executeReadList("Getstorage_server_connectionsByKey", mapper, getCustomMapSqlParameterSource() @@ -110,8 +113,7 @@ .addValue("connection", connection.getconnection()) .addValue("port", connection.getport()) .addValue("portal", connection.getportal()) - .addValue("username", connection.getuser_name()) - .addValue("password", DbFacadeUtils.encryptPassword(connection.getpassword()))); + .addValue("username", connection.getuser_name())); } @Override diff --git a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/StorageServerConnectionDAOTest.java b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/StorageServerConnectionDAOTest.java index 3b83ce1..a3672b8 100644 --- a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/StorageServerConnectionDAOTest.java +++ b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/StorageServerConnectionDAOTest.java @@ -250,6 +250,69 @@ assertEquals(SERVER_CONNECTION_COUNT_FOR_SPECIFIC_STORAGE, result.size()); } + + @Test + public void getAllForForConnection() { + StorageServerConnections conn = dao.get(existingConnection.getid()); + conn.setid("copy"); + dao.save(conn); + assertGetAllForConnectionResult(Arrays.asList(existingConnection, conn), existingConnection); + } + + + @Test + public void getAllForForConnectionWithNullValues() { + StorageServerConnections noNullValConnection = + createConnection("id1", "connection", null, "username", "password", "portal", "port"); + StorageServerConnections noNullValConnection2 = + createConnection("id11", "connection", null, "username", "password", "portal", "port"); + assertGetAllForConnectionResult(Arrays.asList(noNullValConnection, noNullValConnection2), noNullValConnection); + + noNullValConnection = createConnection("id2", "connection", "iqn", null, "password", "portal", "port"); + noNullValConnection2 = createConnection("id12", "connection", "iqn", null, "password", "portal", "port"); + assertGetAllForConnectionResult(Arrays.asList(noNullValConnection, noNullValConnection2), noNullValConnection); + + // testing with different passwords to see that it's not being considered as part of the stored procedure. + noNullValConnection = createConnection("id3", "connection", "iqn", "username", "pass1", "portal", "port"); + noNullValConnection2 = createConnection("id13", "connection", "iqn", "username", "pass2", "portal", "port"); + assertGetAllForConnectionResult(Arrays.asList(noNullValConnection, noNullValConnection2), noNullValConnection); + + noNullValConnection = createConnection("id4", "connection", "iqn", "username", "password", null, "port"); + noNullValConnection2 = createConnection("id14", "connection", "iqn", "username", "password", null, "port"); + assertGetAllForConnectionResult(Arrays.asList(noNullValConnection, noNullValConnection2), noNullValConnection); + + noNullValConnection = createConnection("id5", "connection", "iqn", "username", "password", "portal", null); + noNullValConnection2 = createConnection("id15", "connection", "iqn", "username", "password", "portal", null); + assertGetAllForConnectionResult(Arrays.asList(noNullValConnection, noNullValConnection2), noNullValConnection); + + noNullValConnection = createConnection("id6", "b", null, null, null, null, null); + noNullValConnection2 = createConnection("id16", "b", null, null, null, null, null); + assertGetAllForConnectionResult(Arrays.asList(noNullValConnection, noNullValConnection2), noNullValConnection); + } + + private StorageServerConnections createConnection(String id, + String connection, + String iqn, + String username, + String password, + String portal, + String port) { + StorageServerConnections newConn = new StorageServerConnections(); + newConn.setid(id); + newConn.setconnection(connection); + newConn.setiqn(iqn); + newConn.setportal(portal); + newConn.setport(port); + newConn.setuser_name(username); + newConn.setpassword(password); + dao.save(newConn); + return newConn; + } + + private void assertGetAllForConnectionResult(List<StorageServerConnections> expected, StorageServerConnections forQuery) { + assertTrue(CollectionUtils.disjunction(expected, dao.getAllForConnection(forQuery)).isEmpty()); + } + /** * Ensures saving a connection works as expected. */ diff --git a/backend/manager/modules/dal/src/test/resources/fixtures.xml b/backend/manager/modules/dal/src/test/resources/fixtures.xml index fa1bf96..2e416dd 100644 --- a/backend/manager/modules/dal/src/test/resources/fixtures.xml +++ b/backend/manager/modules/dal/src/test/resources/fixtures.xml @@ -164,8 +164,8 @@ <null /> <null /> <null /> - <value></value> - <value></value> + <null /> + <null /> <value>1</value> <value></value> <value>nfs</value> @@ -179,8 +179,8 @@ <null /> <null /> <null /> - <value></value> - <value></value> + <null /> + <null /> <value>2</value> <value></value> <value>nfs</value> @@ -194,8 +194,8 @@ <null /> <null /> <null /> - <value></value> - <value></value> + <null /> + <null /> <value>1</value> <value></value> <value>nfs</value> @@ -208,8 +208,8 @@ <value>10.35.64.25:/export/share</value> <null /> <null /> - <value></value> - <value></value> + <value>iqn</value> + <null /> <value>1</value> <value>3</value> <value>rw</value> diff --git a/packaging/dbscripts/storages_san_sp.sql b/packaging/dbscripts/storages_san_sp.sql index 3bbc752..e0f9241 100644 --- a/packaging/dbscripts/storages_san_sp.sql +++ b/packaging/dbscripts/storages_san_sp.sql @@ -438,7 +438,7 @@ BEGIN RETURN QUERY SELECT * FROM storage_server_connections - WHERE iqn = v_iqn; + WHERE iqn = v_iqn OR iqn IS NULL AND v_iqn IS NULL; END; $procedure$ LANGUAGE plpgsql; @@ -463,18 +463,16 @@ v_connection VARCHAR(250), v_port VARCHAR(50) , v_portal VARCHAR(50) , - v_username VARCHAR(50) , - v_password text) RETURNS SETOF storage_server_connections STABLE + v_username VARCHAR(50)) RETURNS SETOF storage_server_connections STABLE AS $procedure$ BEGIN RETURN QUERY SELECT * FROM storage_server_connections - WHERE (iqn = v_iqn or iqn is NULL) and - (connection = v_connection) and - (port = v_port or port is NULL) and - (portal = v_portal or portal is NULL) and - (user_name = v_username or user_name is NULL) and - (password = v_password or password is NULL); + WHERE (iqn = v_iqn or (iqn IS NULL AND v_iqn IS NULL)) AND + (connection = v_connection) AND + (port = v_port or (port IS NULL AND v_port IS NULL)) AND + (portal = v_portal or (portal is NULL AND v_portal IS NULL)) AND + (user_name = v_username or (user_name is NULL AND v_username IS NULL)); END; $procedure$ LANGUAGE plpgsql; -- To view, visit http://gerrit.ovirt.org/36236 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I997ba0fbc6e984a9e6782502c6b36b1f9a1f3d8c Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: ovirt-engine-3.5 Gerrit-Owner: Liron Aravot <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
