From fd30229f4cfc605fbbc5d5648e16cc5755fcdc54 Mon Sep 17 00:00:00 2001 From: Suresh Srinivas Date: Tue, 22 Feb 2022 23:45:02 -0800 Subject: [PATCH] Fixes #2899 - Not getting proper relation between parent and child glossary-terms (#2941) --- .../catalog/jdbi3/GlossaryTermRepository.java | 8 +-- .../catalog/jdbi3/TableRepository.java | 1 - .../glossary/GlossaryTermResource.java | 1 + .../openmetadata/catalog/util/EntityUtil.java | 4 ++ .../json/schema/entity/data/glossary.json | 8 +-- .../json/schema/entity/data/glossaryTerm.json | 28 ++++---- .../catalog/resources/EntityResourceTest.java | 2 +- .../glossary/GlossaryTermResourceTest.java | 64 ++++++++++++------- 8 files changed, 67 insertions(+), 49 deletions(-) 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 5047bc3801e..09ce8461eb9 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 @@ -72,14 +72,14 @@ public class GlossaryTermRepository extends EntityRepository { private EntityReference getParent(GlossaryTerm entity) throws IOException { List ids = - findFrom(entity.getId(), Entity.GLOSSARY_TERM, Relationship.PARENT_OF, Entity.GLOSSARY, entity.getDeleted()); + findFrom( + entity.getId(), Entity.GLOSSARY_TERM, Relationship.PARENT_OF, Entity.GLOSSARY_TERM, entity.getDeleted()); return ids.size() == 1 ? Entity.getEntityReference(Entity.GLOSSARY_TERM, UUID.fromString(ids.get(0))) : null; } private List getChildren(GlossaryTerm entity) throws IOException { List ids = - findBoth( - entity.getId(), Entity.GLOSSARY_TERM, Relationship.PARENT_OF, Entity.GLOSSARY_TERM, entity.getDeleted()); + findTo(entity.getId(), Entity.GLOSSARY_TERM, Relationship.PARENT_OF, Entity.GLOSSARY_TERM, entity.getDeleted()); List children = new ArrayList<>(); for (String id : ids) { children.add(Entity.getEntityReference(Entity.GLOSSARY_TERM, UUID.fromString(id))); @@ -120,7 +120,7 @@ public class GlossaryTermRepository extends EntityRepository { entity.setFullyQualifiedName(entity.getGlossary().getName() + "." + entity.getName()); } else { EntityReference parent = Entity.getEntityReference(entity.getParent()); - entity.setFullyQualifiedName(entity.getParent().getName() + "." + entity.getName()); + entity.setFullyQualifiedName(parent.getName() + "." + entity.getName()); entity.setParent(parent); } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java index 2f4ed902330..cadec2fb9d7 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java @@ -367,7 +367,6 @@ public class TableRepository extends EntityRepository { @Override public void storeRelationships(Table table) { // Add relationship from database to table - String databaseId = table.getDatabase().getId().toString(); addRelationship(table.getDatabase().getId(), table.getId(), DATABASE, TABLE, Relationship.CONTAINS); // Add table owner relationship diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/glossary/GlossaryTermResource.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/glossary/GlossaryTermResource.java index b8fbdfecc5d..ef2cc9b7603 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/glossary/GlossaryTermResource.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/glossary/GlossaryTermResource.java @@ -92,6 +92,7 @@ public class GlossaryTermResource { term.withHref(RestUtil.getHref(uriInfo, COLLECTION_PATH, term.getId())); Entity.withHref(uriInfo, term.getGlossary()); Entity.withHref(uriInfo, term.getParent()); + Entity.withHref(uriInfo, term.getChildren()); Entity.withHref(uriInfo, term.getRelatedTerms()); Entity.withHref(uriInfo, term.getReviewers()); return term; diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java index f7e41e5f6e6..c6c76c61ebb 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java @@ -34,6 +34,7 @@ import lombok.extern.slf4j.Slf4j; import org.joda.time.Period; import org.joda.time.format.ISOPeriodFormat; import org.openmetadata.catalog.Entity; +import org.openmetadata.catalog.entity.data.GlossaryTerm; import org.openmetadata.catalog.entity.teams.Team; import org.openmetadata.catalog.entity.teams.User; import org.openmetadata.catalog.exception.CatalogExceptionMessage; @@ -123,6 +124,9 @@ public final class EntityUtil { (filter1, filter2) -> filter1.getEventType().equals(filter2.getEventType()) && filter1.getEntities().equals(filter2.getEntities()); + public static final BiPredicate glossaryTermMatch = + (filter1, filter2) -> filter1.getFullyQualifiedName().equals(filter2.getFullyQualifiedName()); + private EntityUtil() {} /** Validate Ingestion Schedule */ diff --git a/catalog-rest-service/src/main/resources/json/schema/entity/data/glossary.json b/catalog-rest-service/src/main/resources/json/schema/entity/data/glossary.json index c4c5daa75f8..0f489acf295 100644 --- a/catalog-rest-service/src/main/resources/json/schema/entity/data/glossary.json +++ b/catalog-rest-service/src/main/resources/json/schema/entity/data/glossary.json @@ -75,8 +75,6 @@ "default": false } }, - "required": [ - "id", - "name" - ] -} \ No newline at end of file + "required": ["id", "name"], + "additionalProperties": false +} diff --git a/catalog-rest-service/src/main/resources/json/schema/entity/data/glossaryTerm.json b/catalog-rest-service/src/main/resources/json/schema/entity/data/glossaryTerm.json index 476b74334f6..d7a04f9765f 100644 --- a/catalog-rest-service/src/main/resources/json/schema/entity/data/glossaryTerm.json +++ b/catalog-rest-service/src/main/resources/json/schema/entity/data/glossaryTerm.json @@ -23,10 +23,11 @@ "type": "string", "format": "uri" } - } + }, + "additionalProperties": false }, - "status" : { - "type" : "string", + "status": { + "type": "string", "enum": ["Draft", "Approved", "Deprecated"], "default": "Draft" } @@ -62,12 +63,12 @@ } }, "glossary": { - "description": "Glosary that this term belongs to.", - "$ref" : "../../type/entityReference.json" + "description": "Glossary that this term belongs to.", + "$ref": "../../type/entityReference.json" }, "parent": { "description": "Parent glossary term that this term is child of. When `null` this term is the root term of the glossary.", - "$ref" : "../../type/entityReference.json" + "$ref": "../../type/entityReference.json" }, "children": { "description": "Other glossary terms that are children of this glossary term.", @@ -117,9 +118,9 @@ "description": "Change that lead to this version of the entity.", "$ref": "../../type/entityHistory.json#/definitions/changeDescription" }, - "status" : { - "description" : "Status of the glossary term.", - "$ref" : "#/definitions/status" + "status": { + "description": "Status of the glossary term.", + "$ref": "#/definitions/status" }, "deleted": { "description": "When `true` indicates the entity has been soft deleted.", @@ -127,9 +128,6 @@ "default": false } }, - "required": [ - "id", - "name", - "glossary" - ] -} \ No newline at end of file + "required": ["id", "name", "glossary"], + "additionalProperties": false +} diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java index a8a657f3913..a70d09859ac 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java @@ -763,7 +763,7 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { String[] split = entityInterface.getFullyQualifiedName().split("\\."); String actualName = split[split.length - 1]; assertTrue(actualName.contains("foo_DOT_bar")); - assertTrue(!actualName.contains("foo.bar")); + assertFalse(actualName.contains("foo.bar")); } /////////////////////////////////////////////////////////////////////////////////////////////////////////////////// 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 ccbe785ef40..fc32536a3fe 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 @@ -18,6 +18,7 @@ package org.openmetadata.catalog.resources.glossary; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; 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.exception.CatalogExceptionMessage.glossaryTermMismatch; import static org.openmetadata.catalog.util.TestUtils.ADMIN_AUTH_HEADERS; @@ -114,8 +115,12 @@ public class GlossaryTermResourceTest extends EntityResourceTest list = listEntities(null, ADMIN_AUTH_HEADERS); - List expectedTerms = - Arrays.asList( - GLOSSARY_TERM1.getName(), - GLOSSARY_TERM2.getName(), - "term1", - "term11", - "term12", - "term2", - "term21", - "term22"); - assertContains(list, expectedTerms); + Map queryParams = new HashMap<>(); + queryParams.put("fields", "children,relatedTerms,reviewers,tags"); + ResultList list = listEntities(queryParams, ADMIN_AUTH_HEADERS); + List expectedTerms = + Arrays.asList(GLOSSARY_TERM1, GLOSSARY_TERM2, term1, term11, term12, term2, term21, term22); + assertContains(expectedTerms, list.getData()); // List terms under glossary1 - Map queryParams = new HashMap<>(); + queryParams = new HashMap<>(); + queryParams.put("fields", "children,relatedTerms,reviewers,tags"); queryParams.put("glossary", glossary1.getId().toString()); list = listEntities(queryParams, ADMIN_AUTH_HEADERS); - assertContains(list, Arrays.asList("term1", "term11", "term12")); + assertContains(Arrays.asList(term1, term11, term12), list.getData()); // List terms under glossary1 parent term1 queryParams = new HashMap<>(); + queryParams.put("fields", "children,relatedTerms,reviewers,tags"); queryParams.put("glossary", glossary1.getId().toString()); queryParams.put("parent", term1.getId().toString()); list = listEntities(queryParams, ADMIN_AUTH_HEADERS); - assertContains(list, Arrays.asList("term11", "term12")); + assertContains(Arrays.asList(term11, term12), list.getData()); // List terms under glossary2 queryParams = new HashMap<>(); + queryParams.put("fields", "children,relatedTerms,reviewers,tags"); queryParams.put("glossary", glossary2.getId().toString()); list = listEntities(queryParams, ADMIN_AUTH_HEADERS); - assertContains(list, Arrays.asList("term2", "term21", "term22")); + assertContains(Arrays.asList(term2, term21, term22), list.getData()); // List terms under glossary 2 but give glossary term1 in glossary 1 as parent + queryParams.put("fields", "children,relatedTerms,reviewers,tags"); queryParams.put("parent", term1.getId().toString()); Map map = Collections.unmodifiableMap(queryParams); assertResponse( @@ -179,9 +186,20 @@ public class GlossaryTermResourceTest extends EntityResourceTest list, List expectedTerm) { - assertEquals(expectedTerm.size(), list.getData().size()); - list.getData().forEach(term -> assertTrue(expectedTerm.contains(term.getName()))); + public void assertContains(List expectedTerms, List actualTerms) + throws HttpResponseException { + assertEquals(expectedTerms.size(), actualTerms.size()); + for (GlossaryTerm expected : expectedTerms) { + GlossaryTerm actual = + actualTerms.stream().filter(a -> EntityUtil.glossaryTermMatch.test(a, expected)).findAny().orElse(null); + assertNotNull(actual, "Expected glossaryTerm " + expected.getFullyQualifiedName() + " not found"); + assertEquals(expected.getFullyQualifiedName(), actual.getFullyQualifiedName()); + assertEquals(expected.getSynonyms(), actual.getSynonyms()); + assertEquals(expected.getParent(), actual.getParent()); + assertEntityReferenceList(expected.getChildren(), actual.getChildren()); + assertEntityReferenceList(expected.getReviewers(), actual.getReviewers()); + TestUtils.validateTags(expected.getTags(), actual.getTags()); + } } @Override