From 5377a9f21a50a2aa7d1707e5dcc01008d0b9c38b Mon Sep 17 00:00:00 2001 From: Matt Ward Date: Mon, 30 Jan 2017 10:02:10 +0000 Subject: [PATCH] Merged mward/repo-1600-zonesfilter (5.2.1) to 5.2.N (5.2.1) 134741 mward: REPO-1600: added support for zone filtering to GET /people/{personId}/groups Also brought in-line with spec to return an empty list rather than a 404 if the zone is non-existent. git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/BRANCHES/DEV/5.2.N/root@134806 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261 --- .../alfresco/rest/api/impl/GroupsImpl.java | 98 +++++++++++---- .../alfresco/rest/api/tests/GroupsTest.java | 116 +++++++++++++++--- 2 files changed, 177 insertions(+), 37 deletions(-) diff --git a/source/java/org/alfresco/rest/api/impl/GroupsImpl.java b/source/java/org/alfresco/rest/api/impl/GroupsImpl.java index 89c431bdf6..b293f57eea 100644 --- a/source/java/org/alfresco/rest/api/impl/GroupsImpl.java +++ b/source/java/org/alfresco/rest/api/impl/GroupsImpl.java @@ -42,6 +42,7 @@ import java.util.Set; import java.util.stream.Collectors; import org.alfresco.query.CannedQueryPageDetails; +import org.alfresco.query.EmptyPagingResults; import org.alfresco.query.PagingRequest; import org.alfresco.query.PagingResults; import org.alfresco.repo.security.authentication.AuthenticationUtil; @@ -193,22 +194,7 @@ public class GroupsImpl implements Groups List zonesParam = propertyWalker.getZones(); if (zonesParam != null) { - if (zonesParam.size() > MAX_ZONES) - { - throw new IllegalArgumentException("A maximum of " + MAX_ZONES + " zones may be specified."); - } - else if (zonesParam.isEmpty()) - { - throw new IllegalArgumentException("Zones filter list cannot be empty."); - } - // Validate each zone name - zonesParam.forEach(zone -> { - if (zone.isEmpty()) - { - throw new IllegalArgumentException("Zone name cannot be empty (i.e. '')"); - } - }); - + validateZonesParam(zonesParam); zoneFilter = zonesParam.get(0); } @@ -224,7 +210,8 @@ public class GroupsImpl implements Groups } catch (UnknownAuthorityException e) { - throw new EntityNotFoundException("Zone "+zoneFilter+" does not exist."); + // Non-existent zone + pagingResult = new EmptyPagingResults<>(); } // Create response. @@ -249,6 +236,25 @@ public class GroupsImpl implements Groups return CollectionWithPagingInfo.asPaged(paging, groups, pagingResult.hasMoreItems(), totalItems); } + private void validateZonesParam(List zonesParam) + { + if (zonesParam.size() > MAX_ZONES) + { + throw new IllegalArgumentException("A maximum of " + MAX_ZONES + " zones may be specified."); + } + else if (zonesParam.isEmpty()) + { + throw new IllegalArgumentException("Zones filter list cannot be empty."); + } + // Validate each zone name + zonesParam.forEach(zone -> { + if (zone.isEmpty()) + { + throw new IllegalArgumentException("Zone name cannot be empty (i.e. '')"); + } + }); + } + @Override public CollectionWithPagingInfo getGroupsByPersonId(String requestedPersonId, Parameters parameters) { @@ -265,6 +271,20 @@ public class GroupsImpl implements Groups throw new PermissionDeniedException(); } + Query q = parameters.getQuery(); + List zonesParam = null; + if (q != null) + { + GroupsQueryWalker propertyWalker = new GroupsQueryWalker(LIST_GROUPS_EQUALS_QUERY_PROPERTIES, null); + QueryHelper.walk(q, propertyWalker); + zonesParam = propertyWalker.getZones(); + if (zonesParam != null) + { + validateZonesParam(zonesParam); + } + } + final String zoneFilter = zonesParam != null ? zonesParam.get(0) : null; + final List includeParam = parameters.getInclude(); Paging paging = parameters.getPaging(); @@ -280,6 +300,7 @@ public class GroupsImpl implements Groups // a suitable list of AuthorityInfo objects. List groupAuthorities = userAuthorities.stream(). filter(a -> a.startsWith(AuthorityType.GROUP.getPrefixString())). + filter(a -> zonePredicate(a, zoneFilter)). map(this::getAuthorityInfo). sorted(new AuthorityInfoComparator(sortProp.getFirst(), sortProp.getSecond())). collect(Collectors.toList()); @@ -292,8 +313,7 @@ public class GroupsImpl implements Groups // Transform the page of results into Group objects final Set rootAuthorities = getAllRootAuthorities(AuthorityType.GROUP); - // Zone filter not supported currently. - final String zoneFilter = null; + List groups = page.stream(). map(authority -> getGroup(authority, includeParam, rootAuthorities)). collect(Collectors.toList()); @@ -316,7 +336,7 @@ public class GroupsImpl implements Groups // list of root authorities. List authorities = rootAuthorities.stream(). map(this::getAuthorityInfo). - filter(auth -> matchZoneFilter(auth, zoneFilter)). + filter(auth -> zonePredicate(auth.getAuthorityName(), zoneFilter)). collect(Collectors.toList()); groupList = new ArrayList<>(rootAuthorities.size()); groupList.addAll(authorities); @@ -367,12 +387,44 @@ public class GroupsImpl implements Groups return pagingResult; } - private boolean matchZoneFilter(AuthorityInfo authority, String zone) + /** + * Checks to see if the named group authority should be included in results + * when filtered by zone. + * + * @see #zonePredicate(Set, String) + * @param groupName + * @param zone + * @return true if result should be included. + */ + private boolean zonePredicate(String groupName, String zone) { + Set zones = null; if (zone != null) { - Set zones = authorityService.getAuthorityZones(authority.getAuthorityName()); - return zones.contains(zone); + // Don't bother doing this lookup unless the zone filter is non-null. + zones = authorityService.getAuthorityZones(groupName); + } + return zonePredicate(zones, zone); + } + + /** + * Checks a list of zones to see if it matches the supplied zone filter + * ({@code requiredZone} parameter). + *

+ * If the requiredZone parameter is null, then the filter will not be applied (returns true.) + *

+ * If the requiredZone parameter is not null (i.e. a filter must be applied) and the + * {@code zones}) list is {@code null} then the predicate will return false. + * + * @param zones + * @param requiredZone + * @return true if result should be included. + */ + private boolean zonePredicate(Set zones, String requiredZone) + { + if (requiredZone != null) + { + return zones != null && zones.contains(requiredZone); } return true; } diff --git a/source/test-java/org/alfresco/rest/api/tests/GroupsTest.java b/source/test-java/org/alfresco/rest/api/tests/GroupsTest.java index 6304dc2f55..7a9a269521 100644 --- a/source/test-java/org/alfresco/rest/api/tests/GroupsTest.java +++ b/source/test-java/org/alfresco/rest/api/tests/GroupsTest.java @@ -25,16 +25,6 @@ */ package org.alfresco.rest.api.tests; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; - -import java.util.*; - -import javax.servlet.http.HttpServletResponse; - import org.alfresco.repo.security.authentication.AuthenticationUtil; import org.alfresco.rest.AbstractSingleNetworkSiteTest; import org.alfresco.rest.api.tests.client.PublicApiClient; @@ -54,6 +44,11 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; +import javax.servlet.http.HttpServletResponse; +import java.util.*; + +import static org.junit.Assert.*; + /** * V1 REST API tests for managing Groups * @@ -415,6 +410,12 @@ public class GroupsTest extends AbstractSingleNetworkSiteTest assertEquals(1, groups.size()); assertEquals(rootGroup, groups.get(0)); assertTrue(groups.get(0).getZones().contains("APITEST.MYZONE")); + + // Zone that doesn't exist. + otherParams.put("where", "(isRoot=true AND zones in ('I.DO.NOT.EXIST'))"); + response = getGroups(paging, otherParams, "Incorrect response", 200); + groups = response.getList(); + assertTrue(groups.isEmpty()); } // Filter zones while using where isRoot=false @@ -434,6 +435,12 @@ public class GroupsTest extends AbstractSingleNetworkSiteTest assertEquals(groupB, groups.get(1)); // We haven't included the zones info. groups.forEach(group -> assertNull(group.getZones())); + + // Zone that doesn't exist. + otherParams.put("where", "(isRoot=false AND zones in ('I.DO.NOT.EXIST'))"); + response = getGroups(paging, otherParams, "Incorrect response", 200); + groups = response.getList(); + assertTrue(groups.isEmpty()); } // -ve test: invalid zones clause @@ -458,10 +465,6 @@ public class GroupsTest extends AbstractSingleNetworkSiteTest otherParams.put("where", "(zones in ('', 'APP.DEFAULT', '', 'APITEST.MYZONE'))"); getGroups(paging, otherParams, "Incorrect response", 400); - // A zone that isn't in use or doesn't exist - otherParams.put("where", "(zones in ('NON.EXISTENT'))"); - getGroups(paging, otherParams, "Incorrect response", 404); - // OR operator not currently supported otherParams.put("where", "(isRoot=true OR zones in ('APP.DEFAULT'))"); getGroups(paging, otherParams, "Incorrect response", 400); @@ -815,6 +818,91 @@ public class GroupsTest extends AbstractSingleNetworkSiteTest getGroupsByPersonId(personAlice.getId(), paging, otherParams, HttpServletResponse.SC_BAD_REQUEST); } + + // Filter by zone, use the -me- alias. + { + Map params = new HashMap<>(); + params.put("include", org.alfresco.rest.api.Groups.PARAM_INCLUDE_ZONES); + + params.put("where", "(zones in ('APP.DEFAULT'))"); + + // Use the -me- alias + ListResponse response = groupsProxy. + getGroupsByPersonId("-me-", params, "Couldn't get user's groups", 200); + List groups = response.getList(); + + assertFalse(groups.isEmpty()); + // All groups should contain the selected zone. + groups.forEach(group -> assertTrue(group.getZones().contains("APP.DEFAULT"))); + } + + // Filter by zone, use the -me- alias. + { + Map params = new HashMap<>(); + params.put("include", org.alfresco.rest.api.Groups.PARAM_INCLUDE_ZONES); + + params.put("where", "(zones in ('APITEST.MYZONE'))"); + + // Use the -me- alias + ListResponse response = groupsProxy. + getGroupsByPersonId("-me-", params, "Couldn't get user's groups", 200); + List groups = response.getList(); + + assertEquals(3, groups.size()); + // All groups should contain the selected zone. + groups.forEach(group -> assertTrue(group.getZones().contains("APITEST.MYZONE"))); + } + + // Filter by zone - use the person's ID, without "include"-ing zones + { + Map params = new HashMap<>(); + params.put("where", "(zones in ('APITEST.ANOTHER'))"); + + ListResponse response = groupsProxy. + getGroupsByPersonId(personAlice.getId(), params, "Couldn't get user's groups", 200); + List groups = response.getList(); + + assertEquals(1, groups.size()); + // We haven't included the zone info + groups.forEach(group -> assertNull(group.getZones())); + } + + { + // Zone that doesn't exist. + Map params = new HashMap<>(); + params.put("where", "(isRoot=false AND zones in ('I.DO.NOT.EXIST'))"); + ListResponse response = groupsProxy. + getGroupsByPersonId(personAlice.getId(), params, "Incorrect response", 200); + List groups = response.getList(); + assertTrue(groups.isEmpty()); + } + + // -ve test: invalid zones clause + { + Paging paging = getPaging(0, Integer.MAX_VALUE); + Map otherParams = new HashMap<>(); + otherParams.put("include", org.alfresco.rest.api.Groups.PARAM_INCLUDE_ZONES); + + // Empty zone list + otherParams.put("where", "(zones in ())"); + groupsProxy.getGroupsByPersonId(personAlice.getId(), otherParams, "Incorrect response", 400); + + // Empty zone name + otherParams.put("where", "(zones in (''))"); + groupsProxy.getGroupsByPersonId(personAlice.getId(), otherParams, "Incorrect response", 400); + + // Too many zones + otherParams.put("where", "(zones in ('APP.DEFAULT', 'APITEST.MYZONE'))"); + groupsProxy.getGroupsByPersonId(personAlice.getId(), otherParams, "Incorrect response", 400); + + // "A series of unfortunate errors" + otherParams.put("where", "(zones in ('', 'APP.DEFAULT', '', 'APITEST.MYZONE'))"); + groupsProxy.getGroupsByPersonId(personAlice.getId(), otherParams, "Incorrect response", 400); + + // OR operator not currently supported + otherParams.put("where", "(isRoot=true OR zones in ('APP.DEFAULT'))"); + groupsProxy.getGroupsByPersonId(personAlice.getId(), otherParams, "Incorrect response", 400); + } } private void testGetGroupMembersByGroupId() throws Exception