adutra commented on code in PR #2482: URL: https://github.com/apache/polaris/pull/2482#discussion_r2318110814
########## runtime/service/src/main/java/org/apache/polaris/service/events/CatalogsServiceEvents.java: ########## @@ -0,0 +1,148 @@ +/* + * 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; + +import org.apache.polaris.core.admin.model.AddGrantRequest; +import org.apache.polaris.core.admin.model.Catalog; +import org.apache.polaris.core.admin.model.CatalogRole; +import org.apache.polaris.core.admin.model.GrantResource; +import org.apache.polaris.core.admin.model.RevokeGrantRequest; +import org.apache.polaris.core.admin.model.UpdateCatalogRequest; +import org.apache.polaris.core.admin.model.UpdateCatalogRoleRequest; +import org.apache.polaris.core.entity.PolarisPrivilege; + +public class CatalogsServiceEvents { + public record BeforeCatalogCreatedEvent(String catalogName) implements PolarisEvent {} Review Comment: nit: still some naming inconsistencies: most events use a verb in the infinitive form (`BeforeCatalogListEvent`) but some use the past participle (`BeforeCatalogCreatedEvent`). ########## runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java: ########## @@ -630,6 +635,12 @@ public Response listCatalogRolesForPrincipalRole( return Response.ok(catalogRoles).build(); } + record AddGrantToCatalogRoleEntityWrapper( Review Comment: These records have to be public, as they are potentially escaping the package (IOW, a decorator sitting outside this package would be unable to process the response returned by `addGrantToCatalogRole`). These records are OK for now, but we need to include these structures in the OpenAPI spec for this service. Could you please add a TODO for that, or better yet, file an issue? WIthout proper inclusion in the OpenAPI spec, these structures become a form of "undocumented" API. It seems the problematic endpoints are: * `createCatalog` * `createPrincipalRole` * `createCatalogRole` * `addGrantToCatalogRole` * `revokeGrantFromCatalogRole` All of them would need to have their response schema updated in the OpenAPI spec. ########## runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisCatalogsEventServiceDelegator.java: ########## @@ -123,8 +185,25 @@ public Response addGrantToCatalogRole( AddGrantRequest grantRequest, RealmContext realmContext, SecurityContext securityContext) { - return delegate.addGrantToCatalogRole( - catalogName, catalogRoleName, grantRequest, realmContext, securityContext); + polarisEventListener.onBeforeAddGrantToCatalogRole( + new CatalogsServiceEvents.BeforeAddGrantToCatalogRoleEvent( + catalogName, catalogRoleName, grantRequest)); + Response resp = + delegate.addGrantToCatalogRole( + catalogName, catalogRoleName, grantRequest, realmContext, securityContext); + PolarisServiceImpl.AddGrantToCatalogRoleEntityWrapper entityWrapper = + (PolarisServiceImpl.AddGrantToCatalogRoleEntityWrapper) resp.getEntity(); + if (resp.getStatus() != Response.Status.BAD_REQUEST.getStatusCode()) { Review Comment: ```suggestion if (resp.getStatus() == Response.Status.CREATED.getStatusCode()) { ``` ########## runtime/service/src/main/java/org/apache/polaris/service/events/CatalogsServiceEvents.java: ########## @@ -0,0 +1,148 @@ +/* + * 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; + +import org.apache.polaris.core.admin.model.AddGrantRequest; +import org.apache.polaris.core.admin.model.Catalog; +import org.apache.polaris.core.admin.model.CatalogRole; +import org.apache.polaris.core.admin.model.GrantResource; +import org.apache.polaris.core.admin.model.RevokeGrantRequest; +import org.apache.polaris.core.admin.model.UpdateCatalogRequest; +import org.apache.polaris.core.admin.model.UpdateCatalogRoleRequest; +import org.apache.polaris.core.entity.PolarisPrivilege; + +public class CatalogsServiceEvents { + public record BeforeCatalogCreatedEvent(String catalogName) implements PolarisEvent {} + + public record AfterCatalogCreatedEvent(Catalog catalog) implements PolarisEvent {} + + public record BeforeCatalogDeletedEvent(String catalogName) implements PolarisEvent {} + + public record AfterCatalogDeletedEvent(String catalogName) implements PolarisEvent {} + + public record BeforeCatalogGetEvent(String catalogName) implements PolarisEvent {} + + public record AfterCatalogGetEvent(Catalog catalog) implements PolarisEvent {} + + public record BeforeCatalogUpdatedEvent(String catalogName, UpdateCatalogRequest updateRequest) + implements PolarisEvent {} + + public record AfterCatalogUpdatedEvent(Catalog catalog) implements PolarisEvent {} + + public record BeforeCatalogListEvent() implements PolarisEvent {} + + public record AfterCatalogListEvent() implements PolarisEvent {} + + public record BeforeCatalogRoleCreateEvent(String catalogName, String catalogRoleName) + implements PolarisEvent {} + + public record AfterCatalogRoleCreateEvent(String catalogName, CatalogRole catalogRole) + implements PolarisEvent {} + + public record BeforeCatalogRoleDeleteEvent(String catalogName, String catalogRoleName) + implements PolarisEvent {} + + public record AfterCatalogRoleDeleteEvent(String catalogName, String catalogRoleName) + implements PolarisEvent {} + + public record BeforeCatalogRoleGetEvent(String catalogName, String catalogRoleName) + implements PolarisEvent {} + + public record AfterCatalogRoleGetEvent(String catalogName, CatalogRole catalogRole) + implements PolarisEvent {} + + public record BeforeCatalogRoleUpdateEvent( + String catalogName, String catalogRoleName, UpdateCatalogRoleRequest updateRequest) + implements PolarisEvent {} + + public record AfterCatalogRoleUpdateEvent(String catalogName, CatalogRole updatedCatalogRole) + implements PolarisEvent {} + + public record BeforeCatalogRolesListEvent(String catalogName) implements PolarisEvent {} + + public record AfterCatalogRolesListEvent(String catalogName) implements PolarisEvent {} + + /** + * Event fired before a grant is added to a catalog role in Polaris. + * + * @param catalogName the name of the catalog + * @param catalogRoleName the name of the catalog role + * @param grantRequest the grant request + */ + public record BeforeAddGrantToCatalogRoleEvent( + String catalogName, String catalogRoleName, AddGrantRequest grantRequest) + implements PolarisEvent {} + + /** + * Event fired after a grant is added to a catalog role in Polaris. + * + * @param catalogName the name of the catalog + * @param catalogRoleName the name of the catalog role + * @param privilege the privilege granted + * @param grantResource the grant resource + */ + public record AfterAddGrantToCatalogRoleEvent( Review Comment: *Ideally*, if we eventually modify the OpenAPI spec, I think each "after" event should take a response object that mirrors the "before" event and its request object. E.g.: ```java record BeforeAddGrantToCatalogRoleEvent(String, String, AddGrantRequest) record AfterAddGrantToCatalogRoleEvent(String, String, AddGrantResponse) ``` ########## runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisCatalogsEventServiceDelegator.java: ########## @@ -135,8 +214,22 @@ public Response revokeGrantFromCatalogRole( RevokeGrantRequest grantRequest, RealmContext realmContext, SecurityContext securityContext) { - return delegate.revokeGrantFromCatalogRole( - catalogName, catalogRoleName, cascade, grantRequest, realmContext, securityContext); + polarisEventListener.onBeforeRevokeGrantFromCatalogRole( + new CatalogsServiceEvents.BeforeRevokeGrantFromCatalogRoleEvent( + catalogName, catalogRoleName, grantRequest, cascade)); + Response resp = + delegate.revokeGrantFromCatalogRole( + catalogName, catalogRoleName, cascade, grantRequest, realmContext, securityContext); + PolarisServiceImpl.RevokeGrantFromCatalogRoleEntityWrapper entityWrapper = + (PolarisServiceImpl.RevokeGrantFromCatalogRoleEntityWrapper) resp.getEntity(); + polarisEventListener.onAfterRevokeGrantFromCatalogRole( + new CatalogsServiceEvents.AfterRevokeGrantFromCatalogRoleEvent( + catalogName, + catalogRoleName, + entityWrapper.polarisPrivilege(), + entityWrapper.grantResource(), + entityWrapper.cascade())); + return resp; Review Comment: For now, let's not include the entity in the response, since that would be a violation of the spec. ########## runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java: ########## @@ -397,7 +402,7 @@ public Response createPrincipalRole( PrincipalRole newPrincipalRole = new PrincipalRoleEntity(adminService.createPrincipalRole(entity)).asPrincipalRole(); LOGGER.info("Created new principalRole {}", newPrincipalRole); - return Response.status(Response.Status.CREATED).build(); + return Response.status(Response.Status.CREATED).entity(newPrincipalRole).build(); Review Comment: nit: we could standardize the usage of `toResponse` for all endpoints. ########## runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisCatalogsEventServiceDelegator.java: ########## @@ -106,14 +157,25 @@ public Response updateCatalogRole( UpdateCatalogRoleRequest updateRequest, RealmContext realmContext, SecurityContext securityContext) { - return delegate.updateCatalogRole( - catalogName, catalogRoleName, updateRequest, realmContext, securityContext); + polarisEventListener.onBeforeCatalogRoleUpdate( + new CatalogsServiceEvents.BeforeCatalogRoleUpdateEvent( + catalogName, catalogRoleName, updateRequest)); + Response resp = + delegate.updateCatalogRole( + catalogName, catalogRoleName, updateRequest, realmContext, securityContext); + polarisEventListener.onAfterCatalogRoleUpdate( + new CatalogsServiceEvents.AfterCatalogRoleUpdateEvent( + catalogName, (CatalogRole) resp.getEntity())); + return resp; } @Override public Response listCatalogRoles( String catalogName, RealmContext realmContext, SecurityContext securityContext) { - return delegate.listCatalogRoles(catalogName, realmContext, securityContext); + polarisEventListener.onBeforeCatalogList(new CatalogsServiceEvents.BeforeCatalogListEvent()); Review Comment: Is this the right event? We are listing catalog roles, not catalogs. -- 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]
