Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow resolve_type to optionally return resolved object #2976

Merged

Conversation

swalkinshaw
Copy link
Collaborator

@swalkinshaw swalkinshaw commented Jun 8, 2020

Often abstract types in GraphQL are backed by "wrapped" domain objects such as unions, ADTs, etc. Currently resolve_type must return a GraphQL type only even though the wrapped domain object ideally should be unwrapped at this point as well instead of passing through to the next resolver.

This allows abstract type's resolve_type to return an optional "resolved"/unwrapped object in addition the resolved type. To optionally return a new object value, an array can be returned as [type, object].

This is backwards compatible as well and the existing validation still applies to verify the first item in the array is a GraphQL type.

@rmosolgo this is a proposal right now to see what you think. If you think it's useful, I need to update documentation.

Also I'm not 100% sure how this works with lazy types yet. It doesn't break them, but I don't think I supported the [lazy_type, object] case yet. Should that case be supported?

cc @gmalette

Often abstract types in GraphQL are be backed by "wrapped" domain
objects such as unions, ADTs, etc. Currently `resolve_type` must return
a GraphQL type only even though the wrapped domain object ideally should
be unwrapped at this point as well instead of passing through to the next
resolver.

This allows abstract type's `resolve_type` to return an optional
"resolved"/unwrapped object in addition the resolved type. To optionally
return a new object value, an array can be returned as `[type, object]`.

This is backwards compatible as well and the existing validation still
applies to verify the first item in the array is a GraphQL type.
resolved_type_or_lazy = resolve_type(type, value, path)
resolved_type_or_lazy, resolved_value = resolve_type(type, value, path)
resolved_value ||= value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rmosolgo my other option was to add a cast (name tbd) method on interface and unions which would be called separately after resolve_type. It would probably take the resolved type, value, and maybe ctx. The default methods would just pass through value.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't have a preference on the approach, bot this and something like cast_object(type, obj, ctx) would be fine with me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gmalette do you have a preference? I assume in some cases the unwrapping might involve logic which mirrors the type resolving logic so it might make more sense to keep them together. With a cast method you might have to duplicate some logic.

Copy link

Choose a reason for hiding this comment

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

Pretty much your arguments: using cast would likely require duplicated logic and would introduce new affordances for error. The naming of this method is unfortunate if it also maps the value in addition to the type, but I can live with that 🙂

Copy link
Owner

Choose a reason for hiding this comment

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

We could get a twofer and add a new method, like:

# Implement this method to return a two-member array containing: 
# - The concrete type to use for `abstract_type` in this case 
# - A "casted" object, if `object` should be specialized in this case. 
# @return [(Class<GraphQL::Schema::Object>, Object)]
def resolve_type_and_cast_object(abstract_type, object, ctx)
  object_type = resolve_type(abstract_type, object, ctx)
  resolved_obj = cast_object(abstract_type, object_type, object, ctx)
  [object_type, resolved_obj]
end 

# Optionally override this is `obj` should be modified in this context
def cast_object(abs_t, obj_t, obj, ctx)
  obj 
end 

(Maybe they'd be methods on Interface/Union and have different arguments). In something like that, you get to pick new method names, and keep the existing method signature the same. In your case, you could override def resolve_type_and_cast_object, but existing code would stay just the same. (And for better or worse, cast_object could exist as its own API. TBH, you could skip def cast_object and just return [resolved_type, obj], and anyone who wants to cast can override resolve_type_and_cast_object.)

What do you think, worth it to pick a new method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems to make the most sense to colocate all the resolving (type and object) logic in a single method in case they're shared. So I think the only thing resolve_type_and_cast_object buys us is a slightly more explicit method name?

🤔 would resolve_type_and_cast_object require even more changes to Schema#resolve_type as well?

Copy link
Owner

Choose a reason for hiding this comment

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

slightly more explicit method name?

Yeah, that'd be it 🤷

@swalkinshaw swalkinshaw marked this pull request as ready for review June 9, 2020 19:01
@swalkinshaw
Copy link
Collaborator Author

Added docs to the interface face which the union one references as well. Looks like the failing check is allowed to fail.

@rmosolgo rmosolgo added this to the 1.10.11 milestone Jun 11, 2020
@rmosolgo
Copy link
Owner

🎉 🤘 !

@rmosolgo rmosolgo merged commit d5b46b0 into rmosolgo:master Jun 11, 2020
@swalkinshaw swalkinshaw deleted the extend-resolve-to-type-for-return-object branch June 11, 2020 13:55
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.

None yet

3 participants