Skip to content

Replace ComputationDetail with a function adapter registry #1330

Description

@myronmarston

Prep refactor #2 of 2, paving the way for PR #1327 (approximatePercentile). This ticket does not add the percentile function — it makes the framework ready for it.

What to build

Every aggregated value function routes through a function adapter that owns all of that function's datastore-specific behavior: the datastore aggregation name, GraphQL argument extraction, extra aggregation clause options, reading the value out of the response, and fabricating a response for a bucket the datastore omitted. Per-field runtime metadata collapses to a single function name, and ComputationDetail is deleted.

Why

ElasticGraph is designed so aggregated value functions are treated uniformly, with differences expressed via runtime metadata rather than special casing. That works while every function takes no arguments and returns a flat {"value" => ...} response. It breaks for a function that needs a request argument and returns a nested response: there is nowhere for that knowledge to live, so it leaks as if function == :percentiles conditionals across the clause builder, the resolver, and the empty-bucket builder.

After this ticket, a new function is a registry entry plus one adapter — no changes to the clause builder, resolver, or empty-bucket builder.

The adapter interface

Five methods:

Method Responsibility
datastore_function_name The datastore's aggregation type (may differ from the ElasticGraph function name)
extract_args(args, element_names) GraphQL args → canonically-keyed args hash
clause_options(function_args) Extra keys merged into the aggregation clause alongside field
extract_result(raw) Locate the value hash within the datastore's response
empty_bucket_result The complete fabricated response for a bucket the datastore omitted

Two shapes are supported, because adapters need not all be singletons:

  • A data class for the parameterized common case. All five existing functions differ only in their datastore name and their empty-bucket value, so one data class parameterized on those two values covers all of them. Its other three methods are no-ops ({}, {}, identity).
  • A singleton module for a function with no alternate configuration. Where a function's behavior is not a parameterization of anything, the adapter is a module with def self. methods. Use def self.not class << self.

Registry: a frozen hash keyed by Symbol, matching how ElasticGraph already models registered-thing names (the GraphQL resolver registry is Hash[Symbol, ...], and the configured-resolver metadata symbolizes its name on load). The current in-memory function value is already a Symbol, so this preserves the existing type.

Sketch from the design discussion — trimmed to the decision-rich parts:

module FunctionAdapter
  # Adapter for metric aggregations that take no arguments and return a flat
  # `{"value" => ...}` response, which describes all currently supported functions.
  class SimpleMetric < ::Data.define(:datastore_function_name, :empty_bucket_value)
    def extract_args(args, element_names)
      {}
    end

    def clause_options(function_args)
      {}
    end

    def extract_result(raw)
      raw
    end

    def empty_bucket_result
      {"value" => empty_bucket_value}
    end
  end

  BY_NAME = {
    avg: SimpleMetric.new(datastore_function_name: "avg", empty_bucket_value: nil),
    cardinality: SimpleMetric.new(datastore_function_name: "cardinality", empty_bucket_value: 0),
    max: SimpleMetric.new(datastore_function_name: "max", empty_bucket_value: nil),
    min: SimpleMetric.new(datastore_function_name: "min", empty_bucket_value: nil),
    sum: SimpleMetric.new(datastore_function_name: "sum", empty_bucket_value: 0)
  }.freeze
end

Design decisions

Metadata names the adapter; the adapter owns the datastore aggregation name. Currently the metadata function value doubles as the datastore aggregation type. Breaking that coupling means the metadata name and the datastore name can diverge (which the percentile function needs), and it leaves the computation value object holding exactly one function-related thing with every question routing to the adapter. For all five existing functions the metadata string is unchanged, so the artifact diff is purely the removed empty-bucket value plus ticket #1329's key change.

Rejected: a full extension reference (constant name + require path, as GraphQL resolvers use). Over-engineered for a framework-internal concept; the step from a name to an extension reference stays small if a third party ever needs it.

Arguments flow in two phases: extract, then apply. Argument names are customizable via schema element names, so reading them requires access to those names — which the computation value object does not have when building a clause. Query building therefore calls extract_args(args, element_names) at the boundary, producing a canonically-keyed hash (stable regardless of the schema's casing convention) that is stored on the computation; clause building later calls clause_options(function_args), which is pure.

An args hash, not a scalar — future functions may take more than one argument.

Rejected alternatives:

  • Precomputing the clause options at query-build time — the computation would then carry a raw datastore fragment assembled elsewhere, splitting clause construction across two files.
  • Threading element names onto the computation value object — pollutes its identity, since the object's equality is used to deduplicate computations in a Set.

The argument plumbing ships in this ticket even though no function uses it yet. The point of these prep tickets is that the framework is ready; deferring the plumbing would leave the follow-up PR changing the adapter interface itself, which is exactly the framework design work being front-loaded. The cost is two no-op methods on the data class, which serve as honest documentation of the extension point.

Field objects resolve the name to an adapter once, at boot. The GraphQL schema field object looks the name up in the registry when it is constructed and exposes the adapter directly. Fields are built once and cached, so resolution happens per field rather than per query; the registry then has exactly one reader in the codebase; and an unregistered name fails at boot rather than mid-query. The computation value object holds the adapter itself, so the clause builder and empty-bucket builder call adapter methods with no metadata indirection — which is what removes their conditionals.

The empty-bucket value moves from per-field metadata into the registry. It is fully derivable from the function (verified across all 45 aggregated value fields: sum and cardinality yield 0; avg, min, and max yield nil), so per-field storage is redundant and lets a schema-definition author pair a function with a wrong empty value. Moving it also removes a read-path/write-path asymmetry: the adapter returns the entire fabricated response rather than a bare value that the empty-bucket builder must wrap, so the empty-bucket shape is written outright instead of derived by inverting a response-reading path.

Dump-time validation of the function name. The schema-definition setter raises a schema error unless the name is registered. Precedent: elasticgraph-schema_definition already depends on elasticgraph-graphql specifically for this kind of dump-time validation (its gemspec notes the dependency exists to validate scalar coercion adapters), and the built-in type definitions already require GraphQL-gem files directly — so this adds no new gem dependency. Validating at the call site gives an error message with the field in scope, and a mistyped name (:percentiles vs :percentile) is a plausible slip.

The require goes at the top of the file, not inside the setter method.

Not validated: that each adapter implements the interface. The registry is framework-internal and covered by specs; per ElasticGraph's guidance against defensive code for impossible cases, a missing method is a bug that should fail fast.

Renames

Before After
ComputationDetail class deleted (class, spec, RBS, requires)
GraphQLField#computation_detail computation_function (a bare Symbol)
GraphQLField#with_computation_detail deleted (its only caller is its own spec)
Field#runtime_metadata_computation_detail (schema def) computes(function)
Field struct member computation_detail (schema def) computation_function
Schema::Field#computation_detail (graphql) function_adapter, returning the adapter
Computation#detail replaced by function_adapter + function_args

computes :sum reads as part of the field DSL, which is otherwise unprefixed (documentation, mapping, json_schema). The old runtime_metadata_ prefix only marked "internal," which the private annotation already conveys, and it was the sole setter among the runtime_metadata_* methods.

Runtime metadata per field collapses from three keys to one:

approximate_sum:
  computation_function: sum

RBS: type the adapter as a union of the data class and the singleton module(s), following the existing grouping-adapter precedent (singleton(...)). Keep the empty-bucket value typed ::Numeric? — the type the deleted class declared; widen only if a non-numeric empty is genuinely needed later.

PR framing

Present this honestly as a prep refactor for PR #1327. ElasticGraph reviewers are accustomed to prep refactorings. The removal of redundant per-field metadata is a genuine cleanup the new abstraction enables, not the primary motivation.

Acceptance criteria

  • TDD order followed: each test written and observed failing at runtime (not merely failing to load) before the corresponding implementation
  • Adapter spec asserting the data class's full five-method interface contract
  • Schema-definition spec asserting that an unregistered function name raises a schema error naming the offending field
  • The clause builder, the aggregated-values resolver, and the empty-bucket builder each contain zero function-name conditionals
  • The registry is read in exactly one place (field construction)
  • ComputationDetail and with_computation_detail are deleted, along with their specs, RBS signatures, and requires
  • The empty-bucket value no longer appears in any per-field metadata; all approximately 13 schema-definition call sites read f.computes :<function>
  • The adapter require sits at the top of the schema-definition field file
  • Adapters use def self. rather than class << self
  • No percentile adapter is added — the registry contains only the five existing functions
  • Argument plumbing (extract_args, clause_options, the computation's args attribute) is present and exercised
  • Schema artifacts regenerated via bundle exec rake schema_artifacts:dump; each aggregated value field's metadata is a single function-name key
  • RBS uses a union of the data class and singleton module(s); empty-bucket value typed ::Numeric?; script/type_check passes
  • script/run_specs passes with 100% coverage maintained
  • script/quick_build passes
  • PR description frames the change as a prep refactor for Add approximatePercentile aggregated value function #1327

Follow-up

With both prep tickets landed, PR #1327 reduces to: a percentile adapter (singleton module, def self.), one registry entry, the aggregated-values field definition, two new schema element names, docs, and tests. No framework changes.

Blocked by

Metadata

Metadata

Assignees

No one assigned

    Labels

    ready-for-agentTicket is fully specified and agent-grabbablerubyPull requests that update Ruby code

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions