Skip to content

Fix REST responses for failed Admin operations #2291

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

Merged
merged 2 commits into from
Aug 15, 2025
+479 −523
Merged

Fix REST responses for failed Admin operations #2291

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
56ad6dc
Fix REST responses for failed Admin operations
XN137 Aug 7, 2025
18ed553
review: use http 400 BAD_REQUEST
XN137 Aug 12, 2025
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
26 changes: 23 additions & 3 deletions ...main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,9 @@ public static void setup(PolarisApiEndpoints endpoints, ClientCredentials creden

@AfterAll
public static void close() throws Exception {
client.close();
if (client != null) {
client.close();
}
}

@AfterEach
Expand Down Expand Up @@ -1728,7 +1730,7 @@ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
.managementApi(catalogAdminToken)
.request(
"v1/principal-roles/"
+ principalRoleName
+ principalRoleName2
+ "/catalog-roles/"
+ catalogName
+ "/"
Expand All @@ -1743,14 +1745,32 @@ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
managementApi
.request(
"v1/principal-roles/"
+ principalRoleName
+ principalRoleName2
+ "/catalog-roles/"
+ catalogName
+ "/"
+ catalogRoleName)
.delete()) {
assertThat(response).returns(Response.Status.NO_CONTENT.getStatusCode(), Response::getStatus);
}

// trigger an internal error by using "principalRoleName" instead of "principalRoleName2"
try (Response response =
managementApi
.request(
"v1/principal-roles/"
+ principalRoleName
+ "/catalog-roles/"
+ catalogName
+ "/"
+ catalogRoleName)
.delete()) {
assertThat(response)
.returns(Response.Status.BAD_REQUEST.getStatusCode(), Response::getStatus);
assertThat(response.hasEntity()).isTrue();
ErrorResponse errorResponse = response.readEntity(ErrorResponse.class);
assertThat(errorResponse.message()).contains("Operation failed: GRANT_NOT_FOUND");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there could be an argument that GRANT_NOT_FOUND should be a 404 instead of internal server error but it would mean we need to translate all BaseResult.ReturnStatus values to a proper HTTP code.
i think this can be figured out in a followup and we should first stop silently failing in those cases.

Copy link
Member

Choose a reason for hiding this comment

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

Hm - HTTP/500 feels odd, as it's rather a user (argument) error. HTTP/400 would be better IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok switched to HTTP/400

}
}

@Test
Expand Down
189 changes: 81 additions & 108 deletions runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java
View file
Open in desktop

Large diffs are not rendered by default.

132 changes: 84 additions & 48 deletions runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@

import jakarta.enterprise.context.RequestScoped;
import jakarta.inject.Inject;
import jakarta.ws.rs.core.MediaType;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.core.SecurityContext;
import java.util.List;
import java.util.Locale;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.NotAuthorizedException;
import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.polaris.core.admin.model.AddGrantRequest;
import org.apache.polaris.core.admin.model.AuthenticationParameters;
import org.apache.polaris.core.admin.model.Catalog;
Expand Down Expand Up @@ -71,6 +73,8 @@
import org.apache.polaris.core.entity.PrincipalRoleEntity;
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.dao.entity.BaseResult;
import org.apache.polaris.core.persistence.dao.entity.PrivilegeResult;
import org.apache.polaris.core.persistence.resolver.ResolutionManifestFactory;
import org.apache.polaris.core.secrets.UserSecretsManager;
import org.apache.polaris.core.secrets.UserSecretsManagerFactory;
Expand Down Expand Up @@ -134,6 +138,22 @@ private PolarisAdminService newAdminService(
reservedProperties);
}

private static Response toResponse(BaseResult result, Response.Status successStatus) {
if (!result.isSuccess()) {
ErrorResponse icebergErrorResponse =
ErrorResponse.builder()
.responseCode(Response.Status.BAD_REQUEST.getStatusCode())
.withType(result.getReturnStatus().toString())
.withMessage("Operation failed: " + result.getReturnStatus().toString())
.build();
return Response.status(Response.Status.BAD_REQUEST)
.type(MediaType.APPLICATION_JSON_TYPE)
.entity(icebergErrorResponse)
.build();
}
return Response.status(successStatus).build();
}

/** From PolarisCatalogsApiService */
@Override
public Response createCatalog(
Expand Down Expand Up @@ -457,8 +477,9 @@ public Response assignPrincipalRole(
request.getPrincipalRole().getName(),
principalName);
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
adminService.assignPrincipalRole(principalName, request.getPrincipalRole().getName());
return Response.status(Response.Status.CREATED).build();
PrivilegeResult result =
adminService.assignPrincipalRole(principalName, request.getPrincipalRole().getName());
return toResponse(result, Response.Status.CREATED);
}

/** From PolarisPrincipalsApiService */
Expand All @@ -470,8 +491,8 @@ public Response revokePrincipalRole(
SecurityContext securityContext) {
LOGGER.info("Revoking principalRole {} from principal {}", principalRoleName, principalName);
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
adminService.revokePrincipalRole(principalName, principalRoleName);
return Response.status(Response.Status.NO_CONTENT).build();
PrivilegeResult result = adminService.revokePrincipalRole(principalName, principalRoleName);
return toResponse(result, Response.Status.NO_CONTENT);
}

/** From PolarisPrincipalsApiService */
Expand Down Expand Up @@ -503,9 +524,10 @@ public Response assignCatalogRoleToPrincipalRole(
catalogName,
principalRoleName);
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
adminService.assignCatalogRoleToPrincipalRole(
principalRoleName, catalogName, request.getCatalogRole().getName());
return Response.status(Response.Status.CREATED).build();
PrivilegeResult result =
adminService.assignCatalogRoleToPrincipalRole(
principalRoleName, catalogName, request.getCatalogRole().getName());
return toResponse(result, Response.Status.CREATED);
}

/** From PolarisPrincipalRolesApiService */
Expand All @@ -522,9 +544,10 @@ public Response revokeCatalogRoleFromPrincipalRole(
catalogName,
principalRoleName);
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
adminService.revokeCatalogRoleFromPrincipalRole(
principalRoleName, catalogName, catalogRoleName);
return Response.status(Response.Status.NO_CONTENT).build();
PrivilegeResult result =
adminService.revokeCatalogRoleFromPrincipalRole(
principalRoleName, catalogName, catalogRoleName);
return toResponse(result, Response.Status.NO_CONTENT);
}

/** From PolarisPrincipalRolesApiService */
Expand Down Expand Up @@ -574,6 +597,7 @@ public Response addGrantToCatalogRole(
catalogRoleName,
catalogName);
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
PrivilegeResult result;
switch (grantRequest.getGrant()) {
// The per-securable-type Privilege enums must be exact String match for a subset of all
// PolarisPrivilege values.
Expand All @@ -583,11 +607,12 @@ public Response addGrantToCatalogRole(
PolarisPrivilege.valueOf(viewGrant.getPrivilege().toString());
String viewName = viewGrant.getViewName();
String[] namespaceParts = viewGrant.getNamespace().toArray(new String[0]);
adminService.grantPrivilegeOnViewToRole(
catalogName,
catalogRoleName,
TableIdentifier.of(Namespace.of(namespaceParts), viewName),
privilege);
result =
adminService.grantPrivilegeOnViewToRole(
catalogName,
catalogRoleName,
TableIdentifier.of(Namespace.of(namespaceParts), viewName),
privilege);
break;
}
case TableGrant tableGrant:
Expand All @@ -596,27 +621,30 @@ public Response addGrantToCatalogRole(
PolarisPrivilege.valueOf(tableGrant.getPrivilege().toString());
String tableName = tableGrant.getTableName();
String[] namespaceParts = tableGrant.getNamespace().toArray(new String[0]);
adminService.grantPrivilegeOnTableToRole(
catalogName,
catalogRoleName,
TableIdentifier.of(Namespace.of(namespaceParts), tableName),
privilege);
result =
adminService.grantPrivilegeOnTableToRole(
catalogName,
catalogRoleName,
TableIdentifier.of(Namespace.of(namespaceParts), tableName),
privilege);
break;
}
case NamespaceGrant namespaceGrant:
{
PolarisPrivilege privilege =
PolarisPrivilege.valueOf(namespaceGrant.getPrivilege().toString());
String[] namespaceParts = namespaceGrant.getNamespace().toArray(new String[0]);
adminService.grantPrivilegeOnNamespaceToRole(
catalogName, catalogRoleName, Namespace.of(namespaceParts), privilege);
result =
adminService.grantPrivilegeOnNamespaceToRole(
catalogName, catalogRoleName, Namespace.of(namespaceParts), privilege);
break;
}
case CatalogGrant catalogGrant:
{
PolarisPrivilege privilege =
PolarisPrivilege.valueOf(catalogGrant.getPrivilege().toString());
adminService.grantPrivilegeOnCatalogToRole(catalogName, catalogRoleName, privilege);
result =
adminService.grantPrivilegeOnCatalogToRole(catalogName, catalogRoleName, privilege);
break;
}
case PolicyGrant policyGrant:
Expand All @@ -625,11 +653,12 @@ public Response addGrantToCatalogRole(
PolarisPrivilege.valueOf(policyGrant.getPrivilege().toString());
String policyName = policyGrant.getPolicyName();
String[] namespaceParts = policyGrant.getNamespace().toArray(new String[0]);
adminService.grantPrivilegeOnPolicyToRole(
catalogName,
catalogRoleName,
new PolicyIdentifier(Namespace.of(namespaceParts), policyName),
privilege);
result =
adminService.grantPrivilegeOnPolicyToRole(
catalogName,
catalogRoleName,
new PolicyIdentifier(Namespace.of(namespaceParts), policyName),
privilege);
break;
}
default:
Expand All @@ -640,7 +669,7 @@ public Response addGrantToCatalogRole(
.log("Don't know how to handle privilege grant: {}", grantRequest);
return Response.status(Response.Status.BAD_REQUEST).build();
}
return Response.status(Response.Status.CREATED).build();
return toResponse(result, Response.Status.CREATED);
Copy link
Member

Choose a reason for hiding this comment

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

This means that errors were not propagated to clients, right?

Copy link
Contributor Author

@XN137 XN137 Aug 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, some methods do throw exceptions internally (and those get propagated to clients correctly) but the returned boolean value of the methods was often ignored.
as part of this PR we have to fix a test in PolarisManagementServiceIntegrationTest.java which was silently failing before.

}

/** From PolarisCatalogsApiService */
Expand All @@ -663,6 +692,7 @@ public Response revokeGrantFromCatalogRole(
}

PolarisAdminService adminService = newAdminService(realmContext, securityContext);
PrivilegeResult result;
switch (grantRequest.getGrant()) {
// The per-securable-type Privilege enums must be exact String match for a subset of all
// PolarisPrivilege values.
Expand All @@ -672,11 +702,12 @@ public Response revokeGrantFromCatalogRole(
PolarisPrivilege.valueOf(viewGrant.getPrivilege().toString());
String viewName = viewGrant.getViewName();
String[] namespaceParts = viewGrant.getNamespace().toArray(new String[0]);
adminService.revokePrivilegeOnViewFromRole(
catalogName,
catalogRoleName,
TableIdentifier.of(Namespace.of(namespaceParts), viewName),
privilege);
result =
adminService.revokePrivilegeOnViewFromRole(
catalogName,
catalogRoleName,
TableIdentifier.of(Namespace.of(namespaceParts), viewName),
privilege);
break;
}
case TableGrant tableGrant:
Expand All @@ -685,27 +716,31 @@ public Response revokeGrantFromCatalogRole(
PolarisPrivilege.valueOf(tableGrant.getPrivilege().toString());
String tableName = tableGrant.getTableName();
String[] namespaceParts = tableGrant.getNamespace().toArray(new String[0]);
adminService.revokePrivilegeOnTableFromRole(
catalogName,
catalogRoleName,
TableIdentifier.of(Namespace.of(namespaceParts), tableName),
privilege);
result =
adminService.revokePrivilegeOnTableFromRole(
catalogName,
catalogRoleName,
TableIdentifier.of(Namespace.of(namespaceParts), tableName),
privilege);
break;
}
case NamespaceGrant namespaceGrant:
{
PolarisPrivilege privilege =
PolarisPrivilege.valueOf(namespaceGrant.getPrivilege().toString());
String[] namespaceParts = namespaceGrant.getNamespace().toArray(new String[0]);
adminService.revokePrivilegeOnNamespaceFromRole(
catalogName, catalogRoleName, Namespace.of(namespaceParts), privilege);
result =
adminService.revokePrivilegeOnNamespaceFromRole(
catalogName, catalogRoleName, Namespace.of(namespaceParts), privilege);
break;
}
case CatalogGrant catalogGrant:
{
PolarisPrivilege privilege =
PolarisPrivilege.valueOf(catalogGrant.getPrivilege().toString());
adminService.revokePrivilegeOnCatalogFromRole(catalogName, catalogRoleName, privilege);
result =
adminService.revokePrivilegeOnCatalogFromRole(
catalogName, catalogRoleName, privilege);
break;
}
case PolicyGrant policyGrant:
Expand All @@ -714,11 +749,12 @@ public Response revokeGrantFromCatalogRole(
PolarisPrivilege.valueOf(policyGrant.getPrivilege().toString());
String policyName = policyGrant.getPolicyName();
String[] namespaceParts = policyGrant.getNamespace().toArray(new String[0]);
adminService.revokePrivilegeOnPolicyFromRole(
catalogName,
catalogRoleName,
new PolicyIdentifier(Namespace.of(namespaceParts), policyName),
privilege);
result =
adminService.revokePrivilegeOnPolicyFromRole(
catalogName,
catalogRoleName,
new PolicyIdentifier(Namespace.of(namespaceParts), policyName),
privilege);
break;
}
default:
Expand All @@ -729,7 +765,7 @@ public Response revokeGrantFromCatalogRole(
.log("Don't know how to handle privilege revocation: {}", grantRequest);
return Response.status(Response.Status.BAD_REQUEST).build();
}
return Response.status(Response.Status.CREATED).build();
return toResponse(result, Response.Status.CREATED);
}

/** From PolarisCatalogsApiService */
Expand Down
Loading