Skip to content

Fix @key directive missing on deferred indexed types - #1107

Merged
jwils merged 1 commit into
mainfrom
fix-apollo-key-ordering
Apr 5, 2026
Merged

Fix @key directive missing on deferred indexed types#1107
jwils merged 1 commit into
mainfrom
fix-apollo-key-ordering

Conversation

@jwils

@jwils jwils commented Apr 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Types created in after_user_definition_complete callbacks were missing @key(fields: "id") directives and excluded from the _Entity union. In practice, this affected indexed types that are dynamically derived during schema finalization (e.g. aggregation types for location/merchant filtering).
  • Regression from Schema support for inherited indexes #1067 ("Schema support for inherited indexes"): that PR moved @key directive addition into define_apollo_schema_elements, which runs as an early after_user_definition_complete callback. Types created by later callbacks don't exist yet when the @key logic iterates, so they silently lose their federation key.
  • Fix: defer @key and _Entity logic into a nested after_user_definition_complete callback. Ruby's Array#each picks up elements appended during iteration, so the nested callback runs after all previously registered callbacks — ensuring all types exist first.

Test plan

  • Added unit test that creates an indexed type in an after_user_definition_complete callback and verifies it gets @key(fields: "id") and is included in _Entity
  • All 101 apollo unit tests pass
  • Verified end-to-end with tf-sales-eg: schema_artifacts:dump produces no schema.graphql diff (previously OrderLocation and OrderMerchant lost their @key directives)

🤖 Generated with Claude Code

@myronmarston myronmarston left a comment

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.

Left a few questions but LGTM overall.

Comment thread elasticgraph-apollo/lib/elastic_graph/apollo/schema_definition/api_extension.rb Outdated
…complete callbacks

The @key directive addition and _Entity union creation in
define_apollo_schema_elements ran as an early after_user_definition_complete
callback, before types created by later callbacks (e.g. dynamically derived
aggregation types) existed. This caused those types to silently lose their
@key(fields: "id") directive and be excluded from the _Entity union.

Fix by deferring @key and _Entity logic into a nested
after_user_definition_complete callback. Ruby's Array#each picks up appended
elements during iteration, so this callback runs after all previously
registered callbacks, ensuring all types exist first.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jwils
jwils force-pushed the fix-apollo-key-ordering branch from a332fcd to eac18cd Compare April 5, 2026 02:32
@jwils
jwils enabled auto-merge (squash) April 5, 2026 02:32
@jwils
jwils merged commit 61c751e into main Apr 5, 2026
24 checks passed
@jwils
jwils deleted the fix-apollo-key-ordering branch April 5, 2026 02:54
jwils added a commit that referenced this pull request Apr 23, 2026
Distills the patterns Myron Marston applies when reviewing merged PRs
(naming at the caller's level of abstraction, respond_to? as a code
smell, wrapper-class DI pattern, load-bearing tests, RBS/YARD hygiene,
etc.) into an edit-first skill.

Invoked as /myron-polish, the skill walks the current branch's diff
against main and applies fixes directly rather than producing review
comments. It loops — re-scan, apply edits, run script/lint --fix /
script/spellcheck -w / script/type_check / script/run_gem_specs — until
a full iteration makes zero edits and every verification command passes.
Capped at 8 iterations.

Source material: Myron's review bodies and inline comments on merged
PRs #974, #973, #1067, #1066, #1108, #1120, #1131, #1134, #1144, #1107.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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