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.

Sign up for GitHub

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

Jump to bottom

[#5070] improvement(core): Add check for the full name of the metadata object #5075

Merged
merged 4 commits into from
Oct 10, 2024
Merged

[#5070] improvement(core): Add check for the full name of the metadata object #5075

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aef1706
[#5070] improvement(core): Add check for full name of the metadata ob…
jerqi Oct 9, 2024
9d58bff
address comment
jerqi Oct 10, 2024
eff9b4a
Remove column
jerqi Oct 10, 2024
ab3f901
remove parameter
jerqi Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 0 additions & 62 deletions core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
View file
Open in desktop
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
Open in desktop
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);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checkAndImportEntity(...) function only have one line code.
We can directly call MetadataObjectUtil.checkMetadataObject(metalake, metadataObject, env).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is convenient to test. Because we can spy this method easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}
79 changes: 79 additions & 0 deletions core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java
View file
Open in desktop
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we doesn't need GravitinoEnv env params in the checkMetadataObject(...) function.
We can directly call GravitinoEnv.getInstance() in the function body.

Copy link
Contributor

@jerryshao jerryshao Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using env as a method parameter will make the mock test easy to achieve.


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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we support checking columns or not? If the answer is 'No', you can remove case COLUMN: here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading