yujunjun/OpenMetadata
1
0
Fork 0
mirror of https://github.com/open-metadata/OpenMetadata.git synced 2025-10-17 03:38:18 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
* 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 <ashish@getcollate.io>
This commit is contained in:
Sriharsha Chintalapani 2024-10-19 14:18:38 -07:00 committed by GitHub
parent 85cc240e1e
commit 9bffbfc3d5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
14 changed files with 303 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
openmetadata-service/src/main/java/org/openmetadata/service/exception/CatalogExceptionMessage.java
View File

@ -318,6 +318,11 @@ public final class CatalogExceptionMessage {
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);

7
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java
View File

@ -51,6 +51,7 @@ import static org.openmetadata.service.Entity.getEntityFields;
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 @@ public abstract class EntityRepository<T extends EntityInterface> {
@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 @@ public abstract class EntityRepository<T extends EntityInterface> {
validateTags(entity.getTags());
entity.setTags(addDerivedTags(entity.getTags()));
checkMutuallyExclusive(entity.getTags());
checkDisabledTags(entity.getTags());
}
protected void validateTags(List<TagLabel> labels) {

14
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TagRepository.java
View File

@ -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;
@ -24,6 +26,7 @@ import java.util.UUID;
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;
@ -62,6 +65,15 @@ public class TagRepository extends EntityRepository<Tag> {
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();
@ -172,7 +184,7 @@ public class TagRepository extends EntityRepository<Tag> {
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);
}

11
openmetadata-service/src/main/java/org/openmetadata/service/resources/tags/TagLabelUtil.java
View File

@ -144,6 +144,17 @@ public class TagLabelUtil {
}
}
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,

2
openmetadata-service/src/main/resources/elasticsearch/en/tag_index_mapping.json
View File

@ -108,7 +108,7 @@
"type": "boolean"
},
"disabled": {
"type": "text"
"type": "boolean"
},
"entityType": {
"type": "keyword"

2
openmetadata-service/src/main/resources/elasticsearch/indexMapping.json
View File

@ -158,7 +158,7 @@
"indexMappingFile": "/elasticsearch/%s/classification_index_mapping.json",
"alias": "classification",
"parentAliases": ["all"],
"childAliases": []
"childAliases": ["tag"]
},
"user": {
"indexName": "user_search_index",

3
openmetadata-service/src/main/resources/elasticsearch/jp/tag_index_mapping.json
View File

@ -99,6 +99,9 @@
"deleted": {
"type": "text"
},
"disabled": {
"type": "boolean"
},
"classification": {
"properties": {
"id": {

3
openmetadata-service/src/main/resources/elasticsearch/zh/tag_index_mapping.json
View File

@ -97,6 +97,9 @@
"deleted": {
"type": "boolean"
},
"disabled": {
"type": "boolean"
},
"classification": {
"properties": {
"id": {

2
openmetadata-service/src/test/java/org/openmetadata/service/resources/EntityResourceTest.java
View File

@ -2893,7 +2893,7 @@ public abstract class EntityResourceTest<T extends EntityInterface, K extends Cr
/**
* 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,

95
openmetadata-service/src/test/java/org/openmetadata/service/resources/tags/TagResourceTest.java
View File

@ -32,6 +32,8 @@ import static org.openmetadata.service.util.TestUtils.assertListNotNull;
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;
@ -50,10 +52,13 @@ import org.junit.jupiter.api.TestInfo;
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;
@ -61,6 +66,7 @@ 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;
@ -174,7 +180,7 @@ public class TagResourceTest extends EntityResourceTest<Tag, CreateTag> {
}
@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;
@ -417,6 +423,93 @@ public class TagResourceTest extends EntityResourceTest<Tag, CreateTag> {
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 {

3
openmetadata-spec/src/main/resources/json/schema/entity/classification/classification.json
View File

@ -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.",

3
openmetadata-spec/src/main/resources/json/schema/entity/classification/tag.json
View File

@ -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.",

148
openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Tags.spec.ts
View File

@ -13,6 +13,8 @@
import { expect, Page, test } from '@playwright/test';
import { SidebarItem } from '../../constant/sidebar';
import { TableClass } from '../../support/entity/TableClass';
import { ClassificationClass } from '../../support/tag/ClassificationClass';
import { TagClass } from '../../support/tag/TagClass';
import {
clickOutside,
createNewPage,
@ -21,7 +23,7 @@ import {
uuid,
} from '../../utils/common';
import { sidebarClick } from '../../utils/sidebar';
import { submitForm, validateForm } from '../../utils/tag';
import { addTagToTableColumn, submitForm, validateForm } from '../../utils/tag';
const NEW_CLASSIFICATION = {
name: `PlaywrightClassification-${uuid()}`,
@ -57,16 +59,26 @@ const permanentDeleteModal = async (page: Page, entity: string) => {
test.use({ storageState: 'playwright/.auth/admin.json' });
const table = new TableClass();
const classification = new ClassificationClass({
provider: 'system',
});
const tag = new TagClass({
classification: classification.data.name,
});
test.beforeAll(async ({ browser }) => {
const { apiContext, afterAction } = await createNewPage(browser);
await table.create(apiContext);
await classification.create(apiContext);
await tag.create(apiContext);
await afterAction();
});
test.afterAll(async ({ browser }) => {
const { apiContext, afterAction } = await createNewPage(browser);
await table.delete(apiContext);
await classification.delete(apiContext);
await tag.delete(apiContext);
await afterAction();
});
@ -101,6 +113,99 @@ test('Classification Page', async ({ page }) => {
expect(headers).toEqual(['Tag', 'Display Name', 'Description', 'Actions']);
});
await test.step('Disabled system tags should not render', async () => {
const classificationResponse = page.waitForResponse(
`/api/v1/tags?*parent=${classification.responseData.name}*`
);
await page
.locator(`[data-testid="side-panel-classification"]`)
.filter({ hasText: classification.responseData.displayName })
.click();
await classificationResponse;
await page.click('[data-testid="manage-button"]');
const fetchTags = page.waitForResponse(
`/api/v1/tags?fields=usageCount&parent=${classification.responseData.name}*`
);
const disabledTag = page.waitForResponse('/api/v1/classifications/*');
await page.click('[data-testid="enable-disable-title"]');
await disabledTag;
await fetchTags;
await expect(
page.locator(
`[data-testid="classification-${classification.responseData.name}"] [data-testid="disabled"]`
)
).toBeVisible();
await expect(
page.locator('[data-testid="add-new-tag-button"]')
).toBeDisabled();
await expect(
page.locator('[data-testid="no-data-placeholder"]')
).toBeVisible();
// Check if the disabled Classification tag is not visible in the table
await table.visitEntityPage(page);
await page.click(
'[data-testid="classification-tags-0"] [data-testid="entity-tags"] [data-testid="add-tag"]'
);
const tagResponse = page.waitForResponse(
`/api/v1/search/query?q=*${tag.responseData.displayName}***`
);
await page.fill(
'[data-testid="tag-selector"] input',
tag.responseData.displayName
);
await tagResponse;
await expect(
page.locator('[data-testid="tag-selector"] > .ant-select-selector')
).toContainText(tag.responseData.displayName);
await expect(
page.getByTestId(
`[data-testid="tag-${tag.responseData.fullyQualifiedName}"]`
)
).not.toBeVisible();
await expect(page.getByTestId('saveAssociatedTag')).not.toBeVisible();
// Re-enable the disabled Classification
await classification.visitPage(page);
await page.click('[data-testid="manage-button"]');
const enableTagResponse = page.waitForResponse('/api/v1/classifications/*');
await page.click('[data-testid="enable-disable-title"]');
await enableTagResponse;
await expect(
page.locator('[data-testid="add-new-tag-button"]')
).not.toBeDisabled();
await expect(
page.locator(
`[data-testid="classification-${classification.responseData.name}"] [data-testid="disabled"]`
)
).not.toBeVisible();
await table.visitEntityPage(page);
await addTagToTableColumn(page, {
tagName: tag.responseData.name,
tagFqn: tag.responseData.fullyQualifiedName,
tagDisplayName: tag.responseData.displayName,
tableId: table.entityResponseData?.['id'],
columnNumber: 1,
rowName: 'shop_id numeric',
});
});
await test.step('Create classification with validation checks', async () => {
await page.click('[data-testid="add-classification"]');
await page.waitForSelector('.ant-modal-content', {
@ -169,41 +274,14 @@ test('Classification Page', async ({ page }) => {
await table.visitEntityPage(page);
const { name, displayName } = NEW_TAG;
await page.click(
'[data-testid="classification-tags-0"] [data-testid="entity-tags"] [data-testid="add-tag"]'
);
await page.fill('[data-testid="tag-selector"] input', name);
await page.click(`[data-testid="tag-${tagFqn}"]`);
await expect(
page.locator('[data-testid="tag-selector"] > .ant-select-selector')
).toContainText(displayName);
const saveAssociatedTag = page.waitForResponse(
(response) =>
response.request().method() === 'PATCH' &&
response
.url()
.includes(`/api/v1/tables/${table.entityResponseData?.['id']}`)
);
await page.click('[data-testid="saveAssociatedTag"]');
await saveAssociatedTag;
await page.waitForSelector('.ant-select-dropdown', {
state: 'detached',
await addTagToTableColumn(page, {
tagName: name,
tagFqn,
tagDisplayName: displayName,
tableId: table.entityResponseData?.['id'],
columnNumber: 0,
rowName: 'user_id numeric',
});
await expect(
page
.getByRole('row', { name: 'user_id numeric Unique' })
.getByTestId('tags-container')
).toContainText(displayName);
await expect(
page.locator(
'[data-testid="classification-tags-0"] [data-testid="tags-container"] [data-testid="icon"]'
)
).toBeVisible();
});
await test.step(

51
openmetadata-ui/src/main/resources/ui/playwright/utils/tag.ts
View File

@ -89,3 +89,54 @@ export async function validateForm(page: Page) {
await expect(page.getByText(NAME_VALIDATION_ERROR)).toBeVisible();
}
export const addTagToTableColumn = async (
page: Page,
{
tagName,
tagFqn,
tagDisplayName,
tableId,
columnNumber,
rowName,
}: {
tagName: string;
tagFqn: string;
tagDisplayName: string;
tableId: string;
columnNumber: number;
rowName: string;
}
) => {
await page.click(
`[data-testid="classification-tags-${columnNumber}"] [data-testid="entity-tags"] [data-testid="add-tag"]`
);
await page.fill('[data-testid="tag-selector"] input', tagName);
await page.click(`[data-testid="tag-${tagFqn}"]`);
await expect(
page.locator('[data-testid="tag-selector"] > .ant-select-selector')
).toContainText(tagDisplayName);
const saveAssociatedTag = page.waitForResponse(
(response) =>
response.request().method() === 'PATCH' &&
response.url().includes(`/api/v1/tables/${tableId}`)
);
await page.click('[data-testid="saveAssociatedTag"]');
await saveAssociatedTag;
await page.waitForSelector('.ant-select-dropdown', {
state: 'detached',
});
await expect(
page.getByRole('row', { name: rowName }).getByTestId('tags-container')
).toContainText(tagDisplayName);
await expect(
page.locator(
`[data-testid="classification-tags-${columnNumber}"] [data-testid="tags-container"] [data-testid="icon"]`
)
).toBeVisible();
};