OpenRefine icon indicating copy to clipboard operation
OpenRefine copied to clipboard

Replace use of java.util.Properties in Bindings

Open tfmorris opened this issue 1 year ago • 2 comments

Currently our variable storage, called "bindings", uses the java.util.Properties class for its implementation and its interface. It also pre-populates a variety of standard variables with newly instantiated objects WrappedCell, WrappedRow, and CellTuple which are rarely ever used, causing the object creation overhead to be wasted

Although it's strongly discouraged, the current code bypasses the setProperty and getProperty methods to use Hashtable's get() and put() directly, allowing it to support an effective contract of Map<String, Object> (but without support for null values since Hashtable doesn't support them).

Proposed solution

  • Introduce a Bindings class which of HashMap<String, Object> which does lazy creation of all wrapped objects only when they are needed.
  • Extend the interfaces which currently use Properties like Evaluable, HasFields, HasFieldsList, Function, and Control to use the new Bindings class with default implementations for backward compatibility.
  • Add a new Binder2 interface which implements the new protocol variable lookup protocol
  • Change code which does direct creation of bindings using new Properties() to use ExpressionUtils.getBindings(Project) like the main code does. Most occurrences of this pattern are scattered throughout the test code.
  • Log warnings on the console (or notify user at startup?) if any legacy Function, Control, or Binder is registered.

Alternatives considered

  • Not attempt compatibility and force all extension writers to update their code immediately.
  • Compatibility could be implemented on a per-call basis or the entire system could fall back to legacy mode if any legacy Function, Control, or Binder is registered - to be investigated if this simplifies things

Additional context

tfmorris avatar Feb 26 '24 17:02 tfmorris

The Binder extension point is currently used by at least one extension (RDF Transform). If we want to get rid of it, it probably makes sense to offer an alternative.

wetneb avatar Feb 27 '24 06:02 wetneb

The Binder extension point is currently used by at least one extension (RDF Transform).

Thanks for the heads up! I guess that illustrates the danger of assuming that any public interface is unused, even if it's not documented. I'll reintroduce the Binder2 interface that I had and figure out what needs to be done to preserve backward compatibility. I'm pretty sure there's no test coverage of this feature.

I've updated the description with more information on API compatibility.

tfmorris avatar Feb 27 '24 16:02 tfmorris