Skip to content

Commit

Permalink
[apache#5070] improvement(core): Add check for full name of the metad…
Browse files Browse the repository at this point in the history 
…ata object
  • Loading branch information
@jerqi
jerqi committed Oct 9, 2024
1 parent 75ec8ae commit aef1706
Show file tree
Hide file tree
Showing 9 changed files with 254 additions and 123 deletions.
62 changes: 0 additions & 62 deletions core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,12 @@
*/
package org.apache.gravitino.authorization;

import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.collect.Sets;
import java.io.IOException;
import java.util.Collection;
import java.util.List;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Supplier;
import org.apache.gravitino.Catalog;
import org.apache.gravitino.Entity;
import org.apache.gravitino.EntityStore;
Expand Down Expand Up @@ -216,58 +213,6 @@ public static boolean needApplyAuthorizationPluginAllCatalogs(SecurableObject se
return false;
}

// Check every securable object whether exists and is imported.
public static void checkSecurableObject(String metalake, MetadataObject object) {
NameIdentifier identifier = MetadataObjectUtil.toEntityIdent(metalake, object);

Supplier<NoSuchMetadataObjectException> exceptionToThrowSupplier =
() ->
new NoSuchMetadataObjectException(
"Securable object %s type %s doesn't exist", object.fullName(), object.type());

switch (object.type()) {
case METALAKE:
check(
GravitinoEnv.getInstance().metalakeDispatcher().metalakeExists(identifier),
exceptionToThrowSupplier);
break;

case CATALOG:
check(
GravitinoEnv.getInstance().catalogDispatcher().catalogExists(identifier),
exceptionToThrowSupplier);
break;

case SCHEMA:
check(
GravitinoEnv.getInstance().schemaDispatcher().schemaExists(identifier),
exceptionToThrowSupplier);
break;

case FILESET:
check(
GravitinoEnv.getInstance().filesetDispatcher().filesetExists(identifier),
exceptionToThrowSupplier);
break;

case TABLE:
check(
GravitinoEnv.getInstance().tableDispatcher().tableExists(identifier),
exceptionToThrowSupplier);
break;

case TOPIC:
check(
GravitinoEnv.getInstance().topicDispatcher().topicExists(identifier),
exceptionToThrowSupplier);
break;

default:
throw new IllegalArgumentException(
String.format("Doesn't support the type %s", object.type()));
}
}

public static void checkDuplicatedNamePrivilege(Collection<Privilege> privileges) {
Set<Privilege.Name> privilegeNameSet = Sets.newHashSet();
for (Privilege privilege : privileges) {
Expand Down Expand Up @@ -313,13 +258,6 @@ public static void checkPrivilege(
}
}

private static void check(
final boolean expression, Supplier<? extends RuntimeException> exceptionToThrowSupplier) {
if (!expression) {
throw checkNotNull(exceptionToThrowSupplier).get();
}
}

private static void checkCatalogType(
NameIdentifier catalogIdent, Catalog.Type type, Privilege privilege) {
Catalog catalog = GravitinoEnv.getInstance().catalogDispatcher().loadCatalog(catalogIdent);
Expand Down
55 changes: 14 additions & 41 deletions core/src/main/java/org/apache/gravitino/tag/TagManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.Namespace;
import org.apache.gravitino.exceptions.NoSuchEntityException;
import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
import org.apache.gravitino.exceptions.NoSuchTagException;
import org.apache.gravitino.exceptions.NotFoundException;
Expand Down Expand Up @@ -240,14 +241,11 @@ public String[] listTagsForMetadataObject(String metalake, MetadataObject metada
}

public Tag[] listTagsInfoForMetadataObject(String metalake, MetadataObject metadataObject)
throws NotFoundException {
throws NoSuchMetadataObjectException {
NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake, metadataObject);
Entity.EntityType entityType = MetadataObjectUtil.toEntityType(metadataObject);

if (!checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance())) {
throw new NotFoundException(
"Failed to list tags for metadata object %s due to not found", metadataObject);
}
checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance());

return TreeLockUtils.doWithTreeLock(
entityIdent,
Expand All @@ -258,7 +256,7 @@ public Tag[] listTagsInfoForMetadataObject(String metalake, MetadataObject metad
.listAssociatedTagsForMetadataObject(entityIdent, entityType)
.toArray(new Tag[0]);
} catch (NoSuchEntityException e) {
throw new NotFoundException(
throw new NoSuchMetadataObjectException(
e, "Failed to list tags for metadata object %s due to not found", metadataObject);
} catch (IOException e) {
LOG.error("Failed to list tags for metadata object {}", metadataObject, e);
Expand All @@ -268,15 +266,12 @@ public Tag[] listTagsInfoForMetadataObject(String metalake, MetadataObject metad
}

public Tag getTagForMetadataObject(String metalake, MetadataObject metadataObject, String name)
throws NotFoundException {
throws NoSuchMetadataObjectException {
NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake, metadataObject);
Entity.EntityType entityType = MetadataObjectUtil.toEntityType(metadataObject);
NameIdentifier tagIdent = ofTagIdent(metalake, name);

if (!checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance())) {
throw new NotFoundException(
"Failed to get tag for metadata object %s due to not found", metadataObject);
}
checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance());

return TreeLockUtils.doWithTreeLock(
entityIdent,
Expand All @@ -289,7 +284,7 @@ public Tag getTagForMetadataObject(String metalake, MetadataObject metadataObjec
throw new NoSuchTagException(
e, "Tag %s does not exist for metadata object %s", name, metadataObject);
} else {
throw new NotFoundException(
throw new NoSuchMetadataObjectException(
e, "Failed to get tag for metadata object %s due to not found", metadataObject);
}
} catch (IOException e) {
Expand All @@ -301,20 +296,18 @@ public Tag getTagForMetadataObject(String metalake, MetadataObject metadataObjec

public String[] associateTagsForMetadataObject(
String metalake, MetadataObject metadataObject, String[] tagsToAdd, String[] tagsToRemove)
throws NotFoundException, TagAlreadyAssociatedException {
throws NoSuchMetadataObjectException, TagAlreadyAssociatedException {
Preconditions.checkArgument(
!metadataObject.type().equals(MetadataObject.Type.METALAKE)
&& !metadataObject.type().equals(MetadataObject.Type.COLUMN),
&& !metadataObject.type().equals(MetadataObject.Type.COLUMN)
&& !metadataObject.type().equals(MetadataObject.Type.ROLE),
"Cannot associate tags for unsupported metadata object type %s",
metadataObject.type());

NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake, metadataObject);
Entity.EntityType entityType = MetadataObjectUtil.toEntityType(metadataObject);

if (!checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance())) {
throw new NotFoundException(
"Failed to associate tags for metadata object %s due to not found", metadataObject);
}
checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance());

// Remove all the tags that are both set to add and remove
Set<String> tagsToAddSet = tagsToAdd == null ? Sets.newHashSet() : Sets.newHashSet(tagsToAdd);
Expand Down Expand Up @@ -347,7 +340,7 @@ public String[] associateTagsForMetadataObject(
.map(Tag::name)
.toArray(String[]::new);
} catch (NoSuchEntityException e) {
throw new NotFoundException(
throw new NoSuchMetadataObjectException(
e,
"Failed to associate tags for metadata object %s due to not found",
metadataObject);
Expand Down Expand Up @@ -431,27 +424,7 @@ private TagEntity updateTagEntity(TagEntity tagEntity, TagChange... changes) {
// for this entity, with this uid tags can be associated with this entity.
// This method should be called out of the tree lock, otherwise it will cause deadlock.
@VisibleForTesting
boolean checkAndImportEntity(String metalake, MetadataObject metadataObject, GravitinoEnv env) {
NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake, metadataObject);
Entity.EntityType entityType = MetadataObjectUtil.toEntityType(metadataObject);

switch (entityType) {
case METALAKE:
return env.metalakeDispatcher().metalakeExists(entityIdent);
case CATALOG:
return env.catalogDispatcher().catalogExists(entityIdent);
case SCHEMA:
return env.schemaDispatcher().schemaExists(entityIdent);
case TABLE:
return env.tableDispatcher().tableExists(entityIdent);
case TOPIC:
return env.topicDispatcher().topicExists(entityIdent);
case FILESET:
return env.filesetDispatcher().filesetExists(entityIdent);
case COLUMN:
default:
throw new IllegalArgumentException(
"Unsupported metadata object type: " + metadataObject.type());
}
void checkAndImportEntity(String metalake, MetadataObject metadataObject, GravitinoEnv env) {
MetadataObjectUtil.checkMetadataObject(metalake, metadataObject, env);
}
}
79 changes: 79 additions & 0 deletions core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,22 @@
*/
package org.apache.gravitino.utils;

import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.BiMap;
import com.google.common.collect.ImmutableBiMap;
import java.util.Optional;
import java.util.function.Supplier;
import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.Entity;
import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.authorization.AuthorizationUtils;
import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
import org.apache.gravitino.exceptions.NoSuchRoleException;

public class MetadataObjectUtil {

Expand Down Expand Up @@ -98,4 +104,77 @@ public static NameIdentifier toEntityIdent(String metalakeName, MetadataObject m
"Unknown metadata object type: " + metadataObject.type());
}
}

/**
* This method will check if the entity is existed explicitly, internally this check will load the
* entity from underlying sources to entity store if not stored, and will allocate an uid for this
* entity, with this uid tags can be associated with this entity. This method should be called out
* of the tree lock, otherwise it will cause deadlock.
*
* @param metalake The metalake name
* @param object The metadata object
* @param env The Gravitino environment
* @throws NoSuchMetadataObjectException if the metadata object type doesn't exist.
*/
public static void checkMetadataObject(String metalake, MetadataObject object, GravitinoEnv env) {
NameIdentifier identifier = toEntityIdent(metalake, object);

Supplier<NoSuchMetadataObjectException> exceptionToThrowSupplier =
() ->
new NoSuchMetadataObjectException(
"Metadata object %s type %s doesn't exist", object.fullName(), object.type());

switch (object.type()) {
case METALAKE:
NameIdentifierUtil.checkMetalake(identifier);
check(env.metalakeDispatcher().metalakeExists(identifier), exceptionToThrowSupplier);
break;

case CATALOG:
NameIdentifierUtil.checkCatalog(identifier);
check(env.catalogDispatcher().catalogExists(identifier), exceptionToThrowSupplier);
break;

case SCHEMA:
NameIdentifierUtil.checkSchema(identifier);
check(env.schemaDispatcher().schemaExists(identifier), exceptionToThrowSupplier);
break;

case FILESET:
NameIdentifierUtil.checkFileset(identifier);
check(env.filesetDispatcher().filesetExists(identifier), exceptionToThrowSupplier);
break;

case TABLE:
NameIdentifierUtil.checkTable(identifier);
check(env.tableDispatcher().tableExists(identifier), exceptionToThrowSupplier);
break;

case TOPIC:
NameIdentifierUtil.checkTopic(identifier);
check(env.topicDispatcher().topicExists(identifier), exceptionToThrowSupplier);
break;

case ROLE:
AuthorizationUtils.checkRole(identifier);
try {
env.accessControlDispatcher().getRole(metalake, object.fullName());
} catch (NoSuchRoleException nsr) {
throw checkNotNull(exceptionToThrowSupplier).get();
}
break;

case COLUMN:
default:
throw new IllegalArgumentException(
String.format("Doesn't support the type %s", object.type()));
}
}

private static void check(
final boolean expression, Supplier<? extends RuntimeException> exceptionToThrowSupplier) {
if (!expression) {
throw checkNotNull(exceptionToThrowSupplier).get();
}
}
}
6 changes: 2 additions & 4 deletions core/src/test/java/org/apache/gravitino/tag/TestTagManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import static org.apache.gravitino.Configs.TREE_LOCK_MAX_NODE_IN_MEMORY;
import static org.apache.gravitino.Configs.TREE_LOCK_MIN_NODE_IN_MEMORY;
import static org.apache.gravitino.Configs.VERSION_RETENTION_COUNT;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.spy;

import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -167,9 +167,7 @@ public static void setUp() throws IOException, IllegalAccessException {
entityStore.put(table, false /* overwritten */);

tagManager = spy(new TagManager(idGenerator, entityStore));
doReturn(true)
.when(tagManager)
.checkAndImportEntity(Mockito.any(), Mockito.any(), Mockito.any());
doNothing().when(tagManager).checkAndImportEntity(Mockito.any(), Mockito.any(), Mockito.any());
}

@AfterAll
Expand Down
2 changes: 2 additions & 0 deletions server/src/main/java/org/apache/gravitino/server/web/rest/OwnerOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ public Response getOwnerForObject(
return Utils.doAs(
httpRequest,
() -> {
MetadataObjectUtil.checkMetadataObject(metalake, object, GravitinoEnv.getInstance());
NameIdentifier ident = MetadataObjectUtil.toEntityIdent(metalake, object);
Optional<Owner> owner =
TreeLockUtils.doWithTreeLock(
Expand Down Expand Up @@ -112,6 +113,7 @@ public Response setOwnerForObject(
return Utils.doAs(
httpRequest,
() -> {
MetadataObjectUtil.checkMetadataObject(metalake, object, GravitinoEnv.getInstance());
NameIdentifier objectIdent = MetadataObjectUtil.toEntityIdent(metalake, object);
TreeLockUtils.doWithTreeLock(
objectIdent,
Expand Down
5 changes: 3 additions & 2 deletions server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import org.apache.gravitino.metrics.MetricNames;
import org.apache.gravitino.server.authorization.NameBindings;
import org.apache.gravitino.server.web.Utils;
import org.apache.gravitino.utils.MetadataObjectUtil;

@NameBindings.AccessControlInterfaces
@Path("/metalakes/{metalake}/permissions")
Expand Down Expand Up @@ -217,7 +218,7 @@ public Response grantPrivilegeToRole(
AuthorizationUtils.checkPrivilege(privilegeDTO, object, metalake);
}

AuthorizationUtils.checkSecurableObject(metalake, object);
MetadataObjectUtil.checkMetadataObject(metalake, object, GravitinoEnv.getInstance());
return TreeLockUtils.doWithTreeLock(
AuthorizationUtils.ofRole(metalake, role),
LockType.WRITE,
Expand Down Expand Up @@ -262,7 +263,7 @@ public Response revokePrivilegeFromRole(
AuthorizationUtils.checkPrivilege(privilegeDTO, object, metalake);
}

AuthorizationUtils.checkSecurableObject(metalake, object);
MetadataObjectUtil.checkMetadataObject(metalake, object, GravitinoEnv.getInstance());
return TreeLockUtils.doWithTreeLock(
AuthorizationUtils.ofRole(metalake, role),
LockType.WRITE,
Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.apache.gravitino.metrics.MetricNames;
import org.apache.gravitino.server.authorization.NameBindings;
import org.apache.gravitino.server.web.Utils;
import org.apache.gravitino.utils.MetadataObjectUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -142,7 +143,7 @@ public Response createRole(@PathParam("metalake") String metalake, RoleCreateReq
for (Privilege privilege : object.privileges()) {
AuthorizationUtils.checkPrivilege((PrivilegeDTO) privilege, object, metalake);
}
AuthorizationUtils.checkSecurableObject(metalake, object);
MetadataObjectUtil.checkMetadataObject(metalake, object, GravitinoEnv.getInstance());
}

List<SecurableObject> securableObjects =
Expand Down
Loading

0 comments on commit aef1706

Please sign in to comment.