----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74610/#review225792 -----------------------------------------------------------
agents-common/src/main/java/org/apache/ranger/plugin/util/SearchFilter.java Lines 51 (patched) <https://reviews.apache.org/r/74610/#comment314213> consider replacing search parameters startDate and endDate with the following. This will enable search by start/end for both create and update: - createTimeStart - createTimeEnd - updateTimeStart - updateTimeEnd security-admin/src/main/java/org/apache/ranger/biz/GdsDBStore.java Lines 145 (patched) <https://reviews.apache.org/r/74610/#comment314214> - mapDatasetsToDatasetHeaders() => toDatasetHeaders() - this method uses only GdsPermission from the filter. Consider replacing parameter filter with gdsPermission: - toDatasetHeaders(List<RangerDataset> datasets, GdsPermission gdsPermission) - move private methods after all pulic and protected methods, to be consitent with rest of Ranger code base security-admin/src/main/java/org/apache/ranger/service/RangerBaseModelService.java Lines 400 (patched) <https://reviews.apache.org/r/74610/#comment314221> getResourceCount() - is this method needed, given: - it simply forwards to existing method getCountForSearchQuery() - RangerBaseModelService, the base class for several implementations, is not the appropriate class for this method security-admin/src/main/java/org/apache/ranger/service/RangerGdsDatasetService.java Lines 54 (patched) <https://reviews.apache.org/r/74610/#comment314222> Changing the default search to PARTIAL will make it difficult to search by exact name. Consider retaining search fields for introducing DATASET_NAME, PROJECT_NAME, DATA_SHARE_NAME and introduce search fields for for partial search, like: - searchFields.add(new SearchField(SearchFilter.DATASET_NAME_PARTIAL, "obj.name", SearchField.DATA_TYPE.STRING, SearchField.SEARCH_TYPE.PARTIAL)); - searchFields.add(new SearchField(SearchFilter.PROJECT_NAME_PARTIAL, "proj.name", SearchField.DATA_TYPE.STRING, SearchField.SEARCH_TYPE.PARTIAL, "XXGdsDatasetInProject dip, XXGdsProject proj", "obj.id = dip.datasetId and dip.projectId = proj.id")); - searchFields.add(new SearchField(SearchFilter.DATA_SHARE_NAME_PARTIAL, "dsh.name", SearchField.DATA_TYPE.STRING, SearchField.SEARCH_TYPE.PARTIAL, "XXGdsDataShareInDataset dshid, XXGdsDataShare dsh", "obj.id = dshid.datasetId and dshid.dataShareId = dsh.id")); security-admin/src/main/java/org/apache/ranger/service/RangerGdsSharedResourceService.java Lines 251 (patched) <https://reviews.apache.org/r/74610/#comment314220> Is method RangerGdsSharedResourceService.getResourceCountInDataset() needed, given this relies on filter parameter already having the datasetId set in SearchFilter.DATASET_ID? I suggest to either remove this method or replacing it with: public Long getResourceCountInDataset(long datasetId) { SearchFilter filter = new SearchFilter(); filter.setParam(SearchFilter.DATASET_ID, String.valueOf(datasetId)); Long ret = super.getCountForSearchQuery(filter, searchFields); return ret; } Review following methods as well: - RangerGdsDataShareInDatasetService.getDataShareInDatasetCount() - RangerGdsDatasetInProjectService.getDatasetsInProjectCount() security-admin/src/main/java/org/apache/ranger/view/RangerDatasetHeader.java Lines 23 (patched) <https://reviews.apache.org/r/74610/#comment314218> All classes used in REST APIs should be in agents-commons module, preferrably in org.apache.ranger.plugin.model package. security-admin/src/main/java/org/apache/ranger/view/RangerDatasetHeader.java Lines 27 (patched) <https://reviews.apache.org/r/74610/#comment314217> - consider replacing dataSharesActiveCount and dataSharesPendingCount with the following: private Map<GdsShareStatus, Long> dataSharesCountByStatus; - consider replacing usersCount, groupsCount and rolesCount with the following: private Map<PrincipalType, Long> principalsCountByType; security-admin/src/main/java/org/apache/ranger/view/RangerDatasetHeader.java Lines 33 (patched) <https://reviews.apache.org/r/74610/#comment314219> permission => permissionForCaller security-admin/src/main/resources/META-INF/jpa_named_queries.xml Lines 2125 (patched) <https://reviews.apache.org/r/74610/#comment314215> - this query looks for policies for a given serviceType (not serviceName). If yes, please rename findByServiceName => findByServiceType security-admin/src/main/resources/META-INF/jpa_named_queries.xml Lines 2258 (patched) <https://reviews.apache.org/r/74610/#comment314216> it might be efficient to return datashare counts for all status in one call: SELECT obj.status, count(*) FROM XXGdsDataShareInDataset obj WHERE obj.datasetId = :datasetId GROUP BY obj.status - Madhan Neethiraj On Sept. 26, 2023, 5:20 p.m., Subhrat Chaudhary wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74610/ > ----------------------------------------------------------- > > (Updated Sept. 26, 2023, 5:20 p.m.) > > > Review request for ranger, Anand Nadar, Ankita Sinha, Madhan Neethiraj, > Monika Kachhadiya, and Prashant Satam. > > > Bugs: https://reviews.apache.org/r/74532/bugs/RANGER-4323 > > https://issues.apache.org/jira/browse/https://reviews.apache.org/r/74532/bugs/RANGER-4323 > > > Repository: ranger > > > Description > ------- > > We need a new API to get dataset header info, for dataset listing page, based > on ACL evaluation for the logged in user. It supports: > 1. Pagination > 2. GDSPermission as query param, based on which ACL evaluation is done > 3. Following counts (in case the logged in user has VIEW or higher > permission): dataSharesActiveCount, dataSharesPendingCount, usersCount, > groupsCount, rolesCount, projectsCount, resourceCount. > 4. Permission for current user (in case GDSPermission in query-param is LIST) > 5. Sort by creatTime > 6. Search by startDate/endDate > 7. Partial search by dataset and datashare name > > > Diffs > ----- > > agents-common/src/main/java/org/apache/ranger/plugin/util/SearchFilter.java > 1a1a78064 > security-admin/src/main/java/org/apache/ranger/biz/GdsDBStore.java > ff6b2b23e > security-admin/src/main/java/org/apache/ranger/common/RangerSearchUtil.java > 51da7d15d > > security-admin/src/main/java/org/apache/ranger/db/XXGdsDataShareInDatasetDao.java > 52c441104 > security-admin/src/main/java/org/apache/ranger/db/XXPolicyDao.java > f020acb21 > security-admin/src/main/java/org/apache/ranger/rest/GdsREST.java 653e397d4 > > security-admin/src/main/java/org/apache/ranger/security/context/RangerAPIList.java > de9014072 > > security-admin/src/main/java/org/apache/ranger/service/RangerBaseModelService.java > 4128d70df > > security-admin/src/main/java/org/apache/ranger/service/RangerGdsDataShareInDatasetService.java > d32282c27 > > security-admin/src/main/java/org/apache/ranger/service/RangerGdsDatasetInProjectService.java > 0ed51c249 > > security-admin/src/main/java/org/apache/ranger/service/RangerGdsDatasetService.java > 747cc9f17 > > security-admin/src/main/java/org/apache/ranger/service/RangerGdsSharedResourceService.java > 6a963da60 > > security-admin/src/main/java/org/apache/ranger/validation/RangerGdsValidator.java > 374ac046d > > security-admin/src/main/java/org/apache/ranger/view/RangerDatasetHeader.java > PRE-CREATION > security-admin/src/main/resources/META-INF/jpa_named_queries.xml f02101f09 > > > Diff: https://reviews.apache.org/r/74610/diff/1/ > > > Testing > ------- > > Validated: > 1. ACL evaluation based on GDSPermission in query-param > 2. Pagination for the records returned > 3. Confirmed the counts are returned based on the data available: > dataSharesActiveCount, dataSharesPendingCount, usersCount, groupsCount, > rolesCount, projectsCount, resourceCount > 4. Search functionality by startDate/endDate > 5. Partial search by dataset and datashare name > > > Thanks, > > Subhrat Chaudhary > >
