Skip to content

fix(codegen): preserve optional on scalar properties in Avro, GraphQL, Protobuf and Rust #313

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

Closed
ItsAbir005 wants to merge 1 commit into from
Closed

fix(codegen): preserve optional on scalar properties in Avro, GraphQL, Protobuf and Rust #313

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
8 changes: 5 additions & 3 deletions lib/codegen/fromcto/avro/avrovisitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?.()) {
Expand Down Expand Up @@ -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} }`;
}

Expand Down
8 changes: 5 additions & 3 deletions lib/codegen/fromcto/graphql/graphqlvisitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?.()) {
Expand Down Expand Up @@ -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}!`;
}

Expand Down
13 changes: 8 additions & 5 deletions lib/codegen/fromcto/protobuf/protobufvisitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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?.()) {
Expand Down Expand Up @@ -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());
Expand Down
22 changes: 12 additions & 10 deletions lib/codegen/fromcto/rust/rustvisitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?.()) {
Expand Down Expand Up @@ -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?.() || [];

Expand Down Expand Up @@ -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
Expand All @@ -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",'
Expand All @@ -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",'
Expand All @@ -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",'
Expand All @@ -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",'
Expand Down Expand Up @@ -616,15 +618,15 @@ 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",'
);
type = this.wrapAsOption(type);
}
if (this.isDateField(field.type)) {
if (field.isArray?.() && field.isOptional?.()) {
if (field.isArray?.() && isOptional) {
// Option<Vec<DateTime<Utc>>>
parameters.fileWriter.writeLine(
2,
Expand All @@ -644,7 +646,7 @@ class RustVisitor {
2,
'deserialize_with = "deserialize_datetime_array",'
);
} else if (field.isOptional?.()) {
} else if (isOptional) {
// Option<DateTime<Utc>>
parameters.fileWriter.writeLine(
2,
Expand Down
27 changes: 27 additions & 0 deletions test/codegen/fromcto/avro/avrovisitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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);
});
});
});
27 changes: 27 additions & 0 deletions test/codegen/fromcto/graphql/graphqlvisitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
});
});
});
10 changes: 5 additions & 5 deletions test/codegen/fromcto/protobuf/data/org.accordproject.protocol.v1_0_0-expected.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -133,7 +133,7 @@ message _Subclasses_of_class_ConversionOptions {
}

message PdfConversionOptions {
string styles = 1;
optional string styles = 1;
}

enum FeatureType {
Expand All @@ -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 {
Expand Down
26 changes: 26 additions & 0 deletions test/codegen/fromcto/protobuf/protobufvisitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
});
});
});
Loading