Skip to content

Commit 3f28da6

Browse files
author
Ramesh Mani
committed
RANGER-5577:API for update and delete of resources in DataShare - fixed review comments
1 parent 2e2a62b commit 3f28da6

5 files changed

Lines changed: 46 additions & 46 deletions

File tree

‎agents-common/src/main/java/org/apache/ranger/plugin/store/AbstractGdsStore.java‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -132,10 +132,6 @@ public RangerSharedResource updateSharedResource(RangerSharedResource resource)
132132
return null;
133133
}
134134

135-
@Override
136-
public void updateSharedResources(List<Long> sharedResourceIds) throws Exception {
137-
}
138-
139135
@Override
140136
public void removeSharedResources(List<Long> sharedResourceIds) throws Exception {
141137
}

‎agents-common/src/main/java/org/apache/ranger/plugin/store/GdsStore.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,6 @@ public interface GdsStore {
102102

103103
RangerSharedResource updateSharedResource(RangerSharedResource resource) throws Exception;
104104

105-
void updateSharedResources(List<Long> sharedResourceIds) throws Exception;
106-
107105
void removeSharedResources(List<Long> sharedResourceIds) throws Exception;
108106

109107
RangerSharedResource getSharedResource(Long sharedResourceId) throws Exception;

‎security-admin/src/main/java/org/apache/ranger/biz/GdsDBStore.java‎

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -719,27 +719,6 @@ public RangerSharedResource updateSharedResource(RangerSharedResource resource)
719719
return ret;
720720
}
721721

722-
@Override
723-
public void updateSharedResources(List<Long> sharedResourceIds) {
724-
LOG.debug("==> updateSharedResources({})", sharedResourceIds);
725-
726-
for (Long sharedResourceId : sharedResourceIds) {
727-
RangerSharedResource existing = null;
728-
729-
try {
730-
existing = sharedResourceService.read(sharedResourceId);
731-
} catch (Exception excp) {
732-
// ignore
733-
}
734-
735-
if (existing != null) {
736-
updateSharedResource(existing);
737-
}
738-
}
739-
740-
LOG.debug("<== updateSharedResources({})", sharedResourceIds);
741-
}
742-
743722
@Override
744723
public void removeSharedResources(List<Long> sharedResourceIds) {
745724
LOG.debug("==> removeSharedResources({})", sharedResourceIds);

‎security-admin/src/main/java/org/apache/ranger/rest/GdsREST.java‎

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1266,40 +1266,56 @@ public RangerSharedResource updateSharedResource(@PathParam("id") Long resourceI
12661266
@Consumes("application/json")
12671267
@Produces("application/json")
12681268
@PreAuthorize("@rangerPreAuthSecurityHandler.isAPIAccessible(\"" + RangerAPIList.UPDATE_SHARED_RESOURCES + "\")")
1269-
public void updateSharedResources(@QueryParam("forceDelete") @DefaultValue("false") boolean forceDelete, List<Long> resourceIds) {
1270-
LOG.debug("==> GdsREST.updateSharedResources(resourceIds={}, forceDelete={})", resourceIds, forceDelete);
1269+
public void updateSharedResources(@QueryParam("forceDelete") @DefaultValue("false") boolean forceDelete, List<RangerSharedResource> resources) {
1270+
LOG.debug("==> GdsREST.updateSharedResources(resources={}, forceDelete={})", resources, forceDelete);
12711271

12721272
RangerPerfTracer perf = null;
12731273

12741274
try {
1275-
if (resourceIds == null) {
1276-
throw new Exception("resourceIds must not be null");
1275+
if (resources == null) {
1276+
throw new Exception("resources must not be null");
12771277
}
12781278

1279-
if (resourceIds.size() > SHARED_RESOURCES_MAX_BATCH_SIZE) {
1279+
if (resources.size() > SHARED_RESOURCES_MAX_BATCH_SIZE) {
12801280
throw new Exception("updateSharedResources batch size exceeded the configured limit: Maximum allowed is " + SHARED_RESOURCES_MAX_BATCH_SIZE);
12811281
}
12821282

12831283
if (RangerPerfTracer.isPerfTraceEnabled(PERF_LOG)) {
1284-
perf = RangerPerfTracer.getPerfTracer(PERF_LOG, "GdsREST.updateSharedResources(" + resourceIds + ",forceDelete=" + forceDelete + ")");
1284+
perf = RangerPerfTracer.getPerfTracer(PERF_LOG, "GdsREST.updateSharedResources(" + resources + ",forceDelete=" + forceDelete + ")");
12851285
}
12861286

12871287
if (forceDelete) {
1288+
List<Long> resourceIds = new ArrayList<>();
1289+
1290+
for (RangerSharedResource resource : resources) {
1291+
if (resource == null || resource.getId() == null) {
1292+
throw new Exception("resource id must not be null for forceDelete");
1293+
}
1294+
1295+
resourceIds.add(resource.getId());
1296+
}
1297+
12881298
gdsStore.removeSharedResources(resourceIds);
12891299
} else {
1290-
gdsStore.updateSharedResources(resourceIds);
1300+
for (RangerSharedResource resource : resources) {
1301+
if (resource == null) {
1302+
throw new Exception("resource must not be null");
1303+
}
1304+
1305+
gdsStore.updateSharedResource(resource);
1306+
}
12911307
}
12921308
} catch (WebApplicationException excp) {
12931309
throw excp;
12941310
} catch (Throwable excp) {
1295-
LOG.error("updateSharedResources(resourceIds={}, forceDelete={}) failed", resourceIds, forceDelete, excp);
1311+
LOG.error("updateSharedResources(resources={}, forceDelete={}) failed", resources, forceDelete, excp);
12961312

12971313
throw restErrorUtil.createRESTException(excp.getMessage());
12981314
} finally {
12991315
RangerPerfTracer.log(perf);
13001316
}
13011317

1302-
LOG.debug("<== GdsREST.updateSharedResources(resourceIds={}, forceDelete={})", resourceIds, forceDelete);
1318+
LOG.debug("<== GdsREST.updateSharedResources(resources={}, forceDelete={})", resources, forceDelete);
13031319
}
13041320

13051321
@DELETE

‎security-admin/src/test/java/org/apache/ranger/rest/TestGdsREST.java‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -926,37 +926,48 @@ public void testRemoveSharedResource() {
926926

927927
@Test
928928
public void testUpdateSharedResources_updatesWhenNotForceDelete() throws Exception {
929-
List<Long> resourceIds = Arrays.asList(1L, 2L, 3L);
929+
RangerGds.RangerSharedResource resource1 = createSharedResource();
930+
RangerGds.RangerSharedResource resource2 = createSharedResource();
931+
RangerGds.RangerSharedResource resource3 = createSharedResource();
932+
List<RangerGds.RangerSharedResource> resources = Arrays.asList(resource1, resource2, resource3);
930933

931-
doNothing().when(gdsStore).updateSharedResources(resourceIds);
934+
when(gdsStore.updateSharedResource(resource1)).thenReturn(resource1);
935+
when(gdsStore.updateSharedResource(resource2)).thenReturn(resource2);
936+
when(gdsStore.updateSharedResource(resource3)).thenReturn(resource3);
932937

933-
gdsREST.updateSharedResources(false, resourceIds);
938+
gdsREST.updateSharedResources(false, resources);
934939

935-
verify(gdsStore).updateSharedResources(resourceIds);
940+
verify(gdsStore).updateSharedResource(resource1);
941+
verify(gdsStore).updateSharedResource(resource2);
942+
verify(gdsStore).updateSharedResource(resource3);
936943
verify(gdsStore, Mockito.never()).removeSharedResources(Mockito.anyList());
937944
}
938945

939946
@Test
940947
public void testUpdateSharedResources_deletesWhenForceDelete() throws Exception {
941-
List<Long> resourceIds = Arrays.asList(1L, 2L, 3L);
948+
RangerGds.RangerSharedResource resource1 = createSharedResource();
949+
RangerGds.RangerSharedResource resource2 = createSharedResource();
950+
RangerGds.RangerSharedResource resource3 = createSharedResource();
951+
List<RangerGds.RangerSharedResource> resources = Arrays.asList(resource1, resource2, resource3);
952+
List<Long> resourceIds = Arrays.asList(resource1.getId(), resource2.getId(), resource3.getId());
942953

943954
doNothing().when(gdsStore).removeSharedResources(resourceIds);
944955

945-
gdsREST.updateSharedResources(true, resourceIds);
956+
gdsREST.updateSharedResources(true, resources);
946957

947958
verify(gdsStore).removeSharedResources(resourceIds);
948-
verify(gdsStore, Mockito.never()).updateSharedResources(Mockito.anyList());
959+
verify(gdsStore, Mockito.never()).updateSharedResource(Mockito.any(RangerGds.RangerSharedResource.class));
949960
}
950961

951962
@Test
952963
public void testUpdateSharedResourcesBatchSizeExceeded() {
953-
List<Long> resourceIds = new ArrayList<>();
964+
List<RangerGds.RangerSharedResource> resources = new ArrayList<>();
954965
for (long i = 0; i < 101; i++) {
955-
resourceIds.add(i);
966+
resources.add(createSharedResource());
956967
}
957968
Mockito.when(restErrorUtil.createRESTException(Mockito.anyString())).thenReturn(new WebApplicationException());
958969

959-
assertThrows(WebApplicationException.class, () -> gdsREST.updateSharedResources(false, resourceIds));
970+
assertThrows(WebApplicationException.class, () -> gdsREST.updateSharedResources(false, resources));
960971
}
961972

962973
@Test

0 commit comments

Comments
 (0)