Copilot commented on code in PR #12797: URL: https://github.com/apache/gravitino/pull/12797#discussion_r3905950661
########## core/src/main/java/org/apache/gravitino/hook/ViewHookDispatcher.java: ########## @@ -0,0 +1,134 @@ +/* + * 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.gravitino.hook; + +import java.util.Map; +import java.util.function.Supplier; +import javax.annotation.Nullable; +import org.apache.gravitino.Entity; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.authorization.AuthorizationUtils; +import org.apache.gravitino.authorization.Owner; +import org.apache.gravitino.authorization.OwnerDispatcher; +import org.apache.gravitino.catalog.CapabilityHelpers; +import org.apache.gravitino.catalog.CatalogManager; +import org.apache.gravitino.catalog.ViewDispatcher; +import org.apache.gravitino.connector.capability.Capability; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.exceptions.NoSuchViewException; +import org.apache.gravitino.exceptions.ViewAlreadyExistsException; +import org.apache.gravitino.rel.Column; +import org.apache.gravitino.rel.Representation; +import org.apache.gravitino.rel.View; +import org.apache.gravitino.rel.ViewChange; +import org.apache.gravitino.utils.NameIdentifierUtil; +import org.apache.gravitino.utils.PrincipalUtils; + +/** + * {@code ViewHookDispatcher} decorates a {@link ViewDispatcher} with ownership and authorization + * lifecycle hooks. + */ +public class ViewHookDispatcher implements ViewDispatcher { + private final ViewDispatcher dispatcher; + private final Supplier<OwnerDispatcher> ownerDispatcher; + private final CatalogManager catalogManager; + + /** + * Creates a view hook dispatcher. + * + * @param dispatcher the underlying view dispatcher + * @param ownerDispatcher supplies the owner dispatcher, or {@code null} when authorization is + * disabled + * @param catalogManager the catalog manager used to apply catalog capabilities + */ + public ViewHookDispatcher( + ViewDispatcher dispatcher, + Supplier<OwnerDispatcher> ownerDispatcher, + CatalogManager catalogManager) { + this.dispatcher = dispatcher; + this.ownerDispatcher = ownerDispatcher; + this.catalogManager = catalogManager; + } Review Comment: `ownerDispatcher` is invoked unconditionally via `ownerDispatcher.get()` later, so passing a null supplier would cause an immediate NPE. To make the contract explicit and safer for future call sites, either enforce non-null in the constructor (e.g., `Objects.requireNonNull(ownerDispatcher, ...)`) and clarify Javadoc that the *supplier returns null* when authz is disabled, or allow a nullable supplier and guard before calling `get()`. ########## server/src/main/java/org/apache/gravitino/server/web/rest/ViewOperations.java: ########## @@ -68,17 +74,27 @@ public ViewOperations(ViewDispatcher dispatcher) { @Produces("application/vnd.gravitino.v1+json") @Timed(name = "list-view." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) @ResponseMetered(name = "list-view", absolute = true) + @AuthorizationExpression( + expression = AuthorizationExpressionConstants.LOAD_SCHEMA_AUTHORIZATION_EXPRESSION, + accessMetadataType = MetadataObject.Type.SCHEMA) public Response listViews( - @PathParam("metalake") String metalake, - @PathParam("catalog") String catalog, - @PathParam("schema") String schema) { + @PathParam("metalake") @AuthorizationMetadata(type = Entity.EntityType.METALAKE) + String metalake, + @PathParam("catalog") @AuthorizationMetadata(type = Entity.EntityType.CATALOG) String catalog, + @PathParam("schema") @AuthorizationMetadata(type = Entity.EntityType.SCHEMA) String schema) { LOG.info("Received list views request for schema: {}.{}.{}", metalake, catalog, schema); try { return Utils.doAs( httpRequest, () -> { Namespace viewNS = NamespaceUtil.ofView(metalake, catalog, schema); NameIdentifier[] idents = dispatcher.listViews(viewNS); + idents = + MetadataAuthzHelper.filterByExpression( + metalake, + AuthorizationExpressionConstants.FILTER_VIEW_AUTHORIZATION_EXPRESSION, + Entity.EntityType.VIEW, + idents); Review Comment: The PR description and OpenAPI text say list results are filtered *when authorization is enabled*, but the filtering call is unconditional here. If `MetadataAuthzHelper.filterByExpression(...)` is intended to be a no-op when authz is disabled, consider making that condition explicit in this method (using whatever ‘authz enabled’ signal exists in the server) to avoid relying on helper internals and to keep the endpoint semantics self-evident. ########## core/src/main/java/org/apache/gravitino/hook/ViewHookDispatcher.java: ########## @@ -0,0 +1,134 @@ +/* + * 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.gravitino.hook; + +import java.util.Map; +import java.util.function.Supplier; +import javax.annotation.Nullable; +import org.apache.gravitino.Entity; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.authorization.AuthorizationUtils; +import org.apache.gravitino.authorization.Owner; +import org.apache.gravitino.authorization.OwnerDispatcher; +import org.apache.gravitino.catalog.CapabilityHelpers; +import org.apache.gravitino.catalog.CatalogManager; +import org.apache.gravitino.catalog.ViewDispatcher; +import org.apache.gravitino.connector.capability.Capability; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.exceptions.NoSuchViewException; +import org.apache.gravitino.exceptions.ViewAlreadyExistsException; +import org.apache.gravitino.rel.Column; +import org.apache.gravitino.rel.Representation; +import org.apache.gravitino.rel.View; +import org.apache.gravitino.rel.ViewChange; +import org.apache.gravitino.utils.NameIdentifierUtil; +import org.apache.gravitino.utils.PrincipalUtils; + +/** + * {@code ViewHookDispatcher} decorates a {@link ViewDispatcher} with ownership and authorization + * lifecycle hooks. + */ +public class ViewHookDispatcher implements ViewDispatcher { + private final ViewDispatcher dispatcher; + private final Supplier<OwnerDispatcher> ownerDispatcher; + private final CatalogManager catalogManager; + + /** + * Creates a view hook dispatcher. + * + * @param dispatcher the underlying view dispatcher + * @param ownerDispatcher supplies the owner dispatcher, or {@code null} when authorization is + * disabled + * @param catalogManager the catalog manager used to apply catalog capabilities + */ + public ViewHookDispatcher( + ViewDispatcher dispatcher, + Supplier<OwnerDispatcher> ownerDispatcher, + CatalogManager catalogManager) { + this.dispatcher = dispatcher; + this.ownerDispatcher = ownerDispatcher; + this.catalogManager = catalogManager; + } + + @Override + public NameIdentifier[] listViews(Namespace namespace) throws NoSuchSchemaException { + return dispatcher.listViews(namespace); + } + + @Override + public View loadView(NameIdentifier ident) throws NoSuchViewException { + return dispatcher.loadView(ident); + } + + @Override + public boolean viewExists(NameIdentifier ident) { + return dispatcher.viewExists(ident); + } + + @Override + public View createView( + NameIdentifier ident, + @Nullable String comment, + Column[] columns, + Representation[] representations, + @Nullable String defaultCatalog, + @Nullable String defaultSchema, + Map<String, String> properties) + throws NoSuchSchemaException, ViewAlreadyExistsException { + View view = + dispatcher.createView( + ident, comment, columns, representations, defaultCatalog, defaultSchema, properties); + + OwnerDispatcher ownerManager = ownerDispatcher.get(); + if (ownerManager != null) { + NameIdentifier normalizedIdent = + CapabilityHelpers.applyCapabilities(ident, Capability.Scope.VIEW, catalogManager); + ownerManager.setOwner( + normalizedIdent.namespace().level(0), + NameIdentifierUtil.toMetadataObject(normalizedIdent, Entity.EntityType.VIEW), + PrincipalUtils.getCurrentUserName(), + Owner.Type.USER); Review Comment: If `setOwner(...)` throws after `dispatcher.createView(...)` succeeds, the API call fails but the View may already exist without owner metadata, leaving the system in a partially-applied state. Consider wrapping the owner assignment in a try/catch that attempts best-effort cleanup (e.g., `dispatcher.dropView(ident)` or the normalized identifier as appropriate) before rethrowing, or alternatively record/log the failure and return success only if the project’s semantics allow best-effort ownership. -- 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]
