From 4b12c52662977d0628e7708a19be9728a78d9364 Mon Sep 17 00:00:00 2001 From: Suresh Srinivas Date: Sat, 23 Apr 2022 12:58:22 -0700 Subject: [PATCH] Fixes #4400 Glossary and GlossaryTerm Is not getting deleted with recursive=true (#4418) --- .../exception/CatalogExceptionMessage.java | 4 ++ .../catalog/jdbi3/EntityRepository.java | 2 +- .../catalog/jdbi3/GlossaryTermRepository.java | 8 +-- .../resources/glossary/GlossaryResource.java | 6 ++- .../glossary/GlossaryTermResource.java | 6 ++- .../catalog/resources/EntityResourceTest.java | 5 +- .../glossary/GlossaryTermResourceTest.java | 52 +++++++++++++++++++ 7 files changed, 74 insertions(+), 9 deletions(-) diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java index e1163ad04f4..88e6dac80a7 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java @@ -115,4 +115,8 @@ public final class CatalogExceptionMessage { "Found multiple rules with operation %s within policy %s. Please ensure that operation across all rules within the policy are distinct", operation, policy); } + + public static String entityIsNotEmpty(String entityType) { + return String.format("%s is not empty", entityType); + } } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java index 02bf89fe537..bb75f53c3de 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java @@ -498,7 +498,7 @@ public abstract class EntityRepository { if (!contains.isEmpty()) { if (!recursive) { - throw new IllegalArgumentException(entityType + " is not empty"); + throw new IllegalArgumentException(CatalogExceptionMessage.entityIsNotEmpty(entityType)); } // Soft delete all the contained entities for (EntityReference entityReference : contains) { diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/GlossaryTermRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/GlossaryTermRepository.java index f434c60b244..85eb79454b9 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/GlossaryTermRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/GlossaryTermRepository.java @@ -77,12 +77,12 @@ public class GlossaryTermRepository extends EntityRepository { } private EntityReference getParent(GlossaryTerm entity) throws IOException { - List ids = findFrom(entity.getId(), GLOSSARY_TERM, Relationship.PARENT_OF, GLOSSARY_TERM); + List ids = findFrom(entity.getId(), GLOSSARY_TERM, Relationship.CONTAINS, GLOSSARY_TERM); return ids.size() == 1 ? Entity.getEntityReferenceById(GLOSSARY_TERM, UUID.fromString(ids.get(0)), ALL) : null; } private List getChildren(GlossaryTerm entity) throws IOException { - List ids = findTo(entity.getId(), GLOSSARY_TERM, Relationship.PARENT_OF, GLOSSARY_TERM); + List ids = findTo(entity.getId(), GLOSSARY_TERM, Relationship.CONTAINS, GLOSSARY_TERM); return EntityUtil.populateEntityReferences(ids, GLOSSARY_TERM); } @@ -157,7 +157,7 @@ public class GlossaryTermRepository extends EntityRepository { addRelationship( entity.getGlossary().getId(), entity.getId(), Entity.GLOSSARY, GLOSSARY_TERM, Relationship.CONTAINS); if (entity.getParent() != null) { - addRelationship(entity.getParent().getId(), entity.getId(), GLOSSARY_TERM, GLOSSARY_TERM, Relationship.PARENT_OF); + addRelationship(entity.getParent().getId(), entity.getId(), GLOSSARY_TERM, GLOSSARY_TERM, Relationship.CONTAINS); } for (EntityReference relTerm : listOrEmpty(entity.getRelatedTerms())) { // Make this bidirectional relationship @@ -273,7 +273,7 @@ public class GlossaryTermRepository extends EntityRepository { @Override public EntityReference getContainer() { - return null; + return entity.getParent() == null ? entity.getGlossary() : entity.getParent(); } @Override diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/glossary/GlossaryResource.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/glossary/GlossaryResource.java index 508acb65377..1d62ccf92c8 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/glossary/GlossaryResource.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/glossary/GlossaryResource.java @@ -333,13 +333,17 @@ public class GlossaryResource extends EntityResource extends CatalogApplicationTest { assertResponse( () -> containerTest.deleteEntity(container.getId(), ADMIN_AUTH_HEADERS), BAD_REQUEST, - container.getType() + " is not empty"); + entityIsNotEmpty(container.getType())); // Now soft-delete the container with recursive flag on containerTest.deleteEntity(container.getId(), true, false, ADMIN_AUTH_HEADERS); @@ -1324,7 +1325,7 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { target = recursive ? target.queryParam("recursive", true) : target; target = hardDelete ? target.queryParam("hardDelete", true) : target; T entity = TestUtils.delete(target, entityClass, authHeaders); - assertResponse(() -> getEntity(id, authHeaders), NOT_FOUND, entityNotFound(entityType, id)); + assertEntityDeleted(id, hardDelete); return entity; } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/glossary/GlossaryTermResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/glossary/GlossaryTermResourceTest.java index 16c440f1dd8..c537e65c0b9 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/glossary/GlossaryTermResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/glossary/GlossaryTermResourceTest.java @@ -21,6 +21,9 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.openmetadata.catalog.Entity.FIELD_TAGS; +import static org.openmetadata.catalog.Entity.GLOSSARY; +import static org.openmetadata.catalog.Entity.GLOSSARY_TERM; +import static org.openmetadata.catalog.exception.CatalogExceptionMessage.entityIsNotEmpty; import static org.openmetadata.catalog.exception.CatalogExceptionMessage.glossaryTermMismatch; import static org.openmetadata.catalog.util.TestUtils.ADMIN_AUTH_HEADERS; import static org.openmetadata.catalog.util.TestUtils.UpdateType.MINOR_UPDATE; @@ -231,6 +234,55 @@ public class GlossaryTermResourceTest extends EntityResourceTest glossaryResourceTest.deleteEntity(g1.getId(), ADMIN_AUTH_HEADERS), + BAD_REQUEST, + entityIsNotEmpty(GLOSSARY)); + + // t1 is not empty and can't be deleted + assertResponse(() -> deleteEntity(t1.getId(), ADMIN_AUTH_HEADERS), BAD_REQUEST, entityIsNotEmpty(GLOSSARY_TERM)); + + // t11 is not empty and can't be deleted + assertResponse(() -> deleteEntity(t11.getId(), ADMIN_AUTH_HEADERS), BAD_REQUEST, entityIsNotEmpty(GLOSSARY_TERM)); + + // + // Glossary that has terms and glossary terms that have children CAN BE DELETED recursive flag + // + deleteAndCheckEntity(t11, true, true, ADMIN_AUTH_HEADERS); // Delete both t11 and the child t11 + assertEntityDeleted(t11.getId(), true); + assertEntityDeleted(t111.getId(), true); + + glossaryResourceTest.deleteAndCheckEntity(g1, true, true, ADMIN_AUTH_HEADERS); // Delete the entire glossary + glossaryResourceTest.assertEntityDeleted(g1.getId(), true); + assertEntityDeleted(t1.getId(), true); + } + public GlossaryTerm createTerm(Glossary glossary, GlossaryTerm parent, String termName) throws HttpResponseException { EntityReference glossaryRef = new GlossaryEntityInterface(glossary).getEntityReference(); EntityReference parentRef = parent != null ? new GlossaryTermEntityInterface(parent).getEntityReference() : null;