Copilot commented on code in PR #4507:
URL: https://github.com/apache/polaris/pull/4507#discussion_r3273768350
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -671,6 +672,40 @@ public Response createView(
return resp;
}
+ @Override
+ public Response registerView(
+ String prefix,
+ String namespace,
+ RegisterViewRequest registerViewRequest,
+ UUID idempotencyKey,
+ RealmContext realmContext,
+ SecurityContext securityContext) {
+ String catalogName = prefixParser.prefixToCatalogName(prefix);
+ Namespace namespaceObj =
+ NamespaceUtils.splitNamespace(namespace,
NamespaceUtils.DEFAULT_NAMESPACE_SEPARATOR);
+ polarisEventDispatcher.dispatch(
+ new PolarisEvent(
+ PolarisEventType.BEFORE_REGISTER_VIEW,
+ eventMetadataFactory.create(),
+ new EventAttributeMap()
+ .put(EventAttributes.CATALOG_NAME, catalogName)
+ .put(EventAttributes.NAMESPACE, namespaceObj)
+ .put(EventAttributes.REGISTER_VIEW_REQUEST,
registerViewRequest)));
Review Comment:
Unlike the other REST methods in this delegator, `registerView` dispatches
BEFORE events unconditionally. For consistency and to avoid unnecessary event
construction/dispatch overhead when there are no listeners, wrap this dispatch
in a
`polarisEventDispatcher.hasListeners(PolarisEventType.BEFORE_REGISTER_VIEW)`
check (mirroring `createView`, `updateTable`, etc.).
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java:
##########
@@ -519,6 +520,22 @@ public Response createView(
catalog -> Response.ok(catalog.createView(ns,
revisedRequest)).build());
}
+ @Override
+ public Response registerView(
+ String prefix,
+ String namespace,
+ RegisterViewRequest registerViewRequest,
+ UUID idempotencyKey,
+ RealmContext realmContext,
+ SecurityContext securityContext) {
+ Namespace ns =
+ NamespaceUtils.splitNamespace(namespace,
NamespaceUtils.DEFAULT_NAMESPACE_SEPARATOR);
+ return withCatalog(
+ securityContext,
+ prefix,
+ catalog -> Response.ok(catalog.registerView(ns,
registerViewRequest)).build());
+ }
Review Comment:
`registerView` skips the request and identifier validation that similar
endpoints perform (e.g., `registerTable` and `createView`). This can lead to
inconsistent client-facing errors and may allow invalid view identifiers to
reach deeper layers. Consider calling `registerViewRequest.validate()` and
validating the identifier with
`EntityNameValidator.validateIdentifier(TableIdentifier.of(ns,
registerViewRequest.name()))` before invoking the handler.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -671,6 +672,40 @@ public Response createView(
return resp;
}
+ @Override
+ public Response registerView(
+ String prefix,
+ String namespace,
+ RegisterViewRequest registerViewRequest,
+ UUID idempotencyKey,
+ RealmContext realmContext,
+ SecurityContext securityContext) {
+ String catalogName = prefixParser.prefixToCatalogName(prefix);
+ Namespace namespaceObj =
+ NamespaceUtils.splitNamespace(namespace,
NamespaceUtils.DEFAULT_NAMESPACE_SEPARATOR);
+ polarisEventDispatcher.dispatch(
+ new PolarisEvent(
+ PolarisEventType.BEFORE_REGISTER_VIEW,
+ eventMetadataFactory.create(),
+ new EventAttributeMap()
+ .put(EventAttributes.CATALOG_NAME, catalogName)
+ .put(EventAttributes.NAMESPACE, namespaceObj)
+ .put(EventAttributes.REGISTER_VIEW_REQUEST,
registerViewRequest)));
+ Response resp =
+ delegate.registerView(
+ prefix, namespace, registerViewRequest, idempotencyKey,
realmContext, securityContext);
+ polarisEventDispatcher.dispatch(
+ new PolarisEvent(
+ PolarisEventType.AFTER_REGISTER_VIEW,
+ eventMetadataFactory.create(),
+ new EventAttributeMap()
+ .put(EventAttributes.CATALOG_NAME, catalogName)
+ .put(EventAttributes.NAMESPACE, namespaceObj)
+ .put(EventAttributes.VIEW_NAME, registerViewRequest.name())
+ .put(EventAttributes.LOAD_VIEW_RESPONSE, (LoadViewResponse)
resp.getEntity())));
Review Comment:
`registerView` dispatches the AFTER event unconditionally; other methods
guard with `hasListeners(...)`. Please add a
`hasListeners(PolarisEventType.AFTER_REGISTER_VIEW)` check before building the
attribute map (and casting `resp.getEntity()`), to avoid unnecessary work and
keep behavior consistent with the rest of this delegator.
--
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]
