From bcbfd097a644e7eb6b105d2b47f358500f741987 Mon Sep 17 00:00:00 2001 From: Chibuezem Marvinrose Date: Wed, 19 Mar 2025 04:44:24 +0100 Subject: [PATCH 1/2] Fix: Type checker doesn't detect usage of optional properties #11 Signed-off-by: Chibuezem Marvinrose --- src/TemplateMarkInterpreter.ts | 166 ++++++++++++++++++++++++++------- test/issue-11.test.ts | 37 ++++++++ 2 files changed, 170 insertions(+), 33 deletions(-) create mode 100644 test/issue-11.test.ts diff --git a/src/TemplateMarkInterpreter.ts b/src/TemplateMarkInterpreter.ts index 60fecc2..653908d 100644 --- a/src/TemplateMarkInterpreter.ts +++ b/src/TemplateMarkInterpreter.ts @@ -460,23 +460,30 @@ export class TemplateMarkInterpreter { templateClass: ClassDeclaration; clauseLibrary: object; - constructor(modelManager: ModelManager, clauseLibrary:object, templateConceptFqn?: string) { + constructor( + modelManager: ModelManager, + clauseLibrary: object, + templateConceptFqn?: string + ) { this.modelManager = modelManager; this.clauseLibrary = clauseLibrary; - this.templateClass = getTemplateClassDeclaration(this.modelManager,templateConceptFqn); + this.templateClass = getTemplateClassDeclaration( + this.modelManager, + templateConceptFqn + ); } /** - * Checks that a TemplateMark JSON document is valid with respect to the - * TemplateMark model, as well as the template model. - * - * Checks: - * 1. Variable names are valid properties in the template model - * 2. Optional properties have guards - * @param {*} templateMark the TemplateMark JSON object - * @returns {*} TemplateMark JSON that has been typed checked and has type metadata added - * @throws {Error} if the templateMark document is invalid - */ + * Checks that a TemplateMark JSON document is valid with respect to the + * TemplateMark model, as well as the template model. + * + * Checks: + * 1. Variable names are valid properties in the template model + * 2. Optional properties have guards + * @param {*} templateMark the TemplateMark JSON object + * @returns {*} TemplateMark JSON that has been typed checked and has type metadata added + * @throws {Error} if the templateMark document is invalid + */ checkTypes(templateMark: object): object { const modelManager = new ModelManager({ strict: true }); modelManager.addCTOModel(ConcertoMetaModel.MODEL, 'concertometamodel.cto'); @@ -484,32 +491,103 @@ export class TemplateMarkInterpreter { modelManager.addCTOModel(TemplateMarkModel.MODEL, 'templatemark.cto'); const factory = new Factory(modelManager); const serializer = new Serializer(factory, modelManager); + + // Validate basic TemplateMark structure try { serializer.fromJSON(templateMark); - return templateMark; - } - catch(err) { - throw new Error(`Generated invalid agreement: ${err}: ${JSON.stringify(templateMark, null, 2)}`); + } catch (err) { + throw new Error( + `Generated invalid agreement: ${err}: ${JSON.stringify( + templateMark, + null, + 2 + )}` + ); } + + // Get optional properties from the template model + const optionalProperties = new Set(); + const properties = this.templateClass.getProperties(); + properties.forEach((prop) => { + if (prop.isOptional()) { + optionalProperties.add(prop.getName()); + } + }); + + // Traverse TemplateMark to check for unguarded optional variables + traverse(templateMark).forEach(function (node) { + if ( + typeof node === 'object' && + node.$class && + typeof node.$class === 'string' + ) { + const nodeClass = node.$class as string; + + if ( + VARIABLE_DEFINITION_RE.test(nodeClass) || + ENUM_VARIABLE_DEFINITION_RE.test(nodeClass) || + FORMATTED_VARIABLE_DEFINITION_RE.test(nodeClass) + ) { + const varName = node.name; + if (optionalProperties.has(varName)) { + // Check if this variable is guarded by an OptionalDefinition or ConditionalDefinition + const path = this.path; + let isGuarded = false; + + for (let i = path.length - 2; i >= 0; i--) { + const parent = traverse(templateMark).get(path.slice(0, i + 1)); + if (parent && parent.$class) { + const parentClass = parent.$class as string; + if ( + OPTIONAL_DEFINITION_RE.test(parentClass) || + CONDITIONAL_DEFINITION_RE.test(parentClass) + ) { + isGuarded = true; + break; + } + // Stop at other structural nodes (e.g., ClauseDefinition) to avoid over-checking + if ( + CLAUSE_DEFINITION_RE.test(parentClass) || + CONTRACT_DEFINITION_RE.test(parentClass) + ) { + break; + } + } + } + + if (!isGuarded) { + throw new Error( + `Optional property '${varName}' used in template without a guard (e.g., {{#optional ${varName}}} or {{#if ${varName}}}).` + ); + } + } + } + } + }); + + return templateMark; } /** - * Compiles the code nodes containing TS to code nodes containing JS. - * @param {*} templateMark the TemplateMark JSON object - * @returns {*} TemplateMark JSON with JS nodes - * @throws {Error} if the templateMark document is invalid - */ - async compileTypeScriptToJavaScript(templateMark: object) : Promise { + * Compiles the code nodes containing TS to code nodes containing JS. + * @param {*} templateMark the TemplateMark JSON object + * @returns {*} TemplateMark JSON with JS nodes + * @throws {Error} if the templateMark document is invalid + */ + async compileTypeScriptToJavaScript(templateMark: object): Promise { const templateConcept = (templateMark as any).nodes[0].elementType; - if(!templateConcept) { + if (!templateConcept) { throw new Error('TemplateMark is not typed'); } - const compiler = new TemplateMarkToJavaScriptCompiler(this.modelManager, templateConcept); + const compiler = new TemplateMarkToJavaScriptCompiler( + this.modelManager, + templateConcept + ); await compiler.initialize(); return compiler.compile(templateMark); } - validateCiceroMark(ciceroMark: object) : object { + validateCiceroMark(ciceroMark: object): object { const modelManager = new ModelManager({ strict: true }); modelManager.addCTOModel(ConcertoMetaModel.MODEL, 'concertometamodel.cto'); modelManager.addCTOModel(CommonMarkModel.MODEL, 'commonmark.cto'); @@ -518,22 +596,44 @@ export class TemplateMarkInterpreter { const serializer = new Serializer(factory, modelManager); try { return serializer.fromJSON(ciceroMark); - } - catch(err) { - throw new Error(`Generated invalid agreement: ${err}: ${JSON.stringify(ciceroMark, null, 2)}`); + } catch (err) { + throw new Error( + `Generated invalid agreement: ${err}: ${JSON.stringify( + ciceroMark, + null, + 2 + )}` + ); } } - async generate(templateMark: object, data: TemplateData, options?:GenerationOptions): Promise { + async generate( + templateMark: object, + data: TemplateData, + options?: GenerationOptions + ): Promise { const factory = new Factory(this.modelManager); const serializer = new Serializer(factory, this.modelManager); const templateData = serializer.fromJSON(data); - if (templateData.getFullyQualifiedType() !== this.templateClass.getFullyQualifiedName()) { - throw new Error(`Template data must be of type '${this.templateClass.getFullyQualifiedName()}'.`); + if ( + templateData.getFullyQualifiedType() !== + this.templateClass.getFullyQualifiedName() + ) { + throw new Error( + `Template data must be of type '${this.templateClass.getFullyQualifiedName()}'.` + ); } const typedTemplateMark = this.checkTypes(templateMark); - const jsTemplateMark = await this.compileTypeScriptToJavaScript(typedTemplateMark); - const ciceroMark = await generateAgreement(this.modelManager, this.clauseLibrary, jsTemplateMark, data, options); + const jsTemplateMark = await this.compileTypeScriptToJavaScript( + typedTemplateMark + ); + const ciceroMark = await generateAgreement( + this.modelManager, + this.clauseLibrary, + jsTemplateMark, + data, + options + ); return this.validateCiceroMark(ciceroMark); } } \ No newline at end of file diff --git a/test/issue-11.test.ts b/test/issue-11.test.ts new file mode 100644 index 0000000..c742df1 --- /dev/null +++ b/test/issue-11.test.ts @@ -0,0 +1,37 @@ +import { ModelManager } from '@accordproject/concerto-core'; +import { TemplateMarkInterpreter } from '../src/TemplateMarkInterpreter'; +import { TemplateMarkTransformer } from '@accordproject/markdown-template'; + +describe('Issue #11: Unguarded Optional Variables', () => { + let modelManager: ModelManager; + let interpreter: TemplateMarkInterpreter; + let templateMarkTransformer: any; + + beforeEach(async () => { + modelManager = new ModelManager({ strict: true }); + const MODEL = + 'namespace hello@1.0.0\n@template\nconcept HelloWorld {\n o String name\n o String last optional\n}'; + modelManager.addCTOModel(MODEL, 'hello.cto', true); + await modelManager.updateExternalModels(); + interpreter = new TemplateMarkInterpreter( + modelManager, + {}, + 'hello@1.0.0.HelloWorld' + ); + templateMarkTransformer = new TemplateMarkTransformer(); + }); + + it('should throw compile-time error for unguarded optional variable', () => { + const TEMPLATE = + 'Hello {{name}}!\nToday is **{{% return now.toISOString() %}}**.\n{{last}}'; + const templateMark = templateMarkTransformer.fromMarkdownTemplate( + { content: TEMPLATE, templateConceptFqn: 'hello@1.0.0.HelloWorld' }, + modelManager, + 'contract', + { verbose: false } + ); + expect(() => interpreter.checkTypes(templateMark)).toThrow( + 'Optional property \'last\' used in template without a guard (e.g., {{#optional last}} or {{#if last}}).' + ); + }); +}); From 20656c00bacf22e35a25d1fb254b8ede0db35378 Mon Sep 17 00:00:00 2001 From: Chibuezem Marvinrose Date: Wed, 19 Mar 2025 04:45:46 +0100 Subject: [PATCH 2/2] Removed comments Signed-off-by: Chibuezem Marvinrose --- src/TemplateMarkInterpreter.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/TemplateMarkInterpreter.ts b/src/TemplateMarkInterpreter.ts index 653908d..2c79b6c 100644 --- a/src/TemplateMarkInterpreter.ts +++ b/src/TemplateMarkInterpreter.ts @@ -492,7 +492,7 @@ export class TemplateMarkInterpreter { const factory = new Factory(modelManager); const serializer = new Serializer(factory, modelManager); - // Validate basic TemplateMark structure + try { serializer.fromJSON(templateMark); } catch (err) { @@ -505,7 +505,7 @@ export class TemplateMarkInterpreter { ); } - // Get optional properties from the template model + const optionalProperties = new Set(); const properties = this.templateClass.getProperties(); properties.forEach((prop) => { @@ -514,7 +514,7 @@ export class TemplateMarkInterpreter { } }); - // Traverse TemplateMark to check for unguarded optional variables + traverse(templateMark).forEach(function (node) { if ( typeof node === 'object' && @@ -530,7 +530,7 @@ export class TemplateMarkInterpreter { ) { const varName = node.name; if (optionalProperties.has(varName)) { - // Check if this variable is guarded by an OptionalDefinition or ConditionalDefinition + const path = this.path; let isGuarded = false; @@ -545,7 +545,7 @@ export class TemplateMarkInterpreter { isGuarded = true; break; } - // Stop at other structural nodes (e.g., ClauseDefinition) to avoid over-checking + if ( CLAUSE_DEFINITION_RE.test(parentClass) || CONTRACT_DEFINITION_RE.test(parentClass)