diff --git a/core/trino-main/src/main/java/io/trino/execution/CommentTask.java b/core/trino-main/src/main/java/io/trino/execution/CommentTask.java index 1b8dfde93ae3..93bd4371dd4f 100644 --- a/core/trino-main/src/main/java/io/trino/execution/CommentTask.java +++ b/core/trino-main/src/main/java/io/trino/execution/CommentTask.java @@ -39,6 +39,7 @@ import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED; import static io.trino.spi.StandardErrorCode.TABLE_NOT_FOUND; import static io.trino.sql.analyzer.SemanticExceptions.semanticException; +import static java.lang.String.format; import static java.util.Objects.requireNonNull; public class CommentTask @@ -70,43 +71,74 @@ public ListenableFuture execute( Session session = stateMachine.getSession(); if (statement.getType() == Comment.Type.TABLE) { - QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, statement.getName()); - RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName); - if (redirectionAwareTableHandle.getTableHandle().isEmpty()) { - throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: %s", originalTableName); - } - - accessControl.checkCanSetTableComment(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName)); - TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get(); - metadata.setTableComment(session, tableHandle, statement.getComment()); + commentOnTable(statement, session); + } + else if (statement.getType() == Comment.Type.VIEW) { + commentOnView(statement, session); } else if (statement.getType() == Comment.Type.COLUMN) { - Optional prefix = statement.getName().getPrefix(); - if (prefix.isEmpty()) { - throw semanticException(MISSING_TABLE, statement, "Table must be specified"); - } + commentOnColumn(statement, session); + } + else { + throw semanticException(NOT_SUPPORTED, statement, "Unsupported comment type: %s", statement.getType()); + } - QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, prefix.get()); - RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName); - if (redirectionAwareTableHandle.getTableHandle().isEmpty()) { - throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: " + originalTableName); - } - TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get(); + return immediateVoidFuture(); + } + + private void commentOnTable(Comment statement, Session session) + { + QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, statement.getName()); + RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName); + if (redirectionAwareTableHandle.getTableHandle().isEmpty()) { + throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: %s", originalTableName); + } - String columnName = statement.getName().getSuffix(); - Map columnHandles = metadata.getColumnHandles(session, tableHandle); - if (!columnHandles.containsKey(columnName)) { - throw semanticException(COLUMN_NOT_FOUND, statement, "Column does not exist: " + columnName); + accessControl.checkCanSetTableComment(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName)); + TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get(); + metadata.setTableComment(session, tableHandle, statement.getComment()); + } + + private void commentOnView(Comment statement, Session session) + { + QualifiedObjectName viewName = createQualifiedObjectName(session, statement, statement.getName()); + if (metadata.getView(session, viewName).isEmpty()) { + String exceptionMessage = format("View '%s' does not exist", viewName); + if (metadata.getMaterializedView(session, viewName).isPresent()) { + exceptionMessage += ", but a materialized view with that name exists."; + } + else if (metadata.getTableHandle(session, viewName).isPresent()) { + exceptionMessage += ", but a table with that name exists. Did you mean COMMENT ON TABLE " + viewName + " IS ...?"; } + throw semanticException(TABLE_NOT_FOUND, statement, exceptionMessage); + } + + accessControl.checkCanSetViewComment(session.toSecurityContext(), viewName); + metadata.setViewComment(session, viewName, statement.getComment()); + } - accessControl.checkCanSetColumnComment(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName)); + private void commentOnColumn(Comment statement, Session session) + { + Optional prefix = statement.getName().getPrefix(); + if (prefix.isEmpty()) { + throw semanticException(MISSING_TABLE, statement, "Table must be specified"); + } - metadata.setColumnComment(session, tableHandle, columnHandles.get(columnName), statement.getComment()); + QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, prefix.get()); + RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName); + if (redirectionAwareTableHandle.getTableHandle().isEmpty()) { + throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: " + originalTableName); } - else { - throw semanticException(NOT_SUPPORTED, statement, "Unsupported comment type: %s", statement.getType()); + TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get(); + + String columnName = statement.getName().getSuffix(); + Map columnHandles = metadata.getColumnHandles(session, tableHandle); + if (!columnHandles.containsKey(columnName)) { + throw semanticException(COLUMN_NOT_FOUND, statement, "Column does not exist: " + columnName); } - return immediateVoidFuture(); + accessControl.checkCanSetColumnComment(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName)); + + metadata.setColumnComment(session, tableHandle, columnHandles.get(columnName), statement.getComment()); } } diff --git a/core/trino-main/src/main/java/io/trino/metadata/Metadata.java b/core/trino-main/src/main/java/io/trino/metadata/Metadata.java index 875f2947eacb..0ec4173e7e48 100644 --- a/core/trino-main/src/main/java/io/trino/metadata/Metadata.java +++ b/core/trino-main/src/main/java/io/trino/metadata/Metadata.java @@ -212,6 +212,11 @@ Optional getTableHandleForExecute( */ void setTableComment(Session session, TableHandle tableHandle, Optional comment); + /** + * Comments to the specified view. + */ + void setViewComment(Session session, QualifiedObjectName viewName, Optional comment); + /** * Comments to the specified column. */ diff --git a/core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java b/core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java index 177dd8f7fd5b..706a5e08cd05 100644 --- a/core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java +++ b/core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java @@ -675,6 +675,14 @@ public void setTableComment(Session session, TableHandle tableHandle, Optional comment) + { + CatalogName catalogName = new CatalogName(viewName.getCatalogName()); + ConnectorMetadata metadata = getMetadataForWrite(session, catalogName); + metadata.setViewComment(session.toConnectorSession(catalogName), viewName.asSchemaTableName(), comment); + } + @Override public void setColumnComment(Session session, TableHandle tableHandle, ColumnHandle column, Optional comment) { diff --git a/core/trino-main/src/main/java/io/trino/security/AccessControl.java b/core/trino-main/src/main/java/io/trino/security/AccessControl.java index 3902ca444c9b..c266ba9373a0 100644 --- a/core/trino-main/src/main/java/io/trino/security/AccessControl.java +++ b/core/trino-main/src/main/java/io/trino/security/AccessControl.java @@ -197,6 +197,13 @@ public interface AccessControl */ void checkCanSetTableComment(SecurityContext context, QualifiedObjectName tableName); + /** + * Check if identity is allowed to comment the specified view. + * + * @throws AccessDeniedException if not allowed + */ + void checkCanSetViewComment(SecurityContext context, QualifiedObjectName viewName); + /** * Check if identity is allowed to comment the specified column. * diff --git a/core/trino-main/src/main/java/io/trino/security/AccessControlManager.java b/core/trino-main/src/main/java/io/trino/security/AccessControlManager.java index 3524d78fcd56..f6e3d0519765 100644 --- a/core/trino-main/src/main/java/io/trino/security/AccessControlManager.java +++ b/core/trino-main/src/main/java/io/trino/security/AccessControlManager.java @@ -482,6 +482,19 @@ public void checkCanSetTableComment(SecurityContext securityContext, QualifiedOb catalogAuthorizationCheck(tableName.getCatalogName(), securityContext, (control, context) -> control.checkCanSetTableComment(context, tableName.asSchemaTableName())); } + @Override + public void checkCanSetViewComment(SecurityContext securityContext, QualifiedObjectName viewName) + { + requireNonNull(securityContext, "securityContext is null"); + requireNonNull(viewName, "viewName is null"); + + checkCanAccessCatalog(securityContext, viewName.getCatalogName()); + + systemAuthorizationCheck(control -> control.checkCanSetViewComment(securityContext.toSystemSecurityContext(), viewName.asCatalogSchemaTableName())); + + catalogAuthorizationCheck(viewName.getCatalogName(), securityContext, (control, context) -> control.checkCanSetViewComment(context, viewName.asSchemaTableName())); + } + @Override public void checkCanSetColumnComment(SecurityContext securityContext, QualifiedObjectName tableName) { diff --git a/core/trino-main/src/main/java/io/trino/security/AllowAllAccessControl.java b/core/trino-main/src/main/java/io/trino/security/AllowAllAccessControl.java index a9e86932d0b8..f96bcf927300 100644 --- a/core/trino-main/src/main/java/io/trino/security/AllowAllAccessControl.java +++ b/core/trino-main/src/main/java/io/trino/security/AllowAllAccessControl.java @@ -144,6 +144,11 @@ public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName { } + @Override + public void checkCanSetViewComment(SecurityContext context, QualifiedObjectName viewName) + { + } + @Override public void checkCanSetColumnComment(SecurityContext context, QualifiedObjectName tableName) { diff --git a/core/trino-main/src/main/java/io/trino/security/DenyAllAccessControl.java b/core/trino-main/src/main/java/io/trino/security/DenyAllAccessControl.java index 3e9d71cce596..bb5fbb7e2d8a 100644 --- a/core/trino-main/src/main/java/io/trino/security/DenyAllAccessControl.java +++ b/core/trino-main/src/main/java/io/trino/security/DenyAllAccessControl.java @@ -33,6 +33,7 @@ import static io.trino.spi.security.AccessDeniedException.denyAddColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentTable; +import static io.trino.spi.security.AccessDeniedException.denyCommentView; import static io.trino.spi.security.AccessDeniedException.denyCreateMaterializedView; import static io.trino.spi.security.AccessDeniedException.denyCreateRole; import static io.trino.spi.security.AccessDeniedException.denyCreateSchema; @@ -216,6 +217,12 @@ public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName denyCommentTable(tableName.toString()); } + @Override + public void checkCanSetViewComment(SecurityContext context, QualifiedObjectName viewName) + { + denyCommentView(viewName.toString()); + } + @Override public void checkCanSetColumnComment(SecurityContext context, QualifiedObjectName tableName) { diff --git a/core/trino-main/src/main/java/io/trino/security/ForwardingAccessControl.java b/core/trino-main/src/main/java/io/trino/security/ForwardingAccessControl.java index 6fea169c4b63..e86f97dd409b 100644 --- a/core/trino-main/src/main/java/io/trino/security/ForwardingAccessControl.java +++ b/core/trino-main/src/main/java/io/trino/security/ForwardingAccessControl.java @@ -191,6 +191,12 @@ public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName delegate().checkCanSetTableComment(context, tableName); } + @Override + public void checkCanSetViewComment(SecurityContext context, QualifiedObjectName viewName) + { + delegate().checkCanSetViewComment(context, viewName); + } + @Override public void checkCanSetColumnComment(SecurityContext context, QualifiedObjectName tableName) { diff --git a/core/trino-main/src/main/java/io/trino/security/InjectedConnectorAccessControl.java b/core/trino-main/src/main/java/io/trino/security/InjectedConnectorAccessControl.java index ec885716b153..39b9d88aba6b 100644 --- a/core/trino-main/src/main/java/io/trino/security/InjectedConnectorAccessControl.java +++ b/core/trino-main/src/main/java/io/trino/security/InjectedConnectorAccessControl.java @@ -149,6 +149,13 @@ public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTabl accessControl.checkCanSetTableComment(securityContext, getQualifiedObjectName(tableName)); } + @Override + public void checkCanSetViewComment(ConnectorSecurityContext context, SchemaTableName viewName) + { + checkArgument(context == null, "context must be null"); + accessControl.checkCanSetViewComment(securityContext, getQualifiedObjectName(viewName)); + } + @Override public void checkCanSetColumnComment(ConnectorSecurityContext context, SchemaTableName tableName) { diff --git a/core/trino-main/src/main/java/io/trino/testing/AllowAllAccessControlManager.java b/core/trino-main/src/main/java/io/trino/testing/AllowAllAccessControlManager.java index cc72964b556c..213a1e3dd1dd 100644 --- a/core/trino-main/src/main/java/io/trino/testing/AllowAllAccessControlManager.java +++ b/core/trino-main/src/main/java/io/trino/testing/AllowAllAccessControlManager.java @@ -108,6 +108,9 @@ public void checkCanSetTableProperties(SecurityContext context, QualifiedObjectN @Override public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName tableName) {} + @Override + public void checkCanSetViewComment(SecurityContext context, QualifiedObjectName viewName) {} + @Override public void checkCanSetColumnComment(SecurityContext context, QualifiedObjectName tableName) {} diff --git a/core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java b/core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java index 8d928c0a7654..05ecca57fa05 100644 --- a/core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java +++ b/core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java @@ -49,6 +49,7 @@ import static io.trino.spi.security.AccessDeniedException.denyAddColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentTable; +import static io.trino.spi.security.AccessDeniedException.denyCommentView; import static io.trino.spi.security.AccessDeniedException.denyCreateMaterializedView; import static io.trino.spi.security.AccessDeniedException.denyCreateSchema; import static io.trino.spi.security.AccessDeniedException.denyCreateTable; @@ -86,6 +87,7 @@ import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.ADD_COLUMN; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.COMMENT_COLUMN; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.COMMENT_TABLE; +import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.COMMENT_VIEW; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.CREATE_MATERIALIZED_VIEW; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.CREATE_SCHEMA; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.CREATE_TABLE; @@ -387,6 +389,17 @@ public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName } } + @Override + public void checkCanSetViewComment(SecurityContext context, QualifiedObjectName viewName) + { + if (shouldDenyPrivilege(context.getIdentity().getUser(), viewName.getObjectName(), COMMENT_VIEW)) { + denyCommentView(viewName.toString()); + } + if (denyPrivileges.isEmpty()) { + super.checkCanSetViewComment(context, viewName); + } + } + @Override public void checkCanSetColumnComment(SecurityContext context, QualifiedObjectName tableName) { @@ -698,7 +711,7 @@ public enum TestingPrivilegeType EXECUTE_QUERY, VIEW_QUERY, KILL_QUERY, EXECUTE_FUNCTION, CREATE_SCHEMA, DROP_SCHEMA, RENAME_SCHEMA, - SHOW_CREATE_TABLE, CREATE_TABLE, DROP_TABLE, RENAME_TABLE, COMMENT_TABLE, COMMENT_COLUMN, INSERT_TABLE, DELETE_TABLE, UPDATE_TABLE, TRUNCATE_TABLE, SET_TABLE_PROPERTIES, SHOW_COLUMNS, + SHOW_CREATE_TABLE, CREATE_TABLE, DROP_TABLE, RENAME_TABLE, COMMENT_TABLE, COMMENT_VIEW, COMMENT_COLUMN, INSERT_TABLE, DELETE_TABLE, UPDATE_TABLE, TRUNCATE_TABLE, SET_TABLE_PROPERTIES, SHOW_COLUMNS, ADD_COLUMN, DROP_COLUMN, RENAME_COLUMN, SELECT_COLUMN, CREATE_VIEW, RENAME_VIEW, DROP_VIEW, CREATE_VIEW_WITH_SELECT_COLUMNS, CREATE_MATERIALIZED_VIEW, REFRESH_MATERIALIZED_VIEW, DROP_MATERIALIZED_VIEW, RENAME_MATERIALIZED_VIEW, SET_MATERIALIZED_VIEW_PROPERTIES, diff --git a/core/trino-main/src/test/java/io/trino/connector/MockConnector.java b/core/trino-main/src/test/java/io/trino/connector/MockConnector.java index 02c41e1bc170..299581e9338b 100644 --- a/core/trino-main/src/test/java/io/trino/connector/MockConnector.java +++ b/core/trino-main/src/test/java/io/trino/connector/MockConnector.java @@ -489,6 +489,9 @@ public void setTableProperties(ConnectorSession session, ConnectorTableHandle ta @Override public void setTableComment(ConnectorSession session, ConnectorTableHandle tableHandle, Optional comment) {} + @Override + public void setViewComment(ConnectorSession session, SchemaTableName viewName, Optional comment) {} + @Override public void setColumnComment(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnHandle column, Optional comment) {} diff --git a/core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java b/core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java index 416d17cfcfbb..7cd932829dd9 100644 --- a/core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java +++ b/core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java @@ -391,6 +391,21 @@ public void renameView(Session session, QualifiedObjectName source, QualifiedObj views.remove(oldViewName); } + @Override + public void setViewComment(Session session, QualifiedObjectName viewName, Optional comment) + { + ViewDefinition view = views.get(viewName.asSchemaTableName()); + views.put( + viewName.asSchemaTableName(), + new ViewDefinition( + view.getOriginalSql(), + view.getCatalog(), + view.getSchema(), + view.getColumns(), + comment, + view.getRunAsIdentity())); + } + @Override public void renameMaterializedView(Session session, QualifiedObjectName source, QualifiedObjectName target) { diff --git a/core/trino-main/src/test/java/io/trino/execution/TestCommentTask.java b/core/trino-main/src/test/java/io/trino/execution/TestCommentTask.java new file mode 100644 index 000000000000..e78ca89d0432 --- /dev/null +++ b/core/trino-main/src/test/java/io/trino/execution/TestCommentTask.java @@ -0,0 +1,76 @@ +/* + * Licensed 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 io.trino.execution; + +import com.google.common.collect.ImmutableList; +import com.google.common.util.concurrent.ListenableFuture; +import io.trino.execution.warnings.WarningCollector; +import io.trino.metadata.QualifiedObjectName; +import io.trino.security.AllowAllAccessControl; +import io.trino.sql.tree.Comment; +import io.trino.sql.tree.QualifiedName; +import org.testng.annotations.Test; + +import java.util.Optional; + +import static io.airlift.concurrent.MoreFutures.getFutureValue; +import static io.trino.spi.StandardErrorCode.TABLE_NOT_FOUND; +import static io.trino.sql.tree.Comment.Type.VIEW; +import static io.trino.testing.assertions.TrinoExceptionAssert.assertTrinoExceptionThrownBy; +import static org.assertj.core.api.Assertions.assertThat; + +@Test(singleThreaded = true) +public class TestCommentTask + extends BaseDataDefinitionTaskTest +{ + // TODO: Add test for 'COMMENT ON TABLE' and 'COMMENT ON COLUMN' statements + + @Test + public void testCommentView() + { + QualifiedObjectName viewName = qualifiedObjectName("existing_view"); + metadata.createView(testSession, viewName, someView(), false); + assertThat(metadata.isView(testSession, viewName)).isTrue(); + + getFutureValue(setComment(VIEW, asQualifiedName(viewName), Optional.of("new comment"))); + assertThat(metadata.getView(testSession, viewName).get().getComment()).isEqualTo(Optional.of("new comment")); + } + + @Test + public void testCommentViewOnTable() + { + QualifiedObjectName tableName = qualifiedObjectName("existing_table"); + metadata.createTable(testSession, CATALOG_NAME, someTable(tableName), false); + + assertTrinoExceptionThrownBy(() -> getFutureValue(setComment(VIEW, asQualifiedName(tableName), Optional.of("new comment")))) + .hasErrorCode(TABLE_NOT_FOUND) + .hasMessage("View '%1$s' does not exist, but a table with that name exists. Did you mean COMMENT ON TABLE %1$s IS ...?", tableName); + } + + @Test + public void testCommentViewOnMaterializedView() + { + QualifiedObjectName materializedViewName = qualifiedObjectName("existing_materialized_view"); + metadata.createMaterializedView(testSession, QualifiedObjectName.valueOf(materializedViewName.toString()), someMaterializedView(), false, false); + + assertTrinoExceptionThrownBy(() -> getFutureValue(setComment(VIEW, asQualifiedName(materializedViewName), Optional.of("new comment")))) + .hasErrorCode(TABLE_NOT_FOUND) + .hasMessage("View '%s' does not exist, but a materialized view with that name exists.", materializedViewName); + } + + private ListenableFuture setComment(Comment.Type type, QualifiedName viewName, Optional comment) + { + return new CommentTask(metadata, new AllowAllAccessControl()).execute(new Comment(type, viewName, comment), queryStateMachine, ImmutableList.of(), WarningCollector.NOOP); + } +} diff --git a/core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java b/core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java index f391443ce01e..b856ca2febdd 100644 --- a/core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java +++ b/core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java @@ -270,6 +270,12 @@ public void setTableComment(Session session, TableHandle tableHandle, Optional comment) + { + throw new UnsupportedOperationException(); + } + @Override public void setColumnComment(Session session, TableHandle tableHandle, ColumnHandle column, Optional comment) { diff --git a/core/trino-main/src/test/java/io/trino/metadata/CountingAccessMetadata.java b/core/trino-main/src/test/java/io/trino/metadata/CountingAccessMetadata.java index 28e6a1caad75..ce1676b77d94 100644 --- a/core/trino-main/src/test/java/io/trino/metadata/CountingAccessMetadata.java +++ b/core/trino-main/src/test/java/io/trino/metadata/CountingAccessMetadata.java @@ -277,6 +277,12 @@ public void setTableComment(Session session, TableHandle tableHandle, Optional comment) + { + delegate.setViewComment(session, viewName, comment); + } + @Override public void setColumnComment(Session session, TableHandle tableHandle, ColumnHandle column, Optional comment) { diff --git a/core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4 b/core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4 index 4dae229298f2..6f23a2ab9b48 100644 --- a/core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4 +++ b/core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4 @@ -61,6 +61,7 @@ statement | DELETE FROM qualifiedName (WHERE booleanExpression)? #delete | TRUNCATE TABLE qualifiedName #truncateTable | COMMENT ON TABLE qualifiedName IS (string | NULL) #commentTable + | COMMENT ON VIEW qualifiedName IS (string | NULL) #commentView | COMMENT ON COLUMN qualifiedName IS (string | NULL) #commentColumn | ALTER TABLE (IF EXISTS)? from=qualifiedName RENAME TO to=qualifiedName #renameTable diff --git a/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java b/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java index 188983c51aed..1427fec5ed98 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java +++ b/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java @@ -1463,6 +1463,12 @@ protected Void visitComment(Comment node, Integer context) .append(" IS ") .append(comment); break; + case VIEW: + builder.append("COMMENT ON VIEW ") + .append(node.getName()) + .append(" IS ") + .append(comment); + break; case COLUMN: builder.append("COMMENT ON COLUMN ") .append(node.getName()) diff --git a/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java b/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java index e4d6414f6055..de314ca7a176 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java +++ b/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java @@ -634,6 +634,18 @@ public Node visitCommentTable(SqlBaseParser.CommentTableContext context) return new Comment(getLocation(context), Comment.Type.TABLE, getQualifiedName(context.qualifiedName()), comment); } + @Override + public Node visitCommentView(SqlBaseParser.CommentViewContext context) + { + Optional comment = Optional.empty(); + + if (context.string() != null) { + comment = Optional.of(((StringLiteral) visit(context.string())).getValue()); + } + + return new Comment(getLocation(context), Comment.Type.VIEW, getQualifiedName(context.qualifiedName()), comment); + } + @Override public Node visitCommentColumn(SqlBaseParser.CommentColumnContext context) { diff --git a/core/trino-parser/src/main/java/io/trino/sql/tree/Comment.java b/core/trino-parser/src/main/java/io/trino/sql/tree/Comment.java index 9b4f8b26b807..9272b7993c48 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/tree/Comment.java +++ b/core/trino-parser/src/main/java/io/trino/sql/tree/Comment.java @@ -27,7 +27,7 @@ public final class Comment { public enum Type { - TABLE, COLUMN + TABLE, VIEW, COLUMN } private final Type type; diff --git a/core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java b/core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java index 803021e0a27e..d1c9ad99bf81 100644 --- a/core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java +++ b/core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java @@ -1916,6 +1916,14 @@ public void testCommentTable() assertStatement("COMMENT ON TABLE a IS NULL", new Comment(Comment.Type.TABLE, QualifiedName.of("a"), Optional.empty())); } + @Test + public void testCommentView() + { + assertStatement("COMMENT ON VIEW a IS 'test'", new Comment(Comment.Type.VIEW, QualifiedName.of("a"), Optional.of("test"))); + assertStatement("COMMENT ON VIEW a IS ''", new Comment(Comment.Type.VIEW, QualifiedName.of("a"), Optional.of(""))); + assertStatement("COMMENT ON VIEW a IS NULL", new Comment(Comment.Type.VIEW, QualifiedName.of("a"), Optional.empty())); + } + @Test public void testCommentColumn() { diff --git a/core/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java b/core/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java index f7c8558bae9f..f74b55e8f87b 100644 --- a/core/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java +++ b/core/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java @@ -252,6 +252,7 @@ public void testStatementBuilder() printStatement("create table test (a boolean, b bigint) comment 'test' with (a = 'apple')"); printStatement("create table test (a boolean with (a = 'apple', b = 'banana'), b bigint comment 'bla' with (c = 'cherry')) comment 'test' with (a = 'apple')"); printStatement("comment on table test is 'test'"); + printStatement("comment on view test is 'test'"); printStatement("comment on column test.a is 'test'"); printStatement("drop table test"); diff --git a/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorAccessControl.java b/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorAccessControl.java index e1d8b20e359d..a2e1c859e3ef 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorAccessControl.java +++ b/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorAccessControl.java @@ -27,6 +27,7 @@ import static io.trino.spi.security.AccessDeniedException.denyAddColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentTable; +import static io.trino.spi.security.AccessDeniedException.denyCommentView; import static io.trino.spi.security.AccessDeniedException.denyCreateMaterializedView; import static io.trino.spi.security.AccessDeniedException.denyCreateRole; import static io.trino.spi.security.AccessDeniedException.denyCreateSchema; @@ -214,6 +215,16 @@ default void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTab denyCommentTable(tableName.toString()); } + /** + * Check if identity is allowed to comment the specified view. + * + * @throws io.trino.spi.security.AccessDeniedException if not allowed + */ + default void checkCanSetViewComment(ConnectorSecurityContext context, SchemaTableName viewName) + { + denyCommentView(viewName.toString()); + } + /** * Check if identity is allowed to comment the column in the specified table. * diff --git a/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java b/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java index 59095a14a0c9..df92507eb8a5 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java +++ b/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java @@ -390,6 +390,14 @@ default void setTableComment(ConnectorSession session, ConnectorTableHandle tabl throw new TrinoException(NOT_SUPPORTED, "This connector does not support setting table comments"); } + /** + * Comments to the specified view + */ + default void setViewComment(ConnectorSession session, SchemaTableName viewName, Optional comment) + { + throw new TrinoException(NOT_SUPPORTED, "This connector does not support setting view comments"); + } + /** * Comments to the specified column */ diff --git a/core/trino-spi/src/main/java/io/trino/spi/security/AccessDeniedException.java b/core/trino-spi/src/main/java/io/trino/spi/security/AccessDeniedException.java index 89a92271246e..0e47ee866fbe 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/security/AccessDeniedException.java +++ b/core/trino-spi/src/main/java/io/trino/spi/security/AccessDeniedException.java @@ -238,6 +238,16 @@ public static void denyCommentTable(String tableName, String extraInfo) throw new AccessDeniedException(format("Cannot comment table to %s%s", tableName, formatExtraInfo(extraInfo))); } + public static void denyCommentView(String viewName) + { + denyCommentView(viewName, null); + } + + public static void denyCommentView(String viewName, String extraInfo) + { + throw new AccessDeniedException(format("Cannot comment view to %s%s", viewName, formatExtraInfo(extraInfo))); + } + public static void denyCommentColumn(String tableName) { denyCommentColumn(tableName, null); diff --git a/core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java b/core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java index 650f44c3e316..33d703fe8ccd 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java +++ b/core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java @@ -34,6 +34,7 @@ import static io.trino.spi.security.AccessDeniedException.denyCatalogAccess; import static io.trino.spi.security.AccessDeniedException.denyCommentColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentTable; +import static io.trino.spi.security.AccessDeniedException.denyCommentView; import static io.trino.spi.security.AccessDeniedException.denyCreateMaterializedView; import static io.trino.spi.security.AccessDeniedException.denyCreateRole; import static io.trino.spi.security.AccessDeniedException.denyCreateSchema; @@ -384,6 +385,16 @@ default void checkCanSetTableComment(SystemSecurityContext context, CatalogSchem denyCommentTable(table.toString()); } + /** + * Check if identity is allowed to comment the specified view in a catalog. + * + * @throws AccessDeniedException if not allowed + */ + default void checkCanSetViewComment(SystemSecurityContext context, CatalogSchemaTableName view) + { + denyCommentView(view.toString()); + } + /** * Check if identity is allowed to set comment to column in the specified table in a catalog. * diff --git a/docs/src/main/sphinx/sql/comment.rst b/docs/src/main/sphinx/sql/comment.rst index 42a4b7ba56b9..2ec58bcc9c79 100644 --- a/docs/src/main/sphinx/sql/comment.rst +++ b/docs/src/main/sphinx/sql/comment.rst @@ -7,7 +7,7 @@ Synopsis .. code-block:: text - COMMENT ON ( TABLE | COLUMN ) name IS 'comments' + COMMENT ON ( TABLE | VIEW | COLUMN ) name IS 'comments' Description ----------- @@ -21,6 +21,10 @@ Change the comment for the ``users`` table to be ``master table``:: COMMENT ON TABLE users IS 'master table'; +Change the comment for the ``users`` view to be ``master view``:: + + COMMENT ON VIEW users IS 'master view'; + Change the comment for the ``users.name`` column to be ``full name``:: COMMENT ON COLUMN users.name IS 'full name'; diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorAccessControl.java index 618d013aa767..1655d2cbcbbf 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorAccessControl.java @@ -150,6 +150,14 @@ public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTabl } } + @Override + public void checkCanSetViewComment(ConnectorSecurityContext context, SchemaTableName viewName) + { + try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) { + delegate.checkCanSetViewComment(context, viewName); + } + } + @Override public void checkCanSetColumnComment(ConnectorSecurityContext context, SchemaTableName tableName) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java index 64cf7fea432b..2b34ae96a163 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java @@ -439,6 +439,14 @@ public void setTableComment(ConnectorSession session, ConnectorTableHandle table } } + @Override + public void setViewComment(ConnectorSession session, SchemaTableName viewName, Optional comment) + { + try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) { + delegate.setViewComment(session, viewName, comment); + } + } + @Override public void setColumnComment(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnHandle column, Optional comment) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllAccessControl.java index b9e05c45b1c8..2d9c275fdfca 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllAccessControl.java @@ -93,6 +93,11 @@ public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTabl { } + @Override + public void checkCanSetViewComment(ConnectorSecurityContext context, SchemaTableName viewName) + { + } + @Override public void checkCanSetTableProperties(ConnectorSecurityContext context, SchemaTableName tableName, Map> properties) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllSystemAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllSystemAccessControl.java index 274491deb200..2d39f782ef1b 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllSystemAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllSystemAccessControl.java @@ -204,6 +204,11 @@ public void checkCanSetTableComment(SystemSecurityContext context, CatalogSchema { } + @Override + public void checkCanSetViewComment(SystemSecurityContext context, CatalogSchemaTableName view) + { + } + @Override public void checkCanSetColumnComment(SystemSecurityContext context, CatalogSchemaTableName table) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedAccessControl.java index 0fbc4b525fb4..9141f220d818 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedAccessControl.java @@ -49,6 +49,7 @@ import static io.trino.spi.security.AccessDeniedException.denyAddColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentTable; +import static io.trino.spi.security.AccessDeniedException.denyCommentView; import static io.trino.spi.security.AccessDeniedException.denyCreateMaterializedView; import static io.trino.spi.security.AccessDeniedException.denyCreateRole; import static io.trino.spi.security.AccessDeniedException.denyCreateSchema; @@ -283,6 +284,14 @@ public void checkCanSetTableComment(ConnectorSecurityContext identity, SchemaTab } } + @Override + public void checkCanSetViewComment(ConnectorSecurityContext context, SchemaTableName viewName) + { + if (!checkTablePermission(context, viewName, OWNERSHIP)) { + denyCommentView(viewName.toString()); + } + } + @Override public void checkCanSetColumnComment(ConnectorSecurityContext context, SchemaTableName tableName) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java index 81726582b9e6..c316e9b1d02d 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java @@ -66,6 +66,7 @@ import static io.trino.spi.security.AccessDeniedException.denyCatalogAccess; import static io.trino.spi.security.AccessDeniedException.denyCommentColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentTable; +import static io.trino.spi.security.AccessDeniedException.denyCommentView; import static io.trino.spi.security.AccessDeniedException.denyCreateMaterializedView; import static io.trino.spi.security.AccessDeniedException.denyCreateRole; import static io.trino.spi.security.AccessDeniedException.denyCreateSchema; @@ -556,6 +557,14 @@ public void checkCanSetTableComment(SystemSecurityContext context, CatalogSchema } } + @Override + public void checkCanSetViewComment(SystemSecurityContext context, CatalogSchemaTableName view) + { + if (!checkTablePermission(context, view, OWNERSHIP)) { + denyCommentView(view.toString()); + } + } + @Override public void checkCanSetColumnComment(SystemSecurityContext context, CatalogSchemaTableName table) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingConnectorAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingConnectorAccessControl.java index 74702e9ba44e..07f87c9ec4e6 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingConnectorAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingConnectorAccessControl.java @@ -127,6 +127,12 @@ public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTabl delegate().checkCanSetTableComment(context, tableName); } + @Override + public void checkCanSetViewComment(ConnectorSecurityContext context, SchemaTableName viewName) + { + delegate().checkCanSetViewComment(context, viewName); + } + @Override public void checkCanSetColumnComment(ConnectorSecurityContext context, SchemaTableName tableName) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingSystemAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingSystemAccessControl.java index 533fa1bf4289..6a7c61f3147f 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingSystemAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingSystemAccessControl.java @@ -217,6 +217,12 @@ public void checkCanSetTableComment(SystemSecurityContext context, CatalogSchema delegate().checkCanSetTableComment(context, table); } + @Override + public void checkCanSetViewComment(SystemSecurityContext context, CatalogSchemaTableName view) + { + delegate().checkCanSetViewComment(context, view); + } + @Override public void checkCanSetColumnComment(SystemSecurityContext context, CatalogSchemaTableName table) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ReadOnlyAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ReadOnlyAccessControl.java index c57d43c2986e..60a1de000a65 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ReadOnlyAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ReadOnlyAccessControl.java @@ -26,6 +26,7 @@ import static io.trino.spi.security.AccessDeniedException.denyAddColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentTable; +import static io.trino.spi.security.AccessDeniedException.denyCommentView; import static io.trino.spi.security.AccessDeniedException.denyCreateMaterializedView; import static io.trino.spi.security.AccessDeniedException.denyCreateTable; import static io.trino.spi.security.AccessDeniedException.denyCreateView; @@ -112,6 +113,12 @@ public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTabl denyCommentTable(tableName.toString()); } + @Override + public void checkCanSetViewComment(ConnectorSecurityContext context, SchemaTableName viewName) + { + denyCommentView(viewName.toString()); + } + @Override public void checkCanShowTables(ConnectorSecurityContext context, String schemaName) { diff --git a/lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedAccessControl.java b/lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedAccessControl.java index 16558a3bec16..b57ec13f9b7b 100644 --- a/lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedAccessControl.java +++ b/lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedAccessControl.java @@ -317,6 +317,7 @@ public void testTableRules() accessControl.checkCanRenameMaterializedView(ALICE, new SchemaTableName("aliceschema", "alicevaterializediew"), new SchemaTableName("aliceschema", "newaliceaterializedview")); accessControl.checkCanSetMaterializedViewProperties(ADMIN, new SchemaTableName("bobschema", "bobmaterializedview"), ImmutableMap.of()); accessControl.checkCanSetMaterializedViewProperties(ALICE, new SchemaTableName("aliceschema", "alicevaterializediew"), ImmutableMap.of()); + accessControl.checkCanSetViewComment(ALICE, new SchemaTableName("aliceschema", "aliceview")); accessControl.checkCanSetTableProperties(ADMIN, bobTable, ImmutableMap.of()); accessControl.checkCanSetTableProperties(ALICE, aliceTable, ImmutableMap.of()); @@ -325,6 +326,7 @@ public void testTableRules() assertDenied(() -> accessControl.checkCanDropTable(BOB, bobTable)); assertDenied(() -> accessControl.checkCanRenameTable(BOB, bobTable, new SchemaTableName("bobschema", "newbobtable"))); assertDenied(() -> accessControl.checkCanRenameTable(ALICE, aliceTable, new SchemaTableName("bobschema", "newalicetable"))); + assertDenied(() -> accessControl.checkCanSetViewComment(ALICE, new SchemaTableName("bobschema", "newalicetable"))); assertDenied(() -> accessControl.checkCanSetTableProperties(BOB, bobTable, ImmutableMap.of())); assertDenied(() -> accessControl.checkCanInsertIntoTable(BOB, testTable)); assertDenied(() -> accessControl.checkCanSelectFromColumns(ADMIN, new SchemaTableName("secret", "secret"), ImmutableSet.of())); diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/LegacyAccessControl.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/LegacyAccessControl.java index 8d1a0b5886d1..c0beb4e9150f 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/LegacyAccessControl.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/LegacyAccessControl.java @@ -164,6 +164,11 @@ public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTabl } } + @Override + public void checkCanSetViewComment(ConnectorSecurityContext context, SchemaTableName viewName) + { + } + @Override public void checkCanSetColumnComment(ConnectorSecurityContext context, SchemaTableName tableName) { diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/SqlStandardAccessControl.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/SqlStandardAccessControl.java index 8a712b734e7a..41c7fdccdf48 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/SqlStandardAccessControl.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/SqlStandardAccessControl.java @@ -59,6 +59,7 @@ import static io.trino.spi.security.AccessDeniedException.denyAddColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentColumn; import static io.trino.spi.security.AccessDeniedException.denyCommentTable; +import static io.trino.spi.security.AccessDeniedException.denyCommentView; import static io.trino.spi.security.AccessDeniedException.denyCreateMaterializedView; import static io.trino.spi.security.AccessDeniedException.denyCreateRole; import static io.trino.spi.security.AccessDeniedException.denyCreateSchema; @@ -225,6 +226,14 @@ public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTabl } } + @Override + public void checkCanSetViewComment(ConnectorSecurityContext context, SchemaTableName viewName) + { + if (!isTableOwner(context, viewName)) { + denyCommentView(viewName.toString()); + } + } + @Override public void checkCanSetColumnComment(ConnectorSecurityContext context, SchemaTableName tableName) { diff --git a/plugin/trino-memory/src/main/java/io/trino/plugin/memory/MemoryMetadata.java b/plugin/trino-memory/src/main/java/io/trino/plugin/memory/MemoryMetadata.java index 0e58a1b72db0..88aa70b74d9e 100644 --- a/plugin/trino-memory/src/main/java/io/trino/plugin/memory/MemoryMetadata.java +++ b/plugin/trino-memory/src/main/java/io/trino/plugin/memory/MemoryMetadata.java @@ -326,6 +326,20 @@ else if (views.putIfAbsent(viewName, definition) != null) { tableIds.put(viewName, nextTableId.getAndIncrement()); } + @Override + public synchronized void setViewComment(ConnectorSession session, SchemaTableName viewName, Optional comment) + { + ConnectorViewDefinition view = getView(session, viewName).orElseThrow(() -> new ViewNotFoundException(viewName)); + views.put(viewName, new ConnectorViewDefinition( + view.getOriginalSql(), + view.getCatalog(), + view.getSchema(), + view.getColumns(), + comment, + view.getOwner(), + view.isRunAsInvoker())); + } + @Override public synchronized void renameView(ConnectorSession session, SchemaTableName viewName, SchemaTableName newViewName) { diff --git a/plugin/trino-memory/src/test/java/io/trino/plugin/memory/TestMemoryConnectorTest.java b/plugin/trino-memory/src/test/java/io/trino/plugin/memory/TestMemoryConnectorTest.java index 07329c88ef9f..d94676bc3288 100644 --- a/plugin/trino-memory/src/test/java/io/trino/plugin/memory/TestMemoryConnectorTest.java +++ b/plugin/trino-memory/src/test/java/io/trino/plugin/memory/TestMemoryConnectorTest.java @@ -105,6 +105,7 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) return false; case SUPPORTS_CREATE_VIEW: + case SUPPORTS_COMMENT_ON_VIEW: return true; case SUPPORTS_NOT_NULL_CONSTRAINT: diff --git a/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java b/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java index b40bb59fc029..4b4065f91c1f 100644 --- a/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java +++ b/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java @@ -34,6 +34,7 @@ import io.trino.sql.planner.Plan; import io.trino.sql.planner.plan.LimitNode; import io.trino.testing.sql.TestTable; +import io.trino.testing.sql.TestView; import org.intellij.lang.annotations.Language; import org.testng.SkipException; import org.testng.annotations.DataProvider; @@ -80,6 +81,7 @@ import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ARRAY; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_COMMENT_ON_COLUMN; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_COMMENT_ON_TABLE; +import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_COMMENT_ON_VIEW; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_CREATE_MATERIALIZED_VIEW; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_CREATE_SCHEMA; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_CREATE_TABLE; @@ -2420,6 +2422,53 @@ protected String getTableComment(String catalogName, String schemaName, String t return (String) computeActual(sql).getOnlyValue(); } + @Test + public void testCommentView() + { + if (!hasBehavior(SUPPORTS_COMMENT_ON_VIEW)) { + if (hasBehavior(SUPPORTS_CREATE_VIEW)) { + try (TestView view = new TestView(getQueryRunner()::execute, "test_comment_view", "SELECT * FROM region")) { + assertQueryFails("COMMENT ON VIEW " + view.getName() + " IS 'new comment'", "This connector does not support setting view comments"); + } + return; + } + throw new SkipException("Skipping as connector does not support CREATE VIEW"); + } + + String catalogName = getSession().getCatalog().orElseThrow(); + String schemaName = getSession().getSchema().orElseThrow(); + try (TestView view = new TestView(getQueryRunner()::execute, "test_comment_view", "SELECT * FROM region")) { + // comment set + assertUpdate("COMMENT ON VIEW " + view.getName() + " IS 'new comment'"); + assertThat((String) computeActual("SHOW CREATE VIEW " + view.getName()).getOnlyValue()).contains("COMMENT 'new comment'"); + assertThat(getTableComment(catalogName, schemaName, view.getName())).isEqualTo("new comment"); + + // comment updated + assertUpdate("COMMENT ON VIEW " + view.getName() + " IS 'updated comment'"); + assertThat(getTableComment(catalogName, schemaName, view.getName())).isEqualTo("updated comment"); + + // comment set to empty + assertUpdate("COMMENT ON VIEW " + view.getName() + " IS ''"); + assertThat(getTableComment(catalogName, schemaName, view.getName())).isEqualTo(""); + + // comment deleted + assertUpdate("COMMENT ON VIEW " + view.getName() + " IS 'a comment'"); + assertThat(getTableComment(catalogName, schemaName, view.getName())).isEqualTo("a comment"); + assertUpdate("COMMENT ON VIEW " + view.getName() + " IS NULL"); + assertThat(getTableComment(catalogName, schemaName, view.getName())).isEqualTo(null); + } + + String viewName = "test_comment_view" + randomTableSuffix(); + try { + // comment set when creating a table + assertUpdate("CREATE VIEW " + viewName + " COMMENT 'new view comment' AS SELECT * FROM region"); + assertThat(getTableComment(catalogName, schemaName, viewName)).isEqualTo("new view comment"); + } + finally { + assertUpdate("DROP VIEW IF EXISTS " + viewName); + } + } + @Test public void testCommentColumn() { diff --git a/testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java b/testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java index 455054b32a3a..5e21418142e5 100644 --- a/testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java +++ b/testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java @@ -65,6 +65,7 @@ public enum TestingConnectorBehavior SUPPORTS_RENAME_COLUMN, SUPPORTS_COMMENT_ON_TABLE, + SUPPORTS_COMMENT_ON_VIEW(false), SUPPORTS_COMMENT_ON_COLUMN, SUPPORTS_CREATE_VIEW(false), diff --git a/testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java b/testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java index d4c93ac4407a..d5ea4f41aadc 100644 --- a/testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java +++ b/testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java @@ -42,6 +42,7 @@ import static io.trino.spi.security.SelectedRole.Type.ROLE; import static io.trino.spi.type.BigintType.BIGINT; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.ADD_COLUMN; +import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.COMMENT_VIEW; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.CREATE_MATERIALIZED_VIEW; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.CREATE_TABLE; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.CREATE_VIEW; @@ -343,6 +344,16 @@ public void testAnalyzeAccessControl() assertAccessDenied("ANALYZE nation", "Cannot select from columns \\[.*nationkey.*] in table or view .*.nation", privilege("nation.nationkey", SELECT_COLUMN)); } + @Test + public void testCommentView() + { + String viewName = "comment_view" + randomTableSuffix(); + assertUpdate("CREATE VIEW " + viewName + " COMMENT 'old comment' AS SELECT * FROM orders"); + assertAccessDenied("COMMENT ON VIEW " + viewName + " IS 'new comment'", "Cannot comment view to .*", privilege(viewName, COMMENT_VIEW)); + assertThatThrownBy(() -> getQueryRunner().execute(getSession(), "COMMENT ON VIEW " + viewName + " IS 'new comment'")) + .hasMessageContaining("This connector does not support setting view comments"); + } + @Test public void testSetTableProperties() { diff --git a/testing/trino-tests/src/test/java/io/trino/tests/TestMockConnector.java b/testing/trino-tests/src/test/java/io/trino/tests/TestMockConnector.java index 8eee4e5c999d..6836582b43d0 100644 --- a/testing/trino-tests/src/test/java/io/trino/tests/TestMockConnector.java +++ b/testing/trino-tests/src/test/java/io/trino/tests/TestMockConnector.java @@ -27,6 +27,8 @@ import io.trino.spi.connector.ColumnMetadata; import io.trino.spi.connector.ConnectorMaterializedViewDefinition; import io.trino.spi.connector.ConnectorMaterializedViewDefinition.Column; +import io.trino.spi.connector.ConnectorViewDefinition; +import io.trino.spi.connector.ConnectorViewDefinition.ViewColumn; import io.trino.spi.connector.SchemaTableName; import io.trino.spi.metrics.Metrics; import io.trino.spi.session.PropertyMetadata; @@ -73,6 +75,16 @@ protected QueryRunner createQueryRunner() } return new MockConnectorTableHandle(tableName); }) + .withGetViews((session, schemaTablePrefix) -> ImmutableMap.of( + new SchemaTableName("default", "test_view"), + new ConnectorViewDefinition( + "SELECT nationkey FROM mock.default.test_table", + Optional.of("mock"), + Optional.of("default"), + ImmutableList.of(new ViewColumn("nationkey", BIGINT.getTypeId())), + Optional.empty(), + Optional.of("alice"), + false))) .withGetMaterializedViewProperties(() -> ImmutableList.of( durationProperty( "refresh_interval", @@ -127,6 +139,12 @@ public void testRenameSchema() assertUpdate("ALTER SCHEMA mock.default RENAME to renamed"); } + @Test + public void testViewComment() + { + assertUpdate("COMMENT ON VIEW mock.default.test_view IS 'new comment'"); + } + @Test public void testCreateMaterializedView() {