Skip to content

Refactor Execution::Next runner flow - #5726

Closed
ydah wants to merge 1 commit into
rmosolgo:masterfrom
ydah:refactor-execution-next-runner
Closed

ydah wants to merge 1 commit into
rmosolgo:masterfrom
ydah:refactor-execution-next-runner

Conversation

@ydah

@ydah ydah commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

GraphQL::Execution::Runner#execute and #begin_execute currently handle multiplex analysis, query setup, step execution, result finalization, root authorization, operation directives, and root-type dispatch inline.

This PR reorganizes those responsibilities into private methods:

  • Separates multiplex analysis, query setup, step execution, and result finalization.
  • Separates object, abstract, list, and scalar root execution.
  • Extracts root authorization, operation directive handling, and mutation setup.
  • Consolidates repeated root SelectionsStep construction.
  • Keeps isolated mutation step groups on the runner so setup methods can enqueue work.

The existing result ordering, tracing boundaries, directive finalization, mutation isolation, and stack-error handling are preserved.

@rmosolgo

rmosolgo commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Could you please isolate the bug fix from the code reorganization? The bug fix is an easy include, but I doubt I want to include the code reorganization. Whenever possible, I prefer a single method because I can read it from top-to-bottom and know what's going on. When the code is divided into methods, then the reader (me?) has to jump around to find out what's really going on.

From a maintenance perspective, I prefer keeping API surface area as small as possible: private instead of public whenever possible, inline instead of extracted whenever possible, local variable instead of instance variable whenever possible, and so on.

@ydah
ydah force-pushed the refactor-execution-next-runner branch from 22971c2 to 60f2067 Compare September 2, 2026 12:17
@ydah

ydah commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

@rmosolgo Thank you, that makes sense.
I've separated the changes and force-pushed this PR so it now contains only the code reorganization.

@ydah
ydah force-pushed the refactor-execution-next-runner branch from 60f2067 to bb0b120 Compare September 15, 2026 09:31
assert_graphql_equal(expected_result, result)
end

it "doesn't use the query trace return value as the result" do

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I copied this test into master and found that it passed. Was the fix for it pulled into another branch? (Or if not, what motivated adding this test?)

rmosolgo added a commit that referenced this pull request Sep 15, 2026
@rmosolgo

Copy link
Copy Markdown
Owner

I grabbed the tests from this branch in #5737 and confirmed they pass on master. Since this is just a style change, I'd rather not merge it. If you find a performance or behavior reason to propose any of these changes again, I'll definitely reconsider them 👍

@rmosolgo rmosolgo closed this Sep 15, 2026
@ydah
ydah deleted the refactor-execution-next-runner branch September 15, 2026 20:27
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