Fix YAML parsing for keys with dots and slashes (#1690)

dashboard/src/components/UpgradeForm/UpgradeForm.test.tsx

@@ -225,6 +225,13 @@ describe("when receiving new props", () => {
     bar1: value1
 `,
     },
+    {
+      description: "set a value with dots and slashes in the key",
+      defaultValues: "foo.bar/foobar: ",
+      deployedValues: "foo.bar/foobar: value",
+      newDefaultValues: "foo.bar/foobar: ",
+      result: "foo.bar/foobar: value\n",
+    },
   ].forEach(t => {
     it(t.description, () => {
       const deployed = {

dashboard/src/components/UpgradeForm/UpgradeForm.tsx

@@ -63,15 +63,12 @@ class UpgradeForm extends React.Component<IUpgradeFormProps, IUpgradeFormState>
       // And we add any possible change made to the original version
       if (mods.length) {
         mods.forEach(modification => {
-          // Transform the JSON Path to the format expected by setValue
-          // /a/b/c => a.b.c
-          const path = modification.path.replace(/^\//, "").replace(/\//g, ".");
           if (modification.op === "remove") {
-            appValues = deleteValue(appValues, path);
+            appValues = deleteValue(appValues, modification.path);
           } else {
             // Transform the modification as a ReplaceOperation to read its value
             const value = (modification as jsonpatch.ReplaceOperation<any>).value;
-            appValues = setValue(appValues, path, value);
+            appValues = setValue(appValues, modification.path, value);
           }
         });
       }

dashboard/src/shared/schema.test.ts

@@ -78,7 +78,7 @@ describe("retrieveBasicFormParams", () => {
       } as JSONSchema4,
       result: [
         {
-          path: "credentials.user",
+          path: "credentials/user",
           value: "andres",
         } as IBasicFormParam,
       ],
@@ -118,11 +118,11 @@ service: ClusterIP
       } as JSONSchema4,
       result: [
         {
-          path: "credentials.admin.user",
+          path: "credentials/admin/user",
           value: "andres",
         } as IBasicFormParam,
         {
-          path: "credentials.admin.pass",
+          path: "credentials/admin/pass",
           value: "myPassword",
         } as IBasicFormParam,
         {
@@ -179,11 +179,11 @@ externalDatabase:
           type: "object",
           children: [
             {
-              path: "externalDatabase.name",
+              path: "externalDatabase/name",
               type: "string",
             },
             {
-              path: "externalDatabase.port",
+              path: "externalDatabase/port",
               type: "integer",
             },
           ],
@@ -218,13 +218,13 @@ describe("getValue", () => {
     {
       description: "should return a nested value",
       values: "foo:\n  bar: foobar",
-      path: "foo.bar",
+      path: "foo/bar",
       result: "foobar",
     },
     {
       description: "should return a deeply nested value",
       values: "foo:\n  bar:\n    foobar: barfoo",
-      path: "foo.bar.foobar",
+      path: "foo/bar/foobar",
       result: "barfoo",
     },
     {
@@ -236,7 +236,7 @@ describe("getValue", () => {
     {
       description: "should ignore an invalid path (nested)",
       values: "foo:\n  bar:\n    foobar: barfoo",
-      path: "not.exists",
+      path: "not/exists",
       result: undefined,
     },
     {
@@ -246,6 +246,18 @@ describe("getValue", () => {
       default: "BAR",
       result: "BAR",
     },
+    {
+      description: "should return a value with slashes in the key",
+      values: "foo/bar: value",
+      path: "foo~1bar",
+      result: "value",
+    },
+    {
+      description: "should return a value with slashes and dots in the key",
+      values: "kubernetes.io/ingress.class: nginx",
+      path: "kubernetes.io~1ingress.class",
+      result: "nginx",
+    },
   ].forEach(t => {
     it(t.description, () => {
       expect(getValue(t.values, t.path, t.default)).toEqual(t.result);
@@ -265,14 +277,14 @@ describe("setValue", () => {
     {
       description: "should set a nested value",
       values: "foo:\n  bar: foobar",
-      path: "foo.bar",
+      path: "foo/bar",
       newValue: "FOOBAR",
       result: "foo:\n  bar: FOOBAR\n",
     },
     {
       description: "should set a deeply nested value",
       values: "foo:\n  bar:\n    foobar: barfoo",
-      path: "foo.bar.foobar",
+      path: "foo/bar/foobar",
       newValue: "BARFOO",
       result: "foo:\n  bar:\n    foobar: BARFOO\n",
     },
@@ -286,31 +298,31 @@ describe("setValue", () => {
     {
       description: "should add a new nested value",
       values: "foo: bar",
-      path: "this.new",
+      path: "this/new",
       newValue: 1,
       result: "foo: bar\nthis:\n  new: 1\n",
       error: false,
     },
     {
       description: "should add a new deeply nested value",
       values: "foo: bar",
-      path: "this.new.value",
+      path: "this/new/value",
       newValue: 1,
       result: "foo: bar\nthis:\n  new:\n    value: 1\n",
       error: false,
     },
     {
       description: "Adding a value for a path partially defined (null)",
       values: "foo: bar\nthis:\n",
-      path: "this.new.value",
+      path: "this/new/value",
       newValue: 1,
       result: "foo: bar\nthis:\n  new:\n    value: 1\n",
       error: false,
     },
     {
       description: "Adding a value for a path partially defined (object)",
       values: "foo: bar\nthis: {}\n",
-      path: "this.new.value",
+      path: "this/new/value",
       newValue: 1,
       result: "foo: bar\nthis: { new: { value: 1 } }\n",
       error: false,
@@ -323,6 +335,20 @@ describe("setValue", () => {
       result: "foo: bar\n",
       error: false,
     },
+    {
+      description: "should add a value with slashes in the key",
+      values: "foo/bar: test",
+      path: "foo~1bar",
+      newValue: "value",
+      result: "foo/bar: value\n",
+    },
+    {
+      description: "should add a value with slashes and dots in the key",
+      values: "kubernetes.io/ingress.class: default",
+      path: "kubernetes.io~1ingress.class",
+      newValue: "nginx",
+      result: "kubernetes.io/ingress.class: nginx\n",
+    },
   ].forEach(t => {
     it(t.description, () => {
       let res: any;
@@ -350,7 +376,7 @@ describe("deleteValue", () => {
   - bar
   - foobar
 `,
-      path: "foo.0",
+      path: "foo/0",
       result: `foo:
   - foobar
 `,

dashboard/src/shared/schema.ts

@@ -2,6 +2,7 @@
 // In particular, it doesn't contain definitions for `get` and `set`
 // that are used in this package
 import * as AJV from "ajv";
+import * as jsonpatch from "fast-json-patch";
 import * as jsonSchema from "json-schema";
 import { isEmpty, set } from "lodash";
 import * as YAML from "yaml";
@@ -38,15 +39,15 @@ export function retrieveBasicFormParams(
           value,
           children:
             properties[propertyKey].type === "object"
-              ? retrieveBasicFormParams(defaultValues, properties[propertyKey], `${itemPath}.`)
+              ? retrieveBasicFormParams(defaultValues, properties[propertyKey], `${itemPath}/`)
               : undefined,
         };
         params = params.concat(param);
       } else {
         // If the property is an object, iterate recursively
         if (schema.properties![propertyKey].type === "object") {
           params = params.concat(
-            retrieveBasicFormParams(defaultValues, properties[propertyKey], `${itemPath}.`),
+            retrieveBasicFormParams(defaultValues, properties[propertyKey], `${itemPath}/`),
           );
         }
       }
@@ -73,12 +74,31 @@ function getDefinedPath(allElementsButTheLast: string[], doc: YAML.ast.Document)
   return currentPath;
 }
 
+function splitPath(path: string): string[] {
+  return (
+    path
+      // ignore the first slash, if exists
+      .replace(/^\//, "")
+      // split by slashes
+      .split("/")
+  );
+}
+
+function unescapePath(path: string[]): string[] {
+  // jsonpath escapes slashes to not mistake then with objects so we need to revert that
+  return path.map(p => jsonpatch.unescapePathComponent(p));
+}
+
+function parsePath(path: string): string[] {
+  return unescapePath(splitPath(path));
+}
+
 function parsePathAndValue(doc: YAML.ast.Document, path: string, value?: any) {
   if (isEmpty(doc.contents)) {
     // If the doc is empty we have an special case
-    return { value: set({}, path, value), splittedPath: [] };
+    return { value: set({}, path.replace(/^\//, ""), value), splittedPath: [] };
   }
-  let splittedPath = path.split(".");
+  let splittedPath = splitPath(path);
   // If the path is not defined (the parent nodes are undefined)
   // We need to change the path and the value to set to avoid accessing
   // the undefined node. For example, if a.b is undefined:
@@ -93,7 +113,7 @@ function parsePathAndValue(doc: YAML.ast.Document, path: string, value?: any) {
     value = set({}, remainingPath.join("."), value);
     splittedPath = splittedPath.slice(0, definedPath.length + 1);
   }
-  return { splittedPath, value };
+  return { splittedPath: unescapePath(splittedPath), value };
 }
 
 // setValue modifies the current values (text) based on a path
@@ -116,7 +136,7 @@ export function deleteValue(values: string, path: string) {
 // getValue returns the current value of an object based on YAML text and its path
 export function getValue(values: string, path: string, defaultValue?: any) {
   const doc = YAML.parseDocument(values);
-  const splittedPath = path.split(".");
+  const splittedPath = parsePath(path);
   const value = (doc as any).getIn(splittedPath);
   return value === undefined || value === null ? defaultValue : value;
 }