Skip to content

Olap 582 namespaced queries pr2#1176

Open
rossroberts-toast wants to merge 23 commits into
block:mainfrom
rossroberts-toast:OLAP-582_namespaced_queries_pr2
Open

Olap 582 namespaced queries pr2#1176
rossroberts-toast wants to merge 23 commits into
block:mainfrom
rossroberts-toast:OLAP-582_namespaced_queries_pr2

Conversation

@rossroberts-toast
Copy link
Copy Markdown
Contributor

Part 2 of the stacked PR series implementing query namespacing
(#1130). This PR introduces the building blocks without any
consumers yet — the root Query type is converted to use them in
PR 3.

What this PR adds:

  • schema.namespace_type "Foo": schema definition API for
    declaring a GraphQL object type that groups root query fields
    under a shared intermediate path (e.g. Query.olap.widgets
    instead of Query.widgets).
  • :namespace_ref resolver: auto-wired on any no-argument field
    whose return type is a namespace type. Returns {} as a
    non-null passthrough so each child field's own resolver can
    run. Users never reference it directly.
  • :constant_value resolver: a standalone primitive that
    returns a configured value for fields that carry no data of
    their own. Available for user schemas; not coupled to
    namespacing.

Behavior:

A field on any type (Query, a namespace type, or a regular
indexed type) whose return type is a namespace type and which
has no args or explicit resolver is auto-wired to
:namespace_ref. Namespace types are excluded from index
mappings, JSON schemas, and derived GraphQL types (filters,
sort orders, aggregations, highlights), as are fields on
indexed types that reference them, since there's no backing
data to act on.

Infrastructure

  • state.namespace_types_by_name registry populated at schema
    definition time.
  • Field#target_type_is_namespace? predicate used by both the
    resolver auto-wire and the indexing-suppression logic.

There are no consumers of namespace_type yet — PR 3 converts
the root Query type to use these primitives.

The resolver handles both list and aggregation root fields for indexed
types, so the new name reflects its actual responsibility. The
`:list_records` resolver symbol is preserved to avoid a breaking change
to schema artifacts.

Generated with Claude Code
Matches the recent class rename (ListRecords -> IndexedTypeRootFieldsResolver)
and aligns with the existing naming convention where the registered symbol
is the snake_case form of the class name, minus the Resolver suffix (e.g.
GetRecordFieldValue -> :get_record_field_value, NestedRelationships ->
:nested_relationships).

Runtime metadata artifacts regenerate with the new symbol; per the
versioning policy, any schema-artifact regeneration is expected on
upgrade.

Generated with Claude Code
Introduces the `namespace_type` schema definition API for grouping
related root query fields under a shared intermediate object, and a
`:constant_value` built-in resolver that returns a configured value
for fields that carry no data of their own (e.g. namespace placeholders).

Fields on a namespace type whose return type is itself a namespace type
and which have no args or explicit resolver are auto-wired to
`:constant_value` returning `{}`, so authors don't have to hand-wire a
trivial resolver at every intermediate node.

This is infrastructure for upcoming work that converts the root Query
type to a namespace type — there are no consumers of `namespace_type`
yet.

Generated with Claude Code
Inside module Resolvers, the unqualified Object reference resolves
to ElasticGraph::GraphQL::Resolvers::Object — the existing module
defined in resolvers/object.rb — rather than ::Object, causing
NoMethodError: undefined method 'new' for module Resolvers::Object.

Generated with Claude Code
Drop the `has_own_index_def?` guard in `__mark_as_namespace_type!`:
it is unreachable through the public API (`namespace_type` always
creates a fresh type before marking it), and the reverse direction
(`namespace` then `index`) is still guarded by `HasIndices#index`.

Drop `&.` on `resolver` in the auto-wiring specs — those assertions
are on paths where the resolver is always present, so the nil branch
is dead.

Generated with Claude Code
The prior example used `root_query_fields plural: ..., on: "OlapQuery"`,
but `on:` is introduced in a later PR. The doctest runner executes
every YARD example, so the forward reference broke CI. Shorten the
example to just declare the namespace type and expose it on `Query`
with an explicit `:constant_value` resolver, and add a placeholder
field so GraphQL-Ruby doesn't warn about an empty object type.

Generated with Claude Code
Addresses PR review feedback flagging `respond_to?(:namespace?) && namespace?`
in `HasIndices#index` as a Java-instanceof-style code smell.

Instead of feature-testing inside the generic `index` method, promote namespace
semantics into the type system: `NamespaceType < ObjectType` overrides
`namespace?` to return `true`, overrides `index` to raise, and moves
`auto_wire_namespace_subfields` onto itself. `ObjectType#namespace?` now simply
returns `false`, and the smell is removed from `HasIndices#index`.

Generated with Claude Code
Previously, the schema author had to spell out `resolve_with :constant_value,
value: {}` on fields like `Query.olap` that return a namespace type. Fields
defined on a namespace type were auto-wired, but fields on `Query` (or any
other parent type) were not — so the boilerplate leaked into every schema
using namespace types.

Now the auto-wiring lives in `HasIndices#runtime_metadata_graphql_fields_by_name`
and applies to any no-argument field whose return type is a namespace type,
regardless of the parent. This treats `Query` as the "default namespace",
which matches how users already think about it.

`NamespaceType` no longer needs its `after_user_definition_complete` hook
or a custom `auto_wire_namespace_subfields` method. `InterfaceType` and
`UnionType` gain a `namespace?` method (returning false) so the lookup via
`object_types_by_name` stays polymorphic without a `respond_to?` guard.

Generated with Claude Code
Split the state-diving `namespace_type_spec.rb` into four artifact-focused
specs (SDL, JSON schema, index mappings, runtime metadata). This makes the
tests robust to refactors of the underlying schema element classes.

Also mark `NamespaceType` as `graphql_only` so the existing filter in
`Results#json_schema_indexing_field_types_by_name` excludes it naturally,
rather than introducing a `NamespaceType`-specific type check.
  - Replace :constant_value + value: {} auto-wire with a dedicated
    :namespace_ref resolver. Fixes a roundtrip bug: schema.rb's
    recursively_prune_nils_and_empties_from was pruning the empty
    value: {} during dump/load, which would have caused KeyError at
    query time.
  - :constant_value stays as a standalone user-facing primitive with
    no namespace coupling in its tests.
  - Suppress indexing registration for fields whose target is a
    namespace type, keeping them out of index mappings and JSON
    schemas of parent indexed types.
  - Replace the namespace? protocol (spread across Object/Interface/
    Union/NamespaceType) with state.namespace_types_by_name plus a
    single Field#target_type_is_namespace? predicate.
  - Extract NAMESPACE_RESOLVER constant; simplify YARD examples;
    add Widget→OlapQuery coverage across all four schema artifacts.
Copy link
Copy Markdown
Collaborator

@myronmarston myronmarston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not done reviewing but have to call it a day and I wanted to submit my feedback so far.

# Fields pointing at a namespace type have no data to index: namespace types are pure GraphQL
# grouping constructs. Skipping indexing registration here keeps them out of both the index
# mapping and the JSON schema for the parent type.
unless graphql_only || field.target_type_is_namespace?
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little funny that the field is treated as graphql-only here, but field.graphql_only will return false for it. In Field#initialize can we do this?

def initialize(
  # ...
)
  type_ref = schema_def_state.type_ref(type)
  super(
    # ...
  )

  self.graphql_only = true if target_type_is_namespace?
  # ... the rest of the logic ...
end

That way, the field is consistently treated as a graphql-only field, and this can just check unless field.graphql_only

Edit: I just realized a significant problem here. ElasticGraph is designed to produce the same schema artifacts no matter what order types are defined in. But target_type_is_namespace?

field.target_type_is_namespace? is timing-specific: when called on a field referencing a namespace type that has not yet been defined, it'll return false...and then later it'll return true. That means that we have to be very careful with how we use it. It's safe to use after the user is finished defining their schema (while ElasticGraph is generating the schema artifacts) but it's not safe to use while we're evaluating the user's schema definition.

One way we deal with this is to check state.user_definition_complete, like so:

unless user_definition_complete
raise Errors::SchemaError, "Cannot access `user_defined_field_references_by_type_name` until the schema definition is complete."
end

I think we should similarly prevent calls to target_type_is_namespace? when the user definition is incomplete. That way, it'll be accurate every time it's used and when it's called from a context in which it isn't accurate, we'll raise an error.

# schema.on_root_query_type do |t|
# t.field "olap", "OlapQuery!"
# end
# end
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks for the thorough docs!

Comment thread elasticgraph-schema_definition/lib/elastic_graph/schema_definition/factory.rb Outdated
Comment thread elasticgraph-schema_definition/lib/elastic_graph/schema_definition/results.rb Outdated
Addresses Myron's PR review: a field whose return type is a namespace type
was being treated as graphql-only at registration time, but `field.graphql_only`
returned false -- inconsistent. Also, `target_type_is_namespace?` was
order-dependent (returning false if the namespace type was registered later),
violating ElasticGraph's invariant that schema artifacts are independent of
declaration order.

- Defer the namespace check to `after_user_definition_complete`, where it's
  safe and order-independent. There, flip `graphql_only = true` and remove
  the field from the parent's indexing map.
- Guard `type_is_namespace?` to raise unless user definition is complete
  (mirrors the pattern at `state.rb:188`).
- Simplify the indexing-field registration check to `unless graphql_only`.
- Complete the in-flight rename `target_type_is_namespace?` -> `type_is_namespace?`
  at remaining call sites and in the RBS signature.
- Complete the dangling sentence in NamespaceType's doc.
- Drop the stale `value: {}` reference in `runtime_metadata_graphql_fields_by_name`
  (left over from when `:constant_value` was used) and avoid naming the
  internal `:namespace_ref` resolver since it's an implementation detail.
Adds a test that exercises the early-access guard in `Field#type_is_namespace?`,
closing the coverage gap reported by the CI build.
The `t.field id` and `t.index "widgets"` calls were unreachable because the
preceding `f.type_is_namespace?` raises and unwinds the schema definition.
Strip them to keep the spec at 100% line coverage.
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

Successfully merging this pull request may close these issues.

2 participants