From b269289350996a5856847d323389199d1a4d552e Mon Sep 17 00:00:00 2001 From: Thomas Sparks <69657545+thsparks@users.noreply.github.com> Date: Mon, 16 Sep 2024 09:54:14 -0700 Subject: [PATCH] More Parameter Validation and Limitations (Length Limit for AI Prompts) (#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). --- .../teachertool/test/catalog-shared.json | 1 + .../components/CriteriaInstanceDisplay.tsx | 37 ++++++++-- teachertool/src/constants.ts | 6 ++ teachertool/src/state/helpers.ts | 6 ++ .../src/transforms/loadCatalogAsync.ts | 1 - .../src/transforms/runSingleEvaluateAsync.ts | 20 +++++- teachertool/src/types/criteria.ts | 11 +-- teachertool/src/types/criteriaParameters.ts | 47 +++++++++++++ teachertool/src/types/errorCode.ts | 2 + .../src/utils/validateParameterValue.ts | 67 +++++++++++++++++++ 10 files changed, 178 insertions(+), 20 deletions(-) create mode 100644 teachertool/src/types/criteriaParameters.ts create mode 100644 teachertool/src/utils/validateParameterValue.ts diff --git a/common-docs/teachertool/test/catalog-shared.json b/common-docs/teachertool/test/catalog-shared.json index baab5985cae8..f8dbe6d15c17 100644 --- 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 index 50e8f30089e3..0cf977b44726 100644 --- 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,14 +28,36 @@ const InlineInputSegment: React.FC = ({ 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 (
= ({ 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 index ee07e962d639..262dc5831d5c 100644 --- 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 index 61e14759e06e..cd62e7650409 100644 --- 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 index 607f73e72452..594c44446a0b 100644 --- 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(prodFiles, "criteria"); diff --git a/teachertool/src/transforms/runSingleEvaluateAsync.ts b/teachertool/src/transforms/runSingleEvaluateAsync.ts index 06847cd4daf3..755692830993 100644 --- 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 index 80c3422faa9f..2d8ab865d52e 100644 --- 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 new file mode 100644 index 000000000000..4c78e05096cd --- /dev/null +++ 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 index f3222df84a2d..8f0043082d83 100644 --- 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 new file mode 100644 index 000000000000..2f47cd9048f3 --- /dev/null +++ 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 }; +}