Skip to content

RANGER-5577:API for update and delete of resources in DataShare #932

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rameeshm wants to merge 2 commits into from
Closed

RANGER-5577:API for update and delete of resources in DataShare #932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e2a62b
RANGER-5577:API for update and delete of resources in DataShare
May 5, 2026
3f28da6
RANGER-5577:API for update and delete of resources in DataShare - fix…
May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions security-admin/src/main/java/org/apache/ranger/rest/GdsREST.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1261,6 +1261,63 @@ public RangerSharedResource updateSharedResource(@PathParam("id") Long resourceI
return ret;
}

@PUT
@Path("/resources")
@Consumes("application/json")
@Produces("application/json")
@PreAuthorize("@rangerPreAuthSecurityHandler.isAPIAccessible(\"" + RangerAPIList.UPDATE_SHARED_RESOURCES + "\")")
public void updateSharedResources(@QueryParam("forceDelete") @DefaultValue("false") boolean forceDelete, List<RangerSharedResource> resources) {
LOG.debug("==> GdsREST.updateSharedResources(resources={}, forceDelete={})", resources, forceDelete);

RangerPerfTracer perf = null;

try {
if (resources == null) {
throw new Exception("resources must not be null");
}

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

if (RangerPerfTracer.isPerfTraceEnabled(PERF_LOG)) {
perf = RangerPerfTracer.getPerfTracer(PERF_LOG, "GdsREST.updateSharedResources(" + resources + ",forceDelete=" + forceDelete + ")");
}

if (forceDelete) {
Copy link
Copy Markdown
Contributor

@mneethiraj mneethiraj May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API is called UPDATE_SHARED_RESOURCES, but the flag is named a forceDelete. It will help to add a note on the purpose of this API.

Copy link
Copy Markdown
Contributor Author

@rameeshm rameeshm May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mneethiraj we need to use updateSharedResources API to do delete as well, because HTTP DELETE is not accepting payloads and in order to support a bulk delete we are using the flag "forceDelete". Naming the API as deleteSharedResources with PUT doesn't look right, so introduced this flag "forceDelete". Should I have new API list as "UPDATE_OR_DELETE_SHARED_RESOURCES" for this new API? Please advice.
This bulk delete / update will be helpful in the integration of external system which does bulk operations of adding / updating / removing resources in the DataShares.

Copy link
Copy Markdown
Contributor

@mneethiraj mneethiraj May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rameeshm - GdsREST already has following API to bulk-delete shared resources, which I guess takes a comma-separated list of IDs as query parameter. Does that not work?

@DELETE
@Path("/resources")
@PreAuthorize("@rangerPreAuthSecurityHandler.isAPIAccessible(\"" + RangerAPIList.REMOVE_SHARED_RESOURCES + "\")")
public void removeSharedResources(List<Long> resourceIds) {
...
}

API to bulk-add shared resources also already exists. Perhaps adding an API to bulk-update is good enough? i.e. is an API to update or delete necessary?

Copy link
Copy Markdown
Contributor Author

@rameeshm rameeshm May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mneethiraj the existing API may not be useful.

As per (RFC7321) https://datatracker.ietf.org/doc/html/rfc7231#section-4.3.5
A payload within a DELETE request message has no defined semantics; sending a payload body on a DELETE request might cause some existing implementations to reject the request. Because of this while the spec does not explicitly forbid bodies, it warns that many servers, proxies, and load balancers may reject or silently ignore them. To overcome this and have a functionality to bulk delete I am using
public void updateSharedResources(@QueryParam("forceDelete") @DefaultValue("false") boolean forceDelete, List<RangerSharedResource> resources)

Copy link
Copy Markdown
Contributor

@mneethiraj mneethiraj May 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rameeshm - would marking resourceIds parameter as @QueryParam("id") help to receive the IDs as query params instead of body?

@DELETE
@Path("/resources")
@PreAuthorize("@rangerPreAuthSecurityHandler.isAPIAccessible(\"" + RangerAPIList.REMOVE_SHARED_RESOURCES + "\")")
public void removeSharedResources(@QueryParam("id") List<Long> resourceIds) {
...
}

Following curl will result in above method to be called with resourceIds containing two entries - 1 and 2:

curl -u user:password -X DELETE "https://ranger/service/gds/resources?id=1&id=2"

Copy link
Copy Markdown
Contributor Author

@rameeshm rameeshm May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mneethiraj we can do that. I thought it would not be right way if the number of ids are more, but I think it should satisfy the requirement.

Copy link
Copy Markdown
Contributor Author

@rameeshm rameeshm May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mneethiraj I am closing this PR and going to open one with the change you recommended.

List<Long> resourceIds = new ArrayList<>();

for (RangerSharedResource resource : resources) {
if (resource == null || resource.getId() == null) {
throw new Exception("resource id must not be null for forceDelete");
}

resourceIds.add(resource.getId());
}

gdsStore.removeSharedResources(resourceIds);
} else {
for (RangerSharedResource resource : resources) {
if (resource == null) {
throw new Exception("resource must not be null");
}

gdsStore.updateSharedResource(resource);
}
}
} catch (WebApplicationException excp) {
throw excp;
} catch (Throwable excp) {
LOG.error("updateSharedResources(resources={}, forceDelete={}) failed", resources, forceDelete, excp);

throw restErrorUtil.createRESTException(excp.getMessage());
} finally {
RangerPerfTracer.log(perf);
}

LOG.debug("<== GdsREST.updateSharedResources(resources={}, forceDelete={})", resources, forceDelete);
}

@DELETE
@Path("/resource/{id}")
@Produces("application/json")
Expand Down
1 change: 1 addition & 0 deletions security-admin/src/main/java/org/apache/ranger/security/context/RangerAPIList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ public class RangerAPIList {
public static final String ADD_SHARED_RESOURCE = "GdsREST.addSharedResource";
public static final String ADD_SHARED_RESOURCES = "GdsREST.addSharedResources";
public static final String UPDATE_SHARED_RESOURCE = "GdsREST.updateSharedResource";
public static final String UPDATE_SHARED_RESOURCES = "GdsREST.updateSharedResources";
public static final String REMOVE_SHARED_RESOURCE = "GdsREST.removeSharedResource";
public static final String REMOVE_SHARED_RESOURCES = "GdsREST.removeSharedResources";
public static final String GET_SHARED_RESOURCE = "GdsREST.getSharedResource";
Expand Down
1 change: 1 addition & 0 deletions security-admin/src/main/java/org/apache/ranger/security/context/RangerAPIMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,7 @@ private void mapGDSWithAPIs() {
apiAssociatedWithGDS.add(RangerAPIList.ADD_SHARED_RESOURCE);
apiAssociatedWithGDS.add(RangerAPIList.ADD_SHARED_RESOURCES);
apiAssociatedWithGDS.add(RangerAPIList.UPDATE_SHARED_RESOURCE);
apiAssociatedWithGDS.add(RangerAPIList.UPDATE_SHARED_RESOURCES);
apiAssociatedWithGDS.add(RangerAPIList.REMOVE_SHARED_RESOURCE);
apiAssociatedWithGDS.add(RangerAPIList.REMOVE_SHARED_RESOURCES);
apiAssociatedWithGDS.add(RangerAPIList.GET_SHARED_RESOURCE);
Expand Down
53 changes: 53 additions & 0 deletions security-admin/src/test/java/org/apache/ranger/rest/TestGdsREST.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,6 +924,59 @@ public void testRemoveSharedResource() {
verify(gdsStore).removeSharedResources(Arrays.asList(resourceId));
}

@Test
public void testUpdateSharedResources_updatesWhenNotForceDelete() throws Exception {
RangerGds.RangerSharedResource resource1 = createSharedResource();
RangerGds.RangerSharedResource resource2 = createSharedResource();
RangerGds.RangerSharedResource resource3 = createSharedResource();
List<RangerGds.RangerSharedResource> resources = Arrays.asList(resource1, resource2, resource3);

when(gdsStore.updateSharedResource(resource1)).thenReturn(resource1);
when(gdsStore.updateSharedResource(resource2)).thenReturn(resource2);
when(gdsStore.updateSharedResource(resource3)).thenReturn(resource3);

gdsREST.updateSharedResources(false, resources);

verify(gdsStore).updateSharedResource(resource1);
verify(gdsStore).updateSharedResource(resource2);
verify(gdsStore).updateSharedResource(resource3);
verify(gdsStore, Mockito.never()).removeSharedResources(Mockito.anyList());
}

@Test
public void testUpdateSharedResources_deletesWhenForceDelete() throws Exception {
RangerGds.RangerSharedResource resource1 = createSharedResource();
RangerGds.RangerSharedResource resource2 = createSharedResource();
RangerGds.RangerSharedResource resource3 = createSharedResource();
List<RangerGds.RangerSharedResource> resources = Arrays.asList(resource1, resource2, resource3);
List<Long> resourceIds = Arrays.asList(resource1.getId(), resource2.getId(), resource3.getId());

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

gdsREST.updateSharedResources(true, resources);

verify(gdsStore).removeSharedResources(resourceIds);
verify(gdsStore, Mockito.never()).updateSharedResource(Mockito.any(RangerGds.RangerSharedResource.class));
}

@Test
public void testUpdateSharedResourcesBatchSizeExceeded() {
List<RangerGds.RangerSharedResource> resources = new ArrayList<>();
for (long i = 0; i < 101; i++) {
resources.add(createSharedResource());
}
Mockito.when(restErrorUtil.createRESTException(Mockito.anyString())).thenReturn(new WebApplicationException());

assertThrows(WebApplicationException.class, () -> gdsREST.updateSharedResources(false, resources));
}

@Test
public void testUpdateSharedResourcesNullResourceIds() {
Mockito.when(restErrorUtil.createRESTException(Mockito.anyString())).thenReturn(new WebApplicationException());

assertThrows(WebApplicationException.class, () -> gdsREST.updateSharedResources(false, null));
}

@Test
public void testRemoveSharedResourceException() {
Mockito.doThrow(new RuntimeException("err")).when(gdsStore).removeSharedResources(any(List.class));
Expand Down
Loading