From 729547ab80b6519d2bb72c54aca8f54062b2d465 Mon Sep 17 00:00:00 2001 From: abir Date: Mon, 2 Nov 2026 12:55:13 +0000 Subject: [PATCH] fix(codegen): preserve optional on scalar properties in Avro, GraphQL, Protobuf and Rust Field.getScalarField() builds a new field that copies name and array but not optional, so visitField treated `o SSN taxId optional` as required. Pass thing.isOptional() from visit() into visitField as an optional isOptional argument (defaulting to field.isOptional()), and use it wherever visitField checked optionality. Protobuf's concertoToProto3FieldRule gets the same optional argument. Also marks Function.responseType, Logic.stateType, Agreement.state, PdfConversionOptions.styles and TriggerResponse.result as optional in the APAP protocol Protobuf fixture, and regenerates the type declarations. Closes #306 Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01HLiXZ9jMJ2WD3FmLRKWnsi Signed-off-by: Abir Maity --- lib/codegen/fromcto/avro/avrovisitor.js | 8 +++--- lib/codegen/fromcto/graphql/graphqlvisitor.js | 8 +++--- .../fromcto/protobuf/protobufvisitor.js | 13 +++++---- lib/codegen/fromcto/rust/rustvisitor.js | 22 ++++++++------- test/codegen/fromcto/avro/avrovisitor.js | 27 +++++++++++++++++++ .../codegen/fromcto/graphql/graphqlvisitor.js | 27 +++++++++++++++++++ ...cordproject.protocol.v1_0_0-expected.proto | 10 +++---- .../fromcto/protobuf/protobufvisitor.js | 26 ++++++++++++++++++ test/codegen/fromcto/rust/rustvisitor.js | 27 +++++++++++++++++++ .../lib/codegen/fromcto/avro/avrovisitor.d.ts | 2 ++ .../fromcto/graphql/graphqlvisitor.d.ts | 2 ++ .../fromcto/protobuf/protobufvisitor.d.ts | 5 +++- .../lib/codegen/fromcto/rust/rustvisitor.d.ts | 2 ++ 13 files changed, 152 insertions(+), 27 deletions(-) diff --git a/lib/codegen/fromcto/avro/avrovisitor.js b/lib/codegen/fromcto/avro/avrovisitor.js index 107a3835..36cafffc 100644 --- a/lib/codegen/fromcto/avro/avrovisitor.js +++ b/lib/codegen/fromcto/avro/avrovisitor.js @@ -48,7 +48,7 @@ class AvroVisitor { } else if (thing.isMapDeclaration?.()) { return; } else if (thing.isTypeScalar?.()) { - return this.visitField(thing.getScalarField(), parameters); + return this.visitField(thing.getScalarField(), parameters, thing.isOptional()); } else if (thing.isField?.()) { return this.visitField(thing, parameters); } else if (thing.isRelationship?.()) { @@ -167,17 +167,19 @@ class AvroVisitor { * Visitor design pattern * @param {Field} field - the object being visited * @param {Object} parameters - the parameter + * @param {boolean} [isOptional] - whether the property is optional, defaults to `field.isOptional()`. + * Passed explicitly for scalar-typed properties, because `getScalarField()` does not copy `optional`. * @return {Object} the result of visiting or null * @private */ - visitField(field, parameters) { + visitField(field, parameters, isOptional = field.isOptional()) { let avroType = this.toAvroType(field.getType()); if (field.isArray()) { avroType = `array<${avroType}>`; } - if (field.isOptional()) { + if (isOptional) { avroType = `union { null, ${avroType} }`; } diff --git a/lib/codegen/fromcto/graphql/graphqlvisitor.js b/lib/codegen/fromcto/graphql/graphqlvisitor.js index 45fd6ceb..e1cef4ad 100644 --- a/lib/codegen/fromcto/graphql/graphqlvisitor.js +++ b/lib/codegen/fromcto/graphql/graphqlvisitor.js @@ -49,7 +49,7 @@ class GraphQLVisitor { } else if (thing.isModelFile?.()) { return this.visitModelFile(thing, parameters); } else if (thing.isTypeScalar?.()) { - return this.visitField(thing.getScalarField(), parameters); + return this.visitField(thing.getScalarField(), parameters, thing.isOptional()); } else if (thing.isField?.()) { return this.visitField(thing, parameters); } else if (thing.isMapDeclaration?.()) { @@ -140,17 +140,19 @@ class GraphQLVisitor { * Visitor design pattern * @param {Field} field - the object being visited * @param {Object} parameters - the parameter + * @param {boolean} [isOptional] - whether the property is optional, defaults to `field.isOptional()`. + * Passed explicitly for scalar-typed properties, because `getScalarField()` does not copy `optional`. * @return {Object} the result of visiting or null * @private */ - visitField(field, parameters) { + visitField(field, parameters, isOptional = field.isOptional()) { let type = this.toGraphQLType( this.namespaces ? field.getFullyQualifiedTypeName() : field.getType() ); if(field.isArray()) { type = `[${type}]`; } - if(!field.isOptional()) { + if(!isOptional) { type = `${type}!`; } diff --git a/lib/codegen/fromcto/protobuf/protobufvisitor.js b/lib/codegen/fromcto/protobuf/protobufvisitor.js index 9cde4e0a..31abe0b4 100644 --- a/lib/codegen/fromcto/protobuf/protobufvisitor.js +++ b/lib/codegen/fromcto/protobuf/protobufvisitor.js @@ -41,14 +41,16 @@ class ProtobufVisitor { /** * Transform a Concerto meta property into a Proto3 field rule. * @param {Object} field - the Concerto meta property + * @param {boolean} [isOptional] - whether the property is optional, defaults to `field.isOptional()`. + * Callers pass it explicitly for scalar-typed properties, because `getScalarField()` does not copy `optional`. * @return {Object} the Proto3 field rule * @public */ - concertoToProto3FieldRule(field) { + concertoToProto3FieldRule(field, isOptional = field.isOptional()) { // An array (repeated) in Proto3 is implicitly optional. if (field.isArray()) { return 'repeated'; - } else if (field.isOptional()) { + } else if (isOptional) { return 'optional'; } else { return null; @@ -178,7 +180,7 @@ class ProtobufVisitor { } else if (thing.isClassDeclaration?.()) { return this.visitClassDeclaration(thing, parameters); } else if (thing.isTypeScalar?.()) { - return this.visitField(thing.getScalarField(), parameters); + return this.visitField(thing.getScalarField(), parameters, thing.isOptional()); } else if (thing.isField?.()) { return this.visitField(thing, parameters); } else if (thing.isRelationship?.()) { @@ -388,12 +390,13 @@ class ProtobufVisitor { * Visitor design pattern * @param {Object} field - the object being visited * @param {Object} parameters - the parameter + * @param {boolean} [isOptional] - whether the property is optional, defaults to `field.isOptional()` * @private */ - visitField(field, parameters) { + visitField(field, parameters, isOptional = field.isOptional()) { debug('entering visitField', field.getName()); - const preposition = this.concertoToProto3FieldRule(field); + const preposition = this.concertoToProto3FieldRule(field, isOptional); if (ModelUtil.isMap(field)) { const mapDeclaration = field.getModelFile().getType(field.getType()); diff --git a/lib/codegen/fromcto/rust/rustvisitor.js b/lib/codegen/fromcto/rust/rustvisitor.js index a356bf05..4fd1fd93 100644 --- a/lib/codegen/fromcto/rust/rustvisitor.js +++ b/lib/codegen/fromcto/rust/rustvisitor.js @@ -190,7 +190,7 @@ class RustVisitor { } else if (thing.isMapDeclaration?.()) { return this.visitMapDeclaration(thing, parameters); } else if (thing.isTypeScalar?.()) { - return this.visitField(thing.getScalarField(), parameters); + return this.visitField(thing.getScalarField(), parameters, thing.isOptional()); } else if (thing.isField?.()) { return this.visitField(thing, parameters); } else if (thing.isRelationship?.()) { @@ -399,10 +399,12 @@ class RustVisitor { * Visitor design pattern * @param {Field} field - the object being visited * @param {Object} parameters - the parameter + * @param {boolean} [isOptional] - whether the property is optional, defaults to `field.isOptional?.()`. + * Passed explicitly for scalar-typed properties, because `getScalarField()` does not copy `optional`. * @return {Object} the result of visiting or null * @private */ - visitField(field, parameters) { + visitField(field, parameters, isOptional = field.isOptional?.()) { // Check for decorators const decorators = field.getDecorators?.() || []; @@ -511,7 +513,7 @@ class RustVisitor { // Determine HashMap type let hashMapType = `HashMap<${rustKeyType}, ${rustValueType}>`; - if (field.isOptional?.()) { + if (isOptional) { hashMapType = this.wrapAsOption(hashMapType); } // Add description for HashMap fields @@ -522,7 +524,7 @@ class RustVisitor { parameters.fileWriter.writeLine(2, `rename = "${field.name}",`); // Add skip_serializing_if for optional HashMap fields - if (field.isOptional?.()) { + if (isOptional) { parameters.fileWriter.writeLine( 2, 'skip_serializing_if = "Option::is_none",' @@ -535,7 +537,7 @@ class RustVisitor { if (isKeyDateTime && isValueDateTime) { // Both key and value are DateTime - if (field.isOptional?.()) { + if (isOptional) { parameters.fileWriter.writeLine( 2, 'serialize_with = "serialize_hashmap_datetime_both_option",' @@ -556,7 +558,7 @@ class RustVisitor { } } else if (isKeyDateTime) { // Only key is DateTime - if (field.isOptional?.()) { + if (isOptional) { parameters.fileWriter.writeLine( 2, 'serialize_with = "serialize_hashmap_datetime_key_option",' @@ -577,7 +579,7 @@ class RustVisitor { } } else if (isValueDateTime) { // Only value is DateTime - if (field.isOptional?.()) { + if (isOptional) { parameters.fileWriter.writeLine( 2, 'serialize_with = "serialize_hashmap_datetime_value_option",' @@ -616,7 +618,7 @@ class RustVisitor { parameters.fileWriter.writeLine(1, '#[serde('); parameters.fileWriter.writeLine(2, `rename = "${field.name}",`); - if (field.isOptional?.()) { + if (isOptional) { parameters.fileWriter.writeLine( 2, 'skip_serializing_if = "Option::is_none",' @@ -624,7 +626,7 @@ class RustVisitor { type = this.wrapAsOption(type); } if (this.isDateField(field.type)) { - if (field.isArray?.() && field.isOptional?.()) { + if (field.isArray?.() && isOptional) { // Option>> parameters.fileWriter.writeLine( 2, @@ -644,7 +646,7 @@ class RustVisitor { 2, 'deserialize_with = "deserialize_datetime_array",' ); - } else if (field.isOptional?.()) { + } else if (isOptional) { // Option> parameters.fileWriter.writeLine( 2, diff --git a/test/codegen/fromcto/avro/avrovisitor.js b/test/codegen/fromcto/avro/avrovisitor.js index a841e726..e9bb0c71 100644 --- a/test/codegen/fromcto/avro/avrovisitor.js +++ b/test/codegen/fromcto/avro/avrovisitor.js @@ -39,6 +39,18 @@ concept Person identified by email { } `; +const MODEL_OPTIONAL_SCALAR = ` +namespace test@1.0.0 + +scalar SSN extends String + +concept Person { + o SSN ssn + o SSN taxId optional + o String name optional +} +`; + describe('Avro', function () { describe('samples', () => { @@ -76,4 +88,19 @@ describe('Avro', function () { }).should.throw('Unrecognised type: string, value: \'Something of unrecognised type\''); }); }); + + describe('optional scalar properties', () => { + it('should generate an optional scalar property as a nullable union', () => { + const modelManager = new ModelManager(); + modelManager.addCTOModel(MODEL_OPTIONAL_SCALAR); + const visitor = new AvroVisitor(); + const fileWriter = new InMemoryWriter(); + modelManager.accept(visitor, { fileWriter }); + const file = fileWriter.getFilesInMemory().get('test@1.0.0.avdl'); + file.should.match(/^\s*string ssn;$/m); + file.should.match(/^\s*union \{ null, string \} taxId;$/m); + // The same as an optional property declared with the primitive type + file.should.match(/^\s*union \{ null, string \} name;$/m); + }); + }); }); diff --git a/test/codegen/fromcto/graphql/graphqlvisitor.js b/test/codegen/fromcto/graphql/graphqlvisitor.js index fab34b23..8f10f1c1 100644 --- a/test/codegen/fromcto/graphql/graphqlvisitor.js +++ b/test/codegen/fromcto/graphql/graphqlvisitor.js @@ -29,6 +29,7 @@ const Field = require('@accordproject/concerto-core').Field; const RelationshipDeclaration = require('@accordproject/concerto-core').RelationshipDeclaration; const FileWriter = require('@accordproject/concerto-util').FileWriter; const { ModelUtil } = require('@accordproject/concerto-core'); +const { InMemoryWriter } = require('@accordproject/concerto-util'); let sandbox = sinon.createSandbox(); @@ -56,6 +57,18 @@ asset Vehicle { } `; +const MODEL_OPTIONAL_SCALAR = ` +namespace test@1.0.0 + +scalar SSN extends String + +concept Person { + o SSN ssn + o SSN taxId optional + o String name optional +} +`; + describe('GraphQLVisitor', function () { let graphQLVisitor; let mockFileWriter; @@ -424,4 +437,18 @@ ref: String arrayRef: String) on OBJECT | FIELD_DEFINITION`]); }); }); + + describe('optional scalar properties', () => { + it('should not mark an optional scalar property as non-null', () => { + const modelManager = new ModelManager(); + modelManager.addCTOModel(MODEL_OPTIONAL_SCALAR); + const fileWriter = new InMemoryWriter(); + modelManager.accept(new GraphQLVisitor(), { fileWriter }); + const file = fileWriter.getFilesInMemory().get('model.gql'); + file.should.match(/^\s*ssn: String!$/m); + file.should.match(/^\s*taxId: String$/m); + // The same as an optional property declared with the primitive type + file.should.match(/^\s*name: String$/m); + }); + }); }); \ No newline at end of file diff --git a/test/codegen/fromcto/protobuf/data/org.accordproject.protocol.v1_0_0-expected.proto b/test/codegen/fromcto/protobuf/data/org.accordproject.protocol.v1_0_0-expected.proto index 7039a281..055da9cb 100644 --- a/test/codegen/fromcto/protobuf/data/org.accordproject.protocol.v1_0_0-expected.proto +++ b/test/codegen/fromcto/protobuf/data/org.accordproject.protocol.v1_0_0-expected.proto @@ -48,12 +48,12 @@ message Function { repeated string emittedTypes = 2; string name = 3; string requestType = 4; - string responseType = 5; + optional string responseType = 5; } message Logic { repeated Function functions = 1; - string stateType = 2; + optional string stateType = 2; } message Template { @@ -89,7 +89,7 @@ message Agreement { Metadata metadata = 7; repeated string references = 8; repeated Signature signatures = 9; - string state = 10; + optional string state = 10; string template = 11; } @@ -133,7 +133,7 @@ message _Subclasses_of_class_ConversionOptions { } message PdfConversionOptions { - string styles = 1; + optional string styles = 1; } enum FeatureType { @@ -160,7 +160,7 @@ message TriggerResponse { optional string errorDetails = 1; optional string errorMessage = 2; bool isError = 3; - string result = 4; + optional string result = 4; } enum AgreementStatusType { diff --git a/test/codegen/fromcto/protobuf/protobufvisitor.js b/test/codegen/fromcto/protobuf/protobufvisitor.js index 077956df..983340f4 100644 --- a/test/codegen/fromcto/protobuf/protobufvisitor.js +++ b/test/codegen/fromcto/protobuf/protobufvisitor.js @@ -42,6 +42,18 @@ const ModelUtil = require('@accordproject/concerto-core').ModelUtil; let sandbox = sinon.createSandbox(); +const MODEL_OPTIONAL_SCALAR = ` +namespace test@1.0.0 + +scalar SSN extends String + +concept Person { + o SSN ssn + o SSN taxId optional + o String name optional +} +`; + describe('ProtobufVisitor', function () { let protobufVisitor; let mockFileWriter; @@ -774,4 +786,18 @@ describe('ProtobufVisitor', function () { ); }); }); + + describe('optional scalar properties', () => { + it('should generate an optional scalar property with the optional keyword', () => { + const modelManager = new ModelManager(); + modelManager.addCTOModel(MODEL_OPTIONAL_SCALAR); + const fileWriter = new InMemoryWriter(); + modelManager.accept(new ProtobufVisitor(), { fileWriter }); + const file = fileWriter.getFilesInMemory().get('test.v1_0_0.proto'); + file.should.match(/^\s*string ssn = \d+;$/m); + file.should.match(/^\s*optional string taxId = \d+;$/m); + // The same as an optional property declared with the primitive type + file.should.match(/^\s*optional string name = \d+;$/m); + }); + }); }); diff --git a/test/codegen/fromcto/rust/rustvisitor.js b/test/codegen/fromcto/rust/rustvisitor.js index 313856c3..b8aadd68 100644 --- a/test/codegen/fromcto/rust/rustvisitor.js +++ b/test/codegen/fromcto/rust/rustvisitor.js @@ -33,9 +33,22 @@ const ModelManager = require('@accordproject/concerto-core').ModelManager; const RelationshipDeclaration = require('@accordproject/concerto-core').RelationshipDeclaration; const FileWriter = require('@accordproject/concerto-util').FileWriter; +const { InMemoryWriter } = require('@accordproject/concerto-util'); let sandbox = sinon.createSandbox(); +const MODEL_OPTIONAL_SCALAR = ` +namespace test@1.0.0 + +scalar SSN extends String + +concept Person { + o SSN ssn + o SSN taxId optional + o String name optional +} +`; + describe('RustVisitor', function () { let rustVisitor; let mockFileWriter; @@ -1809,4 +1822,18 @@ describe('RustVisitor', function () { ).calledOnce.should.be.ok; }); }); + + describe('optional scalar properties', () => { + it('should generate an optional scalar property as an Option', () => { + const modelManager = new ModelManager(); + modelManager.addCTOModel(MODEL_OPTIONAL_SCALAR); + const fileWriter = new InMemoryWriter(); + modelManager.accept(new RustVisitor(), { fileWriter }); + const file = fileWriter.getFilesInMemory().get('test_1_0_0.rs'); + file.should.match(/pub ssn: String,/); + file.should.match(/rename = "taxId",\s+skip_serializing_if = "Option::is_none",\s+\)\]\s+pub tax_id: Option,/); + // The same as an optional property declared with the primitive type + file.should.match(/rename = "name",\s+skip_serializing_if = "Option::is_none",\s+\)\]\s+pub name: Option,/); + }); + }); }); diff --git a/types/lib/codegen/fromcto/avro/avrovisitor.d.ts b/types/lib/codegen/fromcto/avro/avrovisitor.d.ts index 9f982dae..1903dbd8 100644 --- a/types/lib/codegen/fromcto/avro/avrovisitor.d.ts +++ b/types/lib/codegen/fromcto/avro/avrovisitor.d.ts @@ -57,6 +57,8 @@ declare class AvroVisitor { * Visitor design pattern * @param {Field} field - the object being visited * @param {Object} parameters - the parameter + * @param {boolean} [isOptional] - whether the property is optional, defaults to `field.isOptional()`. + * Passed explicitly for scalar-typed properties, because `getScalarField()` does not copy `optional`. * @return {Object} the result of visiting or null * @private */ diff --git a/types/lib/codegen/fromcto/graphql/graphqlvisitor.d.ts b/types/lib/codegen/fromcto/graphql/graphqlvisitor.d.ts index 6deecc6c..5eaca423 100644 --- a/types/lib/codegen/fromcto/graphql/graphqlvisitor.d.ts +++ b/types/lib/codegen/fromcto/graphql/graphqlvisitor.d.ts @@ -52,6 +52,8 @@ declare class GraphQLVisitor { * Visitor design pattern * @param {Field} field - the object being visited * @param {Object} parameters - the parameter + * @param {boolean} [isOptional] - whether the property is optional, defaults to `field.isOptional()`. + * Passed explicitly for scalar-typed properties, because `getScalarField()` does not copy `optional`. * @return {Object} the result of visiting or null * @private */ diff --git a/types/lib/codegen/fromcto/protobuf/protobufvisitor.d.ts b/types/lib/codegen/fromcto/protobuf/protobufvisitor.d.ts index b0afbff9..3fd8f057 100644 --- a/types/lib/codegen/fromcto/protobuf/protobufvisitor.d.ts +++ b/types/lib/codegen/fromcto/protobuf/protobufvisitor.d.ts @@ -17,10 +17,12 @@ declare class ProtobufVisitor { /** * Transform a Concerto meta property into a Proto3 field rule. * @param {Object} field - the Concerto meta property + * @param {boolean} [isOptional] - whether the property is optional, defaults to `field.isOptional()`. + * Callers pass it explicitly for scalar-typed properties, because `getScalarField()` does not copy `optional`. * @return {Object} the Proto3 field rule * @public */ - public concertoToProto3FieldRule(field: any): any; + public concertoToProto3FieldRule(field: any, isOptional?: boolean): any; /** * Transform a Concerto primitive type into a Proto3 one. * @param {Object} type - the Concerto primitive type @@ -128,6 +130,7 @@ declare class ProtobufVisitor { * Visitor design pattern * @param {Object} field - the object being visited * @param {Object} parameters - the parameter + * @param {boolean} [isOptional] - whether the property is optional, defaults to `field.isOptional()` * @private */ private visitField; diff --git a/types/lib/codegen/fromcto/rust/rustvisitor.d.ts b/types/lib/codegen/fromcto/rust/rustvisitor.d.ts index c8e1ff0f..3c16f321 100644 --- a/types/lib/codegen/fromcto/rust/rustvisitor.d.ts +++ b/types/lib/codegen/fromcto/rust/rustvisitor.d.ts @@ -59,6 +59,8 @@ declare class RustVisitor { * Visitor design pattern * @param {Field} field - the object being visited * @param {Object} parameters - the parameter + * @param {boolean} [isOptional] - whether the property is optional, defaults to `field.isOptional?.()`. + * Passed explicitly for scalar-typed properties, because `getScalarField()` does not copy `optional`. * @return {Object} the result of visiting or null * @private */