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
caraml-dev · Jul 23, 2024 · 1c83ad3 · 1c83ad3
1 parent dd0b448
commit 1c83ad3
Show file tree

Hide file tree

Showing 7 changed files with 55 additions and 70 deletions.
diff --git 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
@@ -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
@@ -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/...omponents/form/components/experiment_config/components/client_config/ClientConfigPanel.js b/...omponents/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">
           <EuiComboBoxSelect
             fullWidth

diff --git a/ui/src/router/components/form/components/experiment_config/validation/schema.js b/ui/src/router/components/form/components/experiment_config/validation/schema.js
@@ -15,21 +15,17 @@ const experimentSchema = yup.object().shape({
 
 const variableSchema = yup.object().shape({
   required: yup.boolean(),
-  field_source: yup.mixed().when("required", {
+  field_source: yup.string().when("required", {
     is: true,
-    then: fieldSourceSchema.required(
-      "Select the field source for the required variable"
-    ),
+    then: fieldSourceSchema,
   }),
-  field: yup.mixed().when("required", {
+  field: yup.string().when("required", {
     is: true,
-    then: fieldSchema("field_source").required(
-      "Specify the field name for the required variable"
-    ),
+    then: fieldSchema("field_source"),
   }),
 });
 
-const standardExperimentConfigSchema = (engineProps) =>
+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
@@ -130,25 +130,25 @@ 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;
     return routes.map((r) => r.id).includes(value);
   });
 
 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
@@ -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"