Custom methods that call built-in methods are broken

[girtsl]

I assume this has something to do with this recent change:

only essential methods, actually used in the expression, are registered within the model context (as opposite to all available methods as before), hence the chance of collision with other identifiers is much decreased

I have created a custom method NowUtc:

Toolchain.Instance.AddFunction("NowUtc", () => DateTime.UtcNow.ToUniversalTime());

which in JS translates to this:

ea.addMethod('NowUtc',
        function() {
            return this.Now();
        });

Since my model only uses the method NowUtc, the built-in method Now is no longer registered and the validation fails with the cryptic message TypeError: this.Now is not a function. It was a bit puzzling initially because we don't use the method Now anywhere.

It would seem that if some expression, e.g. DateTime.UtcNow.ToUniversalTime(), is translated to a JS function that calls some built-in EA method, the built-int method would also be automatically registered.

P.S. For some reason I thought that the build-in method Now() uses local time, hence I created UtcNow() in the first place.

[girtsl]

Seems related to #168.

[jwaliszko]

Hi, this is also related to https://github.com/jwaliszko/ExpressiveAnnotations/issues/167.

Note: All of that is related to client-side only, since server-side is smarter - does not require unique names for methods and properties - we can have property named the same as method and EA parser handles it.

Regarding client-side. The problem is we can have, either all toolchain and custom methods registered within the model context, or just the essential subset of them. By the essential subset I mean only these methods, which are actually used within the expression.

Previously (v2.9.3 and below) we had the following situation:

• due to the fact all - toolchain and custom - methods were registered, the chance of collision with other identifiers was higher, i.e. methods in fact unused by expressions could hide the properties, hence causing side effects, e.g. the expression Length < 10 results in false:
• there was no warning that such a collision can happen,
• when debug flag was ON, the logs were not showing the methods because it was too many of them:

Now we have the following situation:

• only essential methods, actually used in the expression, are registered within the model context (as opposite to all available methods as before), hence the chance of collision with other identifiers is much decreased, e.g. the expression Length < 10 does not cause client-side to register any methods,

• naming collisions with methods, regarding client-side expressions compatibility, are handled now (with appropriate exception thrown by server-side ExpressiveAnnotations.MvcUnobtrusive.dll component when collision is detected, forcing user to fix it) e.g. Length(Length) < 10 throws:

  Naming collisions cannot be accepted by client-side - method Length(...) is colliding with Length field identifier.

• when debug flag is ON, methods are shown for model context in debug logs (context is not as bulky as before):

Solution:

What I could propose is to add new registerAllMethods option to the EA settings, and modify the methods registration logic accordingly (not tested yet, just an ad-hoc idea):

registerMethods: function(model, essentialMethods) {
    var i, name, body;
    this.initialize();

    // ----------------- begin -----------------
    if (ea.settings.registerAllMethods) { // register all methods if desired...
        for (name in this.methods) {
            if (this.methods.hasOwnProperty(name)) {
                if (model.hasOwnProperty(name)) { // ... but skip these one which are in conflict with property identifiers
                    logger.warn('Skipping ' + name + '() method registration - naming conflict with the property identifier.');
                    continue;
                }
                body = this.methods[name];
                model[name] = body;
            }
        }
        return;
    }
    // -----------------  end  -----------------

    for (i = 0; i < essentialMethods.length; i++) {
        name = essentialMethods[i];
        if (this.methods.hasOwnProperty(name)) { // if not available, exception will be thrown later, during expression evaluation (not thrown here on purpose - too early, let the log to become more complete)
            body = this.methods[name];
            model[name] = body;
        }
    }
}

Having this, we can turn on all the methods to be registered within the model context, excluding the ones which are in conflict with the property identifiers. Finally, the log entries will be modified as well to hide the methods if such an option is ON, not to disturb the console output.

[jwaliszko]

Proposal implemented here: https://github.com/jwaliszko/ExpressiveAnnotations/commit/e8876b7ede614ab80adebdba0bdfc2eb31fbfe68.

[jwaliszko]

Released with v2.9.6.

[girtsl]

Sorry for not commenting earlier. The fix works for us. Thank you!