Skip to content

Commit c43b76d

Browse files
siddhant1Siddhantclaude
authored andcommitted
fix(ui): preserve TagLabel server fields when saving tags (#28038) (#28105)
* fix(ui): preserve TagLabel server fields when saving tags TagsContainerV2.handleSave rebuilt each tag from an 8-field allowlist that silently dropped appliedBy, appliedAt, metadata, and reason — added to the TagLabel schema in #24817. The resulting JSON-Patch diff emitted spurious `remove /tags/N/appliedBy` ops, which the backend rejected with 500 when the path no longer existed at apply time (closes #28038). Pass every TagLabel schema field on the option payload through to the PATCH so server-managed fields survive the diff. The Jest schema-coverage test uses `Required<TagLabel>` so adding a new field to tagLabel.json forces the test fixture and assertion to cover it, preventing the same class of bug from recurring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(ui): drop unused TagLabel/EntityTags imports from TagsContainerV2 The two `as` casts those imports backed were removed; TypeScript inference covers the remaining flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Siddhant <siddhant@MacBook-Pro-751.local> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit bba0bf8)
1 parent 6eed724 commit c43b76d

3 files changed

Lines changed: 403 additions & 19 deletions

File tree

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

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -700,3 +700,123 @@ test('Disabled tag should not allow adding assets from Assets tab', async ({
700700
await afterAction();
701701
}
702702
});
703+
704+
test('Adds one tag and removes another in the same save preserves appliedBy on the kept tag', async ({
705+
browser,
706+
page,
707+
}) => {
708+
const { apiContext, afterAction } = await createNewPage(browser);
709+
const fixtureTable = new TableClass();
710+
const addedTag = new TagClass({ classification: classification.data.name });
711+
712+
try {
713+
await fixtureTable.create(apiContext);
714+
await addedTag.create(apiContext);
715+
716+
const keptTagFqn = tag.responseData.fullyQualifiedName;
717+
const removedTagFqn = tag1.responseData.fullyQualifiedName;
718+
const addedTagFqn = addedTag.responseData.fullyQualifiedName;
719+
720+
await fixtureTable.patch({
721+
apiContext,
722+
patchData: [
723+
{
724+
op: 'add',
725+
path: '/tags',
726+
value: [
727+
{
728+
tagFQN: keptTagFqn,
729+
source: 'Classification',
730+
labelType: 'Manual',
731+
state: 'Confirmed',
732+
},
733+
{
734+
tagFQN: removedTagFqn,
735+
source: 'Classification',
736+
labelType: 'Manual',
737+
state: 'Confirmed',
738+
},
739+
],
740+
},
741+
],
742+
});
743+
744+
const seededResponse = await apiContext.get(
745+
`/api/v1/tables/${fixtureTable.entityResponseData?.id}?fields=tags`
746+
);
747+
const seededBody = await seededResponse.json();
748+
const seededKept = (
749+
seededBody.tags as { tagFQN: string; appliedBy?: string }[]
750+
).find((t) => t.tagFQN === keptTagFqn);
751+
752+
expect(seededKept?.appliedBy).toBeTruthy();
753+
754+
await fixtureTable.visitEntityPage(page);
755+
756+
const tagsPanel = page
757+
.getByTestId('KnowledgePanel.Tags')
758+
.getByTestId('tags-container');
759+
760+
await expect(tagsPanel.getByTestId(`tag-${keptTagFqn}`)).toBeVisible();
761+
await expect(tagsPanel.getByTestId(`tag-${removedTagFqn}`)).toBeVisible();
762+
763+
await tagsPanel.getByTestId('edit-button').first().click();
764+
765+
await expect(page.locator('#tagsForm_tags')).toBeVisible();
766+
767+
await page
768+
.getByTestId('tag-selector')
769+
.getByTestId(`selected-tag-${removedTagFqn}`)
770+
.getByTestId('remove-tags')
771+
.locator('svg')
772+
.click();
773+
774+
await page.locator('#tagsForm_tags').click();
775+
await page.locator('#tagsForm_tags').fill(addedTag.data.name);
776+
777+
await expect(page.getByTestId(`tag-${addedTagFqn}`).first()).toBeVisible();
778+
await page.getByTestId(`tag-${addedTagFqn}`).first().click();
779+
780+
await page
781+
.locator('.ant-select-dropdown')
782+
.getByTestId('saveAssociatedTag')
783+
.waitFor({ state: 'visible' });
784+
785+
const patchResponse = page.waitForResponse(
786+
(response) =>
787+
response.url().includes('/api/v1/tables/') &&
788+
response.request().method() === 'PATCH'
789+
);
790+
791+
await page.getByTestId('saveAssociatedTag').click();
792+
793+
const response = await patchResponse;
794+
795+
expect(response.status()).toBe(200);
796+
797+
const patchedTable = await response.json();
798+
const patchedTags = (patchedTable.tags ?? []) as {
799+
tagFQN: string;
800+
appliedBy?: string;
801+
}[];
802+
const patchedFqns = patchedTags.map((t) => t.tagFQN);
803+
804+
expect(patchedFqns).toContain(keptTagFqn);
805+
expect(patchedFqns).toContain(addedTagFqn);
806+
expect(patchedFqns).not.toContain(removedTagFqn);
807+
808+
const survivingTag = patchedTags.find((t) => t.tagFQN === keptTagFqn);
809+
810+
expect(survivingTag?.appliedBy).toBe(seededKept?.appliedBy);
811+
812+
await expect(tagsPanel.getByTestId(`tag-${keptTagFqn}`)).toBeVisible();
813+
await expect(tagsPanel.getByTestId(`tag-${addedTagFqn}`)).toBeVisible();
814+
await expect(
815+
tagsPanel.getByTestId(`tag-${removedTagFqn}`)
816+
).not.toBeVisible();
817+
} finally {
818+
await addedTag.delete(apiContext);
819+
await fixtureTable.delete(apiContext);
820+
await afterAction();
821+
}
822+
});
Lines changed: 264 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,264 @@
1+
/*
2+
* Copyright 2026 Collate.
3+
* Licensed under the Apache License, Version 2.0 (the "License");
4+
* you may not use this file except in compliance with the License.
5+
* You may obtain a copy of the License at
6+
* http://www.apache.org/licenses/LICENSE-2.0
7+
* Unless required by applicable law or agreed to in writing, software
8+
* distributed under the License is distributed on an "AS IS" BASIS,
9+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
10+
* See the License for the specific language governing permissions and
11+
* limitations under the License.
12+
*/
13+
14+
import { act, fireEvent, render, screen } from '@testing-library/react';
15+
import { EntityTags } from 'Models';
16+
import { MemoryRouter } from 'react-router-dom';
17+
import {
18+
LabelType,
19+
State,
20+
TagLabel,
21+
TagLabelMetadata,
22+
TagSource,
23+
} from '../../../generated/type/tagLabel';
24+
import TagsContainerV2 from './TagsContainerV2';
25+
26+
let capturedOnSubmit:
27+
| ((data: { value: string; data?: Partial<EntityTags> }[]) => Promise<void>)
28+
| undefined;
29+
30+
jest.mock('../TagsSelectForm/TagsSelectForm.component', () => {
31+
return jest.fn().mockImplementation((props) => {
32+
capturedOnSubmit = props.onSubmit;
33+
34+
return <div data-testid="mock-tag-select-form">TagSelectForm</div>;
35+
});
36+
});
37+
38+
jest.mock('../TagsViewer/TagsViewer', () =>
39+
jest.fn().mockImplementation(() => <div data-testid="tags-viewer" />)
40+
);
41+
42+
jest.mock('../TagsV1/TagsV1.component', () =>
43+
jest.fn().mockImplementation(() => <div data-testid="tags-v1" />)
44+
);
45+
46+
jest.mock('../../Customization/GenericProvider/GenericProvider', () => ({
47+
useGenericContext: () => ({
48+
onThreadLinkSelect: jest.fn(),
49+
activeTagDropdownKey: null,
50+
updateActiveTagDropdownKey: jest.fn(),
51+
}),
52+
}));
53+
54+
jest.mock('../../Suggestions/SuggestionsProvider/SuggestionsProvider', () => ({
55+
useSuggestionsContext: () => ({ selectedUserSuggestions: undefined }),
56+
}));
57+
58+
jest.mock('../../common/ExpandableCard/ExpandableCard', () =>
59+
jest
60+
.fn()
61+
.mockImplementation(({ children }) => (
62+
<div data-testid="expandable-card">{children}</div>
63+
))
64+
);
65+
66+
jest.mock('../../Suggestions/SuggestionsAlert/SuggestionsAlert', () =>
67+
jest.fn().mockImplementation(() => <div data-testid="suggestions-alert" />)
68+
);
69+
70+
const PERSONAL_DATA_FQN = 'PersonalData.Personal';
71+
const PII_SENSITIVE_FQN = 'PII.Sensitive';
72+
const TIER_GOLD_FQN = 'Tier.Tier1';
73+
74+
const APPLIED_AT_ISO = '2026-01-01T00:00:00Z';
75+
76+
const personalDataTag: EntityTags = {
77+
tagFQN: PERSONAL_DATA_FQN,
78+
source: TagSource.Classification,
79+
labelType: LabelType.Manual,
80+
state: State.Confirmed,
81+
appliedBy: 'admin',
82+
appliedAt: new Date(APPLIED_AT_ISO),
83+
description: 'Personal data',
84+
};
85+
86+
const piiSensitiveTag: EntityTags = {
87+
tagFQN: PII_SENSITIVE_FQN,
88+
source: TagSource.Classification,
89+
labelType: LabelType.Manual,
90+
state: State.Confirmed,
91+
appliedBy: 'bot-classification',
92+
appliedAt: new Date(APPLIED_AT_ISO),
93+
};
94+
95+
const renderTagsContainer = (props: {
96+
selectedTags: EntityTags[];
97+
onSelectionChange: jest.Mock;
98+
}) => {
99+
capturedOnSubmit = undefined;
100+
101+
return render(
102+
<MemoryRouter>
103+
<TagsContainerV2
104+
permission
105+
showInlineEditButton
106+
entityFqn="sample.db.schema.table"
107+
entityType="table"
108+
selectedTags={props.selectedTags}
109+
tagType={TagSource.Classification}
110+
onSelectionChange={props.onSelectionChange}
111+
/>
112+
</MemoryRouter>
113+
);
114+
};
115+
116+
const enterEditMode = () => {
117+
const editButton = screen.getByTestId('edit-button');
118+
fireEvent.click(editButton);
119+
};
120+
121+
describe('TagsContainerV2 handleSave', () => {
122+
beforeEach(() => {
123+
jest.clearAllMocks();
124+
capturedOnSubmit = undefined;
125+
});
126+
127+
it('preserves appliedBy and appliedAt on existing tag when a new tag is added', async () => {
128+
const onSelectionChange = jest.fn().mockResolvedValue(undefined);
129+
renderTagsContainer({
130+
selectedTags: [personalDataTag],
131+
onSelectionChange,
132+
});
133+
134+
enterEditMode();
135+
136+
expect(capturedOnSubmit).toBeDefined();
137+
138+
await act(async () => {
139+
await capturedOnSubmit?.([
140+
{ value: PERSONAL_DATA_FQN, data: personalDataTag },
141+
{
142+
value: TIER_GOLD_FQN,
143+
data: { name: 'Tier1', tagFQN: TIER_GOLD_FQN },
144+
},
145+
]);
146+
});
147+
148+
expect(onSelectionChange).toHaveBeenCalledTimes(1);
149+
150+
const emitted = onSelectionChange.mock.calls[0][0] as EntityTags[];
151+
152+
const survived = emitted.find((t) => t.tagFQN === PERSONAL_DATA_FQN);
153+
154+
expect(survived).toEqual(
155+
expect.objectContaining({
156+
tagFQN: PERSONAL_DATA_FQN,
157+
source: TagSource.Classification,
158+
labelType: LabelType.Manual,
159+
state: State.Confirmed,
160+
appliedBy: 'admin',
161+
appliedAt: personalDataTag.appliedAt,
162+
description: 'Personal data',
163+
})
164+
);
165+
});
166+
167+
it('passes every TagLabel schema field through to onSelectionChange', async () => {
168+
const onSelectionChange = jest.fn().mockResolvedValue(undefined);
169+
// Seed with a different tag so the new selection genuinely changes the FQN list
170+
// and isn't short-circuited by the no-op guard inside handleSave.
171+
renderTagsContainer({
172+
selectedTags: [piiSensitiveTag],
173+
onSelectionChange,
174+
});
175+
176+
enterEditMode();
177+
178+
expect(capturedOnSubmit).toBeDefined();
179+
180+
const fullTag: Required<TagLabel> = {
181+
tagFQN: PERSONAL_DATA_FQN,
182+
source: TagSource.Classification,
183+
labelType: LabelType.Manual,
184+
state: State.Confirmed,
185+
name: 'Personal',
186+
displayName: 'Personal Data',
187+
description: 'Full TagLabel coverage fixture',
188+
style: { color: '#ABCDEF', iconURL: 'icon-url' },
189+
href: 'https://example.openmetadata/api/v1/tags/PersonalData.Personal',
190+
appliedBy: 'admin',
191+
appliedAt: new Date('2026-01-01T00:00:00Z'),
192+
metadata: {
193+
recognizer: {
194+
recognizerId: 'rec-1',
195+
recognizerName: 'pii-recognizer',
196+
score: 0.95,
197+
},
198+
} as TagLabelMetadata,
199+
reason: 'auto-classified',
200+
};
201+
202+
await act(async () => {
203+
await capturedOnSubmit?.([{ value: fullTag.tagFQN, data: fullTag }]);
204+
});
205+
206+
expect(onSelectionChange).toHaveBeenCalledTimes(1);
207+
208+
const emitted = onSelectionChange.mock.calls[0][0] as TagLabel[];
209+
const survived = emitted.find((t) => t.tagFQN === PERSONAL_DATA_FQN);
210+
211+
for (const key of Object.keys(fullTag) as (keyof TagLabel)[]) {
212+
expect(survived?.[key]).toEqual(fullTag[key]);
213+
}
214+
});
215+
216+
it('add-one-remove-another in same save keeps surviving tag fields intact', async () => {
217+
const onSelectionChange = jest.fn().mockResolvedValue(undefined);
218+
renderTagsContainer({
219+
selectedTags: [personalDataTag, piiSensitiveTag],
220+
onSelectionChange,
221+
});
222+
223+
enterEditMode();
224+
225+
expect(capturedOnSubmit).toBeDefined();
226+
227+
await act(async () => {
228+
await capturedOnSubmit?.([
229+
{ value: PERSONAL_DATA_FQN, data: personalDataTag },
230+
{
231+
value: TIER_GOLD_FQN,
232+
data: { name: 'Tier1', tagFQN: TIER_GOLD_FQN },
233+
},
234+
]);
235+
});
236+
237+
expect(onSelectionChange).toHaveBeenCalledTimes(1);
238+
239+
const emitted = onSelectionChange.mock.calls[0][0] as EntityTags[];
240+
241+
expect(emitted).toHaveLength(2);
242+
243+
const survived = emitted.find((t) => t.tagFQN === PERSONAL_DATA_FQN);
244+
const added = emitted.find((t) => t.tagFQN === TIER_GOLD_FQN);
245+
246+
expect(survived).toEqual(
247+
expect.objectContaining({
248+
appliedBy: 'admin',
249+
appliedAt: personalDataTag.appliedAt,
250+
description: 'Personal data',
251+
})
252+
);
253+
expect(added).toEqual(
254+
expect.objectContaining({
255+
tagFQN: TIER_GOLD_FQN,
256+
source: TagSource.Classification,
257+
labelType: LabelType.Manual,
258+
state: State.Confirmed,
259+
})
260+
);
261+
expect(added?.appliedBy).toBeUndefined();
262+
expect(added?.appliedAt).toBeUndefined();
263+
});
264+
});

0 commit comments

Comments
 (0)