Fix #18007: Disabled Classifications or Tags shouldn't be visible in …

…UI (#18242) * Fix #18007: Disabled Classifications or Tags shouldn't be visible in UI * added playwright test for disabled tags should not be visible while search * replace testing tag to new generated one to avoid flakyness * added test for checking tags are re-enabling it from disabled state * fix the playwright test for the wrong column selector --------- Co-authored-by: Ashish Gupta <[email protected]>
open-metadata · Oct 19, 2024 · fff3eea · fff3eea
1 parent 5f0b1c0
commit fff3eea
Show file tree

Hide file tree

Showing 14 changed files with 296 additions and 47 deletions.
diff --git a/...ata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java b/...ata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java
@@ -318,6 +318,11 @@ public static String mutuallyExclusiveLabels(TagLabel tag1, TagLabel tag2) {
         tag1.getTagFQN(), tag2.getTagFQN());
   }
 
+  public static String disabledTag(TagLabel tag) {
+    return String.format(
+        "Tag label %s is disabled and can't be assigned to a data asset.", tag.getTagFQN());
+  }
+
   public static String csvNotSupported(String entityType) {
     return String.format(
         "Upload/download CSV for bulk operations is not supported for entity [%s]", entityType);

diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java
@@ -51,6 +51,7 @@
 import static org.openmetadata.service.exception.CatalogExceptionMessage.csvNotSupported;
 import static org.openmetadata.service.exception.CatalogExceptionMessage.entityNotFound;
 import static org.openmetadata.service.resources.tags.TagLabelUtil.addDerivedTags;
+import static org.openmetadata.service.resources.tags.TagLabelUtil.checkDisabledTags;
 import static org.openmetadata.service.resources.tags.TagLabelUtil.checkMutuallyExclusive;
 import static org.openmetadata.service.util.EntityUtil.compareTagLabel;
 import static org.openmetadata.service.util.EntityUtil.entityReferenceMatch;
@@ -1608,10 +1609,7 @@ protected void applyTags(T entity) {
   @Transaction
   public final void applyTags(List<TagLabel> tagLabels, String targetFQN) {
     for (TagLabel tagLabel : listOrEmpty(tagLabels)) {
-      // Apply tagLabel to targetFQN that identifies an entity or field
-      boolean isTagDerived = tagLabel.getLabelType().equals(TagLabel.LabelType.DERIVED);
-      // Derived Tags should not create Relationships, and needs to be built on the during Read
-      if (!isTagDerived) {
+      if (!tagLabel.getLabelType().equals(TagLabel.LabelType.DERIVED)) {
         daoCollection
             .tagUsageDAO()
             .applyTag(
@@ -2323,6 +2321,7 @@ protected void validateTags(T entity) {
     validateTags(entity.getTags());
     entity.setTags(addDerivedTags(entity.getTags()));
     checkMutuallyExclusive(entity.getTags());
+    checkDisabledTags(entity.getTags());
   }
 
   protected void validateTags(List<TagLabel> labels) {

diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TagRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TagRepository.java
@@ -67,7 +67,11 @@ public void prepare(Tag entity, boolean update) {
 
   @Override
   public void setInheritedFields(Tag tag, Fields fields) {
-    tag.setInheritedRoles(getInheritedRoles(tag));
+    Classification parent =  Entity.getEntity(CLASSIFICATION, tag.getClassification().getId(), "roles", ALL);
+    if (parent.getDisabled() != null && parent.getDisabled()) {
+      tag.setDisabled(true);
+    }
+    tag.setInheritedRoles(classification.getRoles());
   }
 
   @Override
@@ -189,7 +193,7 @@ public TagUpdater(Tag original, Tag updated, Operation operation) {
     public void entitySpecificUpdate() {
       recordChange(
           "mutuallyExclusive", original.getMutuallyExclusive(), updated.getMutuallyExclusive());
-      recordChange("disabled,", original.getDisabled(), updated.getDisabled());
+      recordChange("disabled", original.getDisabled(), updated.getDisabled());
       updateName(original, updated);
       updateParent(original, updated);
     }

diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/tags/TagLabelUtil.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/tags/TagLabelUtil.java
@@ -144,6 +144,17 @@ public static void checkMutuallyExclusive(List<TagLabel> tagLabels) {
     }
   }
 
+  public static void checkDisabledTags(List<TagLabel> tagLabels) {
+    for (TagLabel tagLabel : listOrEmpty(tagLabels)) {
+      if (tagLabel.getSource().equals(TagSource.CLASSIFICATION)) {
+        Tag tag = Entity.getCollectionDAO().tagDAO().findEntityByName(tagLabel.getTagFQN());
+        if (tag.getDisabled()) {
+          throw new IllegalArgumentException(CatalogExceptionMessage.disabledTag(tagLabel));
+        }
+      }
+    }
+  }
+
   public static void checkMutuallyExclusiveForParentAndSubField(
       String assetFqn,
       String assetFqnHash,

diff --git a/openmetadata-service/src/main/resources/elasticsearch/en/tag_index_mapping.json b/openmetadata-service/src/main/resources/elasticsearch/en/tag_index_mapping.json
@@ -108,7 +108,7 @@
         "type": "boolean"
       },
       "disabled": {
-        "type": "text"
+        "type": "boolean"
       },
       "entityType": {
         "type": "keyword"

diff --git a/openmetadata-service/src/main/resources/elasticsearch/indexMapping.json b/openmetadata-service/src/main/resources/elasticsearch/indexMapping.json
@@ -158,7 +158,7 @@
     "indexMappingFile": "/elasticsearch/%s/classification_index_mapping.json",
     "alias": "classification",
     "parentAliases": ["all"],
-    "childAliases": []
+    "childAliases": ["tag"]
   },
   "user": {
     "indexName": "user_search_index",

diff --git a/openmetadata-service/src/main/resources/elasticsearch/jp/tag_index_mapping.json b/openmetadata-service/src/main/resources/elasticsearch/jp/tag_index_mapping.json
@@ -99,6 +99,9 @@
         "deleted": {
           "type": "text"
         },
+        "disabled": {
+          "type": "boolean"
+        },
         "classification": {
           "properties": {
             "id": {

diff --git a/openmetadata-service/src/main/resources/elasticsearch/zh/tag_index_mapping.json b/openmetadata-service/src/main/resources/elasticsearch/zh/tag_index_mapping.json
@@ -97,6 +97,9 @@
       "deleted": {
         "type": "boolean"
       },
+      "disabled": {
+        "type": "boolean"
+      },
       "classification": {
         "properties": {
           "id": {

diff --git a/...metadata-service/src/test/java/org/openmetadata/service/resources/EntityResourceTest.java b/...metadata-service/src/test/java/org/openmetadata/service/resources/EntityResourceTest.java
@@ -2893,7 +2893,7 @@ protected void validateLatestVersion(
   /**
    * Helper function to generate JSON PATCH, submit PATCH API request and validate response.
    */
-  protected final T patchEntityAndCheck(
+  public final T patchEntityAndCheck(
       T updated,
       String originalJson,
       Map<String, String> authHeaders,

diff --git a/...tadata-service/src/test/java/org/openmetadata/service/resources/tags/TagResourceTest.java b/...tadata-service/src/test/java/org/openmetadata/service/resources/tags/TagResourceTest.java
@@ -35,6 +35,8 @@
 import static org.openmetadata.service.util.TestUtils.assertListNull;
 import static org.openmetadata.service.util.TestUtils.assertResponse;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.flipkart.zjsonpatch.JsonDiff;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -53,17 +55,21 @@
 import org.junit.jupiter.api.TestMethodOrder;
 import org.openmetadata.schema.api.classification.CreateClassification;
 import org.openmetadata.schema.api.classification.CreateTag;
+import org.openmetadata.schema.api.data.CreateTable;
 import org.openmetadata.schema.entity.classification.Classification;
 import org.openmetadata.schema.entity.classification.Tag;
 import org.openmetadata.schema.entity.type.Style;
 import org.openmetadata.schema.type.ChangeDescription;
+import org.openmetadata.schema.type.Column;
+import org.openmetadata.schema.type.ColumnDataType;
 import org.openmetadata.schema.type.EntityReference;
 import org.openmetadata.schema.type.Include;
 import org.openmetadata.schema.type.ProviderType;
 import org.openmetadata.schema.type.TagLabel;
 import org.openmetadata.service.Entity;
 import org.openmetadata.service.exception.CatalogExceptionMessage;
 import org.openmetadata.service.resources.EntityResourceTest;
+import org.openmetadata.service.resources.databases.TableResourceTest;
 import org.openmetadata.service.resources.tags.TagResource.TagList;
 import org.openmetadata.service.util.EntityUtil;
 import org.openmetadata.service.util.FullyQualifiedName;
@@ -177,7 +183,7 @@ void delete_systemTag() throws HttpResponseException {
   }
 
   @Test
-  void get_TagsWithPagination_200(TestInfo test) throws IOException {
+  void get_TagsWithPagination_200() throws IOException {
     // get Pagination results for same name entities
     boolean supportsSoftDelete = true;
     int numEntities = 5;
@@ -451,6 +457,93 @@ public Tag updateAndCheckEntity(
     return tag;
   }
 
+  @Test
+  void test_disableClassification_disablesAllTags() throws IOException {
+    String classificationName = "TestClassification";
+    CreateClassification createClassification =
+        classificationResourceTest.createRequest(classificationName);
+    Classification classification =
+        classificationResourceTest.createAndCheckEntity(createClassification, ADMIN_AUTH_HEADERS);
+
+    String tagName1 = "Tag1";
+    String tagName2 = "Tag2";
+    CreateTag createTag1 = createRequest(tagName1).withClassification(classificationName);
+    CreateTag createTag2 = createRequest(tagName2).withClassification(classificationName);
+    Tag tag1 = createEntity(createTag1, ADMIN_AUTH_HEADERS);
+    Tag tag2 = createEntity(createTag2, ADMIN_AUTH_HEADERS);
+
+    Tag getTag1 = getEntity(tag1.getId(), ADMIN_AUTH_HEADERS);
+    Tag getTag2 = getEntity(tag2.getId(), ADMIN_AUTH_HEADERS);
+    assertFalse(getTag1.getDisabled(), "Tag1 should not be disabled");
+    assertFalse(getTag2.getDisabled(), "Tag2 should not be disabled");
+
+    String classificationJson = JsonUtils.pojoToJson(classification);
+    classification.setDisabled(true);
+    ChangeDescription change = getChangeDescription(classification, MINOR_UPDATE);
+    fieldUpdated(change, "disabled", false, true);
+    classification =
+        classificationResourceTest.patchEntityAndCheck(
+            classification,
+            classificationJson,
+            ADMIN_AUTH_HEADERS,
+            UpdateType.MINOR_UPDATE,
+            change);
+
+    getTag1 = getEntity(tag1.getId(), ADMIN_AUTH_HEADERS);
+    getTag2 = getEntity(tag2.getId(), ADMIN_AUTH_HEADERS);
+    assertTrue(
+        getTag1.getDisabled(), "Tag1 should be disabled because its Classification is disabled");
+    assertTrue(
+        getTag2.getDisabled(), "Tag2 should be disabled because its Classification is disabled");
+
+    classificationJson = JsonUtils.pojoToJson(classification);
+    ObjectMapper mapper = new ObjectMapper();
+    classification.setDisabled(false);
+    classificationResourceTest.patchEntity(
+        classification.getId(),
+        JsonDiff.asJson(
+            mapper.readTree(classificationJson),
+            mapper.readTree(JsonUtils.pojoToJson(classification))),
+        ADMIN_AUTH_HEADERS);
+
+    getTag1 = getEntity(tag1.getId(), ADMIN_AUTH_HEADERS);
+    getTag2 = getEntity(tag2.getId(), ADMIN_AUTH_HEADERS);
+    assertFalse(
+        getTag1.getDisabled(), "Tag1 should not be disabled after Classification is enabled");
+    assertFalse(
+        getTag2.getDisabled(), "Tag2 should not be disabled after Classification is enabled");
+
+    CreateTag createTag = createRequest("SingleTag").withClassification(classificationName);
+    Tag getTag = createEntity(createTag, ADMIN_AUTH_HEADERS);
+
+    getTag = getEntity(getTag.getId(), ADMIN_AUTH_HEADERS);
+    assertFalse(getTag.getDisabled(), "Tag should not be disabled initially");
+
+    String tagJson = JsonUtils.pojoToJson(getTag);
+    ChangeDescription change1 = getChangeDescription(getTag, MINOR_UPDATE);
+    getTag.setDisabled(true);
+    fieldUpdated(change1, "disabled", false, true);
+    getTag =
+        patchEntityAndCheck(getTag, tagJson, ADMIN_AUTH_HEADERS, UpdateType.MINOR_UPDATE, change);
+
+    getTag = getEntity(getTag.getId(), ADMIN_AUTH_HEADERS);
+    assertTrue(getTag.getDisabled(), "Tag should be disabled after update");
+
+    CreateTable createTable =
+        new CreateTable()
+            .withName("TestTable")
+            .withDatabaseSchema(DATABASE_SCHEMA.getFullyQualifiedName())
+            .withColumns(List.of(new Column().withName("column1").withDataType(ColumnDataType.INT)))
+            .withTags(List.of(new TagLabel().withTagFQN(getTag.getFullyQualifiedName())));
+    TableResourceTest tableResourceTest = new TableResourceTest();
+
+    assertResponse(
+        () -> tableResourceTest.createEntity(createTable, ADMIN_AUTH_HEADERS),
+        BAD_REQUEST,
+        CatalogExceptionMessage.disabledTag(
+            new TagLabel().withTagFQN(getTag.getFullyQualifiedName())));
+  }
+
   public Tag createTag(
       String name, String classification, String parentFqn, String... associatedTags)
       throws IOException {

diff --git a/openmetadata-spec/src/main/resources/json/schema/entity/classification/classification.json b/openmetadata-spec/src/main/resources/json/schema/entity/classification/classification.json
@@ -72,7 +72,8 @@
     },
     "disabled" : {
       "description": "System classifications can't be deleted. Use this flag to disable them.",
-      "type": "boolean"
+      "type": "boolean",
+      "default": false
     },
     "mutuallyExclusive" : {
       "description" : "Tags under this classification are mutually exclusive. When mutually exclusive is `true` the tags from this classification are used to **classify** an entity. An entity can only be in one class - example, it can only be either `tier1` or `tier2` and not both. When mutually exclusive is `false`, the tags from this classification are used to **categorize** an entity. An entity have multiple tags simultaneously - example a customer can be `newCustomer` and `atRisk` simultaneously.",

diff --git a/openmetadata-spec/src/main/resources/json/schema/entity/classification/tag.json b/openmetadata-spec/src/main/resources/json/schema/entity/classification/tag.json
@@ -88,7 +88,8 @@
     },
     "disabled": {
       "description": "System tags can't be deleted. Use this flag to disable them.",
-      "type": "boolean"
+      "type": "boolean",
+      "default": false
     },
     "mutuallyExclusive": {
       "description": "Children tags under this group are mutually exclusive. When mutually exclusive is `true` the tags from this group are used to **classify** an entity. An entity can only be in one class - example, it can only be either `tier1` or `tier2` and not both. When mutually exclusive is `false`, the tags from this group are used to **categorize** an entity. An entity can be in multiple categories simultaneously - example a customer can be `newCustomer` and `atRisk` simultaneously.",