Skip to content

Commit

Permalink
Feat(DM): deserialization error handling (#6164)
Browse files Browse the repository at this point in the history 
* started to catch deserialization errors

* continuing to add test cases

* error messages preserved

* showing detailed error for lml parse error

* showing detailed error for lml parse error

* lint fixes

* improved tests

---------

Co-authored-by: Krrish Mittal <[email protected]>
  • Loading branch information
@DanielleCogs @takyyon
DanielleCogs and takyyon authored Dec 5, 2024
1 parent 5212228 commit ebb0df3
Show file tree
Hide file tree
Showing 5 changed files with 289 additions and 51 deletions.
22 changes: 12 additions & 10 deletions apps/vs-code-designer/src/app/commands/dataMapper/DataMapperExt.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import { webviewType } from './extensionConfig';
import type { MapDefinitionEntry } from '@microsoft/logic-apps-shared';
import type { IActionContext } from '@microsoft/vscode-azext-utils';
import type { MapDefinitionData } from '@microsoft/vscode-extension-logic-apps';
import * as yaml from 'js-yaml';
import * as path from 'path';
import { Uri, ViewColumn, window } from 'vscode';
import * as yaml from 'js-yaml';
import { localize } from '../../../localize';

export default class DataMapperExt {
Expand Down Expand Up @@ -66,16 +66,18 @@ export default class DataMapperExt {
if this method gets updated, both need to be updated to keep them in sync. This exists as a copy to avoid a
package import issue.
*/
public static loadMapDefinition = (mapDefinitionString: string | undefined): MapDefinitionEntry => {
public static loadMapDefinition = (mapDefinitionString: string | undefined, ext: any): MapDefinitionEntry => {
if (mapDefinitionString) {
// Add extra escapes around custom string values, so that we don't lose which ones are which
const modifiedMapDefinitionString = mapDefinitionString.replaceAll('"', `\\"`);
const mapDefinition = yaml.load(modifiedMapDefinitionString) as MapDefinitionEntry;

// Now that we've parsed the yml, remove the extra escaped quotes to restore the values
DataMapperExt.fixMapDefinitionCustomValues(mapDefinition);

return mapDefinition;
try {
// Add extra escapes around custom string values, so that we don't lose which ones are which
const modifiedMapDefinitionString = mapDefinitionString.replaceAll('"', `\\"`);
const mapDefinition = yaml.load(modifiedMapDefinitionString) as MapDefinitionEntry;
// Now that we've parsed the yml, remove the extra escaped quotes to restore the values
DataMapperExt.fixMapDefinitionCustomValues(mapDefinition);
return mapDefinition;
} catch (e) {
ext.showError('Exception while parsing LML file!', { detail: e.message, modal: true });
}
}
return {};
};
Expand Down
14 changes: 9 additions & 5 deletions apps/vs-code-designer/src/app/commands/dataMapper/dataMapper.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ export const loadDataMapFileCmd = async (context: IActionContext, uri: Uri) => {
canSelectMany: false,
canSelectFiles: true,
canSelectFolders: false,
filters: { 'Data Map Definition': supportedDataMapDefinitionFileExts.map((ext) => ext.replace('.', '')) },
filters: {
'Data Map Definition': supportedDataMapDefinitionFileExts.map((ext) => ext.replace('.', '')),
},
});

if (fileUris && fileUris.length > 0) {
Expand Down Expand Up @@ -81,13 +83,13 @@ export const loadDataMapFileCmd = async (context: IActionContext, uri: Uri) => {
// Try to load the draft file first
if (draftFileIsFoundAndShouldBeUsed) {
const fileContents = await fs.readFile(draftMapDefinitionPath, 'utf-8');
mapDefinition = DataMapperExt.loadMapDefinition(fileContents);
mapDefinition = DataMapperExt.loadMapDefinition(fileContents, ext);
}

//If there is no draft file, or the draft file fails to deserialize, fall back to the base file
if (Object.keys(mapDefinition).length === 0) {
const fileContents = await fs.readFile(mapDefinitionPath, 'utf-8');
mapDefinition = DataMapperExt.loadMapDefinition(fileContents);
mapDefinition = DataMapperExt.loadMapDefinition(fileContents, ext);
}

if (
Expand All @@ -96,8 +98,10 @@ export const loadDataMapFileCmd = async (context: IActionContext, uri: Uri) => {
!mapDefinition.$targetSchema ||
typeof mapDefinition.$targetSchema !== 'string'
) {
context.telemetry.properties.eventDescription = 'Attempted to load invalid map, missing schema definitions';
ext.showError(localize('MissingSourceTargetSchema', 'Invalid map definition: $sourceSchema and $targetSchema must be defined.'));
if (Object.keys(mapDefinition).length !== 0) {
context.telemetry.properties.eventDescription = 'Attempted to load invalid map, missing schema definitions'; // only show error if schemas are missing but object exists
ext.showError(localize('MissingSourceTargetSchema', 'Invalid map definition: $sourceSchema and $targetSchema must be defined.'));
}
return;
}

Expand Down
9 changes: 6 additions & 3 deletions apps/vs-code-designer/src/extensionVariables.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import type { ContainerApp, Site } from '@azure/arm-appservice';
import type { IActionContext, IAzExtOutputChannel } from '@microsoft/vscode-azext-utils';
import type { AzureHostExtensionApi } from '@microsoft/vscode-azext-utils/hostapi';
import type * as cp from 'child_process';
import { window, type ExtensionContext, type WebviewPanel } from 'vscode';
import { type MessageOptions, window, type ExtensionContext, type WebviewPanel } from 'vscode';

/**
* Namespace for common variables used throughout the extension. They must be initialized in the activate() method of extension.ts
Expand Down Expand Up @@ -87,9 +87,12 @@ export namespace ext {
window.showWarningMessage(errMsg);
};

export const showError = (errMsg: string) => {
export const showError = (errMsg: string, options?: MessageOptions) => {
ext.log(errMsg);
window.showErrorMessage(errMsg);
if (options && options.detail) {
ext.log(options.detail);
}
window.showErrorMessage(errMsg, options);
};

export const logTelemetry = (context: IActionContext, key: string, value: string) => {
Expand Down
91 changes: 59 additions & 32 deletions libs/data-mapper-v2/src/mapHandling/MapDefinitionDeserializer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export class MapDefinitionDeserializer {
private readonly _functionsMetadata: FunctionData[];
private _loop: LoopMetadata[];
private _conditional: ConditionalMetadata;
private _warningMessages: string[];

private readonly _sourceSchemaFlattened: SchemaNodeDictionary;
private readonly _targetSchemaFlattened: SchemaNodeDictionary;
Expand All @@ -55,6 +56,7 @@ export class MapDefinitionDeserializer {
this._sourceSchema = sourceSchema;
this._targetSchema = targetSchema;
this._functionsMetadata = functions;
this._warningMessages = [];
this._conditional = {
key: '',
needsConnection: false,
Expand All @@ -80,8 +82,6 @@ export class MapDefinitionDeserializer {
this.createConnectionsForLMLObject(this._mapDefinition[rootNodeKey], `${rootNodeFormatted}`, undefined, connections);
}

//this._cleanupExtraneousConnections(connections);

return connections;
};

Expand Down Expand Up @@ -156,19 +156,22 @@ export class MapDefinitionDeserializer {
const tokens = separateFunctions(key);
const functionMetadata = funcMetadata || createSchemaNodeOrFunction(tokens).term;

let sourceSchemaNode = findNodeForKey(key, this._sourceSchema.schemaTreeRoot, false) as SchemaNodeExtended | undefined;
let possibleSourceSchemaNode = findNodeForKey(key, this._sourceSchema.schemaTreeRoot, false) as SchemaNodeExtended | undefined;

if (!sourceSchemaNode && funcMetadata?.type === 'SingleValueMetadata') {
sourceSchemaNode = findNodeForKey(funcMetadata.value, this._sourceSchema.schemaTreeRoot, false) as SchemaNodeExtended | undefined;
if (!possibleSourceSchemaNode && funcMetadata?.type === 'SingleValueMetadata') {
possibleSourceSchemaNode = findNodeForKey(funcMetadata.value, this._sourceSchema.schemaTreeRoot, false) as
| SchemaNodeExtended
| undefined;
}

if (this._loop.length > 0 && !sourceSchemaNode) {
if (!sourceSchemaNode) {
sourceSchemaNode = this.getSourceNodeForRelativeKeyInLoop(key, connections, targetNode);
if (this._loop.length > 0 && !possibleSourceSchemaNode) {
if (!possibleSourceSchemaNode) {
possibleSourceSchemaNode = this.getSourceNodeForRelativeKeyInLoop(key, connections, targetNode);
}

if (isSchemaNodeExtended(targetNode)) {
addParentConnectionForRepeatingElementsNested(
sourceSchemaNode,
possibleSourceSchemaNode,
targetNode as SchemaNodeExtended,
this._sourceSchemaFlattened,
this._targetSchemaFlattened,
Expand All @@ -177,11 +180,11 @@ export class MapDefinitionDeserializer {
}
}

if (sourceSchemaNode && this._conditional) {
this._conditional.children.push(sourceSchemaNode.key);
if (possibleSourceSchemaNode && this._conditional) {
this._conditional.children.push(possibleSourceSchemaNode.key);
}

if (!sourceSchemaNode && functionMetadata.type === 'Function') {
if (!possibleSourceSchemaNode && functionMetadata.type === 'Function') {
let funcKey = '';
let func: FunctionData;
const metadataString = JSON.stringify(functionMetadata);
Expand Down Expand Up @@ -224,16 +227,19 @@ export class MapDefinitionDeserializer {
this.handleSingleValueOrFunction(srcStr, input, func, connections);
});
}
} else if (!sourceSchemaNode && functionMetadata.type !== 'Function') {
} else if (!possibleSourceSchemaNode && functionMetadata.type !== 'Function') {
// custom value or index
this.handleSingleValue(key, targetNode, connections);
} else if (targetNode && sourceSchemaNode) {
} else if (targetNode && possibleSourceSchemaNode) {
// source schema node
applyConnectionValue(connections, {
targetNode: targetNode,
targetNodeReactFlowKey: this.getTargetKey(targetNode),
findInputSlot: true,
input: createNodeConnection(sourceSchemaNode, addSourceReactFlowPrefix(sourceSchemaNode.key)),
input: createNodeConnection(possibleSourceSchemaNode, addSourceReactFlowPrefix(possibleSourceSchemaNode.key)),
});
} else {
throw new Error(`Cannot find value for ${key} in LML file`); // danielle show error here
}
};

Expand All @@ -253,6 +259,9 @@ export class MapDefinitionDeserializer {
this._sourceSchema.schemaTreeRoot,
false
) as SchemaNodeExtended;
if (!loopSrc) {
throw new Error(`Loop source not found for key ${loop.key}`);
}
let key = addSourceReactFlowPrefix(loopSrc.key);
if (loop.indexFn) {
loopSrc = this.getFunctionMetadataForKey(loop.indexFn) as FunctionData;
Expand Down Expand Up @@ -333,14 +342,25 @@ export class MapDefinitionDeserializer {
}
} else {
Object.entries(rightSideStringOrObject).forEach((child) => {
this.createConnectionsForLMLObject(child[1], child[0], targetNode, connections);
try {
this.createConnectionsForLMLObject(child[1], child[0], targetNode, connections);
} catch (error) {
if (error instanceof Error) {
this._warningMessages.push(error.message);
console.log(error);
}
}
});
}
} else {
this.processLeftSideForOrIf(leftSideKey, parentTargetNode, rightSideStringOrObject, connections);
}
};

public getWarningMessages = () => {
return this._warningMessages;
};

public getLowestCommonParentForConditional = (conditionalChildren: string[]): string => {
if (conditionalChildren.length === 1) {
return conditionalChildren[0].slice(0, conditionalChildren[0].lastIndexOf('/'));
Expand Down Expand Up @@ -396,6 +416,10 @@ export class MapDefinitionDeserializer {
});
}
}
this.resetConditionalStateToEmpty();
};

private resetConditionalStateToEmpty = () => {
this._conditional = {
key: '',
needsConnection: false,
Expand Down Expand Up @@ -461,7 +485,7 @@ export class MapDefinitionDeserializer {
private getSourceLoopFromSequenceFunctions = (metadata: FunctionCreationMetadata) => {
// loop through sequence functions until we get the loop
while (metadata.type === 'Function') {
metadata = metadata.inputs[0];
metadata = metadata.inputs[0]; // repeating input to sequence function is always at index 0
}
return metadata;
};
Expand All @@ -470,6 +494,7 @@ export class MapDefinitionDeserializer {
// take out for statement to get to inner objects
const forFunc = sourceFor as ParseFunc;
const sourceLoopKey = forFunc.inputs[0];

if (sourceLoopKey.type === 'SingleValueMetadata') {
const loopSource = this.getLoopNode(sourceLoopKey.value);
if (loopSource) {
Expand All @@ -479,6 +504,8 @@ export class MapDefinitionDeserializer {
};
this._loop.push(loopMetadata);
this.createIndexFunctionIfNeeded(forFunc, loopSource, connections, loopMetadata);
} else {
throw Error(`Loop source not found for key ${sourceLoopKey.value}`);
}
} else {
const meta: FunctionCreationMetadata = this.getSourceLoopFromSequenceFunctions(sourceFor);
Expand Down Expand Up @@ -510,7 +537,7 @@ export class MapDefinitionDeserializer {
};

private isCustomValue = (value: string): boolean => {
return value.startsWith('"') || !Number.isNaN(Number.parseInt(value));
return value.startsWith('"') || !Number.isNaN(Number.parseInt(value)) || !Number.isNaN(Number.parseFloat(value));
};

private handleSingleValue = (key: string, targetNode: SchemaNodeExtended | FunctionData, connections: ConnectionDictionary) => {
Expand All @@ -532,8 +559,7 @@ export class MapDefinitionDeserializer {
findInputSlot: true,
input: createCustomInputConnection(key),
});
// index
} else if (key.startsWith('$')) {
} else if (this.isIndexValue(key)) {
const indexFnKey = this._createdFunctions[key];
const indexFn = connections[indexFnKey];
if (indexFn) {
Expand All @@ -544,26 +570,27 @@ export class MapDefinitionDeserializer {
input: createNodeConnection(indexFn.self.node, indexFn.self.reactFlowKey),
});
}
} else if (key.includes('[')) {
} else if (this.isDirectAccessValue(key)) {
// using this older function for now
const amendedSourceKey = amendSourceKeyForDirectAccessIfNeeded(key);

const directAccessSeparated = separateFunctions(amendedSourceKey[0]);
const idk = createSchemaNodeOrFunction(directAccessSeparated);

this.handleSingleValueOrFunction('', idk.term, targetNode, connections);
} else if (targetNode) {
this._conditional.children.push(key);
const schemaNodeOrFunction = createSchemaNodeOrFunction(directAccessSeparated);

applyConnectionValue(connections, {
targetNode: targetNode,
targetNodeReactFlowKey: this.getTargetKey(targetNode),
findInputSlot: true,
input: createCustomInputConnection(key),
});
this.handleSingleValueOrFunction('', schemaNodeOrFunction.term, targetNode, connections);
} else {
throw new Error(`Key ${key} not found in source schema`);
}
};

private isDirectAccessValue = (key: string): boolean => {
return key.includes('[');
};

private isIndexValue = (key: string): boolean => {
return key.startsWith('$');
};

private isSchemaNodeTargetKey = (key: string) => {
if (!key.includes(DReservedToken.for) && !key.includes(DReservedToken.if)) {
return true;
Expand Down
Loading

0 comments on commit ebb0df3

Please sign in to comment.