Skip to content

Commit

Permalink
Remove 'IN catalog' syntax from role management commands
Browse files Browse the repository at this point in the history 
Extracted-From: prestodb/presto#10904
  • Loading branch information
@kokosing @sopel39
kokosing authored and sopel39 committed Jan 29, 2019
1 parent 729f646 commit eadfb43
Show file tree
Hide file tree
Showing 18 changed files with 97 additions and 225 deletions.
8 changes: 2 additions & 6 deletions presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveRoles.java
View file
Expand Up @@ -60,8 +60,7 @@ public void testCreateRole()
{
executeFromAdmin("CREATE ROLE role1");
assertEquals(listRoles(), ImmutableSet.of("role1"));
executeFromAdmin("CREATE ROLE role2 IN hive");
assertEquals(listRoles(), ImmutableSet.of("role1", "role2"));
assertEquals(listRoles(), ImmutableSet.of("role1"));
}

@Test
Expand Down Expand Up @@ -100,11 +99,8 @@ public void testDropRole()
throws Exception
{
executeFromAdmin("CREATE ROLE role1");
executeFromAdmin("CREATE ROLE role2");
assertEquals(listRoles(), ImmutableSet.of("role1", "role2"));
executeFromAdmin("DROP ROLE role2");
assertEquals(listRoles(), ImmutableSet.of("role1"));
executeFromAdmin("DROP ROLE role1 IN hive");
executeFromAdmin("DROP ROLE role1");
assertEquals(listRoles(), ImmutableSet.of());
}

Expand Down
2 changes: 1 addition & 1 deletion presto-main/src/main/java/io/prestosql/execution/CreateRoleTask.java
View file
Expand Up @@ -48,7 +48,7 @@ public String getName()
public ListenableFuture<?> execute(CreateRole statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, QueryStateMachine stateMachine, List<Expression> parameters)
{
Session session = stateMachine.getSession();
String catalog = createCatalogName(session, statement, statement.getCatalog().map(c -> c.getValue().toLowerCase(ENGLISH)));
String catalog = createCatalogName(session, statement);
String role = statement.getName().getValue().toLowerCase(ENGLISH);
Optional<PrestoPrincipal> grantor = statement.getGrantor().map(specification -> createPrincipal(session, specification));
accessControl.checkCanCreateRole(session.getRequiredTransactionId(), session.getIdentity(), role, grantor, catalog);
Expand Down
2 changes: 1 addition & 1 deletion presto-main/src/main/java/io/prestosql/execution/DropRoleTask.java
View file
Expand Up @@ -43,7 +43,7 @@ public String getName()
public ListenableFuture<?> execute(DropRole statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, QueryStateMachine stateMachine, List<Expression> parameters)
{
Session session = stateMachine.getSession();
String catalog = createCatalogName(session, statement, statement.getCatalog().map(c -> c.getValue().toLowerCase(ENGLISH)));
String catalog = createCatalogName(session, statement);
String role = statement.getName().getValue().toLowerCase(ENGLISH);
accessControl.checkCanDropRole(session.getRequiredTransactionId(), session.getIdentity(), role, catalog);
Set<String> existingRoles = metadata.listRoles(session, catalog);
Expand Down
3 changes: 1 addition & 2 deletions presto-main/src/main/java/io/prestosql/execution/GrantRolesTask.java
View file
Expand Up @@ -36,7 +36,6 @@
import static io.prestosql.metadata.MetadataUtil.createPrincipal;
import static io.prestosql.spi.security.PrincipalType.ROLE;
import static io.prestosql.sql.analyzer.SemanticErrorCode.MISSING_ROLE;
import static java.util.Locale.ENGLISH;

public class GrantRolesTask
implements DataDefinitionTask<GrantRoles>
Expand All @@ -58,7 +57,7 @@ public ListenableFuture<?> execute(GrantRoles statement, TransactionManager tran
.collect(toImmutableSet());
boolean withAdminOption = statement.isWithAdminOption();
Optional<PrestoPrincipal> grantor = statement.getGrantor().map(specification -> createPrincipal(session, specification));
String catalog = createCatalogName(session, statement, statement.getCatalog().map(c -> c.getValue().toLowerCase(ENGLISH)));
String catalog = createCatalogName(session, statement);

Set<String> availableRoles = metadata.listRoles(session, catalog);
Set<String> specifiedRoles = new LinkedHashSet<>();
Expand Down
3 changes: 1 addition & 2 deletions presto-main/src/main/java/io/prestosql/execution/RevokeRolesTask.java
View file
Expand Up @@ -36,7 +36,6 @@
import static io.prestosql.metadata.MetadataUtil.createPrincipal;
import static io.prestosql.spi.security.PrincipalType.ROLE;
import static io.prestosql.sql.analyzer.SemanticErrorCode.MISSING_ROLE;
import static java.util.Locale.ENGLISH;

public class RevokeRolesTask
implements DataDefinitionTask<RevokeRoles>
Expand All @@ -58,7 +57,7 @@ public ListenableFuture<?> execute(RevokeRoles statement, TransactionManager tra
.collect(toImmutableSet());
boolean adminOptionFor = statement.isAdminOptionFor();
Optional<PrestoPrincipal> grantor = statement.getGrantor().map(specification -> createPrincipal(session, specification));
String catalog = createCatalogName(session, statement, statement.getCatalog().map(c -> c.getValue().toLowerCase(ENGLISH)));
String catalog = createCatalogName(session, statement);

Set<String> availableRoles = metadata.listRoles(session, catalog);
Set<String> specifiedRoles = new LinkedHashSet<>();
Expand Down
2 changes: 1 addition & 1 deletion presto-main/src/main/java/io/prestosql/execution/SetRoleTask.java
View file
Expand Up @@ -45,7 +45,7 @@ public String getName()
public ListenableFuture<?> execute(SetRole statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, QueryStateMachine stateMachine, List<Expression> parameters)
{
Session session = stateMachine.getSession();
String catalog = createCatalogName(session, statement, statement.getCatalog().map(c -> c.getValue().toLowerCase(ENGLISH)));
String catalog = createCatalogName(session, statement);
if (statement.getType() == SetRole.Type.ROLE) {
accessControl.checkCanSetRole(
session.getRequiredTransactionId(),
Expand Down
8 changes: 2 additions & 6 deletions presto-main/src/main/java/io/prestosql/metadata/MetadataUtil.java
View file
Expand Up @@ -98,16 +98,12 @@ public static ColumnMetadata findColumnMetadata(ConnectorTableMetadata tableMeta
return null;
}

public static String createCatalogName(Session session, Node node, Optional<String> catalog)
public static String createCatalogName(Session session, Node node)
{
if (catalog.isPresent()) {
return catalog.get();
}

Optional<String> sessionCatalog = session.getCatalog();

if (!sessionCatalog.isPresent()) {
throw new SemanticException(CATALOG_NOT_SPECIFIED, node, "Catalog must be specified when session catalog is not set");
throw new SemanticException(CATALOG_NOT_SPECIFIED, node, "Session catalog must be set");
}

return sessionCatalog.get();
Expand Down
12 changes: 7 additions & 5 deletions presto-main/src/test/java/io/prestosql/execution/TestSetRoleTask.java
View file
Expand Up @@ -46,8 +46,8 @@
import java.util.concurrent.ExecutorService;

import static io.airlift.concurrent.Threads.daemonThreadsNamed;
import static io.prestosql.SessionTestUtils.TEST_SESSION;
import static io.prestosql.testing.TestingSession.createBogusTestingCatalog;
import static io.prestosql.testing.TestingSession.testSessionBuilder;
import static io.prestosql.transaction.InMemoryTransactionManager.createTestTransactionManager;
import static java.util.concurrent.Executors.newCachedThreadPool;
import static org.testng.Assert.assertEquals;
Expand Down Expand Up @@ -101,17 +101,19 @@ public void tearDown()
public void testSetRole()
throws Exception
{
assertSetRole("SET ROLE ALL IN foo", ImmutableMap.of(CATALOG_NAME, new SelectedRole(SelectedRole.Type.ALL, Optional.empty())));
assertSetRole("SET ROLE NONE IN foo", ImmutableMap.of(CATALOG_NAME, new SelectedRole(SelectedRole.Type.NONE, Optional.empty())));
assertSetRole("SET ROLE bar IN foo", ImmutableMap.of(CATALOG_NAME, new SelectedRole(SelectedRole.Type.ROLE, Optional.of("bar"))));
assertSetRole("SET ROLE ALL", ImmutableMap.of(CATALOG_NAME, new SelectedRole(SelectedRole.Type.ALL, Optional.empty())));
assertSetRole("SET ROLE NONE", ImmutableMap.of(CATALOG_NAME, new SelectedRole(SelectedRole.Type.NONE, Optional.empty())));
assertSetRole("SET ROLE bar", ImmutableMap.of(CATALOG_NAME, new SelectedRole(SelectedRole.Type.ROLE, Optional.of("bar"))));
}

private void assertSetRole(String statement, Map<String, SelectedRole> expected)
{
SetRole setRole = (SetRole) parser.createStatement(statement);
QueryStateMachine stateMachine = QueryStateMachine.begin(
statement,
TEST_SESSION,
testSessionBuilder()
.setCatalog(CATALOG_NAME)
.build(),
URI.create("fake://uri"),
new ResourceGroupId("test"),
false,
Expand Down
13 changes: 5 additions & 8 deletions presto-parser/src/main/antlr4/io/prestosql/sql/parser/SqlBase.g4
View file
Expand Up @@ -61,22 +61,19 @@ statement
| DROP VIEW (IF EXISTS)? qualifiedName #dropView
| CALL qualifiedName '(' (callArgument (',' callArgument)*)? ')' #call
| CREATE ROLE name=identifier
(WITH ADMIN grantor)?
(IN catalog=identifier)? #createRole
| DROP ROLE name=identifier (IN catalog=identifier)? #dropRole
(WITH ADMIN grantor)? #createRole
| DROP ROLE name=identifier #dropRole
| GRANT
roles
TO principal (',' principal)*
(WITH ADMIN OPTION)?
(GRANTED BY grantor)?
(IN catalog=identifier)? #grantRoles
(GRANTED BY grantor)? #grantRoles
| REVOKE
(ADMIN OPTION FOR)?
roles
FROM principal (',' principal)*
(GRANTED BY grantor)?
(IN catalog=identifier)? #revokeRoles
| SET ROLE (ALL | NONE | role=identifier) (IN catalog=identifier)? #setRole
(GRANTED BY grantor)? #revokeRoles
| SET ROLE (ALL | NONE | role=identifier) #setRole
| GRANT
(privilege (',' privilege)* | ALL PRIVILEGES)
ON TABLE? qualifiedName TO grantee=principal
Expand Down
15 changes: 0 additions & 15 deletions presto-parser/src/main/java/io/prestosql/sql/SqlFormatter.java
View file
Expand Up @@ -1105,19 +1105,13 @@ protected Void visitCreateRole(CreateRole node, Integer context)
if (node.getGrantor().isPresent()) {
builder.append(" WITH ADMIN ").append(formatGrantor(node.getGrantor().get()));
}
if (node.getCatalog().isPresent()) {
builder.append(" IN ").append(node.getCatalog().get());
}
return null;
}

@Override
protected Void visitDropRole(DropRole node, Integer context)
{
builder.append("DROP ROLE ").append(node.getName());
if (node.getCatalog().isPresent()) {
builder.append(" IN ").append(node.getCatalog().get());
}
return null;
}

Expand All @@ -1138,9 +1132,6 @@ protected Void visitGrantRoles(GrantRoles node, Integer context)
if (node.getGrantor().isPresent()) {
builder.append(" GRANTED BY ").append(formatGrantor(node.getGrantor().get()));
}
if (node.getCatalog().isPresent()) {
builder.append(" IN ").append(node.getCatalog().get());
}
return null;
}

Expand All @@ -1161,9 +1152,6 @@ protected Void visitRevokeRoles(RevokeRoles node, Integer context)
if (node.getGrantor().isPresent()) {
builder.append(" GRANTED BY ").append(formatGrantor(node.getGrantor().get()));
}
if (node.getCatalog().isPresent()) {
builder.append(" IN ").append(node.getCatalog().get());
}
return null;
}

Expand All @@ -1183,9 +1171,6 @@ protected Void visitSetRole(SetRole node, Integer context)
default:
throw new IllegalArgumentException("Unsupported type: " + type);
}
if (node.getCatalog().isPresent()) {
builder.append(" IN ").append(node.getCatalog().get());
}
return null;
}

Expand Down
14 changes: 5 additions & 9 deletions presto-parser/src/main/java/io/prestosql/sql/parser/AstBuilder.java
View file
Expand Up @@ -831,17 +831,15 @@ public Node visitCreateRole(SqlBaseParser.CreateRoleContext context)
return new CreateRole(
getLocation(context),
(Identifier) visit(context.name),
getGrantorSpecificationIfPresent(context.grantor()),
getIdentifierIfPresent(context.catalog));
getGrantorSpecificationIfPresent(context.grantor()));
}

@Override
public Node visitDropRole(SqlBaseParser.DropRoleContext context)
{
return new DropRole(
getLocation(context),
(Identifier) visit(context.name),
getIdentifierIfPresent(context.catalog));
(Identifier) visit(context.name));
}

@Override
Expand All @@ -852,8 +850,7 @@ public Node visitGrantRoles(SqlBaseParser.GrantRolesContext context)
ImmutableSet.copyOf(getIdentifiers(context.roles().identifier())),
ImmutableSet.copyOf(getPrincipalSpecifications(context.principal())),
context.OPTION() != null,
getGrantorSpecificationIfPresent(context.grantor()),
getIdentifierIfPresent(context.catalog));
getGrantorSpecificationIfPresent(context.grantor()));
}

@Override
Expand All @@ -864,8 +861,7 @@ public Node visitRevokeRoles(SqlBaseParser.RevokeRolesContext context)
ImmutableSet.copyOf(getIdentifiers(context.roles().identifier())),
ImmutableSet.copyOf(getPrincipalSpecifications(context.principal())),
context.OPTION() != null,
getGrantorSpecificationIfPresent(context.grantor()),
getIdentifierIfPresent(context.catalog));
getGrantorSpecificationIfPresent(context.grantor()));
}

@Override
Expand All @@ -878,7 +874,7 @@ public Node visitSetRole(SqlBaseParser.SetRoleContext context)
else if (context.NONE() != null) {
type = SetRole.Type.NONE;
}
return new SetRole(getLocation(context), type, getIdentifierIfPresent(context.role), getIdentifierIfPresent(context.catalog));
return new SetRole(getLocation(context), type, getIdentifierIfPresent(context.role));
}

@Override
Expand Down
23 changes: 7 additions & 16 deletions presto-parser/src/main/java/io/prestosql/sql/tree/CreateRole.java
View file
Expand Up @@ -27,24 +27,22 @@ public class CreateRole
{
private final Identifier name;
private final Optional<GrantorSpecification> grantor;
private final Optional<Identifier> catalog;

public CreateRole(Identifier name, Optional<GrantorSpecification> grantor, Optional<Identifier> catalog)
public CreateRole(Identifier name, Optional<GrantorSpecification> grantor)
{
this(Optional.empty(), name, grantor, catalog);
this(Optional.empty(), name, grantor);
}

public CreateRole(NodeLocation location, Identifier name, Optional<GrantorSpecification> grantor, Optional<Identifier> catalog)
public CreateRole(NodeLocation location, Identifier name, Optional<GrantorSpecification> grantor)
{
this(Optional.of(location), name, grantor, catalog);
this(Optional.of(location), name, grantor);
}

private CreateRole(Optional<NodeLocation> location, Identifier name, Optional<GrantorSpecification> grantor, Optional<Identifier> catalog)
private CreateRole(Optional<NodeLocation> location, Identifier name, Optional<GrantorSpecification> grantor)
{
super(location);
this.name = requireNonNull(name, "name is null");
this.grantor = requireNonNull(grantor, "grantor is null");
this.catalog = requireNonNull(catalog, "catalog is null");
}

public Identifier getName()
Expand All @@ -57,11 +55,6 @@ public Optional<GrantorSpecification> getGrantor()
return grantor;
}

public Optional<Identifier> getCatalog()
{
return catalog;
}

@Override
public boolean equals(Object o)
{
Expand All @@ -73,14 +66,13 @@ public boolean equals(Object o)
}
CreateRole that = (CreateRole) o;
return Objects.equals(name, that.name) &&
Objects.equals(grantor, that.grantor) &&
Objects.equals(catalog, that.catalog);
Objects.equals(grantor, that.grantor);
}

@Override
public int hashCode()
{
return Objects.hash(name, grantor, catalog);
return Objects.hash(name, grantor);
}

@Override
Expand All @@ -89,7 +81,6 @@ public String toString()
return toStringHelper(this)
.add("name", name)
.add("grantor", grantor)
.add("catalog", catalog)
.toString();
}

Expand Down
23 changes: 7 additions & 16 deletions presto-parser/src/main/java/io/prestosql/sql/tree/DropRole.java
View file
Expand Up @@ -26,35 +26,28 @@ public class DropRole
extends Statement
{
private final Identifier name;
private final Optional<Identifier> catalog;

public DropRole(Identifier name, Optional<Identifier> catalog)
public DropRole(Identifier name)
{
this(Optional.empty(), name, catalog);
this(Optional.empty(), name);
}

public DropRole(NodeLocation location, Identifier name, Optional<Identifier> catalog)
public DropRole(NodeLocation location, Identifier name)
{
this(Optional.of(location), name, catalog);
this(Optional.of(location), name);
}

private DropRole(Optional<NodeLocation> location, Identifier name, Optional<Identifier> catalog)
private DropRole(Optional<NodeLocation> location, Identifier name)
{
super(location);
this.name = requireNonNull(name, "name is null");
this.catalog = requireNonNull(catalog, "catalog is null");
}

public Identifier getName()
{
return name;
}

public Optional<Identifier> getCatalog()
{
return catalog;
}

@Override
public boolean equals(Object o)
{
Expand All @@ -65,22 +58,20 @@ public boolean equals(Object o)
return false;
}
DropRole dropRole = (DropRole) o;
return Objects.equals(name, dropRole.name) &&
Objects.equals(catalog, dropRole.catalog);
return Objects.equals(name, dropRole.name);
}

@Override
public int hashCode()
{
return Objects.hash(name, catalog);
return Objects.hash(name);
}

@Override
public String toString()
{
return toStringHelper(this)
.add("name", name)
.add("catalog", catalog)
.toString();
}

Expand Down

0 comments on commit eadfb43

Please sign in to comment.