Skip to content
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.

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

Adding custom functions to shared function libraries can unexpectedly change the behaviour of a consuming system #74

Open
TheCakeMonster opened this issue Jun 21, 2023 · 1 comment

Comments

@TheCakeMonster
Copy link

TLDR;
Adding custom functions to published NCalc function libraries, or NCalc itself, can cause unexpected breaking changes to the way an expression is evaluated if the names of the functions clash. It is unexpected that the addition of new functions would be a breaking change, especially as it does not generate any error message, either at compile time or runtime.

Detailed Explanation
We publish and use an open source library of functions to make NCalc even more useful, namely PanoramicData.NCalcExtensions. PanoramicData.NCalcExtensions extends NCalc by inheriting from the Expression type and adding an event handler to the EvaluateFunction event handler.

One of our internal systems provides even more, system-specific extensions by inheriting from the derived ExtendedExpression class from PanoramicData.NCalcExtensions - adding a third set/layer of functions. Within this system we can therefore evaluate expressions containing functions from NCalc, from PanoramicData.NCalcExtensions and from our system itself.

Recently, a new function was added to PanoramicData.NCalcExtensions with the same name as one of the custom functions in our own system. Suddenly, the function in the PanoramicData.NCalcExtensions library is being used in preference to the one in our own system, breaking our consuming system's functionality. It turns out that adding new functions to a shared, custom functions library can be a breaking change, and this was unexpected.

Possible changes to resolve the issue (not exclusive):

  1. Ideally, I think NCalc would follow the principals of overriding, namely that a function in a derived class was used in preference to the function with the same name in the base class. This makes adding functions to NCalc, and to function libraries, somewhat safer.
  2. An alternative, although not as ideal, is to add a check for a clash of function naming in multiple extensions so that developers are at least made aware that this might cause them a problem.

Fully resolving this issue using my preferred solution (change 1 above) is probably a breaking change, as I think the behaviour can only be changed by replacing the EvaluateFunction event handler with another mechanism that has the ability to guarantee - and control - the order of evaluating which function to use.

I am aware that this library is not actively maintained, but I thought I would post my discovery so that other people were aware of the problem - this might help other people track down an issue they have. Furthermore, should any changes be made to NCalc in the future, this change can be considered as part of those efforts.

Disclaimer: I work for Panoramic Data. I don't see a conflict of interest because of this, but I mention it in the spirit of full disclosure.

@david-brink-talogy
Copy link
Collaborator

david-brink-talogy commented Aug 16, 2023

It seems like you're bumping up against the original design of NCalc here. NCalc doesn't really allow you to register custom functions. It allows you to register a custom event handler any time a function is evaluated. All registered event handlers will be invoked and can potentially set Result, yielding the behavior you've described. I believe .net event handlers are called in the order they were added so if your internal library adds itself after PanoramicData.NCalcExtensions I think it should set Result last and thus win, but you'll be doing extra work.

Some options that wouldn't require NCalc changes:

  • Modify PanoramicData.NCalcExtensions.ExtendedExpression to make Extend protected. In your internal library then override Extend to handle any functions and then call base.Extend()
  • "Namespace" your internal functions. This would be a breaking change to your library, but adding some prefix would limit collisions. internal_add() vs add()
public class InternalExtendedExpression : ExtendedExpression {
  protected override void Extend(string functionName, FunctionArgs functionArgs) {
    switch(functionName) {
      case "collision":
        functionArgs.Result = "correct";
        return;
    }
    
    return base.Extend(functionName, functionArgs);
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants