More Parameter Validation and Limitations (Length Limit for AI Prompt…

…s) (#10170) This change started with just adding a limit to the AI prompt length, but as I was adding yet another field to the CriteriaParameter type definition (maxCharacters) which only really applies to a subset of parameter types, I decided it was time to tidy that up. Hence, a larger change. Now, each parameter type has a corresponding type which can have its own fields, and a validate function switches on that type to perform different validations as applicable. Currently, I've added maxCharacters for strings (used for the AI prompt), and min and max for numbers (tested manually but not used; I mostly wanted a proof of concept around 2 different classes with 2 different types of validation).
microsoft · Sep 16, 2024 · b269289 · b269289
1 parent 8e0daec
commit b269289
Show file tree

Hide file tree

Showing 10 changed files with 178 additions and 20 deletions.
diff --git a/common-docs/teachertool/test/catalog-shared.json b/common-docs/teachertool/test/catalog-shared.json
@@ -13,6 +13,7 @@
                 {
                     "name": "question",
                     "type": "longString",
+                    "maxCharacters": 1000,
                     "paths": ["checks[0].question"]
                 },
                 {

diff --git a/teachertool/src/components/CriteriaInstanceDisplay.tsx b/teachertool/src/components/CriteriaInstanceDisplay.tsx
@@ -1,17 +1,18 @@
 import css from "./styling/CriteriaInstanceDisplay.module.scss";
-import { getCatalogCriteriaWithId } from "../state/helpers";
+import { getCatalogCriteriaWithId, getParameterDefinition } from "../state/helpers";
 import { CriteriaInstance, CriteriaParameterValue } from "../types/criteria";
 import { logDebug } from "../services/loggingService";
 import { setParameterValue } from "../transforms/setParameterValue";
 import { classList } from "react-common/components/util";
 import { getReadableBlockString, splitCriteriaTemplate } from "../utils";
-import { useContext, useMemo, useState } from "react";
+import { useContext, useEffect, useMemo, useState } from "react";
 import { Input } from "react-common/components/controls/Input";
 import { Button } from "react-common/components/controls/Button";
 import { AppStateContext } from "../state/appStateContext";
 import { Strings } from "../constants";
 import { showModal } from "../transforms/showModal";
 import { BlockPickerOptions } from "../types/modalOptions";
+import { validateParameterValue } from "../utils/validateParameterValue";
 
 interface InlineInputSegmentProps {
     initialValue: string;
@@ -27,24 +28,46 @@ const InlineInputSegment: React.FC<InlineInputSegmentProps> = ({
     shouldExpand,
     numeric,
 }) => {
-    const [isEmpty, setIsEmpty] = useState(!initialValue);
+    const [errorMessage, setErrorMessage] = useState(initialValue ? "" : Strings.ValueRequired);
+    const paramDefinition = useMemo(() => getParameterDefinition(instance.catalogCriteriaId, param.name), [param]);
+
+    useEffect(() => {
+        if (!paramDefinition) {
+            return;
+        }
+
+        // We still allow some invalid values to be set on the parameter so the user can see what they typed
+        // and the associated error.
+        // Without this, we risk erroring too soon (i.e. typing in first digit of number with min > 10),
+        // losing the user's input (which could be long), or desynchronizing the UI from the state.
+        // It will still be blocked via a separate check when the user tries to evaluate the criteria.
+        const paramValidation = validateParameterValue(paramDefinition, initialValue);
+        if (!paramValidation.valid) {
+            setErrorMessage(paramValidation.message ?? Strings.InvalidValue);
+        } else {
+            setErrorMessage("");
+        }
+    }, [initialValue]);
 
     function onChange(newValue: string) {
-        setIsEmpty(!newValue);
+        if (!newValue) {
+            setErrorMessage(Strings.ValueRequired);
+        }
+
         setParameterValue(instance.instanceId, param.name, newValue);
     }
 
-    const tooltip = isEmpty ? `${param.name}: ${Strings.ValueRequired}` : param.name;
+    const tooltip = errorMessage ? `${param.name}: ${errorMessage}` : param.name;
     return (
         <div title={tooltip} className={css["inline-input-wrapper"]}>
             <Input
                 className={classList(
                     css["inline-input"],
                     numeric ? css["number-input"] : css["string-input"],
                     shouldExpand ? css["long"] : undefined,
-                    isEmpty ? css["error"] : undefined
+                    errorMessage ? css["error"] : undefined
                 )}
-                icon={isEmpty ? "fas fa-exclamation-triangle" : undefined}
+                icon={errorMessage ? "fas fa-exclamation-triangle" : undefined}
                 initialValue={initialValue}
                 onChange={onChange}
                 preserveValueOnBlur={true}

diff --git a/teachertool/src/constants.ts b/teachertool/src/constants.ts
@@ -53,6 +53,11 @@ export namespace Strings {
     export const UnableToEvaluatePartial = lf("Unable to evaluate some criteria");
     export const GiveFeedback = lf("Give Feedback");
     export const MaxReached = lf("Maximum count reached for this item");
+    export const ExceedsMaxLength = lf("Exceeds maximum length");
+    export const MustBeANumber = lf("Must be a number");
+    export const BelowMin = lf("Below minimum value");
+    export const ExceedsMax = lf("Exceeds maximum value");
+    export const InvalidValue = lf("Invalid value");
 }
 
 export namespace Ticks {
@@ -77,6 +82,7 @@ export namespace Ticks {
     export const Print = "teachertool.print";
     export const UnhandledEvalError = "teachertool.evaluateerror";
     export const FeedbackForm = "teachertool.feedbackform";
+    export const ParamErrorMissingMessage = "teachertool.paramerrormissingmessage";
 }
 
 namespace Misc {

diff --git a/teachertool/src/state/helpers.ts b/teachertool/src/state/helpers.ts
@@ -5,6 +5,7 @@ import { Checklist } from "../types/checklist";
 import { stateAndDispatch } from "./appStateContext";
 import { AppState } from "./state";
 import { Strings } from "../constants";
+import { CriteriaParameter } from "../types/criteriaParameters";
 
 export function getCatalogCriteriaWithId(id: string): CatalogCriteria | undefined {
     const { state } = stateAndDispatch();
@@ -15,6 +16,11 @@ export function getCriteriaInstanceWithId(state: AppState, id: string): Criteria
     return state.checklist.criteria.find(c => c.instanceId === id);
 }
 
+export function getParameterDefinition(catalogCriteriaId: string, paramName: string): CriteriaParameter | undefined {
+    const catalogCriteria = getCatalogCriteriaWithId(catalogCriteriaId);
+    return catalogCriteria?.params?.find(p => p.name === paramName);
+}
+
 export function getParameterValue(state: AppState, instanceId: string, paramName: string): string | undefined {
     const instance = getCriteriaInstanceWithId(state, instanceId);
     return instance?.params?.find(p => p.name === paramName)?.value;

diff --git a/teachertool/src/transforms/loadCatalogAsync.ts b/teachertool/src/transforms/loadCatalogAsync.ts
@@ -8,7 +8,6 @@ const prodFiles = [
     "/teachertool/catalog-shared.json", // shared across all targets
     "/teachertool/catalog.json", // target-specific catalog
 ];
-
 export async function loadCatalogAsync() {
     const { dispatch } = stateAndDispatch();
     const fullCatalog = await loadTestableCollectionFromDocsAsync<CatalogCriteria>(prodFiles, "criteria");

diff --git a/teachertool/src/transforms/runSingleEvaluateAsync.ts b/teachertool/src/transforms/runSingleEvaluateAsync.ts
@@ -13,6 +13,8 @@ import { mergeEvalResult } from "./mergeEvalResult";
 import { setEvalResult } from "./setEvalResult";
 import { setUserFeedback } from "./setUserFeedback";
 import { Strings, Ticks } from "../constants";
+import { SystemParameter } from "../types/criteriaParameters";
+import { validateParameterValue } from "../utils/validateParameterValue";
 
 function generateValidatorPlan(
     criteriaInstance: CriteriaInstance,
@@ -53,8 +55,11 @@ function generateValidatorPlan(
             return undefined;
         }
 
-        if (catalogParam.type === "system" && catalogParam.key) {
-            param.value = getSystemParameter(catalogParam.key, teacherTool);
+        if (catalogParam.type === "system") {
+            const systemParam = catalogParam as SystemParameter;
+            if (systemParam.key) {
+                param.value = getSystemParameter(systemParam.key, teacherTool);
+            }
             if (!param.value) {
                 param.value = catalogParam.default;
             }
@@ -71,6 +76,17 @@ function generateValidatorPlan(
             return undefined;
         }
 
+        const validationResult = validateParameterValue(catalogParam, param.value);
+        if (!validationResult.valid) {
+            if (showErrors) {
+                logError(ErrorCode.invalidParameterValue, validationResult.message, {
+                    catalogId: criteriaInstance.catalogCriteriaId,
+                    paramName: param.name,
+                });
+            }
+            return undefined;
+        }
+
         for (const path of catalogParam.paths) {
             jp.apply(plan, path, () => param.value);
         }

diff --git a/teachertool/src/types/criteria.ts b/teachertool/src/types/criteria.ts
@@ -1,4 +1,5 @@
 import { UserFeedback } from ".";
+import { CriteriaParameter } from "./criteriaParameters";
 
 // A criteria defined in the catalog of all possible criteria for the user to choose from when creating a checklist.
 export interface CatalogCriteria {
@@ -22,16 +23,6 @@ export interface CriteriaInstance {
     userFeedback?: UserFeedback;
 }
 
-// Represents a parameter definition in a catalog criteria.
-export type CriteriaParameterType = "string" | "longString" | "number" | "block" | "system";
-export interface CriteriaParameter {
-    name: string;
-    type: CriteriaParameterType;
-    default: string | undefined;
-    key: string | undefined;
-    paths: string[]; // The json path(s) to update with the parameter value in the catalog criteria.
-}
-
 // Represents a parameter value in a criteria instance.
 export interface CriteriaParameterValue {
     name: string;

diff --git a/teachertool/src/types/criteriaParameters.ts b/teachertool/src/types/criteriaParameters.ts
@@ -0,0 +1,47 @@
+export type CriteriaParameterType = "string" | "longString" | "number" | "block" | "system";
+
+// Represents a parameter definition in a catalog criteria.
+export type CriteriaParameterBase = {
+    name: string;
+    type: CriteriaParameterType;
+    default: string | undefined;
+    paths: string[]; // The json path(s) to update with the parameter value in the catalog criteria.
+}
+
+export type StringParameterBase = CriteriaParameterBase & {
+    maxCharacters?: number;
+}
+
+export type StringParameter = StringParameterBase & {
+    type: "string";
+}
+
+export type LongStringParameter = StringParameterBase & {
+    type: "longString";
+}
+
+export type NumberParameter = CriteriaParameterBase & {
+    type: "number";
+    min?: number;
+    max?: number;
+}
+
+export type BlockParameter = CriteriaParameterBase & {
+    type: "block";
+}
+
+/**
+ * System parameters are fields that can change for a criteria but which are not set directly by the user.
+ * For example, the project id could be a parameter, but we fill it automatically at eval-time based on the loaded project.
+ */
+export type SystemParameter = CriteriaParameterBase & {
+    type: "system";
+    key?: string;
+}
+
+export type CriteriaParameter = StringParameter | LongStringParameter | NumberParameter | BlockParameter | SystemParameter;
+
+export interface CriteriaParameterValidationResult {
+    valid: boolean;
+    message?: string;
+}
diff --git a/teachertool/src/types/errorCode.ts b/teachertool/src/types/errorCode.ts
@@ -32,4 +32,6 @@ export enum ErrorCode {
     signInFailed = "signInFailed",
     loginApiError = "loginApiError",
     authCheckFailed = "authCheckFailed",
+    unrecognizedParameterType = "unrecognizedParameterType",
+    invalidParameterValue = "invalidParameterValue",
 }
diff --git a/teachertool/src/utils/validateParameterValue.ts b/teachertool/src/utils/validateParameterValue.ts
@@ -0,0 +1,67 @@
+import { Strings } from "../constants";
+import {
+    CriteriaParameter,
+    CriteriaParameterValidationResult,
+    LongStringParameter,
+    NumberParameter,
+    StringParameter,
+    StringParameterBase,
+} from "../types/criteriaParameters";
+
+export function validateParameterValue(param: CriteriaParameter, value: string): CriteriaParameterValidationResult {
+    switch (param.type) {
+        case "string":
+            return validateStringParameter(param, value);
+        case "longString":
+            return validateLongStringParameter(param, value);
+        case "number":
+            return validateNumberParameter(param, value);
+        case "block":
+        // Fall through to default case.
+        case "system":
+        // Fall through to default case.
+        default:
+            return { valid: true } as CriteriaParameterValidationResult;
+    }
+}
+
+function validateStringParameterBase(param: StringParameterBase, value: string): CriteriaParameterValidationResult {
+    if (!value) return { valid: true }; // Unset is okay for initial value
+
+    if (param.maxCharacters && value.length > param.maxCharacters) {
+        return { valid: false, message: Strings.ExceedsMaxLength };
+    }
+    return { valid: true };
+}
+
+function validateStringParameter(param: StringParameter, value: string): CriteriaParameterValidationResult {
+    return validateStringParameterBase(param, value);
+}
+
+function validateLongStringParameter(param: LongStringParameter, value: string): CriteriaParameterValidationResult {
+    return validateStringParameterBase(param, value);
+}
+
+function validateNumberParameter(param: NumberParameter, value: string): CriteriaParameterValidationResult {
+    // Ensure the value is numeric and within the specified range.
+    const num = Number(value);
+    if (isNaN(num)) {
+        return {
+            valid: false,
+            message: Strings.MustBeANumber,
+        };
+    }
+    if (param.min !== undefined && num < param.min) {
+        return {
+            valid: false,
+            message: Strings.BelowMin,
+        };
+    }
+    if (param.max !== undefined && num > param.max) {
+        return {
+            valid: false,
+            message: Strings.ExceedsMax,
+        };
+    }
+    return { valid: true };
+}