Skip to content

Commit 72b1418

Browse files
Rohit0301Rohit0301
authored
Make the semantics rule field in the data contract required (#26897)
* Make the semantics rule field in the data contract required * fixed unit test and addressed gitar comment * fixed lint issues * fix lint issues * fixed lint issues * addressed PR comments
1 parent 348fe05 commit 72b1418

4 files changed

Lines changed: 202 additions & 60 deletions

File tree

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

Lines changed: 85 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2728,7 +2728,7 @@ test.describe('Data Contracts Semantics Rule DisplayName', () => {
27282728
await selectOption(
27292729
page,
27302730
ruleLocator.locator('.rule--value .ant-select'),
2731-
table.entityResponseData.displayName,
2731+
table.entityResponseData.displayName || '',
27322732
true
27332733
);
27342734

@@ -2818,7 +2818,7 @@ test.describe('Data Contracts Semantics Rule DisplayName', () => {
28182818
await selectOption(
28192819
page,
28202820
ruleLocator.locator('.rule--value .ant-select'),
2821-
table.entityResponseData.displayName,
2821+
table.entityResponseData.displayName || '',
28222822
true
28232823
);
28242824

@@ -2907,7 +2907,7 @@ test.describe('Data Contracts Semantics Rule DisplayName', () => {
29072907
await selectOption(
29082908
page,
29092909
ruleLocator.locator('.rule--value .ant-select'),
2910-
table.entityResponseData.displayName,
2910+
table.entityResponseData.displayName || '',
29112911
true
29122912
);
29132913

@@ -2996,7 +2996,7 @@ test.describe('Data Contracts Semantics Rule DisplayName', () => {
29962996
await selectOption(
29972997
page,
29982998
ruleLocator.locator('.rule--value .ant-select'),
2999-
table.entityResponseData.displayName,
2999+
table.entityResponseData.displayName || '',
30003000
true
30013001
);
30023002

@@ -3799,3 +3799,84 @@ test.describe('Data Contracts Semantics Rule Updated on', () => {
37993799
});
38003800
});
38013801
});
3802+
3803+
test.describe('Data Contract - Semantics Fields Validation', () => {
3804+
const table = new TableClass();
3805+
3806+
test.beforeAll('Setup', async ({ browser }) => {
3807+
const { apiContext, afterAction } = await performAdminLogin(browser);
3808+
await table.create(apiContext);
3809+
await afterAction();
3810+
});
3811+
3812+
test('Validate semantics fields', async ({ page }) => {
3813+
await test.step('Navigate to semantics tab', async () => {
3814+
await redirectToHomePage(page);
3815+
await table.visitEntityPage(page);
3816+
await performInitialStepForRules(page);
3817+
await page.getByRole('tab', { name: 'Semantics' }).click();
3818+
});
3819+
3820+
await test.step('Click save and verify rule error is shown', async () => {
3821+
await page.getByTestId('save-semantic-button').click();
3822+
3823+
await expect(page.getByText(/rule is required/i)).toBeVisible();
3824+
await expect(page.getByText(/name is required/i)).toBeVisible();
3825+
await expect(page.getByText(/description is required/i)).toBeVisible();
3826+
});
3827+
3828+
await test.step('Verify delete button is not visible with only one rule', async () => {
3829+
await expect(page.locator('.action--DELETE')).not.toBeVisible();
3830+
});
3831+
3832+
await test.step('fill the first rule completely', async () => {
3833+
await page.fill('#semantics_0_name', DATA_CONTRACT_SEMANTICS1.name);
3834+
await page.fill(
3835+
'#semantics_0_description',
3836+
DATA_CONTRACT_SEMANTICS1.description
3837+
);
3838+
3839+
const ruleLocator = page.locator('.group').nth(0);
3840+
await selectOption(
3841+
page,
3842+
ruleLocator.locator('.group--field .ant-select'),
3843+
'Owners',
3844+
true
3845+
);
3846+
await selectOption(
3847+
page,
3848+
ruleLocator.locator('.rule--operator .ant-select'),
3849+
'Is Set'
3850+
);
3851+
});
3852+
3853+
await test.step('Add a second rule condition', async () => {
3854+
await page.getByTestId('add-new-rule-btn').click();
3855+
3856+
await expect(page.locator('.action--DELETE').first()).toBeVisible();
3857+
});
3858+
3859+
await test.step('Delete the filled rule condition and verify rule error is shown', async () => {
3860+
const deleteButtons = page.locator('.action--DELETE');
3861+
await deleteButtons.first().click();
3862+
3863+
await expect(page.locator('.action--DELETE')).not.toBeVisible();
3864+
await expect(page.getByText(/rule is required/i)).toBeVisible();
3865+
});
3866+
3867+
await test.step('select Is Set operator and error is hidden', async () => {
3868+
await selectOption(
3869+
page,
3870+
page.locator('.rule--field .ant-select'),
3871+
'Owners',
3872+
true
3873+
);
3874+
await selectOption(
3875+
page,
3876+
page.locator('.rule--operator .ant-select'),
3877+
'Is Set'
3878+
);
3879+
await expect(page.getByText(/rule is required/i)).not.toBeVisible();
3880+
});
3881+
});
3882+
});

‎openmetadata-ui/src/main/resources/ui/src/components/DataContract/ContractSemanticFormTab/ContractSemanticFormTab.tsx‎

Lines changed: 45 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,10 @@ import {
3939
DataContract,
4040
SemanticsRule,
4141
} from '../../../generated/entity/data/dataContract';
42-
import { getSematicRuleFields } from '../../../utils/DataContract/DataContractUtils';
42+
import {
43+
getSematicRuleFields,
44+
semanticRuleValidator,
45+
} from '../../../utils/DataContract/DataContractUtils';
4346
import jsonLogicSearchClassBase from '../../../utils/JSONLogicSearchClassBase';
4447
import ExpandableCard from '../../common/ExpandableCard/ExpandableCard';
4548
import { EditIconButton } from '../../common/IconButtons/EditIconButton';
@@ -89,9 +92,7 @@ export const ContractSemanticFormTab: React.FC<{
8992
const handleDeleteSemantic = useCallback(
9093
(key: number) => {
9194
const filteredValue =
92-
semanticsFormData
93-
?.filter((_item, idx) => idx !== key)
94-
?.filter(Boolean) ?? [];
95+
semanticsFormData?.filter((_item, idx) => idx !== key) ?? [];
9596
form.setFieldsValue({ semantics: filteredValue });
9697
onChange({ semantics: filteredValue });
9798
},
@@ -107,22 +108,23 @@ export const ContractSemanticFormTab: React.FC<{
107108
rule: string,
108109
tree?: JsonTree
109110
) => {
110-
const modifyRule = JSON.stringify(
111-
jsonLogicSearchClassBase.getNegativeQueryForNotContainsReverserOperation(
112-
JSON.parse(rule)
113-
)
114-
);
111+
let modifyRule = '';
112+
if (rule) {
113+
try {
114+
modifyRule = JSON.stringify(
115+
jsonLogicSearchClassBase.getNegativeQueryForNotContainsReverserOperation(
116+
JSON.parse(rule)
117+
)
118+
);
119+
} catch {
120+
modifyRule = '';
121+
}
122+
}
123+
115124
form.setFields([
116125
{
117126
name: ['semantics', field.name, 'rule'],
118127
value: modifyRule,
119-
errors: modifyRule
120-
? []
121-
: [
122-
t('message.field-text-is-required', {
123-
fieldText: t('label.rule'),
124-
}),
125-
],
126128
},
127129
]);
128130
form.setFieldsValue({
@@ -318,29 +320,40 @@ export const ContractSemanticFormTab: React.FC<{
318320
</Form.Item>
319321
</Col>
320322
<Col span={24}>
321-
<QueryBuilderWidgetV1
322-
entityType={EntityType.TABLE}
323-
fields={queryBuilderFields}
324-
getQueryActions={handleAddQueryBuilderRule}
325-
key={field.name}
323+
<Form.Item
324+
{...field}
326325
label={t('label.rule')}
327-
outputType={SearchOutputType.JSONLogic}
328-
tree={
329-
editFieldData?.jsonTree
330-
? JSON.parse(editFieldData?.jsonTree)
331-
: undefined
332-
}
333-
value={editFieldData?.rule ?? ''}
334-
onChange={(rule: string, tree?: JsonTree) =>
335-
handleQueryBuilderChange(field, rule, tree)
336-
}
337-
/>
326+
name={[field.name, 'rule']}
327+
rules={[
328+
{
329+
required: true,
330+
validator: semanticRuleValidator,
331+
},
332+
]}>
333+
<QueryBuilderWidgetV1
334+
entityType={EntityType.TABLE}
335+
fields={queryBuilderFields}
336+
getQueryActions={handleAddQueryBuilderRule}
337+
key={field.name}
338+
outputType={SearchOutputType.JSONLogic}
339+
tree={
340+
editFieldData?.jsonTree
341+
? JSON.parse(editFieldData?.jsonTree)
342+
: undefined
343+
}
344+
value={editFieldData?.rule ?? ''}
345+
onChange={(rule: string, tree?: JsonTree) =>
346+
handleQueryBuilderChange(field, rule, tree)
347+
}
348+
/>
349+
</Form.Item>
338350
</Col>
339351
</Row>
340352

341353
<div className="semantic-form-item-actions">
342354
<Button
343355
className="add-semantic-button"
356+
data-testid="add-new-rule-btn"
344357
disabled={!queryBuilderAddRule?.addRule}
345358
icon={<Icon component={PlusIcon} />}
346359
type="link"

‎openmetadata-ui/src/main/resources/ui/src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.tsx‎

Lines changed: 56 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,10 @@ import { InfoCircleOutlined } from '@ant-design/icons';
1414
import {
1515
Actions,
1616
Builder,
17+
BuilderProps,
18+
ButtonProps,
1719
Config,
20+
ConfigContext,
1821
ImmutableTree,
1922
JsonTree,
2023
Query,
@@ -33,7 +36,15 @@ import {
3336
import classNames from 'classnames';
3437
import { debounce, isEmpty, isUndefined } from 'lodash';
3538
import Qs from 'qs';
36-
import { FC, useCallback, useEffect, useMemo, useState } from 'react';
39+
import {
40+
FC,
41+
ReactElement,
42+
useCallback,
43+
useEffect,
44+
useMemo,
45+
useRef,
46+
useState,
47+
} from 'react';
3748
import { useTranslation } from 'react-i18next';
3849
import { EntityType } from '../../../enums/entity.enum';
3950
import { SearchIndex } from '../../../enums/search.enum';
@@ -106,7 +117,7 @@ const QueryBuilderWidgetV1: FC<{
106117

107118
const { t } = useTranslation();
108119
const [queryURL, setQueryURL] = useState<string>('');
109-
const [queryActions, setQueryActions] = useState<Actions>();
120+
const queryActionsRef = useRef<Actions>();
110121

111122
const onTreeUpdate = (nTree: ImmutableTree, nConfig: Config) => {
112123
setTreeInternal(nTree);
@@ -125,10 +136,9 @@ const QueryBuilderWidgetV1: FC<{
125136
config
126137
).fixedTree;
127138

128-
const queryFilterString = !isEmpty(tree)
129-
? Qs.stringify({ queryFilter: JSON.stringify(tree) })
130-
: '';
131-
139+
const queryFilterString = isEmpty(tree)
140+
? ''
141+
: Qs.stringify({ queryFilter: JSON.stringify(tree) });
132142
setQueryURL(`${getExplorePath({})}${queryFilterString}`);
133143

134144
try {
@@ -186,7 +196,7 @@ const QueryBuilderWidgetV1: FC<{
186196
);
187197
}
188198

189-
onChange?.(!isEmpty(data) ? JSON.stringify(qFilter) : '');
199+
onChange?.(isEmpty(data) ? '' : JSON.stringify(qFilter));
190200
} else {
191201
const jsonTree = QbUtils.getTree(nTree);
192202
try {
@@ -199,10 +209,44 @@ const QueryBuilderWidgetV1: FC<{
199209
};
200210

201211
useEffect(() => {
202-
if (props.getQueryActions && queryActions) {
203-
props.getQueryActions(queryActions);
212+
if (props.getQueryActions && queryActionsRef.current) {
213+
props.getQueryActions(queryActionsRef.current);
204214
}
205-
}, [queryActions]);
215+
}, [treeInternal, props.getQueryActions]);
216+
217+
const hasOnlyOneRule = useMemo(
218+
() => (QbUtils.getTree(treeInternal).children1?.length ?? 0) <= 1,
219+
[treeInternal]
220+
);
221+
222+
const renderBuilder = useCallback(
223+
(builderProps: BuilderProps) => {
224+
queryActionsRef.current = builderProps.actions;
225+
const builderConfig = {
226+
...builderProps.config,
227+
settings: {
228+
...builderProps.config?.settings,
229+
renderButton: (btnProps: ButtonProps, ctx?: ConfigContext) => {
230+
if (
231+
(hasOnlyOneRule && btnProps.type === 'delRule') ||
232+
!builderProps.config?.settings?.renderButton
233+
) {
234+
return null as unknown as ReactElement<typeof btnProps>;
235+
}
236+
237+
return builderProps.config?.settings?.renderButton?.(btnProps, ctx);
238+
},
239+
},
240+
};
241+
242+
return (
243+
<div className="query-builder-container query-builder qb-lite">
244+
<Builder {...builderProps} config={builderConfig} />
245+
</div>
246+
);
247+
},
248+
[hasOnlyOneRule]
249+
);
206250

207251
return (
208252
<div
@@ -215,7 +259,7 @@ const QueryBuilderWidgetV1: FC<{
215259
'p-t-sm': outputType === SearchOutputType.ElasticSearch,
216260
})}
217261
span={24}>
218-
{outputType === SearchOutputType.JSONLogic && (
262+
{outputType === SearchOutputType.JSONLogic && props.label && (
219263
<>
220264
<Typography.Text className="query-filter-label text-grey-muted">
221265
{props.label}
@@ -225,18 +269,7 @@ const QueryBuilderWidgetV1: FC<{
225269
)}
226270
<Query
227271
{...config}
228-
renderBuilder={(props) => {
229-
// Store the actions for external access
230-
if (!queryActions) {
231-
setQueryActions(props.actions);
232-
}
233-
234-
return (
235-
<div className="query-builder-container query-builder qb-lite">
236-
<Builder {...props} />
237-
</div>
238-
);
239-
}}
272+
renderBuilder={renderBuilder}
240273
value={treeInternal}
241274
onChange={handleChange}
242275
/>

0 commit comments

Comments
 (0)