Skip to content

Keep legacy field results aligned after authorization - #5738

Open
ydah wants to merge 1 commit into
rmosolgo:masterfrom
ydah:fix-legacy-field-authorization-alignment
Open

ydah wants to merge 1 commit into
rmosolgo:masterfrom
ydah:fix-legacy-field-authorization-alignment

Conversation

@ydah

@ydah ydah commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Execution::Next filters objects and result locations when field authorization rejects an object.

The resolve_legacy_instance_method execution mode ignored that filtered object list and instead resolved fields against every GraphQL object from the parent selection. When an object was rejected, the resolved values no longer aligned with their result locations:

# Expected
[nil, "legacy-Nightshades", "legacy-Curcurbits"]

# Before
[nil, "legacy-Legumes", "legacy-Nightshades"]

This could expose a field value from an unauthorized object under another object's result.

This PR uses the authorized object list when legacy instance methods are resolved. The existing cached GraphQL object instances are retained when the object list is unchanged; filtered lists are wrapped again so their values remain aligned with the authorized result locations.

graphql_objects = if objects.equal?(@selections_step.objects)
@selections_step.graphql_objects
else
objects.map { |object| @parent_type.scoped_new(object, context) }

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.

Will this create new GraphQL::Object instances for each field that uses legacy execution? Is there any way that we could reuse them? For example @selections_step.graphql_objects_for(objects) which

Wait a minute -- this else block only applies when field authorization has eliminated an object from objects, right?

If that's the case, then could we somehow use the instances from @selections_step.graphql_objects, but omit the ones that were removed by field authorization?

Or maybe it's not worth the effort. I plan to remove legacy field resolution before too long anyways.

What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, we could carry the original wrapper instances through field authorization, but it gets more complicated because unauthorized_field may return replacement objects and field extensions may replace the object list.

Also, the fallback currently applies whenever field authorization creates a new array, even if every object passes, not only when an object is removed.

Given that legacy field resolution will be removed soon, I think the current implementation is the better tradeoff: it preserves the existing cached instances on the unchanged fast path and keeps the authorization fix small and straightforward. WDYT?

This branch has not been deployed

No deployments
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