fix: flawed config handling in the FQBN

Akos Kitta · Akos Kitta · commit b0422a714394 · 2023-06-26T09:04:56.000+02:00
Also relaxed the LS assertions. As turned out, the CLI can detect FQBNs with already appended board config values. Closes #1588 Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
diff --git a/arduino-ide-extension/src/browser/contributions/ino-language.ts b/arduino-ide-extension/src/browser/contributions/ino-language.ts
@@ -6,7 +6,6 @@ import { inject, injectable } from '@theia/core/shared/inversify';
 import { Mutex } from 'async-mutex';
 import {
   ArduinoDaemon,
-  assertSanitizedFqbn,
   BoardsService,
   ExecutableService,
   sanitizeFqbn,
@@ -151,7 +150,6 @@ export class InoLanguage extends SketchContribution {
         }
         return;
       }
-      assertSanitizedFqbn(fqbn);
       const fqbnWithConfig = await this.boardDataStore.appendConfigToFqbn(fqbn);
       if (!fqbnWithConfig) {
         throw new Error(
diff --git a/arduino-ide-extension/src/common/protocol/boards-service.ts b/arduino-ide-extension/src/common/protocol/boards-service.ts
@@ -180,7 +180,7 @@ export namespace BoardSearch {
     'Partner',
     'Arduino@Heart',
   ] as const;
-  export type Type = typeof TypeLiterals[number];
+  export type Type = (typeof TypeLiterals)[number];
   export namespace Type {
     export function is(arg: unknown): arg is Type {
       return typeof arg === 'string' && TypeLiterals.includes(arg as Type);
@@ -347,7 +347,7 @@ export namespace Port {
 
   export namespace Protocols {
     export const KnownProtocolLiterals = ['serial', 'network'] as const;
-    export type KnownProtocol = typeof KnownProtocolLiterals[number];
+    export type KnownProtocol = (typeof KnownProtocolLiterals)[number];
     export namespace KnownProtocol {
       export function is(protocol: unknown): protocol is KnownProtocol {
         return (
@@ -476,29 +476,105 @@ export namespace ConfigOption {
     fqbn: string,
     configOptions: ConfigOption[]
   ): string {
-    if (!configOptions.length) {
-      return fqbn;
+    const failInvalidFQBN = (): never => {
+      throw new Error(`Invalid FQBN: ${fqbn}`);
+    };
+
+    const [vendor, arch, id, rest] = fqbn.split(':');
+    if (!vendor || !arch || !id) {
+      return failInvalidFQBN();
+    }
+
+    const existingOptions: Record<string, string> = {};
+    if (rest) {
+      // If rest exists, it must have the key=value(,key=value)* format. Otherwise, fail.
+      const tuples = rest.split(',');
+      for (const tuple of tuples) {
+        const segments = tuple.split('=');
+        if (segments.length !== 2) {
+          failInvalidFQBN();
+        }
+        const [option, value] = segments;
+        if (!option || !value) {
+          failInvalidFQBN();
+        }
+        if (existingOptions[option]) {
+          console.warn(
+            `Config value already exists for '${option}' on FQBN. Skipping it. Existing value: ${existingOptions[option]}, new value: ${value}, FQBN: ${fqbn}`
+          );
+        } else {
+          existingOptions[option] = value;
+        }
+      }
     }
 
-    const toValue = (values: ConfigValue[]) => {
+    const newOptions: Record<string, string> = {};
+    for (const configOption of configOptions) {
+      const { option, values } = configOption;
+      if (!option) {
+        console.warn(
+          `Detected empty option on config options. Skipping it. ${JSON.stringify(
+            configOption
+          )}`
+        );
+        continue;
+      }
       const selectedValue = values.find(({ selected }) => selected);
-      if (!selectedValue) {
+      if (selectedValue) {
+        const { value } = selectedValue;
+        if (!value) {
+          console.warn(
+            `Detected empty selected value on config options. Skipping it. ${JSON.stringify(
+              configOption
+            )}`
+          );
+          continue;
+        }
+        if (newOptions[option]) {
+          console.warn(
+            `Config value already exists for '${option}' in config options. Skipping it. Existing value: ${
+              newOptions[option]
+            }, new value: ${value}, config option: ${JSON.stringify(
+              configOption
+            )}`
+          );
+        } else {
+          newOptions[option] = value;
+        }
+      } else {
         console.warn(
-          `None of the config values was selected. Values were: ${JSON.stringify(
-            values
+          `None of the config values was selected. Config options was: ${JSON.stringify(
+            configOption
           )}`
         );
-        return undefined;
       }
-      return selectedValue.value;
-    };
-    const options = configOptions
-      .map(({ option, values }) => [option, toValue(values)])
-      .filter(([, value]) => !!value)
+    }
+
+    // Collect all options from the FQBN. Call them existing.
+    // Collect all incoming options to decorate the FQBN with. Call them new.
+    // To keep the order, iterate through the existing ones and append to FQBN.
+    // If a new(er) value exists for the same option, use the new value.
+    // If there is a new value, "mark" it as visited by deleting it from new. Otherwise, use the existing value.
+    // Append all new ones to the FQBN.
+    const mergedOptions: Record<string, string> = {};
+    for (const existing of Object.entries(existingOptions)) {
+      const [option, value] = existing;
+      const newValue = newOptions[option];
+      if (newValue) {
+        mergedOptions[option] = newValue;
+        delete newOptions[option];
+      } else {
+        mergedOptions[option] = value;
+      }
+    }
+    Array.from(Object.entries(newOptions)).forEach(
+      ([option, value]) => (mergedOptions[option] = value)
+    );
+
+    const configSuffix = Object.entries(mergedOptions)
       .map(([option, value]) => `${option}=${value}`)
       .join(',');
-
-    return `${fqbn}:${options}`;
+    return `${vendor}:${arch}:${id}:${configSuffix}`;
   }
 
   export class ConfigOptionError extends Error {
diff --git a/arduino-ide-extension/src/test/common/boards-service.test.ts b/arduino-ide-extension/src/test/common/boards-service.test.ts
@@ -4,6 +4,7 @@ import { expect } from 'chai';
 import {
   AttachedBoardsChangeEvent,
   BoardInfo,
+  ConfigOption,
   getBoardInfo,
   noNativeSerialPort,
   nonSerialPort,
@@ -93,6 +94,114 @@ describe('boards-service', () => {
     });
   });
 
+  describe('ConfigOption#decorate', () => {
+    ['invalid', 'a:b:c:foo=', 'a:b:c:foo=bar=baz', 'a:b:c:foo+bar'].map(
+      (fqbn) =>
+        it(`should error when the FQBN is invalid (FQBN: ${fqbn})`, () => {
+          expect(() => ConfigOption.decorate(fqbn, [])).to.throw(
+            /^Invalid FQBN:*/
+          );
+        })
+    );
+
+    it('should be noop when config options is empty', () => {
+      const fqbn = 'a:b:c:menu1=value1';
+      const actual = ConfigOption.decorate(fqbn, []);
+      expect(actual).to.be.equal(fqbn);
+    });
+
+    it('should not duplicate config options', () => {
+      const fqbn = 'a:b:c:menu1=value1';
+      const actual = ConfigOption.decorate(fqbn, [
+        {
+          label: 'Menu 1',
+          option: 'menu1',
+          values: [
+            { label: 'Value 1', value: 'value1', selected: true },
+            { label: 'Another Value', value: 'another1', selected: false },
+          ],
+        },
+      ]);
+      expect(actual).to.be.equal(fqbn);
+    });
+
+    it('should update config options', () => {
+      const fqbn = 'a:b:c:menu1=value1,menu2=value2';
+      const actual = ConfigOption.decorate(fqbn, [
+        {
+          label: 'Menu 1',
+          option: 'menu1',
+          values: [
+            { label: 'Value 1', value: 'value1', selected: false },
+            { label: 'Another Value', value: 'another1', selected: true },
+          ],
+        },
+      ]);
+      expect(actual).to.be.equal('a:b:c:menu1=another1,menu2=value2');
+    });
+
+    it('should favor first over rest when there are duplicate options (duplicate on FQBN)', () => {
+      const fqbn = 'a:b:c:menu1=value1,menu1=value2';
+      const actual = ConfigOption.decorate(fqbn, [
+        {
+          label: 'Menu 1',
+          option: 'menu1',
+          values: [
+            { label: 'Value 1', value: 'value1', selected: false },
+            { label: 'Another Value', value: 'another1', selected: true },
+          ],
+        },
+      ]);
+      expect(actual).to.be.equal('a:b:c:menu1=another1');
+    });
+
+    it('should favor first over rest when there are duplicate options (duplicate in config)', () => {
+      const fqbn = 'a:b:c';
+      const actual = ConfigOption.decorate(fqbn, [
+        {
+          label: 'Menu 1',
+          option: 'menu1',
+          values: [
+            { label: 'Value 1', value: 'value1', selected: false },
+            { label: 'Another Value', value: 'another1', selected: true },
+          ],
+        },
+        {
+          label: 'Menu 1',
+          option: 'menu1',
+          values: [
+            { label: 'Value 1', value: 'value1', selected: true },
+            { label: 'Another Value', value: 'another1', selected: false },
+          ],
+        },
+      ]);
+      expect(actual).to.be.equal('a:b:c:menu1=another1');
+    });
+
+    it('should not change the existing config order', () => {
+      const fqbn = 'a:b:c:menu1=value1';
+      const actual = ConfigOption.decorate(fqbn, [
+        {
+          label: 'Menu 0',
+          option: 'menu0',
+          values: [
+            { label: 'Value 0', value: 'value0', selected: true },
+            { label: 'Another Value', value: 'another0', selected: false },
+          ],
+        },
+        {
+          label: 'Menu 1',
+          option: 'menu1',
+          values: [
+            { label: 'Value 1', value: 'value1', selected: false },
+            { label: 'Another Value', value: 'another1', selected: true },
+          ],
+        },
+      ]);
+      expect(actual).to.be.equal('a:b:c:menu1=another1,menu0=value0');
+    });
+  });
+
   describe('getBoardInfo', () => {
     const vid = '0x0';
     const pid = '0x1';
