adobe / json-formula

Query language for JSON documents
http://opensource.adobe.com/json-formula/
Apache License 2.0
19 stars 8 forks source link

Restrictions on `register` #149

Closed Eswcvlad closed 4 months ago

Eswcvlad commented 5 months ago

With the current implementation if you call register with an invalid function name, the call silently works without any warnings or errors. Ex. register("", &42), register("0foo", &42), register("фоо", &42).

Would make sense to throw an error, if name does not match [a-zA-Z_$][a-zA-Z0-9_$]* for easier debugging.


Another thing I've noticed, is that calling register with a name of an already existing function does not register the new function, with just a warning debug message shown. While I see the point, why we wouldn't want to allow overriding standard functions, this could cause backward-compatibility issues, if any new standard functions will be added. Ex. if there will be a document in the wild with register("product", &@[0] * @[1]) included in the form and a product function is added in the standard later, it will start failing.

JohnBrinkman commented 5 months ago

this could cause backward-compatibility issues, if any new standard functions will be added

Two possible solutions:

  1. Allow standard functions to be overridden
  2. Advise users to use domain-specific name for custom functions. e.g. 'acmeProduct()'

preference?

Eswcvlad commented 5 months ago

If it was up to me, I would go with (1).

And, potentially, have something like "standard function names do not begin with _ (or $, not sure), so prefer such names, if possible". Might also have a potential side effect, that it will be easier to spot user-defined functions.

I was thinking, that a likely use case for register would be to extract common expression parts under a short readable name, which goes against (2) somewhat.

JohnBrinkman commented 5 months ago

I think I will also restrict the name construction to: /[_a-zA-Z][_a-zA-Z0-9$]*/. i.e. a subset of identifier

JohnBrinkman commented 5 months ago

We talked this over internally, and propose that we solve this by:

  1. Do not allow overrides on existing functions
  2. require registered functions to follow a naming pattern where they begin with an underscore
JohnBrinkman commented 4 months ago

agreed