Skip to content

Commit c5c103d

Browse files
github-actions[bot]roryqi
github-actions[bot]
and
roryqi
authored
[#5070] improvement(core): Add check for the full name of the metadata object (#5091)
### 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. Co-authored-by: roryqi <roryqi@apache.org>
1 parent e9ba2b9 commit c5c103d

File tree

9 files changed

+241
-134
lines changed

9 files changed

+241
-134
lines changed

‎core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java‎

Lines changed: 0 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,12 @@
1818
*/
1919
package org.apache.gravitino.authorization;
2020

21-
import static com.google.common.base.Preconditions.checkNotNull;
22-
2321
import com.google.common.collect.Sets;
2422
import java.io.IOException;
2523
import java.util.Collection;
2624
import java.util.List;
2725
import java.util.Set;
2826
import java.util.function.Consumer;
29-
import java.util.function.Supplier;
3027
import org.apache.gravitino.Catalog;
3128
import org.apache.gravitino.Entity;
3229
import org.apache.gravitino.EntityStore;
@@ -216,58 +213,6 @@ public static boolean needApplyAuthorizationPluginAllCatalogs(SecurableObject se
216213
return false;
217214
}
218215

219-
// Check every securable object whether exists and is imported.
220-
public static void checkSecurableObject(String metalake, MetadataObject object) {
221-
NameIdentifier identifier = MetadataObjectUtil.toEntityIdent(metalake, object);
222-
223-
Supplier<NoSuchMetadataObjectException> exceptionToThrowSupplier =
224-
() ->
225-
new NoSuchMetadataObjectException(
226-
"Securable object %s type %s doesn't exist", object.fullName(), object.type());
227-
228-
switch (object.type()) {
229-
case METALAKE:
230-
check(
231-
GravitinoEnv.getInstance().metalakeDispatcher().metalakeExists(identifier),
232-
exceptionToThrowSupplier);
233-
break;
234-
235-
case CATALOG:
236-
check(
237-
GravitinoEnv.getInstance().catalogDispatcher().catalogExists(identifier),
238-
exceptionToThrowSupplier);
239-
break;
240-
241-
case SCHEMA:
242-
check(
243-
GravitinoEnv.getInstance().schemaDispatcher().schemaExists(identifier),
244-
exceptionToThrowSupplier);
245-
break;
246-
247-
case FILESET:
248-
check(
249-
GravitinoEnv.getInstance().filesetDispatcher().filesetExists(identifier),
250-
exceptionToThrowSupplier);
251-
break;
252-
253-
case TABLE:
254-
check(
255-
GravitinoEnv.getInstance().tableDispatcher().tableExists(identifier),
256-
exceptionToThrowSupplier);
257-
break;
258-
259-
case TOPIC:
260-
check(
261-
GravitinoEnv.getInstance().topicDispatcher().topicExists(identifier),
262-
exceptionToThrowSupplier);
263-
break;
264-
265-
default:
266-
throw new IllegalArgumentException(
267-
String.format("Doesn't support the type %s", object.type()));
268-
}
269-
}
270-
271216
public static void checkDuplicatedNamePrivilege(Collection<Privilege> privileges) {
272217
Set<Privilege.Name> privilegeNameSet = Sets.newHashSet();
273218
for (Privilege privilege : privileges) {
@@ -313,13 +258,6 @@ public static void checkPrivilege(
313258
}
314259
}
315260

316-
private static void check(
317-
final boolean expression, Supplier<? extends RuntimeException> exceptionToThrowSupplier) {
318-
if (!expression) {
319-
throw checkNotNull(exceptionToThrowSupplier).get();
320-
}
321-
}
322-
323261
private static void checkCatalogType(
324262
NameIdentifier catalogIdent, Catalog.Type type, Privilege privilege) {
325263
Catalog catalog = GravitinoEnv.getInstance().catalogDispatcher().loadCatalog(catalogIdent);

‎core/src/main/java/org/apache/gravitino/tag/TagManager.java‎

Lines changed: 12 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
*/
1919
package org.apache.gravitino.tag;
2020

21-
import com.google.common.annotations.VisibleForTesting;
2221
import com.google.common.base.Preconditions;
2322
import com.google.common.collect.Maps;
2423
import com.google.common.collect.Sets;
@@ -31,11 +30,11 @@
3130
import org.apache.gravitino.Entity;
3231
import org.apache.gravitino.EntityAlreadyExistsException;
3332
import org.apache.gravitino.EntityStore;
34-
import org.apache.gravitino.GravitinoEnv;
3533
import org.apache.gravitino.MetadataObject;
3634
import org.apache.gravitino.NameIdentifier;
3735
import org.apache.gravitino.Namespace;
3836
import org.apache.gravitino.exceptions.NoSuchEntityException;
37+
import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
3938
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
4039
import org.apache.gravitino.exceptions.NoSuchTagException;
4140
import org.apache.gravitino.exceptions.NotFoundException;
@@ -240,14 +239,11 @@ public String[] listTagsForMetadataObject(String metalake, MetadataObject metada
240239
}
241240

242241
public Tag[] listTagsInfoForMetadataObject(String metalake, MetadataObject metadataObject)
243-
throws NotFoundException {
242+
throws NoSuchMetadataObjectException {
244243
NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake, metadataObject);
245244
Entity.EntityType entityType = MetadataObjectUtil.toEntityType(metadataObject);
246245

247-
if (!checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance())) {
248-
throw new NotFoundException(
249-
"Failed to list tags for metadata object %s due to not found", metadataObject);
250-
}
246+
MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
251247

252248
return TreeLockUtils.doWithTreeLock(
253249
entityIdent,
@@ -258,7 +254,7 @@ public Tag[] listTagsInfoForMetadataObject(String metalake, MetadataObject metad
258254
.listAssociatedTagsForMetadataObject(entityIdent, entityType)
259255
.toArray(new Tag[0]);
260256
} catch (NoSuchEntityException e) {
261-
throw new NotFoundException(
257+
throw new NoSuchMetadataObjectException(
262258
e, "Failed to list tags for metadata object %s due to not found", metadataObject);
263259
} catch (IOException e) {
264260
LOG.error("Failed to list tags for metadata object {}", metadataObject, e);
@@ -268,15 +264,12 @@ public Tag[] listTagsInfoForMetadataObject(String metalake, MetadataObject metad
268264
}
269265

270266
public Tag getTagForMetadataObject(String metalake, MetadataObject metadataObject, String name)
271-
throws NotFoundException {
267+
throws NoSuchMetadataObjectException {
272268
NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake, metadataObject);
273269
Entity.EntityType entityType = MetadataObjectUtil.toEntityType(metadataObject);
274270
NameIdentifier tagIdent = ofTagIdent(metalake, name);
275271

276-
if (!checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance())) {
277-
throw new NotFoundException(
278-
"Failed to get tag for metadata object %s due to not found", metadataObject);
279-
}
272+
MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
280273

281274
return TreeLockUtils.doWithTreeLock(
282275
entityIdent,
@@ -289,7 +282,7 @@ public Tag getTagForMetadataObject(String metalake, MetadataObject metadataObjec
289282
throw new NoSuchTagException(
290283
e, "Tag %s does not exist for metadata object %s", name, metadataObject);
291284
} else {
292-
throw new NotFoundException(
285+
throw new NoSuchMetadataObjectException(
293286
e, "Failed to get tag for metadata object %s due to not found", metadataObject);
294287
}
295288
} catch (IOException e) {
@@ -301,20 +294,18 @@ public Tag getTagForMetadataObject(String metalake, MetadataObject metadataObjec
301294

302295
public String[] associateTagsForMetadataObject(
303296
String metalake, MetadataObject metadataObject, String[] tagsToAdd, String[] tagsToRemove)
304-
throws NotFoundException, TagAlreadyAssociatedException {
297+
throws NoSuchMetadataObjectException, TagAlreadyAssociatedException {
305298
Preconditions.checkArgument(
306299
!metadataObject.type().equals(MetadataObject.Type.METALAKE)
307-
&& !metadataObject.type().equals(MetadataObject.Type.COLUMN),
300+
&& !metadataObject.type().equals(MetadataObject.Type.COLUMN)
301+
&& !metadataObject.type().equals(MetadataObject.Type.ROLE),
308302
"Cannot associate tags for unsupported metadata object type %s",
309303
metadataObject.type());
310304

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

314-
if (!checkAndImportEntity(metalake, metadataObject, GravitinoEnv.getInstance())) {
315-
throw new NotFoundException(
316-
"Failed to associate tags for metadata object %s due to not found", metadataObject);
317-
}
308+
MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
318309

319310
// Remove all the tags that are both set to add and remove
320311
Set<String> tagsToAddSet = tagsToAdd == null ? Sets.newHashSet() : Sets.newHashSet(tagsToAdd);
@@ -347,7 +338,7 @@ public String[] associateTagsForMetadataObject(
347338
.map(Tag::name)
348339
.toArray(String[]::new);
349340
} catch (NoSuchEntityException e) {
350-
throw new NotFoundException(
341+
throw new NoSuchMetadataObjectException(
351342
e,
352343
"Failed to associate tags for metadata object %s due to not found",
353344
metadataObject);
@@ -425,33 +416,4 @@ private TagEntity updateTagEntity(TagEntity tagEntity, TagChange... changes) {
425416
.build())
426417
.build();
427418
}
428-
429-
// This method will check if the entity is existed explicitly, internally this check will load
430-
// the entity from underlying sources to entity store if not stored, and will allocate an uid
431-
// for this entity, with this uid tags can be associated with this entity.
432-
// This method should be called out of the tree lock, otherwise it will cause deadlock.
433-
@VisibleForTesting
434-
boolean checkAndImportEntity(String metalake, MetadataObject metadataObject, GravitinoEnv env) {
435-
NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake, metadataObject);
436-
Entity.EntityType entityType = MetadataObjectUtil.toEntityType(metadataObject);
437-
438-
switch (entityType) {
439-
case METALAKE:
440-
return env.metalakeDispatcher().metalakeExists(entityIdent);
441-
case CATALOG:
442-
return env.catalogDispatcher().catalogExists(entityIdent);
443-
case SCHEMA:
444-
return env.schemaDispatcher().schemaExists(entityIdent);
445-
case TABLE:
446-
return env.tableDispatcher().tableExists(entityIdent);
447-
case TOPIC:
448-
return env.topicDispatcher().topicExists(entityIdent);
449-
case FILESET:
450-
return env.filesetDispatcher().filesetExists(entityIdent);
451-
case COLUMN:
452-
default:
453-
throw new IllegalArgumentException(
454-
"Unsupported metadata object type: " + metadataObject.type());
455-
}
456-
}
457419
}

‎core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,22 @@
1818
*/
1919
package org.apache.gravitino.utils;
2020

21+
import static com.google.common.base.Preconditions.checkNotNull;
22+
2123
import com.google.common.base.Joiner;
2224
import com.google.common.base.Preconditions;
2325
import com.google.common.collect.BiMap;
2426
import com.google.common.collect.ImmutableBiMap;
2527
import java.util.Optional;
28+
import java.util.function.Supplier;
2629
import org.apache.commons.lang3.StringUtils;
2730
import org.apache.gravitino.Entity;
31+
import org.apache.gravitino.GravitinoEnv;
2832
import org.apache.gravitino.MetadataObject;
2933
import org.apache.gravitino.NameIdentifier;
3034
import org.apache.gravitino.authorization.AuthorizationUtils;
35+
import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
36+
import org.apache.gravitino.exceptions.NoSuchRoleException;
3137

3238
public class MetadataObjectUtil {
3339

@@ -98,4 +104,76 @@ public static NameIdentifier toEntityIdent(String metalakeName, MetadataObject m
98104
"Unknown metadata object type: " + metadataObject.type());
99105
}
100106
}
107+
108+
/**
109+
* This method will check if the entity is existed explicitly, internally this check will load the
110+
* entity from underlying sources to entity store if not stored, and will allocate an uid for this
111+
* entity, with this uid tags can be associated with this entity. This method should be called out
112+
* of the tree lock, otherwise it will cause deadlock.
113+
*
114+
* @param metalake The metalake name
115+
* @param object The metadata object
116+
* @throws NoSuchMetadataObjectException if the metadata object type doesn't exist.
117+
*/
118+
public static void checkMetadataObject(String metalake, MetadataObject object) {
119+
GravitinoEnv env = GravitinoEnv.getInstance();
120+
NameIdentifier identifier = toEntityIdent(metalake, object);
121+
122+
Supplier<NoSuchMetadataObjectException> exceptionToThrowSupplier =
123+
() ->
124+
new NoSuchMetadataObjectException(
125+
"Metadata object %s type %s doesn't exist", object.fullName(), object.type());
126+
127+
switch (object.type()) {
128+
case METALAKE:
129+
NameIdentifierUtil.checkMetalake(identifier);
130+
check(env.metalakeDispatcher().metalakeExists(identifier), exceptionToThrowSupplier);
131+
break;
132+
133+
case CATALOG:
134+
NameIdentifierUtil.checkCatalog(identifier);
135+
check(env.catalogDispatcher().catalogExists(identifier), exceptionToThrowSupplier);
136+
break;
137+
138+
case SCHEMA:
139+
NameIdentifierUtil.checkSchema(identifier);
140+
check(env.schemaDispatcher().schemaExists(identifier), exceptionToThrowSupplier);
141+
break;
142+
143+
case FILESET:
144+
NameIdentifierUtil.checkFileset(identifier);
145+
check(env.filesetDispatcher().filesetExists(identifier), exceptionToThrowSupplier);
146+
break;
147+
148+
case TABLE:
149+
NameIdentifierUtil.checkTable(identifier);
150+
check(env.tableDispatcher().tableExists(identifier), exceptionToThrowSupplier);
151+
break;
152+
153+
case TOPIC:
154+
NameIdentifierUtil.checkTopic(identifier);
155+
check(env.topicDispatcher().topicExists(identifier), exceptionToThrowSupplier);
156+
break;
157+
158+
case ROLE:
159+
AuthorizationUtils.checkRole(identifier);
160+
try {
161+
env.accessControlDispatcher().getRole(metalake, object.fullName());
162+
} catch (NoSuchRoleException nsr) {
163+
throw checkNotNull(exceptionToThrowSupplier).get();
164+
}
165+
break;
166+
167+
default:
168+
throw new IllegalArgumentException(
169+
String.format("Doesn't support the type %s", object.type()));
170+
}
171+
}
172+
173+
private static void check(
174+
final boolean expression, Supplier<? extends RuntimeException> exceptionToThrowSupplier) {
175+
if (!expression) {
176+
throw checkNotNull(exceptionToThrowSupplier).get();
177+
}
178+
}
101179
}

‎core/src/test/java/org/apache/gravitino/tag/TestTagManager.java‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,9 @@
3131
import static org.apache.gravitino.Configs.TREE_LOCK_MAX_NODE_IN_MEMORY;
3232
import static org.apache.gravitino.Configs.TREE_LOCK_MIN_NODE_IN_MEMORY;
3333
import static org.apache.gravitino.Configs.VERSION_RETENTION_COUNT;
34-
import static org.mockito.Mockito.doReturn;
35-
import static org.mockito.Mockito.spy;
34+
import static org.mockito.ArgumentMatchers.any;
35+
import static org.mockito.Mockito.mock;
36+
import static org.mockito.Mockito.when;
3637

3738
import com.google.common.collect.ImmutableMap;
3839
import com.google.common.collect.ImmutableSet;
@@ -54,6 +55,9 @@
5455
import org.apache.gravitino.GravitinoEnv;
5556
import org.apache.gravitino.MetadataObject;
5657
import org.apache.gravitino.Namespace;
58+
import org.apache.gravitino.catalog.CatalogDispatcher;
59+
import org.apache.gravitino.catalog.SchemaDispatcher;
60+
import org.apache.gravitino.catalog.TableDispatcher;
5761
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
5862
import org.apache.gravitino.exceptions.NoSuchTagException;
5963
import org.apache.gravitino.exceptions.NotFoundException;
@@ -66,6 +70,7 @@
6670
import org.apache.gravitino.meta.SchemaEntity;
6771
import org.apache.gravitino.meta.SchemaVersion;
6872
import org.apache.gravitino.meta.TableEntity;
73+
import org.apache.gravitino.metalake.MetalakeDispatcher;
6974
import org.apache.gravitino.storage.IdGenerator;
7075
import org.apache.gravitino.storage.RandomIdGenerator;
7176
import org.apache.gravitino.utils.NameIdentifierUtil;
@@ -91,6 +96,10 @@ public class TestTagManager {
9196
private static final String SCHEMA = "schema_for_tag_test";
9297

9398
private static final String TABLE = "table_for_tag_test";
99+
private static final MetalakeDispatcher metalakeDispatcher = mock(MetalakeDispatcher.class);
100+
private static final CatalogDispatcher catalogDispatcher = mock(CatalogDispatcher.class);
101+
private static final SchemaDispatcher schemaDispatcher = mock(SchemaDispatcher.class);
102+
private static final TableDispatcher tableDispatcher = mock(TableDispatcher.class);
94103

95104
private static EntityStore entityStore;
96105

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

169-
tagManager = spy(new TagManager(idGenerator, entityStore));
170-
doReturn(true)
171-
.when(tagManager)
172-
.checkAndImportEntity(Mockito.any(), Mockito.any(), Mockito.any());
178+
tagManager = new TagManager(idGenerator, entityStore);
179+
180+
FieldUtils.writeField(
181+
GravitinoEnv.getInstance(), "metalakeDispatcher", metalakeDispatcher, true);
182+
FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogDispatcher", catalogDispatcher, true);
183+
FieldUtils.writeField(GravitinoEnv.getInstance(), "schemaDispatcher", schemaDispatcher, true);
184+
FieldUtils.writeField(GravitinoEnv.getInstance(), "tableDispatcher", tableDispatcher, true);
185+
186+
when(metalakeDispatcher.metalakeExists(any())).thenReturn(true);
187+
when(catalogDispatcher.catalogExists(any())).thenReturn(true);
188+
when(schemaDispatcher.schemaExists(any())).thenReturn(true);
189+
when(tableDispatcher.tableExists(any())).thenReturn(true);
173190
}
174191

175192
@AfterAll

0 commit comments

Comments
 (0)