Allon Mureinik has posted comments on this change. Change subject: core: Add queries for fetching Permitted Storage Domains ......................................................................
Patch Set 2: (12 inline comments) Basically looks good to me (+1), except for a bunch of inline comments. There's quite a bit of them, but not of them are major/critical, IMHO. .................................................... File backend/manager/dbscripts/storages_sp.sql Line 384: Rename to Getstorage_domains_by_STORAGEPOOLID_with_permitted_action to remain consistent with Getstorage_domains_By_storagePoolId. (capitalization is only to make the comment clear - don't capitalize the name this way :-)) Line 391: WHERE storage_pool_id = v_storage_pool_id I'd use an IN or EXISTS construct instead of forcing get_entity_permissions to act as a line function. Line 396: Rename to Getstorage_domainS_by_id_with_permitted_action to remain consistent with Getstorage_domains_by_id. (capitalization is only to make the comment clear - don't capitalize the name this way :-)) Line 403: WHERE id = v_storage_id same. .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetStorageDomainsByVmTemplateIdQuery.java Line 38: Looping over a set of IDs and querying each one separately is simply bad database design - you should be able to do this as a batch operation. However, this flow was already present before this change - not sure this patch is the correct scope to rewrite it. up to you. .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetEntitiesWithPermittedActionParameters.java Line 4: Consider doing this (much welcome!) cleanup in a separate patch. .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java Line 224: GetLunsByVgId, Are these queries strictly for the webadmin? You won't be able to use them in the User Portal once the query permissions feature is enabled, since they are not defined as User Queries. IMHO, this is an oversight - the query is always issued on the user that executed it, and, by definition, returns only the objects relevant to that user. Might as well define it as a user query, even if not strictly required right now. .................................................... File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageDomainDAO.java Line 150: */ Minor issue - to me it seems more intuitive to have the storagePoolId before the actionGroup, can't really explain why. Line 163: */ Same here. .................................................... File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/StorageDomainDAOTest.java Line 320: @Test The convention in this class is to begin all tests with "test", like JUnit 3 standard. It doesn't have any functional impact, but it's more readable. Line 331: @Test same. Line 341: @Test same. -- To view, visit http://gerrit.ovirt.org/3436 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I155cb9f187b951b399e99b54007cf24e3163251a Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
