Skip to content

Commit

Permalink
[#5070] improvement(core): Add check for the full name of the metadat…
Browse files Browse the repository at this point in the history 
…a object (#5075)

### What changes were proposed in this pull request?

Add check for full name of the metadata object.

### Why are the changes needed?

Fix: #5070 

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Add UTs.
  • Loading branch information
@jerqi
jerqi authored Oct 10, 2024
1 parent fce6147 commit 630cf67
Show file tree
Hide file tree
Showing 9 changed files with 241 additions and 134 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
62 changes: 12 additions & 50 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 @@ -18,7 +18,6 @@
*/
package org.apache.gravitino.tag;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
Expand All @@ -31,11 +30,11 @@
import org.apache.gravitino.Entity;
import org.apache.gravitino.EntityAlreadyExistsException;
import org.apache.gravitino.EntityStore;
import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.MetadataObject;
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 +239,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);
}
MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);

return TreeLockUtils.doWithTreeLock(
entityIdent,
Expand All @@ -258,7 +254,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 +264,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);
}
MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);

return TreeLockUtils.doWithTreeLock(
entityIdent,
Expand All @@ -289,7 +282,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 +294,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);
}
MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);

// 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 +338,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 @@ -425,33 +416,4 @@ private TagEntity updateTagEntity(TagEntity tagEntity, TagChange... changes) {
.build())
.build();
}

// 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.
@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());
}
}
}
78 changes: 78 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,76 @@ 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
* @throws NoSuchMetadataObjectException if the metadata object type doesn't exist.
*/
public static void checkMetadataObject(String metalake, MetadataObject object) {
GravitinoEnv env = GravitinoEnv.getInstance();
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;

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();
}
}
}
29 changes: 23 additions & 6 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,8 +31,9 @@
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.spy;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand All @@ -54,6 +55,9 @@
import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.Namespace;
import org.apache.gravitino.catalog.CatalogDispatcher;
import org.apache.gravitino.catalog.SchemaDispatcher;
import org.apache.gravitino.catalog.TableDispatcher;
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
import org.apache.gravitino.exceptions.NoSuchTagException;
import org.apache.gravitino.exceptions.NotFoundException;
Expand All @@ -66,6 +70,7 @@
import org.apache.gravitino.meta.SchemaEntity;
import org.apache.gravitino.meta.SchemaVersion;
import org.apache.gravitino.meta.TableEntity;
import org.apache.gravitino.metalake.MetalakeDispatcher;
import org.apache.gravitino.storage.IdGenerator;
import org.apache.gravitino.storage.RandomIdGenerator;
import org.apache.gravitino.utils.NameIdentifierUtil;
Expand All @@ -91,6 +96,10 @@ public class TestTagManager {
private static final String SCHEMA = "schema_for_tag_test";

private static final String TABLE = "table_for_tag_test";
private static final MetalakeDispatcher metalakeDispatcher = mock(MetalakeDispatcher.class);
private static final CatalogDispatcher catalogDispatcher = mock(CatalogDispatcher.class);
private static final SchemaDispatcher schemaDispatcher = mock(SchemaDispatcher.class);
private static final TableDispatcher tableDispatcher = mock(TableDispatcher.class);

private static EntityStore entityStore;

Expand Down Expand Up @@ -166,10 +175,18 @@ public static void setUp() throws IOException, IllegalAccessException {
.build();
entityStore.put(table, false /* overwritten */);

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

FieldUtils.writeField(
GravitinoEnv.getInstance(), "metalakeDispatcher", metalakeDispatcher, true);
FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogDispatcher", catalogDispatcher, true);
FieldUtils.writeField(GravitinoEnv.getInstance(), "schemaDispatcher", schemaDispatcher, true);
FieldUtils.writeField(GravitinoEnv.getInstance(), "tableDispatcher", tableDispatcher, true);

when(metalakeDispatcher.metalakeExists(any())).thenReturn(true);
when(catalogDispatcher.catalogExists(any())).thenReturn(true);
when(schemaDispatcher.schemaExists(any())).thenReturn(true);
when(tableDispatcher.tableExists(any())).thenReturn(true);
}

@AfterAll
Expand Down
Loading

0 comments on commit 630cf67

Please sign in to comment.