snazy commented on code in PR #1314:
URL: https://github.com/apache/polaris/pull/1314#discussion_r2031151468
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java:
##########
@@ -112,18 +122,26 @@ public Map<String, String> getConfigOverrides() {
}
}
- protected static final Namespace NS = Namespace.of("newdb");
- protected static final TableIdentifier TABLE = TableIdentifier.of(NS,
"table");
- public static final String CATALOG_NAME = "polaris-catalog";
- public static final String TEST_ACCESS_KEY = "test_access_key";
- public static final String SECRET_ACCESS_KEY = "secret_access_key";
- public static final String SESSION_TOKEN = "session_token";
-
- private static final Namespace NS1 = Namespace.of("ns1");
- private static final PolicyIdentifier POLICY1 = new PolicyIdentifier(NS1,
"p1");
- private static final PolicyIdentifier POLICY2 = new PolicyIdentifier(NS1,
"p2");
- private static final PolicyIdentifier POLICY3 = new PolicyIdentifier(NS1,
"p3");
- private static final PolicyIdentifier POLICY4 = new PolicyIdentifier(NS1,
"p4");
+ private static final Namespace NS = Namespace.of("ns1");
Review Comment:
Those are all useful constants, why this change to make those `private`?
##########
service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java:
##########
@@ -251,25 +247,219 @@ public Policy updatePolicy(
public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean
detachAll) {
// TODO: Implement detachAll when we support attach/detach policy
- PolarisResolvedPathWrapper resolvedEntities =
- resolvedEntityView.getPassthroughResolvedPath(
- policyIdentifier, PolarisEntityType.POLICY,
PolarisEntitySubType.NULL_SUBTYPE);
- if (resolvedEntities == null) {
- throw new NoSuchPolicyException(String.format("Policy does not exist:
%s", policyIdentifier));
- }
-
- List<PolarisEntity> catalogPath = resolvedEntities.getRawParentPath();
- PolarisEntity leafEntity = resolvedEntities.getRawLeafEntity();
+ var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier);
+ var catalogPath = resolvedPolicyPath.getRawParentPath();
+ var policyEntity = resolvedPolicyPath.getRawLeafEntity();
- DropEntityResult dropEntityResult =
- metaStoreManager.dropEntityIfExists(
+ return metaStoreManager
+ .dropEntityIfExists(
callContext.getPolarisCallContext(),
PolarisEntity.toCoreList(catalogPath),
- leafEntity,
+ policyEntity,
Map.of(),
- false);
+ false)
+ .isSuccess();
+ }
+
+ public boolean attachPolicy(
+ PolicyIdentifier policyIdentifier,
+ PolicyAttachmentTarget target,
+ Map<String, String> parameters) {
+
+ var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier);
+ var policyCatalogPath =
PolarisEntity.toCoreList(resolvedPolicyPath.getRawParentPath());
+ var policyEntity = PolicyEntity.of(resolvedPolicyPath.getRawLeafEntity());
+
+ var resolvedTargetPath = getResolvedPathWrapper(target);
+ var targetCatalogPath =
PolarisEntity.toCoreList(resolvedTargetPath.getRawParentPath());
+ var targetEntity = resolvedTargetPath.getRawLeafEntity();
+
+ PolicyValidators.validateAttach(policyEntity, targetEntity);
+
+ var result =
+ metaStoreManager.attachPolicyToEntity(
+ callContext.getPolarisCallContext(),
+ targetCatalogPath,
+ targetEntity,
+ policyCatalogPath,
+ policyEntity,
+ parameters);
+
+ if (result.getReturnStatus() ==
POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS) {
+ var targetId = catalogEntity.getName();
+ if (target.getType() != CATALOG) {
+ targetId += "." + String.join(".", target.getPath());
+ }
+ throw new PolicyMappingAlreadyExistsException(
+ "The policy mapping of same type(%s) for %s already exists",
+ policyEntity.getPolicyType().getName(), targetId);
+ }
+
+ return result.isSuccess();
+ }
+
+ public boolean detachPolicy(PolicyIdentifier policyIdentifier,
PolicyAttachmentTarget target) {
+ var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier);
+ var policyCatalogPath =
PolarisEntity.toCoreList(resolvedPolicyPath.getRawParentPath());
+ var policyEntity = PolicyEntity.of(resolvedPolicyPath.getRawLeafEntity());
+
+ var resolvedTargetPath = getResolvedPathWrapper(target);
+ var targetCatalogPath =
PolarisEntity.toCoreList(resolvedTargetPath.getRawParentPath());
+ var targetEntity = resolvedTargetPath.getRawLeafEntity();
+
+ return metaStoreManager
+ .detachPolicyFromEntity(
+ callContext.getPolarisCallContext(),
+ targetCatalogPath,
+ targetEntity,
+ policyCatalogPath,
+ policyEntity)
+ .isSuccess();
Review Comment:
Just yielding a `boolean` makes it impossible to determine the cause of the
error.
It's inconsistent behavior that some functions throw exceptions and some
just return `false` in case of an error (and hiding the cause).
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java:
##########
@@ -47,6 +47,7 @@
import org.apache.polaris.core.entity.PolarisPrivilege;
import org.apache.polaris.core.entity.PolarisTaskConstants;
import org.apache.polaris.core.persistence.*;
+import org.apache.polaris.core.persistence.PolicyMappingAlreadyExistsException;
Review Comment:
Why this change?
BTW: wildcard imports shouldn't be used.
##########
service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java:
##########
@@ -251,25 +247,219 @@ public Policy updatePolicy(
public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean
detachAll) {
// TODO: Implement detachAll when we support attach/detach policy
- PolarisResolvedPathWrapper resolvedEntities =
- resolvedEntityView.getPassthroughResolvedPath(
- policyIdentifier, PolarisEntityType.POLICY,
PolarisEntitySubType.NULL_SUBTYPE);
- if (resolvedEntities == null) {
- throw new NoSuchPolicyException(String.format("Policy does not exist:
%s", policyIdentifier));
- }
-
- List<PolarisEntity> catalogPath = resolvedEntities.getRawParentPath();
- PolarisEntity leafEntity = resolvedEntities.getRawLeafEntity();
+ var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier);
+ var catalogPath = resolvedPolicyPath.getRawParentPath();
+ var policyEntity = resolvedPolicyPath.getRawLeafEntity();
- DropEntityResult dropEntityResult =
- metaStoreManager.dropEntityIfExists(
+ return metaStoreManager
+ .dropEntityIfExists(
callContext.getPolarisCallContext(),
PolarisEntity.toCoreList(catalogPath),
- leafEntity,
+ policyEntity,
Map.of(),
- false);
+ false)
+ .isSuccess();
+ }
+
+ public boolean attachPolicy(
+ PolicyIdentifier policyIdentifier,
+ PolicyAttachmentTarget target,
+ Map<String, String> parameters) {
+
+ var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier);
+ var policyCatalogPath =
PolarisEntity.toCoreList(resolvedPolicyPath.getRawParentPath());
+ var policyEntity = PolicyEntity.of(resolvedPolicyPath.getRawLeafEntity());
+
+ var resolvedTargetPath = getResolvedPathWrapper(target);
+ var targetCatalogPath =
PolarisEntity.toCoreList(resolvedTargetPath.getRawParentPath());
+ var targetEntity = resolvedTargetPath.getRawLeafEntity();
+
+ PolicyValidators.validateAttach(policyEntity, targetEntity);
+
+ var result =
+ metaStoreManager.attachPolicyToEntity(
+ callContext.getPolarisCallContext(),
+ targetCatalogPath,
+ targetEntity,
+ policyCatalogPath,
+ policyEntity,
+ parameters);
+
+ if (result.getReturnStatus() ==
POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS) {
+ var targetId = catalogEntity.getName();
+ if (target.getType() != CATALOG) {
+ targetId += "." + String.join(".", target.getPath());
+ }
+ throw new PolicyMappingAlreadyExistsException(
+ "The policy mapping of same type(%s) for %s already exists",
+ policyEntity.getPolicyType().getName(), targetId);
+ }
+
+ return result.isSuccess();
Review Comment:
What if this returns `false`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
