sririshindra commented on code in PR #4225:
URL: https://github.com/apache/polaris/pull/4225#discussion_r3359327421
##########
runtime/service/src/main/java/org/apache/polaris/service/events/listeners/PolarisPersistenceEventListener.java:
##########
@@ -19,38 +19,37 @@
package org.apache.polaris.service.events.listeners;
-import com.google.common.collect.ImmutableMap;
+import jakarta.inject.Inject;
+import java.util.LinkedHashMap;
import java.util.Map;
-import org.apache.iceberg.TableMetadata;
-import org.apache.iceberg.TableMetadataParser;
+import java.util.Optional;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
-import org.apache.iceberg.rest.responses.LoadTableResponse;
-import org.apache.polaris.core.admin.model.Catalog;
+import org.apache.iceberg.rest.requests.CreateTableRequest;
+import org.apache.iceberg.rest.requests.RegisterTableRequest;
import org.apache.polaris.core.auth.PolarisPrincipal;
+import org.apache.polaris.service.events.EventAttributeMap;
import org.apache.polaris.service.events.EventAttributes;
+import org.apache.polaris.service.events.EventPayloadPruner;
import org.apache.polaris.service.events.PolarisEvent;
+import org.apache.polaris.service.events.PolarisEventType;
public abstract class PolarisPersistenceEventListener implements
PolarisEventListener {
- @Override
- public void onEvent(PolarisEvent event) {
- switch (event.type()) {
- case AFTER_CREATE_TABLE -> handleAfterCreateTable(event);
- case AFTER_CREATE_CATALOG -> handleAfterCreateCatalog(event);
- default -> {
- // Other events not handled by this listener
- }
- }
+ @Inject EventPayloadPruner payloadPruner;
+
+ protected PolarisPersistenceEventListener() {}
+
+ PolarisPersistenceEventListener(EventPayloadPruner payloadPruner) {
+ this.payloadPruner = payloadPruner;
}
- private void handleAfterCreateTable(PolarisEvent event) {
- LoadTableResponse loadTableResponse =
- event.attributes().getRequired(EventAttributes.LOAD_TABLE_RESPONSE);
- TableMetadata tableMetadata = loadTableResponse.tableMetadata();
- String catalogName =
event.attributes().getRequired(EventAttributes.CATALOG_NAME);
- Namespace namespace =
event.attributes().getRequired(EventAttributes.NAMESPACE);
- String tableName =
event.attributes().getRequired(EventAttributes.TABLE_NAME);
+ @Override
+ public void onEvent(PolarisEvent event) {
+ String catalogName = resolveCatalogName(event);
Review Comment:
You are right that `catalogId` is `NOT NULL` in the persistence schema, so
realm-scoped events (principal, principal-role, policy, etc.) can't store
`NULL` directly today. The `__realm__` sentinel is a transitional
placeholder.
Making the column nullable is a schema migration that touches `PolarisEvent`
in `polaris-core` and the underlying persistence — bigger blast radius than
this PR should carry. I've added an inline `TODO` on
`PolarisEvent.REALM_SCOPED` calling out the planned removal:
```java
// TODO: Remove this sentinel once the catalogId column is made nullable.
// Realm-scoped events should store NULL, not a magic string, so
// downstream queries can use IS NULL instead of filtering out a
// reserved value.
```
I'll file a follow-up issue with the migration plan and link it here once
opened. Happy to keep this PR focused on the routing/sanitization changes
and tackle the schema change separately — let me know if you'd prefer it
bundled.
##########
runtime/service/src/main/java/org/apache/polaris/service/events/PolarisEventListeners.java:
##########
@@ -80,8 +82,13 @@ public void onStartup(@Observes StartupEvent event) {
private void deliverEvent(
PolarisEvent event, String listenerName, PolarisEventListener listener) {
LOGGER.debug("Delivering {} event to listener '{}' ({})", event.type(),
listenerName, listener);
+ // TODO: Add an opt-in mechanism (e.g., a marker interface or annotation)
for specialized
Review Comment:
Done — implemented exactly as suggested:
- Added `org.apache.polaris.service.events.listeners.RawEventAccess`, an
empty
marker interface with Javadoc spelling out that it is an explicit
security opt-out and should be implemented only by listeners with a
documented need for sensitive attributes (e.g., forensic audit sinks).
- Updated the dispatcher in `PolarisEventListeners.deliverEvent`:
```java
PolarisEvent toDeliver =
(listener instanceof RawEventAccess) ? event :
eventSanitizer.sanitize(event);
listener.onEvent(toDeliver);
```
- Removed the placeholder `TODO` referencing the dev@ thread.
- Added `RawEventAccessTest` (a separate `@QuarkusTest` with its own profile)
to verify a `RawEventAccess` listener receives the unsanitized event with
the `CATALOG` attribute intact.
To answer the standing question: this PR does not introduce per-listener
sanitizer selection. There is one global `EventSanitizer` bean. The opt-in
is binary — sanitize, or pass through raw. If we later need per-listener
policy, that becomes a strategy choice inside the sanitizer rather than
another branch in `deliverEvent`.
##########
runtime/service/src/main/java/org/apache/polaris/service/events/EventSanitizer.java:
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.events;
+
+/**
+ * Global, dispatcher-enforced sanitizer for events. Invoked exactly once at
the choke point in
Review Comment:
Good catch — fixed. Updated the Javadoc on `EventSanitizer` to read "Invoked
once per listener delivery from `PolarisEventListeners.deliverEvent`, before
each listener that has not opted in via `RawEventAccess`. Each invocation
produces a fresh sanitized copy; the original event is never mutated."
Also dropped the "future raw-event escape-hatch" wording from the second
paragraph since `RawEventAccess` now exists (see reply on
`PolarisEventListeners.java:84`).
--
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]
