Cannot have a property on a request called validate

[dselman]

Get a runtime failure because when we try to call the validate function on the Concerto object we are actually accessing the field.

[dselman]

This has been reported by a user.

[jeromesimeon]

Should this issue be moved to https://github.com/accordproject/concerto ?

(maybe with the cool issue transfer feature in GitHub)

[mttrbrts]

Replicate with the following script ...

const ModelManager = require('@accordproject/concerto-core').ModelManager;
const Factory = require('@accordproject/concerto-core').Factory;
const Serializer = require('@accordproject/concerto-core').Serializer;

const modelManager = new ModelManager();
const concertoModel = `
namespace issue121

concept TestConcept {
  o String validate
}
`;

modelManager.addModelFile( concertoModel, 'file.cto');

const factory = new Factory(modelManager);
const resource = factory.newConcept('issue121', 'TestConcept');
// resource.validate = 'foo';

const serializer = new Serializer(factory, modelManager);
const plainJsObject = serializer.toJSON(resource);

console.log(JSON.stringify(plainJsObject, null, 2));

This produces the output:

/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/serializer/resourcevalidator.js:433
        throw new ValidationException(formatter({
        ^

ValidationException: Model violation in instance undefined field validate has value undefined (function) expected type String
    at Function.reportFieldTypeViolation (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/serializer/resourcevalidator.js:433:15)
    at ResourceValidator.checkItem (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/serializer/resourcevalidator.js:321:35)
    at ResourceValidator.visitField (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/serializer/resourcevalidator.js:223:22)
    at ResourceValidator.visit (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/serializer/resourcevalidator.js:79:25)
    at Field.accept (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/introspect/decorated.js:65:24)
    at ResourceValidator.visitClassDeclaration (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/serializer/resourcevalidator.js:176:26)
    at ResourceValidator.visit (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/serializer/resourcevalidator.js:75:25)
    at ConceptDeclaration.accept (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/introspect/decorated.js:65:24)
    at Serializer.toJSON (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/serializer.js:109:30)
    at Object.<anonymous> (/Users/matt/dev/sbx/concerto-issue121/index.js:24:34)

Compare this to the output another required field with a missing value

/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/serializer/resourcevalidator.js:482
        throw new ValidationException(formatter({
        ^

ValidationException: Instance undefined missing required field foo
    at Function.reportMissingRequiredProperty (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/serializer/resourcevalidator.js:482:15)
    at ResourceValidator.visitClassDeclaration (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/serializer/resourcevalidator.js:180:39)
    at ResourceValidator.visit (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/serializer/resourcevalidator.js:75:25)
    at ConceptDeclaration.accept (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/introspect/decorated.js:65:24)
    at Serializer.toJSON (/Users/matt/dev/sbx/concerto-issue121/node_modules/@accordproject/concerto-core/lib/serializer.js:109:30)
    at Object.<anonymous> (/Users/matt/dev/sbx/concerto-issue121/index.js:23:34)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)

[mttrbrts]

It appears that this is down to instances of the Typed class storing properties on the native object,

concerto/packages/concerto-core/lib/model/typed.js

Line 109 in f2d9942

this[propName] = value;

This means that all internal API methods for this class and its subtypes are reserved property names, e.g. accept, instanceOf, setPropertyValue, etc.

One solution would be to push field properties into a $values object on the resource. However, this means that we would need to change all client code that reads or writes resource properties programatically. For example,

resource.foo = "bar";
// becomes either
resource.$values.foo = "bar";
resource.setPropertyValue(foo, "bar");

Alternatively, we could add prefixes to all internal method names to reduce the likelihood of collisions. For example, ValidatedConcept.validate becomes ValidatedConcept._validate.

A third option would be to retain the current behaviour but to add better detection and warnings when property names clash with internal methods.

Any other ideas @dselman @jeromesimeon ?

[jeromesimeon]

Alternatively, we could add prefixes to all internal method names to reduce the likelihood of collisions. For example, ValidatedConcept.validate becomes ValidatedConcept._validate.

@mttrbrts Thanks for the reproducibility + analysis!

I think the easiest would be the last option with a twist: use a prefix which is not allowed in the Concerto syntax (I think $validate or '000validate' would do the trick).

[dselman]

This is now supported via the new Concerto API, merged into the release-1.0 branch.