Alon Bar-Lev has posted comments on this change. Change subject: aaa: Changing ExternalId to String ......................................................................
Patch Set 1: (7 comments) http://gerrit.ovirt.org/#/c/26134/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddPermissionCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddPermissionCommand.java: Line 233: } Line 234: return null; Line 235: } Line 236: } Line 237: // There is no such group in the directory: ? Line 238: return null; Line 239: } Line 240: http://gerrit.ovirt.org/#/c/26134/1/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/IdType.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/IdType.java: Line 1: package org.ovirt.engine.extensions.aaa.builtin.kerberosldap; Line 2: Line 3: import org.ovirt.engine.core.compat.Guid; are you sure you need this function? why not perform conversion on demand? Line 4: Line 5: /** Line 6: * This is used as a wrapper to external ids, to distinguish them from other strings when passed as parameters to ldap Line 7: * calls (external ids need to go encoding) http://gerrit.ovirt.org/#/c/26134/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DbGroupDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DbGroupDAODbFacadeImpl.java: Line 53: Line 54: // In older versions of the engine the internal and external identifiers were the same, so we also need to check Line 55: // if the internal id is really an external id: Line 56: if (domain != null && id != null) { Line 57: externalId = guidToExternalId(id); this should not be after conversion. Line 58: DbGroup existing = getByExternalId(domain, externalId); Line 59: if (existing != null) { Line 60: return existing; Line 61: } Line 131: Line 132: private String guidToExternalId(Guid guid) { Line 133: return "\\x" + guid.toString().replace("-", ""); Line 134: Line 135: } this should not be after conversion. Line 136: http://gerrit.ovirt.org/#/c/26134/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DbUserDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DbUserDAODbFacadeImpl.java: Line 112: Line 113: // In older versions of the engine the internal and external identifiers were the same, so we also need to check Line 114: // if the internal id is really an external id: Line 115: if (domain != null && id != null) { Line 116: externalId = guidToExternalId(id); this should not be after conversion. Line 117: DbUser existing = getByExternalId(domain, externalId); Line 118: if (existing != null) { Line 119: return existing; Line 120: } Line 169: Line 170: private String guidToExternalId(Guid guid) { Line 171: return "\\x" + guid.toString().replace("-", ""); Line 172: Line 173: } this should not be after conversion. Line 174: http://gerrit.ovirt.org/#/c/26134/1/packaging/dbscripts/upgrade/03_05_0180_change_external_id_to_string.sql File packaging/dbscripts/upgrade/03_05_0180_change_external_id_to_string.sql: Line 13: UPDATE users SET Line 14: external_id = substring(external_id from 3 for 8) || '-' || Line 15: substring(external_id from 11 for 4) || '-' || Line 16: substring(external_id from 15 for 4) || '-' || Line 17: substring(external_id from 19 for 4) || '-' || trailing spaces? also this is not reentrant and cannot be kept as you assume that also future ids are uuid, no? you should change only what is at N length and belongs to legacy providers. Line 18: substring(external_id from 23); -- To view, visit http://gerrit.ovirt.org/26134 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I086e6be2e47518238f2011702ee9ef17071fc2b6 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
