Skip to content

refactor(isthmus): unify ConverterProvider customization on the builder #1048

Description

@nielspardon

Context

Review discussion on #1036: now that ConverterProvider has a builder, there are two
customization mechanisms — the builder and subclassing — and they can be mixed. As
@vbarua noted,
it isn't obvious which one a user should reach for, and the built-in subclasses are
themselves an example of the ambiguity:

// today
new DynamicConverterProvider(ConverterProvider.builder().extensions(extensions))

// could plausibly be
ConverterProvider.builder()
    .extensions(extensions)
    .scalarFunctionConverter(...)
    .sqlOperatorTable(...)
    .build()

What's missing

DynamicConverterProvider and AutomaticDynamicFunctionMappingConverterProvider subclass
rather than configure because the builder can't express what they override:

getSqlOperatorTable() needs the derived extensions/type factory to compute its value, so
exposing it as a plain builder setting also means giving callers a way to build those pieces
(e.g. a public helper that turns an extension collection into the dynamic operators).

Proposal

  1. Add callConverters to ConverterProvider.Builder. Done in feat(isthmus): let the builder transform the call converter list #1166, as
    callConverters(UnaryOperator<List<CallConverter>>) — a transform over the assembled list
    rather than a replacement for it. A plain list cannot work here: scalarFunctionConverter goes
    last in getCallConverters(), is derived during construction, and DynamicConverterProvider
    reassigns it after super(builder) returns, so anything frozen at build() captures the
    pre-override converter; a caller supplying the whole list themselves would also trip the feat(isthmus): introduce a ConverterProvider builder #1036
    guards. The transform is handed the built-ins with late-bound values in place and runs per
    conversion, so it expresses insert-at-position, append, prepend, replace and filter.
  2. Add sqlOperatorTable to ConverterProvider.Builder. Whether this wants the same transform
    shape is open — unlike the converter list it has no derived-late component, so a plain setting
    may be enough.
  3. Expose the derivation helpers the dynamic providers use so a caller can assemble the same
    configuration. Smaller than originally scoped: no helper is needed for the call-converter half,
    since the transform already receives the derived converter.
  4. Re-express both dynamic providers as builder configuration (static factory methods returning
    a preconfigured Builder), and deprecate the subclasses.
  5. Document one recommended path: the builder for composition, subclassing only for behaviour
    that must be computed per call.

Notes

#1036 added guards that throw IllegalArgumentException when a builder passed to either dynamic
provider configures a function converter those providers derive themselves — a stopgap that makes
the discarded-setting case loud. This issue is about removing the need for that overlap.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions