From 6cf357ed2acdca43e98af1b3e105e56750c074d6 Mon Sep 17 00:00:00 2001 From: Suresh Srinivas Date: Tue, 4 Apr 2023 06:51:19 -0700 Subject: [PATCH] Fixes #10907 Parent glossary term should not be allowed moved under its child term (#10908) --- .../service/exception/CatalogExceptionMessage.java | 4 ++++ .../service/jdbi3/GlossaryTermRepository.java | 11 +++++++++++ .../openmetadata/service/jdbi3/MlModelRepository.java | 1 - .../service/resources/tags/TagResource.java | 8 ++------ .../openmetadata/service/util/FullyQualifiedName.java | 5 +++++ .../resources/glossary/GlossaryResourceTest.java | 9 +++++++++ .../service/util/FullyQualifiedNameTest.java | 11 +++++++++++ 7 files changed, 42 insertions(+), 7 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java b/openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java index f771ad6e572..7d5263322f5 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java @@ -202,4 +202,8 @@ public final class CatalogExceptionMessage { public static String userAlreadyBot(String userName, String botName) { return String.format("Bot user [%s] is already used by [%s] bot", userName, botName); } + + public static String invalidGlossaryTermMove(String term, String newParent) { + return String.format("Can't move Glossary term %s to its child Glossary term %s", term, newParent); + } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java index 3a3fcfbd0d8..623e17322c2 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java @@ -20,6 +20,7 @@ import static org.openmetadata.common.utils.CommonUtil.listOrEmpty; import static org.openmetadata.schema.type.Include.ALL; import static org.openmetadata.service.Entity.GLOSSARY; import static org.openmetadata.service.Entity.GLOSSARY_TERM; +import static org.openmetadata.service.exception.CatalogExceptionMessage.invalidGlossaryTermMove; import static org.openmetadata.service.util.EntityUtil.entityReferenceMatch; import static org.openmetadata.service.util.EntityUtil.getId; import static org.openmetadata.service.util.EntityUtil.stringMatch; @@ -254,6 +255,7 @@ public class GlossaryTermRepository extends EntityRepository { @Override public void entitySpecificUpdate() throws IOException { + validateParent(); updateStatus(original, updated); updateSynonyms(original, updated); updateReferences(original, updated); @@ -362,5 +364,14 @@ public class GlossaryTermRepository extends EntityRepository { recordChange("parent", original.getParent(), updated.getParent(), true, entityReferenceMatch); } } + + private void validateParent() { + String fqn = original.getFullyQualifiedName(); + String newParentFqn = updated.getParent() == null ? null : updated.getParent().getFullyQualifiedName(); + // A glossary term can't be moved under its child + if (newParentFqn != null && FullyQualifiedName.isParent(newParentFqn, fqn)) { + throw new IllegalArgumentException(invalidGlossaryTermMove(fqn, newParentFqn)); + } + } } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/MlModelRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/MlModelRepository.java index f58c6d8b83c..e27a73689c7 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/MlModelRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/MlModelRepository.java @@ -129,7 +129,6 @@ public class MlModelRepository extends EntityRepository { @Override public void prepare(MlModel mlModel) throws IOException { populateService(mlModel); - setFullyQualifiedName(mlModel); if (!nullOrEmpty(mlModel.getMlFeatures())) { validateReferences(mlModel.getMlFeatures()); } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/tags/TagResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/tags/TagResource.java index 44b0dcc7282..193d2a2f8ea 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/tags/TagResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/tags/TagResource.java @@ -498,12 +498,8 @@ public class TagResource extends EntityResource { private Tag getTag(CreateTag create, String updateBy) throws IOException { String parentFQN = create.getParent() != null ? create.getParent() : create.getClassification(); - EntityReference classification = - new EntityReference().withFullyQualifiedName(create.getClassification()).withType(CLASSIFICATION); - EntityReference parent = - create.getParent() == null - ? null - : new EntityReference().withFullyQualifiedName(create.getParent()).withType(TAG); + EntityReference classification = getEntityReference(CLASSIFICATION, create.getClassification()); + EntityReference parent = create.getParent() == null ? null : getEntityReference(TAG, create.getParent()); return copy(new Tag(), create, updateBy) .withFullyQualifiedName(FullyQualifiedName.add(parentFQN, create.getName())) .withParent(parent) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/util/FullyQualifiedName.java b/openmetadata-service/src/main/java/org/openmetadata/service/util/FullyQualifiedName.java index b19c7d98eff..15da02483c1 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/util/FullyQualifiedName.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/util/FullyQualifiedName.java @@ -98,6 +98,11 @@ public class FullyQualifiedName { return split[0]; } + public static boolean isParent(String childFqn, String parentFqn) { + // Returns true if the childFqn is indeed the child of parentFqn + return childFqn.startsWith(parentFqn) && childFqn.length() > parentFqn.length(); + } + private static class SplitListener extends FqnBaseListener { final List list = new ArrayList<>(); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java index eb14bcfd8d5..fd9d5a14825 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java @@ -314,8 +314,17 @@ public class GlossaryResourceTest extends EntityResourceTest glossaryTermResourceTest.moveGlossaryTerm(g.getEntityReference(), t11.getEntityReference(), t1), + Status.BAD_REQUEST, + CatalogExceptionMessage.invalidGlossaryTermMove(t1.getFullyQualifiedName(), t11.getFullyQualifiedName())); } + @Test + void patch_moveGlossaryTermParentToChild() {} + @Test void testCsvDocumentation() throws HttpResponseException { assertEquals(GlossaryCsv.DOCUMENTATION, getCsvDocumentation()); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/util/FullyQualifiedNameTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/util/FullyQualifiedNameTest.java index 46a933ed6a2..a050120f933 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/util/FullyQualifiedNameTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/util/FullyQualifiedNameTest.java @@ -1,8 +1,10 @@ package org.openmetadata.service.util; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.List; import org.antlr.v4.runtime.misc.ParseCancellationException; @@ -96,4 +98,13 @@ class FullyQualifiedNameTest { assertEquals("a", FullyQualifiedName.getRoot("a.b")); assertNull(FullyQualifiedName.getRoot("a")); } + + @Test + void test_isParent() { + assertTrue(FullyQualifiedName.isParent("a.b.c", "a.b")); + assertTrue(FullyQualifiedName.isParent("a.b.c", "a")); + assertFalse(FullyQualifiedName.isParent("a", "a.b.c")); + assertFalse(FullyQualifiedName.isParent("a.b", "a.b.c")); + assertFalse(FullyQualifiedName.isParent("a.b.c", "a.b.c")); + } }