Skip to content

Commit

Permalink
handle user cancellation for variables (#11406)
Browse files Browse the repository at this point in the history 
Allow a variable to throw a cancellation error to notify the variable
resolver that the resolution process should be aborted.

Remove `checkAllResolved` option from `VariableResolveOptions`.
  • Loading branch information
@paul-marechal
paul-marechal authored Jul 13, 2022
1 parent 433bed8 commit e44aac7
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 89 deletions.
3 changes: 1 addition & 2 deletions packages/debug/src/browser/debug-session-manager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,7 @@ export class DebugSessionManager {
context: options.workspaceFolderUri ? new URI(options.workspaceFolderUri) : undefined,
configurationSection: 'launch',
commandIdVariables,
configuration,
checkAllResolved: true
configuration
});

if (configuration) {
Expand Down
8 changes: 4 additions & 4 deletions packages/task/src/browser/process/process-task-resolver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,20 +62,20 @@ export class ProcessTaskResolver implements TaskResolver {
const result: ProcessTaskConfiguration = {
...processTaskConfig,
command: await this.variableResolverService.resolve(processTaskConfig.command, variableResolverOptions),
args: processTaskConfig.args ? await this.variableResolverService.resolveArray(processTaskConfig.args, variableResolverOptions) : undefined,
args: processTaskConfig.args ? await this.variableResolverService.resolve(processTaskConfig.args, variableResolverOptions) : undefined,
windows: processTaskConfig.windows ? {
command: await this.variableResolverService.resolve(processTaskConfig.windows.command, variableResolverOptions),
args: processTaskConfig.windows.args ? await this.variableResolverService.resolveArray(processTaskConfig.windows.args, variableResolverOptions) : undefined,
args: processTaskConfig.windows.args ? await this.variableResolverService.resolve(processTaskConfig.windows.args, variableResolverOptions) : undefined,
options: processTaskConfig.windows.options
} : undefined,
osx: processTaskConfig.osx ? {
command: await this.variableResolverService.resolve(processTaskConfig.osx.command, variableResolverOptions),
args: processTaskConfig.osx.args ? await this.variableResolverService.resolveArray(processTaskConfig.osx.args, variableResolverOptions) : undefined,
args: processTaskConfig.osx.args ? await this.variableResolverService.resolve(processTaskConfig.osx.args, variableResolverOptions) : undefined,
options: processTaskConfig.osx.options
} : undefined,
linux: processTaskConfig.linux ? {
command: await this.variableResolverService.resolve(processTaskConfig.linux.command, variableResolverOptions),
args: processTaskConfig.linux.args ? await this.variableResolverService.resolveArray(processTaskConfig.linux.args, variableResolverOptions) : undefined,
args: processTaskConfig.linux.args ? await this.variableResolverService.resolve(processTaskConfig.linux.args, variableResolverOptions) : undefined,
options: processTaskConfig.linux.options
} : undefined,
options: {
Expand Down
18 changes: 11 additions & 7 deletions packages/variable-resolver/src/browser/common-variable-contribution.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { ResourceContextKey } from '@theia/core/lib/browser/resource-context-key
import { VariableInput } from './variable-input';
import { QuickInputService, QuickPickValue } from '@theia/core/lib/browser';
import { MaybeArray, RecursivePartial } from '@theia/core/lib/common/types';
import { cancelled } from '@theia/core/lib/common/cancellation';

@injectable()
export class CommonVariableContribution implements VariableContribution {
Expand Down Expand Up @@ -76,14 +77,17 @@ export class CommonVariableContribution implements VariableContribution {
});
variables.registerVariable({
name: 'command',
resolve: async (_, name, __, commandIdVariables, configuration) => {
let commandId = name;
if (name && commandIdVariables) {
const mappedValue = commandIdVariables[name];
commandId = mappedValue ? mappedValue : name;
resolve: async (contextUri, commandId, configurationSection, commandIdVariables, configuration) => {
if (commandId) {
if (commandIdVariables?.[commandId]) {
commandId = commandIdVariables[commandId];
}
const result = await this.commands.executeCommand(commandId, configuration);
// eslint-disable-next-line no-null/no-null
if (result === null) {
throw cancelled();
}
}
const result = commandId && await this.commands.executeCommand(commandId, configuration);
return result ? result : undefined;
}
});
variables.registerVariable({
Expand Down
60 changes: 26 additions & 34 deletions packages/variable-resolver/src/browser/variable-resolver-service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,9 @@
// *****************************************************************************

import * as chai from 'chai';
import { Container, ContainerModule } from '@theia/core/shared/inversify';
import { ILogger } from '@theia/core/lib/common';
import { MockLogger } from '@theia/core/lib/common/test/mock-logger';
import { Variable, VariableRegistry } from './variable';
import { Container } from '@theia/core/shared/inversify';
import { cancelled } from '@theia/core/lib/common';
import { VariableRegistry } from './variable';
import { VariableResolverService } from './variable-resolver-service';

const expect = chai.expect;
Expand All @@ -31,37 +30,25 @@ before(() => {
describe('variable-resolver-service', () => {

let testContainer: Container;

before(() => {
testContainer = new Container();
const module = new ContainerModule((bind, unbind, isBound, rebind) => {
bind(ILogger).to(MockLogger);
bind(VariableRegistry).toSelf().inSingletonScope();
bind(VariableResolverService).toSelf();
});
testContainer.load(module);
});

let variableRegistry: VariableRegistry;
let variableResolverService: VariableResolverService;

beforeEach(() => {
testContainer = new Container();
testContainer.bind(VariableRegistry).toSelf().inSingletonScope();
testContainer.bind(VariableResolverService).toSelf().inSingletonScope();
variableRegistry = testContainer.get(VariableRegistry);
variableRegistry.registerVariable({
name: 'file',
description: 'current file',
resolve: () => Promise.resolve('package.json')
});
variableRegistry.registerVariable({
name: 'lineNumber',
description: 'current line number',
resolve: () => Promise.resolve('6')
});
variableResolverService = testContainer.get(VariableResolverService);

const variables: Variable[] = [
{
name: 'file',
description: 'current file',
resolve: () => Promise.resolve('package.json')
},
{
name: 'lineNumber',
description: 'current line number',
resolve: () => Promise.resolve('6')
}
];
variables.forEach(v => variableRegistry.registerVariable(v));
});

it('should resolve known variables in a text', async () => {
Expand All @@ -81,11 +68,16 @@ describe('variable-resolver-service', () => {
expect(resolved).is.equal('workspace: ${workspaceRoot}; file: package.json; line: 6');
});

it('should check if all variables are resolved', async () => {
const options = {
checkAllResolved: true
};
const resolved = await variableResolverService.resolve('workspace: ${command:testCommand}; file: ${file}; line: ${lineNumber}', options);
it('should return undefined when a variable throws with `cancelled()` while resolving', async () => {
variableRegistry.registerVariable({
name: 'command',
resolve: (contextUri, commandId) => {
if (commandId === 'testCommand') {
throw cancelled();
}
}
});
const resolved = await variableResolverService.resolve('workspace: ${command:testCommand}; file: ${file}; line: ${lineNumber}');
expect(resolved).equal(undefined);
});
});
82 changes: 41 additions & 41 deletions packages/variable-resolver/src/browser/variable-resolver-service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
import { injectable, inject } from '@theia/core/shared/inversify';
import { VariableRegistry } from './variable';
import URI from '@theia/core/lib/common/uri';
import { JSONExt, ReadonlyJSONValue } from '@theia/core/shared/@phosphor/coreutils';
import { CommandIdVariables } from '../common/variable-types';
import { isCancelled } from '@theia/core';

export interface VariableResolveOptions {
context?: URI;
Expand All @@ -30,8 +30,6 @@ export interface VariableResolveOptions {
configurationSection?: string;
commandIdVariables?: CommandIdVariables;
configuration?: unknown;
// Return 'undefined' if not all variables were successfully resolved.
checkAllResolved?: boolean;
}

/**
Expand All @@ -46,32 +44,35 @@ export class VariableResolverService {

/**
* Resolve the variables in the given string array.
* @param value The array of data to resolve
* @param options options of the variable resolution
* @returns promise resolved to the provided string array with already resolved variables.
* Never reject.
* @param value The array of data to resolve variables in.
* @param options Options of the variable resolution.
* @returns Promise to array with variables resolved. Never rejects.
*
* @deprecated since 1.28.0 use {@link resolve} instead.
*/
resolveArray(value: string[], options: VariableResolveOptions = {}): Promise<string[] | undefined> {
return this.resolve(value, options);
}

/**
* Resolve the variables in the given string.
* @param value Data to resolve
* @param options options of the variable resolution
* @returns promise resolved to the provided string with already resolved variables.
* Never reject.
* Resolve the variables for all strings found in the object and nested objects.
* @param value Data to resolve variables in.
* @param options Options of the variable resolution
* @returns Promise to object with variables resolved. Returns `undefined` if a variable resolution was cancelled.
*/
async resolve<T>(value: T, options: VariableResolveOptions = {}): Promise<T | undefined> {
const context = new VariableResolverService.Context(this.variableRegistry, options);
const resolved = await this.doResolve(value, context);
if (options.checkAllResolved && !context.allDefined()) {
return undefined;
try {
return await this.doResolve(value, context);
} catch (error) {
if (isCancelled(error)) {
return undefined;
}
throw error;
}
return resolved as any;
}

protected async doResolve(value: Object | undefined, context: VariableResolverService.Context): Promise<Object | undefined> {
protected async doResolve(value: any, context: VariableResolverService.Context): Promise<any> {
// eslint-disable-next-line no-null/no-null
if (value === undefined || value === null) {
return value;
Expand Down Expand Up @@ -128,6 +129,7 @@ export class VariableResolverService {
}
}
export namespace VariableResolverService {

export class Context {

protected readonly resolved = new Map<string, string | undefined>();
Expand All @@ -141,45 +143,43 @@ export namespace VariableResolverService {
return this.resolved.get(name);
}

allDefined(): boolean {
for (const value of this.resolved.values()) {
if (value === undefined) {
return false;
}
}
return true;
}

async resolve(name: string): Promise<void> {
if (this.resolved.has(name)) {
return;
}
try {
let variableName = name;
let argument: string | undefined;
const parts = name.split(':');
const parts = name.split(':', 2);
if (parts.length > 1) {
variableName = parts[0];
argument = parts[1];
}
const variable = this.variableRegistry.getVariable(variableName);
const value =
variable &&
(await variable.resolve(
this.options.context,
argument,
this.options.configurationSection,
this.options.commandIdVariables,
this.options.configuration
));
// eslint-disable-next-line no-null/no-null
const stringValue = value !== undefined && value !== null && JSONExt.isPrimitive(value as ReadonlyJSONValue) ? String(value) : undefined;
this.resolved.set(name, stringValue);
const resolved = await variable?.resolve(
this.options.context,
argument,
this.options.configurationSection,
this.options.commandIdVariables,
this.options.configuration
);
if (
typeof resolved === 'bigint' ||
typeof resolved === 'boolean' ||
typeof resolved === 'number' ||
typeof resolved === 'string'
) {
this.resolved.set(name, `${resolved}`);
} else {
this.resolved.set(name, undefined);
}
} catch (e) {
console.error(`Failed to resolved '${name}' variable`, e);
if (isCancelled(e)) {
throw e;
}
this.resolved.set(name, undefined);
console.error(`Failed to resolve '${name}' variable:`, e);
}
}

}
}
3 changes: 2 additions & 1 deletion packages/variable-resolver/src/browser/variable.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ export interface Variable {
configurationSection?: string,
commandIdVariables?: CommandIdVariables,
configuration?: unknown
): MaybePromise<Object | undefined>;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
): MaybePromise<any>;
}

export const VariableContribution = Symbol('VariableContribution');
Expand Down

0 comments on commit e44aac7

Please sign in to comment.