Skip to content

Conversation

@ravangen
Copy link
Contributor

@ravangen ravangen commented Dec 8, 2025

Objective: Allow for legacy_invalid_empty_selections_on_union hook to know which type had an empty selection set. This way we can log and report to the client what the offending part of the operation was.

This is backwards incompatible change, so I'm not sure how you feel about it. I imagine this API isn't widely used yet.

This functionality was introduced in #5322 (2.5.3).

@rmosolgo
Copy link
Owner

rmosolgo commented Dec 8, 2025

I'm definitely game to support this but I'd like to not 💥 people who have already implemented it. How about accomodating that case somehow? A few possibilities come to mind:

  • Check arity before calling it and warn if it only receives one argument (then only pass one)
  • Implement GraphQL::Schema.method_added to handle this method and crash if it's defined with only one argument. That would at least move the 💥 to development
  • Add a wrapper method, eg ..._with_type, call that method from runtime code instead, and implement that method to call the legacy hook by default (if defined?). Also update docs and warnings to name the new method instead of the old one

If you have another suggestion on how to avoid disruption, I'm certainly open to it -- I'm just offering those in case they're helpful. What do you think?

@ravangen ravangen force-pushed the ravangen.invalid-empty-selection-on-union-type branch from bc122b8 to 560359b Compare December 9, 2025 03:40
…_type to replace legacy_invalid_empty_selections_on_union
@ravangen ravangen force-pushed the ravangen.invalid-empty-selection-on-union-type branch from 560359b to d779607 Compare December 9, 2025 03:46
@ravangen
Copy link
Contributor Author

ravangen commented Dec 9, 2025

I went with the wrapper method approach as it felt simplest.

Validation rule calls legacy_invalid_empty_selections_on_union_with_type which by default calls legacy_invalid_empty_selections_on_union.

The doc strings advocate for overriding legacy_invalid_empty_selections_on_union_with_type, but the original still works.

@rmosolgo rmosolgo added this to the 2.5.15 milestone Dec 9, 2025
@rmosolgo rmosolgo merged commit 5c0a2d5 into rmosolgo:master Dec 9, 2025
12 of 13 checks passed
@rmosolgo
Copy link
Owner

rmosolgo commented Dec 9, 2025

Thanks fort this improvement!

@rmosolgo
Copy link
Owner

rmosolgo commented Dec 9, 2025

🚢 in 2.5.15!

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