dennishuo commented on code in PR #1805:
URL: https://github.com/apache/polaris/pull/1805#discussion_r2136169734
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java:
##########
@@ -473,6 +475,12 @@ private void revokeGrantRecord(
ms.persistStorageIntegrationIfNeededInCurrentTxn(callCtx, catalog,
integration);
+ // CATALOG_PRE_PERSIST mutation point
+ EntityMutationEngine entityMutationEngine =
callCtx.getEntityMutationEngine();
+ if (entityMutationEngine != null) {
+ entityMutationEngine.applyMutations(MutationPoint.CATALOG_PRE_PERSIST,
catalog);
Review Comment:
Does this need to be:
catalog =
entityMutationEngine.applyMutations(MutationPoint.CATALOG_PRE_PERSIST, catalog);
?
##########
polaris-core/src/main/java/org/apache/polaris/core/identity/mutation/EntityMutationEngine.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.core.identity.mutation;
+
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+
+/**
+ * Engine responsible for applying a sequence of {@link EntityMutator}
transformations to a {@link
+ * PolarisBaseEntity}.
+ *
+ * <p>This abstraction allows Polaris to customize or enrich entities during
runtime or persistence,
+ * based on configured or contextual logic (e.g., injecting service identity
info, computing derived
+ * fields).
+ */
+public interface EntityMutationEngine {
+ /**
+ * Applies all registered entity mutators to the provided entity, in order.
+ *
+ * @param mutationPoint The point in the entity lifecycle where mutations
should be applied.
+ * @param entity The original Polaris entity to mutate.
Review Comment:
It's too bad we don't have a way to express C++-style `const` correctness as
we move towards immutable PolarisBaseEntity, but maybe we can at least mention
here that the intention is that implementations should *not* mutate the input
entity, but instead only create the new transformed entity.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java:
##########
@@ -473,6 +475,12 @@ private void revokeGrantRecord(
ms.persistStorageIntegrationIfNeededInCurrentTxn(callCtx, catalog,
integration);
+ // CATALOG_PRE_PERSIST mutation point
Review Comment:
For now, unfortunately we didn't quite manage to refactor all of the shared
logic nryerrn `AtomicOperationMetaStoreManager` and
`TransactionalMetaStoreManagerImpl` into a base class yet; the best we have is
the `prepareToPersistNewEntity` hook defined in `BaseMetaStoreManager`.
This means either this logic needs to be added in both the
`AtomicOperationMetaStoreManager` and `TransactionalMetaStoreManagerImpl`, or
needs to be abstracted into `BaseMetaStoreManager` somehow while making sure
both of those impls invoke the shared logic. `persistNewEntity` isn't specific
to catalog-creation so you'd need an enum on entityType there if you just put
this logic in there.
##########
polaris-core/src/main/java/org/apache/polaris/core/identity/mutation/EntityMutationEngine.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.core.identity.mutation;
+
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+
+/**
+ * Engine responsible for applying a sequence of {@link EntityMutator}
transformations to a {@link
+ * PolarisBaseEntity}.
+ *
+ * <p>This abstraction allows Polaris to customize or enrich entities during
runtime or persistence,
+ * based on configured or contextual logic (e.g., injecting service identity
info, computing derived
+ * fields).
+ */
+public interface EntityMutationEngine {
Review Comment:
It's unfortunate that we haven't had a chance yet to make
`PolarisBaseEntity` immutable, but maybe our naming for this set of classes
should be something that more strongly implies producing a "new transformed
copy of the entity" rather than make it tempting for anyone to try mutating
them in-place.
What does everyone think of naming these instead:
- EntityTransformationEngine/EntityTransformer/TransformationPoint
- EntityDecorationEngine/EntityDecorator/DecorationPoint
?
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/identity/mutation/CatalogEntityConnectionConfigMutator.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.quarkus.identity.mutation;
+
+import io.smallrye.common.annotation.Identifier;
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.inject.Inject;
+import org.apache.polaris.core.connection.AuthenticationParametersDpo;
+import org.apache.polaris.core.connection.AuthenticationType;
+import org.apache.polaris.core.connection.ConnectionConfigInfoDpo;
+import org.apache.polaris.core.entity.CatalogEntity;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.core.identity.ServiceIdentityType;
+import org.apache.polaris.core.identity.dpo.ServiceIdentityInfoDpo;
+import org.apache.polaris.service.identity.mutation.AppliesTo;
+import org.apache.polaris.core.identity.mutation.EntityMutator;
+import org.apache.polaris.core.identity.mutation.MutationPoint;
+import org.apache.polaris.core.identity.registry.ServiceIdentityRegistry;
+
+/**
+ * Entity mutator that injects a {@link ServiceIdentityInfoDpo} into a
passthrough {@link
+ * CatalogEntity}, allowing the Polaris service identity to be surfaced to
Polaris users.
+ *
+ * <p>This is necessary for authentication mechanisms such as SigV4, where
Polaris users must
+ * explicitly allowlist the service identity that Polaris uses to access their
services.
+ */
+@RequestScoped
+@Identifier("catalog-connection-config")
+@AppliesTo(MutationPoint.CATALOG_PRE_PERSIST)
+public class CatalogEntityConnectionConfigMutator implements EntityMutator {
+
+ private final ServiceIdentityRegistry serviceIdentityRegistry;
+
+ @Inject
+ CatalogEntityConnectionConfigMutator(ServiceIdentityRegistry
serviceIdentityRegistry) {
+ this.serviceIdentityRegistry = serviceIdentityRegistry;
+ }
+
+ /**
+ * If the entity is a passthrough {@link CatalogEntity} and its connection
uses AWS SIGV4
+ * authentication, this method injects a service identity into its
connection configuration.
+ *
+ * @param entity the original Polaris entity
+ * @return the mutated entity with service identity injected, or the
original if no change is
+ * needed
+ */
+ @Override
+ public PolarisBaseEntity apply(PolarisBaseEntity entity) {
+ if (!(entity instanceof CatalogEntity catalogEntity) ||
!catalogEntity.isPassthroughFacade()) {
+ return entity;
+ }
+
+ ConnectionConfigInfoDpo connectionConfigInfoDpo =
catalogEntity.getConnectionConfigInfoDpo();
+ AuthenticationParametersDpo authenticationParameters =
+ connectionConfigInfoDpo.getAuthenticationParameters();
+
+ if (authenticationParameters.getAuthenticationType() ==
AuthenticationType.SIGV4) {
+ CatalogEntity.Builder builder = new CatalogEntity.Builder(catalogEntity);
+ ConnectionConfigInfoDpo injectedConnectionConfigInfoDpo =
+ connectionConfigInfoDpo.withServiceIdentity(
+
serviceIdentityRegistry.assignServiceIdentity(ServiceIdentityType.AWS_IAM));
+ builder.setConnectionConfigInfoDpo(injectedConnectionConfigInfoDpo);
+ builder.setEntityVersion(entity.getEntityVersion() + 1);
Review Comment:
Normally we should only increment the entityVersion if the intermediate
version is possibly visible to the outside world. It may or may not end up
being harmless to modify the version multiple times during construction of the
entity, but this would be inconsistent with other steps of the entity-building
process.
On the other side, it's true that it might be nice to have something that
represents the complete entity contents as a version or hashCode so that we can
compare across different transformations.
For now, we should probably avoid incrementing the entityVersion here for
consistency with how other things also still construct parts of the entity
during creation/updates without incrementing the version for every intermediate
state.
--
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]
