Skip to content

fix(csharp): harden decorator namespace mapping and required/scalar codegen semantics #234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jamieshorten merged 1 commit into from
May 26, 2026
+222 −21
Merged

fix(csharp): harden decorator namespace mapping and required/scalar codegen semantics #234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 103 additions & 17 deletions lib/codegen/fromcto/csharp/csharpvisitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,11 @@ class CSharpVisitor {
.filter(namespace => namespace !== modelFile.getNamespace()) // Skip own namespace.
.filter((v, i, a) => a.indexOf(v) === i) // Remove any duplicates from direct imports
.forEach(namespace => {
// concerto.decorator types are provided by the .NET runtime package.
if (ModelUtil.parseNamespace(namespace).name === 'concerto.decorator') {
parameters.fileWriter.writeLine(0, 'using AccordProject.Concerto.Metamodel;');
return;
}
const otherModelFile = modelFile.getModelManager()?.getModelFile(namespace);
if (!otherModelFile) {
// Couldn't resolve the other model file.
Expand Down Expand Up @@ -250,8 +255,13 @@ class CSharpVisitor {
*/
getDotNetNamespaceOfType(type, classDeclaration, parameters) {
let dotnetNs = '';
const parsedNamespace = type ? ModelUtil.parseNamespace(ModelUtil.getNamespace(type)).name : null;
// concerto.decorator base types are provided by the .NET runtime package.
if (parsedNamespace === 'concerto.decorator') {
return 'AccordProject.Concerto.Metamodel.';
}
// Resolve to dotnet namespace only if its a non system type
if (type && ModelUtil.parseNamespace(ModelUtil.getNamespace(type)).name !== 'concerto') {
if (type && parsedNamespace !== 'concerto') {
const mm = classDeclaration.getModelFile()?.getModelManager();
let typeNamespace = mm?.getType(type)?.getNamespace();
if (typeNamespace && typeNamespace !== classDeclaration.getNamespace()) {
Expand Down Expand Up @@ -282,6 +292,9 @@ class CSharpVisitor {
const csharpType = fqn === 'concerto.scalar.UUID'
? 'System.Guid'
: this.toCSharpType(scalarDeclaration.getType());
// If the scalar type itself is named Value, using Value as the generated
// record parameter/member causes a C# CS0542 collision.
const scalarMemberName = identifier === 'Value' ? 'RawValue' : 'Value';
const validatorLines = this.buildScalarValidatorLines(scalarDeclaration);
const converterName = `${identifier}JsonConverter`;
const useNewtonsoft = !!parameters.useNewtonsoftJson;
Expand All @@ -300,42 +313,42 @@ class CSharpVisitor {
: `public override void Write(System.Text.Json.Utf8JsonWriter w, ${identifier} v, System.Text.Json.JsonSerializerOptions o)`;

parameters.fileWriter.writeLine(0, converterAttr);
parameters.fileWriter.writeLine(0, `public readonly record struct ${identifier}(${csharpType} Value)`);
parameters.fileWriter.writeLine(0, `public readonly record struct ${identifier}(${csharpType} ${scalarMemberName})`);
parameters.fileWriter.writeLine(0, '{');
if (validatorLines.length > 0) {
validatorLines.forEach(line => parameters.fileWriter.writeLine(1, line));
parameters.fileWriter.writeLine(1, `public ${csharpType} Value { get; init; } = Value;`);
parameters.fileWriter.writeLine(1, `public ${csharpType} ${scalarMemberName} { get; init; } = ${scalarMemberName};`);
}
parameters.fileWriter.writeLine(1, `public static implicit operator ${csharpType}(${identifier} s) => s.Value;`);
parameters.fileWriter.writeLine(1, `public static implicit operator ${csharpType}(${identifier} s) => s.${scalarMemberName};`);
parameters.fileWriter.writeLine(1, `public static implicit operator ${identifier}(${csharpType} v) => new(v);`);
parameters.fileWriter.writeLine(1, 'public override string ToString() => Value.ToString();');
parameters.fileWriter.writeLine(1, `public override string ToString() => ${scalarMemberName}.ToString();`);
parameters.fileWriter.writeLine(0, '}');

// Companion converter — one per scalar, flavoured by the active serializer
let readExpr, writeExpr;
if (csharpType === 'System.Guid') {
readExpr = useNewtonsoft ? 'System.Guid.Parse((string)r.Value!)' : 'r.GetGuid()';
writeExpr = useNewtonsoft ? 'w.WriteValue(v.Value.ToString())' : 'w.WriteStringValue(v.Value.ToString())';
writeExpr = useNewtonsoft ? `w.WriteValue(v.${scalarMemberName}.ToString())` : `w.WriteStringValue(v.${scalarMemberName}.ToString())`;
} else if (csharpType === 'string') {
readExpr = useNewtonsoft ? '(string)r.Value!' : 'r.GetString()!';
writeExpr = useNewtonsoft ? 'w.WriteValue(v.Value)' : 'w.WriteStringValue(v.Value)';
writeExpr = useNewtonsoft ? `w.WriteValue(v.${scalarMemberName})` : `w.WriteStringValue(v.${scalarMemberName})`;
} else if (csharpType === 'bool') {
readExpr = useNewtonsoft ? '(bool)r.Value!' : 'r.GetBoolean()';
writeExpr = useNewtonsoft ? 'w.WriteValue(v.Value)' : 'w.WriteBooleanValue(v.Value)';
writeExpr = useNewtonsoft ? `w.WriteValue(v.${scalarMemberName})` : `w.WriteBooleanValue(v.${scalarMemberName})`;
} else if (csharpType === 'int') {
readExpr = useNewtonsoft ? 'System.Convert.ToInt32(r.Value)' : 'r.GetInt32()';
writeExpr = useNewtonsoft ? 'w.WriteValue(v.Value)' : 'w.WriteNumberValue(v.Value)';
writeExpr = useNewtonsoft ? `w.WriteValue(v.${scalarMemberName})` : `w.WriteNumberValue(v.${scalarMemberName})`;
} else if (csharpType === 'long') {
readExpr = useNewtonsoft ? 'System.Convert.ToInt64(r.Value)' : 'r.GetInt64()';
writeExpr = useNewtonsoft ? 'w.WriteValue(v.Value)' : 'w.WriteNumberValue(v.Value)';
writeExpr = useNewtonsoft ? `w.WriteValue(v.${scalarMemberName})` : `w.WriteNumberValue(v.${scalarMemberName})`;
} else if (csharpType === 'double') {
readExpr = useNewtonsoft ? 'System.Convert.ToDouble(r.Value)' : 'r.GetDouble()';
writeExpr = useNewtonsoft ? 'w.WriteValue(v.Value)' : 'w.WriteNumberValue(v.Value)';
writeExpr = useNewtonsoft ? `w.WriteValue(v.${scalarMemberName})` : `w.WriteNumberValue(v.${scalarMemberName})`;
} else {
readExpr = useNewtonsoft
? `(${csharpType})System.Convert.ChangeType((string)r.Value!, typeof(${csharpType}))`
: `(${csharpType})System.Convert.ChangeType(r.GetString()!, typeof(${csharpType}))`;
writeExpr = useNewtonsoft ? 'w.WriteValue(v.Value.ToString()!)' : 'w.WriteStringValue(v.Value.ToString()!)';
writeExpr = useNewtonsoft ? `w.WriteValue(v.${scalarMemberName}.ToString()!)` : `w.WriteStringValue(v.${scalarMemberName}.ToString()!)`;
}

parameters.fileWriter.writeLine(0, `public class ${converterName} : ${converterBase}`);
Expand Down Expand Up @@ -492,7 +505,8 @@ class CSharpVisitor {
const { keyType, valueType } = this.resolveMapTypes(mapDeclaration, parameters);
const nullable = field.isOptional() ? '?' : '';
const resolvedType = `Dictionary<${keyType}, ${valueType}>`;
const lines = this.toCSharpProperty('public', field.getParent()?.getName(), field.getName(), null, '', nullable, '{ get; set; }', parameters, resolvedType);
const emitRequired = !!parameters.useRequiredForNonOptionalReferenceTypes && !field.isOptional();
const lines = this.toCSharpProperty('public', field.getParent()?.getName(), field.getName(), null, '', nullable, '{ get; set; }', parameters, resolvedType, emitRequired);
lines.forEach(line => parameters.fileWriter.writeLine(1, line));
return null;
}
Expand Down Expand Up @@ -571,6 +585,22 @@ class CSharpVisitor {
: (externalFieldType !== undefined ? scalarDefaultValue : field.getDefaultValue());
const csDefault = this.formatDefaultLiteral(rawDefault, rawFieldType, !!externalFieldType, field, fieldType);
const getset = csDefault != null ? `{ get; set; } = ${csDefault};` : '{ get; set; }';
const csharpType = this.toCSharpType(fieldType, parameters);
let isEnum = false;
try {
isEnum = field.getModelFile().getType(field.getType())?.isEnum?.() || false;
} catch (e) {
// Keep false when declaration cannot be resolved.
}
const emitRequired = this.shouldEmitRequired(parameters, {
nullableType,
hasDefault: csDefault != null,
isArray: field.isArray(),
isScalarAlias: externalFieldType !== undefined,
isPrimitive: field.isPrimitive(),
isEnum,
csharpType
});

const lines = this.toCSharpProperty(
'public',
Expand All @@ -580,12 +610,40 @@ class CSharpVisitor {
array,
nullableType,
getset,
parameters
parameters,
undefined,
emitRequired
);
lines.forEach(line => parameters.fileWriter.writeLine(1, line));
return null;
}

/**
* Determines if a property should emit the C# `required` modifier.
* This centralizes required-emission decisions for fields and relationships.
* @param {Object} parameters - visitor parameters
* @param {Object} options - decision options
* @param {string} [options.nullableType] - nullable marker (`?`) when present
* @param {boolean} [options.hasDefault] - true when a default initializer is emitted
* @param {boolean} [options.isArray] - true when property is an array type
* @param {boolean} [options.isScalarAlias] - true for scalar alias value-type wrappers
* @param {boolean} [options.isPrimitive] - true for Concerto primitive fields
* @param {boolean} [options.isEnum] - true for enum fields
* @param {string} [options.csharpType] - resolved C# type string
* @returns {boolean} true if `required` should be emitted
* @private
*/
shouldEmitRequired(parameters, options = {}) {
if (!parameters.useRequiredForNonOptionalReferenceTypes) {return false;}
if (options.nullableType) {return false;}
if (options.hasDefault) {return false;}
// Scalar aliases are generated as record structs (value types), not reference types.
if (options.isScalarAlias) {return false;}
if (options.isEnum) {return false;}
if (!options.csharpType) {return false;}
return this.isCSharpReferenceType(options.csharpType, !!options.isArray);
}

/**
* Visitor design pattern
* @param {EnumValueDeclaration} enumValueDeclaration - the object being visited
Expand Down Expand Up @@ -652,6 +710,17 @@ class CSharpVisitor {
type = `${fqn}${ModelUtil.getShortName(relationship.getFullyQualifiedTypeName())}`;
}

const csharpType = this.toCSharpType(type, parameters);
const emitRequired = this.shouldEmitRequired(parameters, {
nullableType: optional,
hasDefault: false,
isArray: !!array,
isScalarAlias: false,
isPrimitive: false,
isEnum: false,
csharpType
});

// we export all relationships
const lines = this.toCSharpProperty(
'public',
Expand All @@ -661,12 +730,28 @@ class CSharpVisitor {
array,
optional,
'{ get; set; }',
parameters
parameters,
undefined,
emitRequired
);
lines.forEach(line => parameters.fileWriter.writeLine(1, line));
return null;
}

/**
* Determines whether a generated C# property type is a reference type.
* @param {string} csharpType - resolved C# type name
* @param {boolean} isArray - whether the property is an array
* @returns {boolean} true if reference type
* @private
*/
isCSharpReferenceType(csharpType, isArray) {
if (isArray) {return true;}
if (csharpType === 'string') {return true;}
if (csharpType === 'System.Guid' || csharpType === 'System.DateTime') {return false;}
return !csharpBuiltInTypes.includes(csharpType);
}

/**
* Format a Concerto default value as a C# literal suitable for a property initializer.
* String values are quoted; scalar-typed fields wrap the literal in `new(...)`.
Expand Down Expand Up @@ -714,9 +799,10 @@ class CSharpVisitor {
* @param {string} getset the getter and setter declaration
* @param {Object} [parameters] - the parameter
* @param {string} [resolvedType] - pre-built C# type string; when provided, skips toCSharpType
* @param {boolean} [emitRequired] - true to emit the C# `required` modifier
* @returns {string} the property declaration
*/
toCSharpProperty(access, parentName, propertyName, propertyType, array, nullableType, getset, parameters, resolvedType = undefined) {
toCSharpProperty(access, parentName, propertyName, propertyType, array, nullableType, getset, parameters, resolvedType = undefined, emitRequired = false) {
const identifier = this.toCSharpIdentifier(parentName, propertyName, parameters);
const type = resolvedType ?? this.toCSharpType(propertyType, parameters);

Expand All @@ -731,7 +817,7 @@ class CSharpVisitor {
}
}

lines.push(`${access} ${type}${array}${nullableType} ${identifier} ${getset}`);
lines.push(`${access} ${emitRequired ? 'required ' : ''}${type}${array}${nullableType} ${identifier} ${getset}`);
return lines;
}

Expand Down
8 changes: 4 additions & 4 deletions test/codegen/__snapshots__/codegen.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ public abstract class Decorator : Concept {
}
[AccordProject.Concerto.Type(Namespace = "concerto.decorator", Version = "1.0.0", Name = "DotNetNamespace")]
[System.Text.Json.Serialization.JsonConverter(typeof(AccordProject.Concerto.ConcertoConverterFactorySystem))]
public class DotNetNamespace : Decorator {
public class DotNetNamespace : AccordProject.Concerto.Metamodel.Decorator {
[System.Text.Json.Serialization.JsonPropertyName("$class")]
public override string _class { get; } = "concerto.decorator@1.0.0.DotNetNamespace";
[System.Text.Json.Serialization.JsonPropertyName("namespace")]
Expand All @@ -648,7 +648,7 @@ exports[`codegen #formats check we can convert all formats from namespace versio
"value": "namespace AccordProject.Concerto;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using concerto.decorator;
using AccordProject.Concerto.Metamodel;
[AccordProject.Concerto.Type(Namespace = "concerto", Version = "1.0.0", Name = "Concept")]
[System.Text.Json.Serialization.JsonConverter(typeof(AccordProject.Concerto.ConcertoConverterFactorySystem))]
public abstract class Concept {
Expand Down Expand Up @@ -785,11 +785,11 @@ exports[`codegen #formats check we can convert all formats from namespace versio
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using org.acme.hr.base;
using concerto.decorator;
using AccordProject.Concerto.Metamodel;
using AccordProject.Concerto;
[AccordProject.Concerto.Type(Namespace = "org.acme.hr", Version = "1.0.0", Name = "pii")]
[System.Text.Json.Serialization.JsonConverter(typeof(AccordProject.Concerto.ConcertoConverterFactorySystem))]
public class pii : concerto.decorator.Decorator {
public class pii : AccordProject.Concerto.Metamodel.Decorator {
[System.Text.Json.Serialization.JsonPropertyName("$class")]
public override string _class { get; } = "org.acme.hr@1.0.0.pii";
public bool isPii { get; set; }
Expand Down
Loading
Loading