Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Renaming view #1060

Open
wants to merge 2 commits into
base: master
from

Conversation

@Praveen2112
Copy link
Member

commented Jun 29, 2019

Fixes #1037

denyRenameView(viewName, newViewName, null);
}

public static void denyRenameView(String schemaName, String newSchemaName, String extraInfo)

This comment has been minimized.

Copy link
@ebyhr

ebyhr Jun 30, 2019

Contributor
Suggested change
public static void denyRenameView(String schemaName, String newSchemaName, String extraInfo)
public static void denyRenameView(String viewName, String newViewName, String extraInfo)

public static void denyRenameView(String schemaName, String newSchemaName, String extraInfo)
{
throw new AccessDeniedException(format("Cannot rename schema from %s to %s%s", schemaName, newSchemaName, formatExtraInfo(extraInfo)));

This comment has been minimized.

Copy link
@ebyhr

ebyhr Jun 30, 2019

Contributor
Suggested change
throw new AccessDeniedException(format("Cannot rename schema from %s to %s%s", schemaName, newSchemaName, formatExtraInfo(extraInfo)));
throw new AccessDeniedException(format("Cannot rename view from %s to %s%s", viewName, newViewName, formatExtraInfo(extraInfo)));
@ebyhr

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2019

It seems AllowAllAccessControl.checkCanRenameView and TestingAccessControlManager.checkCanRenameView are missing.

@Praveen2112 Praveen2112 force-pushed the Praveen2112:rename_view branch from 7fb82e9 to 4ef74d7 Jul 1, 2019

@cla-bot cla-bot bot added the cla-signed label Jul 1, 2019

@Praveen2112

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

@ebyhr Thanks for your comments. Have updated it.

@@ -18,6 +18,7 @@
import io.prestosql.spi.connector.ConnectorTransactionHandle;
import io.prestosql.spi.connector.SchemaTableName;
import io.prestosql.spi.security.ConnectorIdentity;
import io.prestosql.spi.security.Identity;

This comment has been minimized.

Copy link
@ebyhr

ebyhr Jul 1, 2019

Contributor

Unused import.

This comment has been minimized.

Copy link
@Praveen2112

Praveen2112 Jul 1, 2019

Author Member

Removed..Thanks

@Praveen2112 Praveen2112 force-pushed the Praveen2112:rename_view branch from 4ef74d7 to bcaa58b Jul 1, 2019

@dain
Copy link
Member

left a comment

My comments are mostly typos. I think we will need a review from someone that understands the planner like @martint or @findepi.

@Override
public void checkCanRenameView(ConnectorTransactionHandle transaction, ConnectorIdentity identity, SchemaTableName viewName, SchemaTableName newViewName)
{
if (!isTableOwner(transaction, identity, viewName)) {

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

I would assume that you need to be both the table owner and the database owner, since this is the equivalent of drop and add, but I see that canRenameTable has this behavior. @electrum @findepi thoughts?

{
CatalogMetadata catalogMetadata = getCatalogMetadataForWrite(session, target.getCatalogName());
CatalogName catalogName = catalogMetadata.getCatalogName();
ConnectorMetadata metadata = catalogMetadata.getMetadata();

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

I'd add the same rename across catalog safety check that renameTable has


authenticationCheck(() -> checkCanAccessCatalog(identity, viewName.getCatalogName()));

authorizationCheck(() -> systemAccessControl.get().checkCanRenameTable(identity, viewName.asCatalogSchemaTableName(), newViewName.asCatalogSchemaTableName()));

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

checkCanRenameTable?

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

Do we have tests for this kind of thing somewhere?


CatalogAccessControlEntry entry = getConnectorAccessControl(transactionId, viewName.getCatalogName());
if (entry != null) {
authorizationCheck(() -> entry.getAccessControl().checkCanRenameTable(entry.getTransactionHandle(transactionId), identity.toConnectorIdentity(viewName.getCatalogName()), viewName.asSchemaTableName(), newViewName.asSchemaTableName()));

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

checkCanRenameTable?

@@ -39,6 +39,7 @@
SCHEMA_ALREADY_EXISTS,
TABLE_ALREADY_EXISTS,
COLUMN_ALREADY_EXISTS,
VIEW_ALREADY_EXISTS,

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

Can we update CreateViewTask to throw this?

@Override
public void checkCanRenameView(ConnectorTransactionHandle transaction, ConnectorIdentity identity, SchemaTableName viewName, SchemaTableName newViewName)
{
if (!checkTablePermission(identity, viewName, OWNERSHIP)) {

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

This should be owner of old and new name.

@Override
public void checkCanRenameView(ConnectorTransactionHandle transaction, ConnectorIdentity identity, SchemaTableName viewName, SchemaTableName newViewName)
{
denyRenameTable(viewName.toString(), newViewName.toString());

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

denyRenameTable?

*/
default void checkCanRenameView(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, SchemaTableName viewName, SchemaTableName newViewName)
{
denyRenameTable(viewName.toString(), newViewName.toString());

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

denyRenameTable?

throw new PrestoException(ALREADY_EXISTS, "Table already exists: " + viewName);
}

if (views.containsKey(newViewName)) {

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

One of these should be viewName and one should be newViewName... the exception message is wrong also

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.