Skip to content

Commit f0e112a

Browse files
XN137XN137
committed
Fix REST responses for failed Admin operations
1 parent 067bb9d commit f0e112a

File tree

9 files changed

+479
-523
lines changed

9 files changed

+479
-523
lines changed

‎integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,9 @@ public static void setup(PolarisApiEndpoints endpoints, ClientCredentials creden
124124

125125
@AfterAll
126126
public static void close() throws Exception {
127-
client.close();
127+
if (client != null) {
128+
client.close();
129+
}
128130
}
129131

130132
@AfterEach
@@ -1728,7 +1730,7 @@ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
17281730
.managementApi(catalogAdminToken)
17291731
.request(
17301732
"v1/principal-roles/"
1731-
+ principalRoleName
1733+
+ principalRoleName2
17321734
+ "/catalog-roles/"
17331735
+ catalogName
17341736
+ "/"
@@ -1743,14 +1745,32 @@ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
17431745
managementApi
17441746
.request(
17451747
"v1/principal-roles/"
1746-
+ principalRoleName
1748+
+ principalRoleName2
17471749
+ "/catalog-roles/"
17481750
+ catalogName
17491751
+ "/"
17501752
+ catalogRoleName)
17511753
.delete()) {
17521754
assertThat(response).returns(Response.Status.NO_CONTENT.getStatusCode(), Response::getStatus);
17531755
}
1756+
1757+
// trigger an internal error by using "principalRoleName" instead of "principalRoleName2"
1758+
try (Response response =
1759+
managementApi
1760+
.request(
1761+
"v1/principal-roles/"
1762+
+ principalRoleName
1763+
+ "/catalog-roles/"
1764+
+ catalogName
1765+
+ "/"
1766+
+ catalogRoleName)
1767+
.delete()) {
1768+
assertThat(response)
1769+
.returns(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), Response::getStatus);
1770+
assertThat(response.hasEntity()).isTrue();
1771+
ErrorResponse errorResponse = response.readEntity(ErrorResponse.class);
1772+
assertThat(errorResponse.message()).contains("Operation failed: GRANT_NOT_FOUND");
1773+
}
17541774
}
17551775

17561776
@Test

‎runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

Lines changed: 81 additions & 108 deletions
Large diffs are not rendered by default.

‎runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java

Lines changed: 84 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,15 @@
2020

2121
import jakarta.enterprise.context.RequestScoped;
2222
import jakarta.inject.Inject;
23+
import jakarta.ws.rs.core.MediaType;
2324
import jakarta.ws.rs.core.Response;
2425
import jakarta.ws.rs.core.SecurityContext;
2526
import java.util.List;
2627
import java.util.Locale;
2728
import org.apache.iceberg.catalog.Namespace;
2829
import org.apache.iceberg.catalog.TableIdentifier;
2930
import org.apache.iceberg.exceptions.NotAuthorizedException;
31+
import org.apache.iceberg.rest.responses.ErrorResponse;
3032
import org.apache.polaris.core.admin.model.AddGrantRequest;
3133
import org.apache.polaris.core.admin.model.AuthenticationParameters;
3234
import org.apache.polaris.core.admin.model.Catalog;
@@ -71,6 +73,8 @@
7173
import org.apache.polaris.core.entity.PrincipalRoleEntity;
7274
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
7375
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
76+
import org.apache.polaris.core.persistence.dao.entity.BaseResult;
77+
import org.apache.polaris.core.persistence.dao.entity.PrivilegeResult;
7478
import org.apache.polaris.core.persistence.resolver.ResolutionManifestFactory;
7579
import org.apache.polaris.core.secrets.UserSecretsManager;
7680
import org.apache.polaris.core.secrets.UserSecretsManagerFactory;
@@ -134,6 +138,22 @@ private PolarisAdminService newAdminService(
134138
reservedProperties);
135139
}
136140

141+
private static Response toResponse(BaseResult result, Response.Status successStatus) {
142+
if (!result.isSuccess()) {
143+
ErrorResponse icebergErrorResponse =
144+
ErrorResponse.builder()
145+
.responseCode(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode())
146+
.withType(result.getReturnStatus().toString())
147+
.withMessage("Operation failed: " + result.getReturnStatus().toString())
148+
.build();
149+
return Response.status(Response.Status.INTERNAL_SERVER_ERROR)
150+
.type(MediaType.APPLICATION_JSON_TYPE)
151+
.entity(icebergErrorResponse)
152+
.build();
153+
}
154+
return Response.status(successStatus).build();
155+
}
156+
137157
/** From PolarisCatalogsApiService */
138158
@Override
139159
public Response createCatalog(
@@ -457,8 +477,9 @@ public Response assignPrincipalRole(
457477
request.getPrincipalRole().getName(),
458478
principalName);
459479
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
460-
adminService.assignPrincipalRole(principalName, request.getPrincipalRole().getName());
461-
return Response.status(Response.Status.CREATED).build();
480+
PrivilegeResult result =
481+
adminService.assignPrincipalRole(principalName, request.getPrincipalRole().getName());
482+
return toResponse(result, Response.Status.CREATED);
462483
}
463484

464485
/** From PolarisPrincipalsApiService */
@@ -470,8 +491,8 @@ public Response revokePrincipalRole(
470491
SecurityContext securityContext) {
471492
LOGGER.info("Revoking principalRole {} from principal {}", principalRoleName, principalName);
472493
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
473-
adminService.revokePrincipalRole(principalName, principalRoleName);
474-
return Response.status(Response.Status.NO_CONTENT).build();
494+
PrivilegeResult result = adminService.revokePrincipalRole(principalName, principalRoleName);
495+
return toResponse(result, Response.Status.NO_CONTENT);
475496
}
476497

477498
/** From PolarisPrincipalsApiService */
@@ -503,9 +524,10 @@ public Response assignCatalogRoleToPrincipalRole(
503524
catalogName,
504525
principalRoleName);
505526
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
506-
adminService.assignCatalogRoleToPrincipalRole(
507-
principalRoleName, catalogName, request.getCatalogRole().getName());
508-
return Response.status(Response.Status.CREATED).build();
527+
PrivilegeResult result =
528+
adminService.assignCatalogRoleToPrincipalRole(
529+
principalRoleName, catalogName, request.getCatalogRole().getName());
530+
return toResponse(result, Response.Status.CREATED);
509531
}
510532

511533
/** From PolarisPrincipalRolesApiService */
@@ -522,9 +544,10 @@ public Response revokeCatalogRoleFromPrincipalRole(
522544
catalogName,
523545
principalRoleName);
524546
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
525-
adminService.revokeCatalogRoleFromPrincipalRole(
526-
principalRoleName, catalogName, catalogRoleName);
527-
return Response.status(Response.Status.NO_CONTENT).build();
547+
PrivilegeResult result =
548+
adminService.revokeCatalogRoleFromPrincipalRole(
549+
principalRoleName, catalogName, catalogRoleName);
550+
return toResponse(result, Response.Status.NO_CONTENT);
528551
}
529552

530553
/** From PolarisPrincipalRolesApiService */
@@ -574,6 +597,7 @@ public Response addGrantToCatalogRole(
574597
catalogRoleName,
575598
catalogName);
576599
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
600+
PrivilegeResult result;
577601
switch (grantRequest.getGrant()) {
578602
// The per-securable-type Privilege enums must be exact String match for a subset of all
579603
// PolarisPrivilege values.
@@ -583,11 +607,12 @@ public Response addGrantToCatalogRole(
583607
PolarisPrivilege.valueOf(viewGrant.getPrivilege().toString());
584608
String viewName = viewGrant.getViewName();
585609
String[] namespaceParts = viewGrant.getNamespace().toArray(new String[0]);
586-
adminService.grantPrivilegeOnViewToRole(
587-
catalogName,
588-
catalogRoleName,
589-
TableIdentifier.of(Namespace.of(namespaceParts), viewName),
590-
privilege);
610+
result =
611+
adminService.grantPrivilegeOnViewToRole(
612+
catalogName,
613+
catalogRoleName,
614+
TableIdentifier.of(Namespace.of(namespaceParts), viewName),
615+
privilege);
591616
break;
592617
}
593618
case TableGrant tableGrant:
@@ -596,27 +621,30 @@ public Response addGrantToCatalogRole(
596621
PolarisPrivilege.valueOf(tableGrant.getPrivilege().toString());
597622
String tableName = tableGrant.getTableName();
598623
String[] namespaceParts = tableGrant.getNamespace().toArray(new String[0]);
599-
adminService.grantPrivilegeOnTableToRole(
600-
catalogName,
601-
catalogRoleName,
602-
TableIdentifier.of(Namespace.of(namespaceParts), tableName),
603-
privilege);
624+
result =
625+
adminService.grantPrivilegeOnTableToRole(
626+
catalogName,
627+
catalogRoleName,
628+
TableIdentifier.of(Namespace.of(namespaceParts), tableName),
629+
privilege);
604630
break;
605631
}
606632
case NamespaceGrant namespaceGrant:
607633
{
608634
PolarisPrivilege privilege =
609635
PolarisPrivilege.valueOf(namespaceGrant.getPrivilege().toString());
610636
String[] namespaceParts = namespaceGrant.getNamespace().toArray(new String[0]);
611-
adminService.grantPrivilegeOnNamespaceToRole(
612-
catalogName, catalogRoleName, Namespace.of(namespaceParts), privilege);
637+
result =
638+
adminService.grantPrivilegeOnNamespaceToRole(
639+
catalogName, catalogRoleName, Namespace.of(namespaceParts), privilege);
613640
break;
614641
}
615642
case CatalogGrant catalogGrant:
616643
{
617644
PolarisPrivilege privilege =
618645
PolarisPrivilege.valueOf(catalogGrant.getPrivilege().toString());
619-
adminService.grantPrivilegeOnCatalogToRole(catalogName, catalogRoleName, privilege);
646+
result =
647+
adminService.grantPrivilegeOnCatalogToRole(catalogName, catalogRoleName, privilege);
620648
break;
621649
}
622650
case PolicyGrant policyGrant:
@@ -625,11 +653,12 @@ public Response addGrantToCatalogRole(
625653
PolarisPrivilege.valueOf(policyGrant.getPrivilege().toString());
626654
String policyName = policyGrant.getPolicyName();
627655
String[] namespaceParts = policyGrant.getNamespace().toArray(new String[0]);
628-
adminService.grantPrivilegeOnPolicyToRole(
629-
catalogName,
630-
catalogRoleName,
631-
new PolicyIdentifier(Namespace.of(namespaceParts), policyName),
632-
privilege);
656+
result =
657+
adminService.grantPrivilegeOnPolicyToRole(
658+
catalogName,
659+
catalogRoleName,
660+
new PolicyIdentifier(Namespace.of(namespaceParts), policyName),
661+
privilege);
633662
break;
634663
}
635664
default:
@@ -640,7 +669,7 @@ public Response addGrantToCatalogRole(
640669
.log("Don't know how to handle privilege grant: {}", grantRequest);
641670
return Response.status(Response.Status.BAD_REQUEST).build();
642671
}
643-
return Response.status(Response.Status.CREATED).build();
672+
return toResponse(result, Response.Status.CREATED);
644673
}
645674

646675
/** From PolarisCatalogsApiService */
@@ -663,6 +692,7 @@ public Response revokeGrantFromCatalogRole(
663692
}
664693

665694
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
695+
PrivilegeResult result;
666696
switch (grantRequest.getGrant()) {
667697
// The per-securable-type Privilege enums must be exact String match for a subset of all
668698
// PolarisPrivilege values.
@@ -672,11 +702,12 @@ public Response revokeGrantFromCatalogRole(
672702
PolarisPrivilege.valueOf(viewGrant.getPrivilege().toString());
673703
String viewName = viewGrant.getViewName();
674704
String[] namespaceParts = viewGrant.getNamespace().toArray(new String[0]);
675-
adminService.revokePrivilegeOnViewFromRole(
676-
catalogName,
677-
catalogRoleName,
678-
TableIdentifier.of(Namespace.of(namespaceParts), viewName),
679-
privilege);
705+
result =
706+
adminService.revokePrivilegeOnViewFromRole(
707+
catalogName,
708+
catalogRoleName,
709+
TableIdentifier.of(Namespace.of(namespaceParts), viewName),
710+
privilege);
680711
break;
681712
}
682713
case TableGrant tableGrant:
@@ -685,27 +716,31 @@ public Response revokeGrantFromCatalogRole(
685716
PolarisPrivilege.valueOf(tableGrant.getPrivilege().toString());
686717
String tableName = tableGrant.getTableName();
687718
String[] namespaceParts = tableGrant.getNamespace().toArray(new String[0]);
688-
adminService.revokePrivilegeOnTableFromRole(
689-
catalogName,
690-
catalogRoleName,
691-
TableIdentifier.of(Namespace.of(namespaceParts), tableName),
692-
privilege);
719+
result =
720+
adminService.revokePrivilegeOnTableFromRole(
721+
catalogName,
722+
catalogRoleName,
723+
TableIdentifier.of(Namespace.of(namespaceParts), tableName),
724+
privilege);
693725
break;
694726
}
695727
case NamespaceGrant namespaceGrant:
696728
{
697729
PolarisPrivilege privilege =
698730
PolarisPrivilege.valueOf(namespaceGrant.getPrivilege().toString());
699731
String[] namespaceParts = namespaceGrant.getNamespace().toArray(new String[0]);
700-
adminService.revokePrivilegeOnNamespaceFromRole(
701-
catalogName, catalogRoleName, Namespace.of(namespaceParts), privilege);
732+
result =
733+
adminService.revokePrivilegeOnNamespaceFromRole(
734+
catalogName, catalogRoleName, Namespace.of(namespaceParts), privilege);
702735
break;
703736
}
704737
case CatalogGrant catalogGrant:
705738
{
706739
PolarisPrivilege privilege =
707740
PolarisPrivilege.valueOf(catalogGrant.getPrivilege().toString());
708-
adminService.revokePrivilegeOnCatalogFromRole(catalogName, catalogRoleName, privilege);
741+
result =
742+
adminService.revokePrivilegeOnCatalogFromRole(
743+
catalogName, catalogRoleName, privilege);
709744
break;
710745
}
711746
case PolicyGrant policyGrant:
@@ -714,11 +749,12 @@ public Response revokeGrantFromCatalogRole(
714749
PolarisPrivilege.valueOf(policyGrant.getPrivilege().toString());
715750
String policyName = policyGrant.getPolicyName();
716751
String[] namespaceParts = policyGrant.getNamespace().toArray(new String[0]);
717-
adminService.revokePrivilegeOnPolicyFromRole(
718-
catalogName,
719-
catalogRoleName,
720-
new PolicyIdentifier(Namespace.of(namespaceParts), policyName),
721-
privilege);
752+
result =
753+
adminService.revokePrivilegeOnPolicyFromRole(
754+
catalogName,
755+
catalogRoleName,
756+
new PolicyIdentifier(Namespace.of(namespaceParts), policyName),
757+
privilege);
722758
break;
723759
}
724760
default:
@@ -729,7 +765,7 @@ public Response revokeGrantFromCatalogRole(
729765
.log("Don't know how to handle privilege revocation: {}", grantRequest);
730766
return Response.status(Response.Status.BAD_REQUEST).build();
731767
}
732-
return Response.status(Response.Status.CREATED).build();
768+
return toResponse(result, Response.Status.CREATED);
733769
}
734770

735771
/** From PolarisCatalogsApiService */

0 commit comments

Comments
 (0)