gh-yzou commented on code in PR #1298:
URL: https://github.com/apache/polaris/pull/1298#discussion_r2027940930
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1495,10 +1490,10 @@ public boolean grantPrivilegeOnTableToRole(
PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.ADD_TABLE_GRANT_TO_CATALOG_ROLE;
authorizeGrantOnTableLikeOperationOrThrow(
- op, catalogName, PolarisEntitySubType.ICEBERG_TABLE, identifier,
catalogRoleName);
+ op, catalogName, PolarisEntitySubType.ANY_SUBTYPE, identifier,
catalogRoleName);
return grantPrivilegeOnTableLikeToRole(
- catalogName, catalogRoleName, identifier,
PolarisEntitySubType.ICEBERG_TABLE, privilege);
+ catalogName, catalogRoleName, identifier,
PolarisEntitySubType.ANY_SUBTYPE, privilege);
Review Comment:
i think we can do similar thing for grantPrivilegeOnTableLikeToRole also
##########
service/common/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalogAdapter.java:
##########
@@ -19,22 +19,86 @@
package org.apache.polaris.service.catalog.generic;
import jakarta.enterprise.context.RequestScoped;
+import jakarta.inject.Inject;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.core.SecurityContext;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal;
+import org.apache.polaris.core.auth.PolarisAuthorizer;
+import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.persistence.PolarisEntityManager;
+import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import
org.apache.polaris.service.catalog.api.PolarisCatalogGenericTableApiService;
import org.apache.polaris.service.types.CreateGenericTableRequest;
+import org.apache.polaris.service.types.ListGenericTablesResponse;
+import org.apache.polaris.service.types.LoadGenericTableResponse;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
@RequestScoped
public class GenericTableCatalogAdapter implements
PolarisCatalogGenericTableApiService {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(GenericTableCatalogAdapter.class);
+
+ private final CallContext callContext;
+ private final PolarisEntityManager entityManager;
+ private final PolarisMetaStoreManager metaStoreManager;
+ private final PolarisAuthorizer polarisAuthorizer;
+
+ @Inject
+ public GenericTableCatalogAdapter(
+ CallContext callContext,
+ PolarisEntityManager entityManager,
+ PolarisMetaStoreManager metaStoreManager,
+ PolarisAuthorizer polarisAuthorizer) {
+ this.callContext = callContext;
+ this.entityManager = entityManager;
+ this.metaStoreManager = metaStoreManager;
+ this.polarisAuthorizer = polarisAuthorizer;
+ }
+
+ private Namespace rawStringToNamespace(String namespace) {
Review Comment:
How about move this function to a util class
https://github.com/apache/polaris/blob/main/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java#L235,
and then reuse the same decode function for all namespaces, we are using the
same namespace concept across Polaris, it would be better if we share the same
namespace uitlities
##########
service/common/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -356,4 +348,23 @@ protected void authorizeRenameTableLikeOperationOrThrow(
initializeCatalog();
}
+
+ /**
+ * Helper function for when a TABLE_LIKE entity is not found & we want to
throw the appropriate
+ * exception
+ *
+ * @param subType The subtype of the entity that the exception should report
doesn't exist
+ */
+ public static void throwNotFoundException(
Review Comment:
nit : Maybe we can make the name of this function more clear, like
throwTableLikeEntityNotFoundException
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -472,11 +471,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() ==
ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() ==
PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s",
identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundException(identifier, subType);
Review Comment:
Also, we might need to do a verification after line 480 to make sure the
tableLikeEntity we found is table or view based on the isTable configuration.
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -472,11 +471,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
throw new NotFoundException("Catalog not found: %s", catalogName);
} else if (status.getStatus() ==
ResolverStatus.StatusEnum.PATH_COULD_NOT_BE_FULLY_RESOLVED) {
if (status.getFailedToResolvePath().getLastEntityType() ==
PolarisEntityType.TABLE_LIKE) {
- if (subType == PolarisEntitySubType.ICEBERG_TABLE) {
- throw new NoSuchTableException("Table does not exist: %s",
identifier);
- } else {
- throw new NoSuchViewException("View does not exist: %s", identifier);
- }
+ CatalogHandler.throwNotFoundException(identifier, subType);
Review Comment:
I see the subtype is passed as PolarisEntitySubType.ANY_SUBTYPE here, and
based on how the function is implemented, it seems we will just thrown
```
throw new IllegalStateException("Unrecognized entity subtype " + subType);
```
So the error message may not give the correct information here.
I took a look of the function, the subtype here doesn't seem doing much
here. I think we can simply update the subType parameter to boolean isTable
like following:
```
private void authorizeGrantOnTableLikeOperationOrThrow(
PolarisAuthorizableOperation op,
String catalogName,
boolean isTable,
TableIdentifier identifier,
String catalogRoleName)
```
when isTable is True, we can throw, no Iceberg or Generic Table is found.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
