From 1c83ad3708df0b079bbc94bada46117d63337a0d Mon Sep 17 00:00:00 2001 From: Ewe Zi Yi <36802364+deadlycoconuts@users.noreply.github.com> Date: Tue, 23 Jul 2024 15:32:54 +0800 Subject: [PATCH] fix(ui): Fix yup validation bugs (#387) * Fix incorrect client id error being displayed * Update yup validation schemas * Update mlp * Undo unnecessary line changes * Temporarily remove codecov checks for api server --- .github/workflows/turing.yaml | 8 --- ui/package.json | 2 +- ui/src/components/validation.js | 24 +++---- .../client_config/ClientConfigPanel.js | 4 +- .../experiment_config/validation/schema.js | 14 ++-- .../components/form/validation/schema.js | 65 ++++++++++--------- ui/yarn.lock | 8 +-- 7 files changed, 55 insertions(+), 70 deletions(-) diff --git a/.github/workflows/turing.yaml b/.github/workflows/turing.yaml index 71a722bc7..fcbbb3975 100644 --- a/.github/workflows/turing.yaml +++ b/.github/workflows/turing.yaml @@ -263,14 +263,6 @@ jobs: working-directory: api args: --timeout 3m --verbose - - uses: codecov/codecov-action@v4 - with: - flags: api-test - name: api-test - token: ${{ secrets.CODECOV_TOKEN }} - working-directory: api - codecov_yml_path: ../.github/workflows/codecov-config/codecov.yml - test-engines-router: runs-on: ubuntu-latest defaults: diff --git a/ui/package.json b/ui/package.json index fd1ac7162..0c623bed8 100644 --- a/ui/package.json +++ b/ui/package.json @@ -7,7 +7,7 @@ "//": "[@sentry/browser] pinned to 7.118.0 because craco/module federation has issues resolving this dependency; see", "//": "https://github.com/caraml-dev/turing/pull/384#discussion_r1666418144 for more details", "dependencies": { - "@caraml-dev/ui-lib": "^1.13.0-build.3-a234b6b", + "@caraml-dev/ui-lib": "^1.13.0-build.4-09c363a", "@elastic/datemath": "^5.0.3", "@elastic/eui": "88.2.0", "@emotion/css": "^11.11.2", diff --git a/ui/src/components/validation.js b/ui/src/components/validation.js index 6bfa88baf..68d4931a8 100644 --- a/ui/src/components/validation.js +++ b/ui/src/components/validation.js @@ -1,29 +1,25 @@ import * as yup from "yup"; -const httpHeaderSchema = yup +const httpHeaderSchema = (_) => yup .string() .required('Valid Request Header value is required, e.g. "x-session-id"'); -const jsonPathSchema = yup +const jsonPathSchema = (_) => yup .string() - .required( - 'Valid Request Payload json path is required, e.g. "my_object.session_id"' - ); + .required('Valid Request Payload json path is required, e.g. "my_object.session_id"'); -const predictionContextSchema = yup -.string() -.required( - 'Valid Prediction Context value is required, e.g. "model_name"' -); +const predictionContextSchema = (_) => yup + .string() + .required('Valid Prediction Context value is required, e.g. "model_name"'); -export const fieldSourceSchema = yup - .mixed() +export const fieldSourceSchema = (_) => yup + .string() .required("Valid Field Source should be selected") .oneOf(["header", "payload", "prediction_context"], "Valid Field Source should be selected"); -export const fieldSchema = (fieldSource) => +export const fieldSchema = (fieldSource) => (_) => yup - .mixed() + .string() .when(fieldSource, { is: "header", then: httpHeaderSchema, diff --git a/ui/src/router/components/form/components/experiment_config/components/client_config/ClientConfigPanel.js b/ui/src/router/components/form/components/experiment_config/components/client_config/ClientConfigPanel.js index f7279ff81..bdb0e80ec 100644 --- a/ui/src/router/components/form/components/experiment_config/components/client_config/ClientConfigPanel.js +++ b/ui/src/router/components/form/components/experiment_config/components/client_config/ClientConfigPanel.js @@ -74,8 +74,8 @@ export const ClientConfigPanel = ({ client, onChangeHandler, errors = {} }) => { content="Select your Client ID from the provided list" /> } - isInvalid={!!errors.id} - error={errors.id} + isInvalid={!!errors.username} + error={errors.username} display="row"> +const standardExperimentConfigSchema = (engineProps) => (_) => yup.object().shape({ client: engineProps?.standard_experiment_manager_config ?.client_selection_enabled diff --git a/ui/src/router/components/form/validation/schema.js b/ui/src/router/components/form/validation/schema.js index 13f22294a..141493229 100644 --- a/ui/src/router/components/form/validation/schema.js +++ b/ui/src/router/components/form/validation/schema.js @@ -130,7 +130,7 @@ const routeSchema = yup.object().shape({ }); const validRouteSchema = yup - .mixed() + .object() .test("valid-route", "Valid route is required", function(value) { const configSchema = this.from.slice(-1).pop(); const { routes } = configSchema.value.config; @@ -138,17 +138,17 @@ const validRouteSchema = yup }); const ruleConditionSchema = yup.object().shape({ - field_source: fieldSourceSchema, - field: fieldSchema("field_source"), + field_source: fieldSourceSchema(), + field: fieldSchema("field_source")(), operator: yup - .mixed() + .string() .oneOf(["in"], "One of supported operators should be specified"), values: yup .array(yup.string()) - .required("At least one value should be provided"), + .min(1, "At least one value should be provided"), }); -const defaultTrafficRuleSchema = yup.object().shape({ +const defaultTrafficRuleSchema = (_) => yup.object().shape({ routes: yup .array() .of(validRouteSchema) @@ -244,7 +244,7 @@ const autoscalingPolicySchema = yup.object().shape({ const enricherSchema = yup.object().shape({ type: yup - .mixed() + .string() .required("Valid Enricher type should be selected") .oneOf(["nop", "docker"], "Valid Enricher type should be selected"), }); @@ -256,7 +256,7 @@ const dockerImageSchema = yup "Valid Docker Image value should be provided, e.g. kennethreitz/httpbin:latest" ); -const dockerDeploymentSchema = (maxAllowedReplica) => +const dockerDeploymentSchema = (maxAllowedReplica) => (_) => yup.object().shape({ image: dockerImageSchema.required("Docker Image is required"), endpoint: yup.string().required("Endpoint value is required"), @@ -271,7 +271,7 @@ const dockerDeploymentSchema = (maxAllowedReplica) => autoscaling_policy: autoscalingPolicySchema, }); -const pyfuncDeploymentSchema = (maxAllowedReplica) => +const pyfuncDeploymentSchema = (maxAllowedReplica) => (_) => yup.object().shape({ project_id: yup.number().integer().required("Project ID is required"), ensembler_id: yup.number().integer().required("Ensembler ID is required"), @@ -287,7 +287,7 @@ const mappingSchema = yup.object().shape({ route: yup.string().required("Treatment needs to be mapped back to a route"), }); -const standardEnsemblerConfigSchema = yup +const standardEnsemblerConfigSchema = (_) => yup .object() .shape({ route_name_path: yup.string().nullable(), @@ -310,7 +310,7 @@ const standardEnsemblerConfigSchema = yup } ); -const bigQueryConfigSchema = yup.object().shape({ +const bigQueryConfigSchema = (_) => yup.object().shape({ table: yup .string() .required("BigQuery table name is required") @@ -321,7 +321,7 @@ const bigQueryConfigSchema = yup.object().shape({ service_account_secret: yup.string().required("Service Account is required"), }); -const kafkaConfigSchema = yup.object().shape({ +const kafkaConfigSchema = (_) => yup.object().shape({ brokers: yup .string() .required("Kafka broker(s) is required") @@ -337,7 +337,7 @@ const kafkaConfigSchema = yup.object().shape({ "A valid Kafka topic name may only contain letters, numbers, dot, hyphen or underscore" ), serialization_format: yup - .mixed() + .string() .required("Serialzation format should be selected") .oneOf( ["json", "protobuf"], @@ -364,10 +364,9 @@ const schema = (maxAllowedReplica) => [ .test("unique-rule-names", validateRuleNames), routes: yup .array(routeSchema) - .required() .unique("id", "Route Id must be unique") .min(1, "At least one route should be configured") - .when(["rules"], (rules, schema) => { + .when(["rules"], ([rules], schema) => { if (rules.length > 0) { return schema.test("no-dangling-routes", validateDanglingRoutes); } @@ -391,20 +390,19 @@ const schema = (maxAllowedReplica) => [ config: yup.object().shape({ experiment_engine: yup.object().shape({ type: yup - .mixed() + .string() .required("Valid Experiment Engine should be selected") - .when("$experimentEngineOptions", (options, schema) => + .when("$experimentEngineOptions", ([options], schema) => schema.oneOf(options, "Valid Experiment Engine should be selected") ), - config: yup.mixed().when("type", (engine, schema) => + config: yup.object().when("type", ([engine], schema) => engine === "nop" ? schema - : yup - .mixed() - .when("$getEngineProperties", (getEngineProperties) => { + : schema + .when("$getEngineProperties", ([getEngineProperties], schema) => { const engineProps = getEngineProperties(engine); return engineProps.type === "standard" - ? standardExperimentConfigSchema(engineProps) + ? standardExperimentConfigSchema(engineProps)(schema) : engineProps.custom_experiment_manager_config ?.parsed_experiment_config_schema || schema; }) @@ -418,7 +416,7 @@ const schema = (maxAllowedReplica) => [ switch (value.type) { case "docker": return enricherSchema.concat( - dockerDeploymentSchema(maxAllowedReplica) + dockerDeploymentSchema(maxAllowedReplica)() ); default: return enricherSchema; @@ -430,27 +428,30 @@ const schema = (maxAllowedReplica) => [ config: yup.object().shape({ ensembler: yup.object().shape({ type: yup - .mixed() + .string() .required("Valid Ensembler type should be selected") .oneOf( ["nop", "docker", "standard", "pyfunc"], "Valid Ensembler type should be selected" ), - nop_config: yup.mixed().when("type", { + nop_config: yup.object().when("type", { is: "nop", - then: yup.object().shape({ + then: (_) => yup.object().shape({ final_response_route_id: validRouteSchema, }), }), - docker_config: yup.mixed().when("type", { + some_field: yup.string().when("some_other_field", ([some_other_field], schema) => + some_other_field ? yup.string().required("this is needed") : schema + ), + docker_config: yup.object().when("type", { is: "docker", then: dockerDeploymentSchema(maxAllowedReplica), }), - standard_config: yup.mixed().when("type", { + standard_config: yup.object().when("type", { is: "standard", then: standardEnsemblerConfigSchema, }), - pyfunc_config: yup.mixed().when("type", { + pyfunc_config: yup.object().when("type", { is: "pyfunc", then: pyfuncDeploymentSchema(maxAllowedReplica), }), @@ -461,17 +462,17 @@ const schema = (maxAllowedReplica) => [ config: yup.object().shape({ log_config: yup.object().shape({ result_logger_type: yup - .mixed() + .string() .required("Valid Results Logging type should be selected") .oneOf( ["nop", "bigquery", "kafka"], "Valid Results Logging type should be selected" ), - bigquery_config: yup.mixed().when("result_logger_type", { + bigquery_config: yup.object().when("result_logger_type", { is: "bigquery", then: bigQueryConfigSchema, }), - kafka_config: yup.mixed().when("result_logger_type", { + kafka_config: yup.object().when("result_logger_type", { is: "kafka", then: kafkaConfigSchema, }), diff --git a/ui/yarn.lock b/ui/yarn.lock index af9460c18..8288433a5 100644 --- a/ui/yarn.lock +++ b/ui/yarn.lock @@ -1267,10 +1267,10 @@ resolved "https://registry.yarnpkg.com/@bcoe/v8-coverage/-/v8-coverage-0.2.3.tgz#75a2e8b51cb758a7553d6804a5932d7aace75c39" integrity sha512-0hYQ8SB4Db5zvZB4axdMHGwEaQjkZzFjQiN9LVYvIFB2nSUHW9tYpxWriPrWDASIxiaXax83REcLxuSdnGPZtw== -"@caraml-dev/ui-lib@^1.13.0-build.3-a234b6b": - version "1.13.0-build.3-a234b6b" - resolved "https://registry.yarnpkg.com/@caraml-dev/ui-lib/-/ui-lib-1.13.0-build.3-a234b6b.tgz#f24d09d00c77f1288953da8f06893970cdeeda13" - integrity sha512-aiOWz2QNKJ3uK8No8r9FAfHHQNHe9cvSV5Bgznj7PRaTMpJbfDDSY8U+z+B60SjdtZ7qeeTiC14cEpmv/Ot07Q== +"@caraml-dev/ui-lib@^1.13.0-build.4-09c363a": + version "1.13.0-build.4-09c363a" + resolved "https://registry.yarnpkg.com/@caraml-dev/ui-lib/-/ui-lib-1.13.0-build.4-09c363a.tgz#c80987fa5c7989450095d354acf51a023ca94e15" + integrity sha512-kqb/5koS2IQtdLJIh5tk+t30ZX2GvvQO7kAuE9oKXKlwBPr83Q+lvLxcvSg+rxILYxjOUeNn2izoej0ZGMDoRg== dependencies: "@react-oauth/google" "0.12.1" classnames "^2.5.1"