Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Oct 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1614,10 +1615,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(
Expand Down Expand Up @@ -2329,6 +2327,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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@

package org.openmetadata.service.jdbi3;

import static org.openmetadata.schema.type.Include.ALL;
import static org.openmetadata.schema.type.Include.NON_DELETED;
import static org.openmetadata.service.Entity.CLASSIFICATION;
import static org.openmetadata.service.Entity.TAG;
import static org.openmetadata.service.util.EntityUtil.entityReferenceMatch;
import static org.openmetadata.service.util.EntityUtil.getId;
Expand All @@ -24,6 +26,7 @@
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.tuple.ImmutablePair;
import org.jdbi.v3.sqlobject.transaction.Transaction;
import org.openmetadata.schema.entity.classification.Classification;
import org.openmetadata.schema.entity.classification.Tag;
import org.openmetadata.schema.type.EntityReference;
import org.openmetadata.schema.type.ProviderType;
Expand Down Expand Up @@ -62,6 +65,15 @@ public void prepare(Tag entity, boolean update) {
entity.setClassification(classification);
}

@Override
public void setInheritedFields(Tag tag, Fields fields) {
Classification parent =
Entity.getEntity(CLASSIFICATION, tag.getClassification().getId(), "", ALL);
if (parent.getDisabled() != null && parent.getDisabled()) {
tag.setDisabled(true);
}
}

@Override
public void storeEntity(Tag tag, boolean update) {
EntityReference classification = tag.getClassification();
Expand Down Expand Up @@ -172,7 +184,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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@
"type": "boolean"
},
"disabled": {
"type": "text"
"type": "boolean"
},
"entityType": {
"type": "keyword"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@
"indexMappingFile": "/elasticsearch/%s/classification_index_mapping.json",
"alias": "classification",
"parentAliases": ["all"],
"childAliases": []
"childAliases": ["tag"]
},
"user": {
"indexName": "user_search_index",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@
"deleted": {
"type": "text"
},
"disabled": {
"type": "boolean"
},
"classification": {
"properties": {
"id": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@
"deleted": {
"type": "boolean"
},
"disabled": {
"type": "boolean"
},
"classification": {
"properties": {
"id": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,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;
Expand All @@ -50,17 +52,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;
Expand Down Expand Up @@ -174,7 +180,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;
Expand Down Expand Up @@ -417,6 +423,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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,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.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,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.",
Expand Down
Loading
Loading