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.

Sign up for GitHub

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

bug fix - nested restrictions #236

Merged
merged 2 commits into from
Nov 18, 2024
Merged

bug fix - nested restrictions #236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
77de4f9
bug fix - nested restrictions
leoraba Oct 31, 2024
82fe79f
add unit test on conditional restrictions
leoraba Nov 14, 2024
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
1 change: 1 addition & 0 deletions packages/client/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export type {
ParseDictionaryData,
ParseDictionaryFailure,
ParseDictionaryResult,
ParseFieldError,
ParseSchemaError,
ParseSchemaFailureData,
ParseSchemaResult,
Expand Down
2 changes: 1 addition & 1 deletion packages/validation/src/parseValues/matchCodeListFormatting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const collectAllNestedCodeLists = (
return TypeUtils.asArray(restrictions).flatMap((restrictionsObject) => {
if ('if' in restrictionsObject) {
const thenCodeLists = restrictionsObject.then ? collectAllNestedCodeLists(restrictionsObject.then) : [];
const elseCodeLists = restrictionsObject.else ? collectAllNestedCodeLists(restrictionsObject) : [];
const elseCodeLists = restrictionsObject.else ? collectAllNestedCodeLists(restrictionsObject.else) : [];
return [...thenCodeLists, ...elseCodeLists];
} else {
return restrictionsObject.codeList ? restrictionsObject.codeList : [];
Expand Down
27 changes: 27 additions & 0 deletions ...t/fixtures/fields/conditionalRestrictions/fieldStringConditionalExistsWithouthThenElse.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in file name: Withouth

Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { SchemaField, type SchemaStringField } from '@overture-stack/lectern-dictionary';
import { fieldStringNoRestriction } from '../noRestrictions/fieldStringNoRestriction';
import { validateFixture } from '../../../testUtils/validateFixture';

export const fieldStringConditionalExistsWithouthThenElse = {
name: 'conditional-field',
valueType: 'string',
description: 'Required if `fieldStringNoRestriction` field exists, otherwise must be empty',
restrictions: {
if: {
conditions: [
{
fields: [fieldStringNoRestriction.name],
match: {
exists: true,
},
},
],
},
},
} as const satisfies SchemaStringField;

validateFixture(
fieldStringConditionalExistsWithouthThenElse,
SchemaField,
'fieldStringConditionalExistsWithouthThenElse is not a valid SchemaField',
);
30 changes: 30 additions & 0 deletions packages/validation/test/parseValues/parseField.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import { fieldBooleanArrayRequired } from '../fixtures/fields/simpleRestrictions
import { fieldStringCodeList } from '../fixtures/fields/simpleRestrictions/string/fieldStringCodeList';
import { codeListString } from '../fixtures/restrictions/codeListsFixtures';
import { fieldStringArrayCodeList } from '../fixtures/fields/simpleRestrictions/string/fieldStringArrayCodeList';
import { fieldStringConditionalExists } from '../fixtures/fields/conditionalRestrictions/fieldStringConditionalExists';
import { fieldStringConditionalExistsWithouthThenElse } from '../fixtures/fields/conditionalRestrictions/fieldStringConditionalExistsWithouthThenElse';

describe('Parse Values - parseFieldValue', () => {
describe('Single Value Fields', () => {
Expand Down Expand Up @@ -165,6 +167,34 @@ describe('Parse Values - parseFieldValue', () => {
expect(parseFieldValue(' !@#$%^&* ()_+ ', fieldStringNoRestriction).success).true;
expect(parseFieldValue(' !@#$%^&* ()_+ ', fieldStringNoRestriction).data).equals('!@#$%^&* ()_+');
});
it('Successfuly parses strings, with conditional restrictions', () => {
const value = 'any random string value!!!';
const result = parseFieldValue(value, fieldStringConditionalExists);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testing parsing any string using a Schema Field Definition having if/then/else restrictions

expect(result.success).true;
expect(result.data).equal(value);

expect(parseFieldValue(' 123', fieldStringConditionalExists).success).true;
expect(parseFieldValue(' 123', fieldStringConditionalExists).data).equals('123');
expect(parseFieldValue('false ', fieldStringConditionalExists).success).true;
expect(parseFieldValue('false ', fieldStringConditionalExists).data).equals('false');
expect(parseFieldValue(' !@#$%^&* ()_+ ', fieldStringConditionalExists).success).true;
expect(parseFieldValue(' !@#$%^&* ()_+ ', fieldStringConditionalExists).data).equals('!@#$%^&* ()_+');
});
it('Successfuly parses strings, with conditional restrictions without then or else', () => {
const value = 'any random string value!!!';
const result = parseFieldValue(value, fieldStringConditionalExistsWithouthThenElse);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing parsing any string using a Schema Field Definition having anif restriction without then/else. This is not a functional Schema definition however to cover every scenario possible we have to test it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the expected behaviour that an if without then/else just never applies restrictions?

Should we consider adding validation to lectern dictionaries to require an if to have a then or else (at least one of the two)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not then/else is found in the schema definition the expected behaviour is do not apply the restriction. Based on test resolveFieldRestrictions.spec.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to add a validation to require at least one between then/else. Although it could be added in a separate PR

expect(result.success).true;
expect(result.data).equal(value);

expect(parseFieldValue(' 123', fieldStringConditionalExistsWithouthThenElse).success).true;
expect(parseFieldValue(' 123', fieldStringConditionalExistsWithouthThenElse).data).equals('123');
expect(parseFieldValue('false ', fieldStringConditionalExistsWithouthThenElse).success).true;
expect(parseFieldValue('false ', fieldStringConditionalExistsWithouthThenElse).data).equals('false');
expect(parseFieldValue(' !@#$%^&* ()_+ ', fieldStringConditionalExistsWithouthThenElse).success).true;
expect(parseFieldValue(' !@#$%^&* ()_+ ', fieldStringConditionalExistsWithouthThenElse).data).equals(
'!@#$%^&* ()_+',
);
});
it('Updates string to match formatting of codeList value', () => {
const value = 'banana';
const result = parseFieldValue(value, fieldStringCodeList);
Expand Down