From b0422a714394e1074ead721cbd9fcf2e8ca6f2b0 Mon Sep 17 00:00:00 2001 From: Akos Kitta Date: Fri, 23 Jun 2023 18:02:30 +0200 Subject: [PATCH] fix: flawed config handling in the FQBN 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 --- .../src/browser/contributions/ino-language.ts | 2 - .../src/common/protocol/boards-service.ts | 108 ++++++++++++++--- .../src/test/common/boards-service.test.ts | 109 ++++++++++++++++++ 3 files changed, 201 insertions(+), 18 deletions(-) diff --git a/arduino-ide-extension/src/browser/contributions/ino-language.ts b/arduino-ide-extension/src/browser/contributions/ino-language.ts index 2577d5a73..df7513456 100644 --- 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 index c955b9462..7545fd1d9 100644 --- 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 = {}; + 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 = {}; + 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 = {}; + 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 index d2cae5a53..564f94b62 100644 --- 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';