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

feat: added graphql.UnmarshalInputFromContext #2131

Merged
merged 7 commits into from
Apr 28, 2022
Merged

feat: added graphql.UnmarshalInputFromContext #2131

merged 7 commits into from
Apr 28, 2022

Conversation

giautm
Copy link
Contributor

@giautm giautm commented Apr 27, 2022

This allows unmarshaling the input object from a context.

This PR is the support for ent/contrib#297: entgql: Unmarshal whereInput using graphql package

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@a8m a8m self-assigned this Apr 27, 2022
graphql/input.go Outdated Show resolved Hide resolved
graphql/input.go Outdated Show resolved Hide resolved
codegen/generated!.gotpl Outdated Show resolved Hide resolved
Co-authored-by: Ariel Mashraki <7413593+a8m@users.noreply.github.com>
@giautm giautm marked this pull request as ready for review April 27, 2022 15:28
maps := make(map[reflect.Type]reflect.Value)
for _, v := range unmarshaler {
ft := reflect.TypeOf(v)
if ft.Kind() != reflect.Func {
panic("unmarshaler must be a function")
if ft.Kind() == reflect.Func {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer this avoiding of panics, but if the kind is not a function... what now happens when nothing is added to the maps here?

@giautm
Copy link
Contributor Author

giautm commented Apr 28, 2022

This function can lead to another issue, that is resolver/directives can be executed twice times during query phrase. The first time by UnmarshalInputFromContext, then the second by gqlgen runtime.

@coveralls
Copy link

coveralls commented Apr 28, 2022

Coverage Status

Coverage decreased (-0.2%) to 74.57% when pulling 54635e1 on giautm:giautm-unmarshal-input into 0465dcb on 99designs:master.

graphql/input.go Show resolved Hide resolved
@StevenACoffman
Copy link
Collaborator

This function can lead to another issue, that is resolver/directives can be executed twice times during query phrase. The first time by UnmarshalInputFromContext, then the second by gqlgen runtime.

What is the symptom of this and how can we work around this problem?

graphql/input.go Outdated Show resolved Hide resolved
Co-authored-by: Ariel Mashraki <7413593+a8m@users.noreply.github.com>
@giautm
Copy link
Contributor Author

giautm commented Apr 28, 2022

What is the symptom of this and how can we work around this problem?

For example, the below directive will be called twice times. But I think it won't have any critical effects (because the most directive are stateless). And only called twice time when using gqlgen with entgql.

input TodoInput {
  blahBlah: String! @directiveCount
}

@a8m
Copy link
Collaborator

a8m commented Apr 28, 2022

This function can lead to another issue, that is resolver/directives can be executed twice times during query phrase. The first time by UnmarshalInputFromContext, then the second by gqlgen runtime.

What is the symptom of this and how can we work around this problem?

@StevenACoffman, currently, ent executes the unmarshaling "ahead of time" and resolves all fields (in all levels) in the root query. However, gqlgen runtime does not know that and calls all fields' resolvers even though they were resolved - ent returns the cached result in this case. We plan to send additional patches to skip this and improve performance for ent users.

@StevenACoffman
Copy link
Collaborator

Ok, I can merge this now, but I would ask for a fast follow with patches to have gqlgen runtime skip the redundant resolving so other users of this functionality do not have a problem that seems like it would be very hard to see the cause of.

For example, I can imagine someone who wanted to build a GraphQL federatation gateway using gqlgen using this same functionality with non-idempotent directives.

@StevenACoffman StevenACoffman merged commit fce3a11 into 99designs:master Apr 28, 2022
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.

4 participants