fix: checkReturnType in ExpressionValidation throws unexpected error

Composer/cypress/integration/NotificationPage.spec.ts

@@ -55,7 +55,7 @@ context('Notification Page', () => {
     cy.withinEditor('PropertyEditor', () => {
       cy.findByText('Condition').should('exist');
       cy.findByTestId('expression-type-dropdown-Condition').focus().should('contain.text', 'expression');
-      cy.get('#root\\.condition').click().type('foo = bar', { delay: 200 });
+      cy.get('#root\\.condition').click().type('=foo = bar', { delay: 200 });
       cy.findByTestId('FieldErrorMessage').should('exist');
     });
 

Composer/packages/lib/indexers/__tests__/validations/expressionValidation.test.ts

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 import { LgFile } from '@bfc/shared';
+import { ReturnType } from 'adaptive-expressions';
 
 import { validate } from '../../src/validations/expressionValidation/validation';
 
@@ -29,36 +30,42 @@ describe('search lg custom function', () => {
 
 describe('validate expression', () => {
   it('if string expression do nothing', () => {
-    const expression = { value: 'hello', required: false, path: 'test', types: ['string'] };
+    const expression = { value: 'hello', required: false, path: 'test', types: [ReturnType.String] };
     const result = validate(expression, []);
     expect(result).toBeNull();
   });
 
   it('if start with =, but type is not match', () => {
-    const expression = { value: '=13', required: false, path: 'test', types: ['string'] };
+    const expression = { value: '=13', required: false, path: 'test', types: [ReturnType.String] };
     const result = validate(expression, []);
-    expect(result?.message).toBe('the expression type is not match');
+    expect(result?.message).toBe('the return type does not match');
   });
 
   it('if start with =, and type is match', () => {
-    const expression = { value: '=13', required: false, path: 'test', types: ['integer'] };
+    const expression = { value: '=13', required: false, path: 'test', types: [ReturnType.Number] };
     const result = validate(expression, []);
     expect(result).toBeNull();
     expression.value = '=true';
-    expression.types[0] = 'boolean';
+    expression.types[0] = 1;
     const result1 = validate(expression, []);
     expect(result1).toBeNull();
   });
 
-  it('use custom functions, but lg file does not export', () => {
-    const expression = { value: '=foo.bar()', required: false, path: 'test', types: ['boolean'] };
+  it('use custom functions will not throw error', () => {
+    const expression = { value: '=foo.bar()', required: false, path: 'test', types: [ReturnType.Boolean] };
     const result = validate(expression, []);
-    expect(result).not.toBeNull();
+    expect(result).toBeNull();
   });
 
   it('use custom functions, and lg file does export', () => {
-    const expression = { value: '=foo.bar()', required: false, path: 'test', types: ['boolean'] };
+    const expression = { value: '=foo.bar()', required: false, path: 'test', types: [ReturnType.Boolean] };
     const result = validate(expression, ['foo.bar']);
     expect(result).toBeNull();
   });
+
+  it('built-in function return type', () => {
+    const expression = { value: "=concat('test', '1')", required: false, path: 'test', types: [ReturnType.String] };
+    const result = validate(expression, []);
+    expect(result).toBeNull();
+  });
 });

Composer/packages/lib/indexers/src/validations/expressionValidation/types.ts

@@ -2,14 +2,16 @@
 // Licensed under the MIT License.
 
 import { Diagnostic, LgFile, LuFile } from '@bfc/shared';
+import { ReturnType } from 'adaptive-expressions';
 
-export enum ExpressionType {
-  number = 'number',
-  integer = 'integer',
-  boolean = 'boolean',
-  string = 'string',
-  array = 'array',
-}
+export const StringMapExpressionType = {
+  number: ReturnType.Number,
+  string: ReturnType.String,
+  boolean: ReturnType.Boolean,
+  object: ReturnType.Object,
+  array: ReturnType.Array,
+  integer: ReturnType.Number,
+};
 
 export type ValidateFunc = (
   path: string,
@@ -21,8 +23,8 @@ export type ValidateFunc = (
 ) => Diagnostic[] | null; // error msg
 
 export type ExpressionProperty = {
-  value: string | boolean | number;
+  value: any;
   required: boolean; //=true, the value is required in dialog
   path: string; //the json path of the value
-  types: string[]; //supported expression type of the value
+  types: number[]; //supported expression type of the value
 };

Composer/packages/lib/indexers/src/validations/expressionValidation/utils.ts

@@ -4,7 +4,7 @@
 import values from 'lodash/values';
 import { FieldNames } from '@bfc/shared';
 
-import { ExpressionType } from './types';
+import { StringMapExpressionType } from './types';
 
 export const createPath = (path: string, type: string): string => {
   let list = path.split('.');
@@ -41,18 +41,23 @@ export function findRequiredProperties(schema: any): { [key: string]: boolean }
   return required;
 }
 
-export function findTypes(schema: any): string[] {
+export function findTypes(schema: any): number[] {
   if (!schema) return [];
-  let types: string[] = [];
+  let types: number[] = [];
   if (schema.type) {
     if (Array.isArray(schema.type)) {
-      types = [...types, ...schema.type];
+      types = schema.type.map((item: string) => StringMapExpressionType[item]);
     } else {
-      types.push(schema.type);
+      types.push(StringMapExpressionType[schema.type]);
     }
-  } else {
-    types = schema.oneOf?.filter((item) => !!ExpressionType[item.type]).map((item) => item.type);
+  } else if (schema.oneOf) {
+    types = schema.oneOf.reduce((result: string[], item) => {
+      if (StringMapExpressionType[item.type]) {
+        result.push(StringMapExpressionType[item.type]);
+      }
+      return result;
+    }, []);
   }
 
-  return Array.from(new Set<string>(types));
+  return Array.from(new Set<number>(types));
 }

Composer/packages/lib/indexers/src/validations/expressionValidation/validation.ts

@@ -1,49 +1,54 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
+
+/* eslint-disable no-bitwise */
 import { Expression, ReturnType } from 'adaptive-expressions';
 import formatMessage from 'format-message';
 import { Diagnostic } from '@bfc/shared';
 import startsWith from 'lodash/startsWith';
 
-import { ExpressionType, ExpressionProperty } from './types';
+import { ExpressionProperty } from './types';
+
+const EMPTY = formatMessage(`is missing or empty`);
+const RETURNTYPE_NOT_MATCH = formatMessage('the return type does not match');
+const BUILT_IN_FUNCTION_ERROR = formatMessage("it's not a built-in function or a custom function.");
+
+const expressionErrorMessage = (error: string) => formatMessage('must be an expression: {error}', { error });
 
 const customFunctionErrorMessage = (func: string) =>
   formatMessage(`Error: {func} does not have an evaluator, it's not a built-in function or a custom function`, {
     func,
   });
 
-const ExpressionTypeMapString = {
-  [ReturnType.Number]: 'number',
-  [ReturnType.String]: 'string',
-  [ReturnType.Boolean]: 'boolean',
-  [ReturnType.Object]: 'object',
-  [ReturnType.Array]: 'array',
+//bitwise operation
+export const addReturnType = (currentType: number, newType: number) => {
+  return currentType | newType;
 };
 
-const isExpression = (value: string | boolean | number, types: string[]): boolean => {
+export const checkStringExpression = (exp: string): number => {
   //StringExpression always assumes string interpolation unless prefixed with =, producing a string
-  return (typeof value === 'string' && value[0] === '=') || types.length !== 1 || types[0] !== ExpressionType.string;
-};
+  if (exp.trim().startsWith('=')) {
+    return Expression.parse(exp.trim().substring(1)).returnType;
+  }
 
-//The return type should match the schema type
-//TODO: returnType is number, schem type is string, need map or unify
-const checkReturnType = (returnType: ReturnType, types: string[]): string => {
-  return returnType === ReturnType.Object ||
-    types.includes(ExpressionTypeMapString[returnType]) ||
-    (returnType === ReturnType.Number && types.includes(ExpressionType.integer))
-    ? ''
-    : formatMessage('the return type does not match');
+  return ReturnType.String;
 };
 
-export const checkExpression = (
-  exp: string | boolean | number,
-  required: boolean,
-  types: string[],
-  customFunctions: string[]
-): string => {
-  let message = '';
-  let returnType: ReturnType = ReturnType.Object;
+export const checkExpression = (exp: any, required: boolean): number => {
+  if ((exp === undefined || '') && required) {
+    throw new Error(EMPTY);
+  }
+
+  let returnType = 0;
+
   switch (typeof exp) {
+    case 'object': {
+      returnType = ReturnType.Object;
+      if (Array.isArray(exp)) {
+        returnType = addReturnType(returnType, ReturnType.Array);
+      }
+      break;
+    }
     case 'boolean': {
       returnType = ReturnType.Boolean;
       break;
@@ -52,44 +57,58 @@ export const checkExpression = (
       returnType = ReturnType.Number;
       break;
     }
-    default: {
-      if (!exp && required) message = formatMessage(`is missing or empty`);
-      try {
-        returnType = Expression.parse(exp).returnType;
-      } catch (error) {
-        if (
-          customFunctions.length &&
-          customFunctions.some((item) => startsWith(error, customFunctionErrorMessage(item)))
-        ) {
-          message = '';
-        } else {
-          message = `${formatMessage('must be an expression:')} ${error})`;
-        }
-      }
+    case 'string': {
+      returnType = checkStringExpression(exp);
+      break;
     }
+    default:
+      break;
   }
-  if (!message) message = checkReturnType(returnType, types);
-  return message;
+
+  return returnType;
 };
 
-export const validate = (expression: ExpressionProperty, customFunctions: string[]): Diagnostic | null => {
-  const { required, path, types } = expression;
-  let value = expression.value;
-  //if there is no type do nothing
-  //if the json type length more than 2, the type assumes string interpolation
-  if (!types.length || types.length > 2 || !isExpression(value, types)) {
-    return null;
+//The return type should match the schema type
+// the return type use binary number to store
+// if returnType = 24, the expression result is 16+8. so the type is string or array
+const checkReturnType = (returnType: number, types: number[]): string => {
+  // if return type contain object do nothing.
+  if (returnType & ReturnType.Object) return '';
+
+  return types.some((type) => type & returnType) ? '' : RETURNTYPE_NOT_MATCH;
+};
+
+const filterCustomFunctionError = (error: string, CustomFunctions: string[]): string => {
+  let errorMessage = expressionErrorMessage(error);
+
+  //Now all customFunctions is from lg file content.
+  if (CustomFunctions.some((item) => startsWith(error, customFunctionErrorMessage(item)))) {
+    errorMessage = '';
+  }
+
+  //Todo: if the custom functions are defined in runtime, use the field from settings to filter
+  // settings.customFunctions.some();
+  if (error.endsWith(BUILT_IN_FUNCTION_ERROR)) {
+    errorMessage = '';
   }
 
-  //remove '='
-  if (typeof value === 'string' && value[0] === '=') {
-    value = value.substring(1);
+  return errorMessage;
+};
+
+export const validate = (expression: ExpressionProperty, customFunctions: string[]): Diagnostic | null => {
+  const { required, path, types, value } = expression;
+  let errorMessage = '';
+
+  try {
+    const returnType = checkExpression(value, required);
+    errorMessage = checkReturnType(returnType, types);
+  } catch (error) {
+    errorMessage = filterCustomFunctionError(error.message, customFunctions);
   }
 
-  const message = checkExpression(value, required, types, customFunctions);
-  if (!message) return null;
+  if (!errorMessage) return null;
 
-  const diagnostic = new Diagnostic(message, '');
+  const diagnostic = new Diagnostic(errorMessage, '');
   diagnostic.path = path;
   return diagnostic;
 };