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

Resolver returning type to be a slice of structs #738

Closed
malekjaroslav opened this issue Jun 8, 2019 · 16 comments · Fixed by #874
Closed

Resolver returning type to be a slice of structs #738

malekjaroslav opened this issue Jun 8, 2019 · 16 comments · Fixed by #874

Comments

@malekjaroslav
Copy link

malekjaroslav commented Jun 8, 2019

What happened?

Now resolvers are supposed to return slices of pointers to structs. When using Prisma, their methods always return slices of structs. This means in every method I have to retype the slice to a slice of pointers. Do you know about some way to get around it, or do you think Prisma returning type is to be edited?

What did you expect?

Generated resolvers to be typed to return a slice of structs..

Minimal graphql.schema and models to reproduce

// Cities returns a list of cities
func (r *queryResolver) Cities(
	ctx context.Context,
	where *prisma.CityWhereInput,
	orderBy *prisma.CityOrderByInput,
	skip *int,
	after *string,
	before *string,
	first *int,
	last *int,
) ([]*prisma.City, error) {

	// Returns a slice of structs
	result, err := r.Prisma.Cities(&prisma.CitiesParams{
		Where:   where,
		OrderBy: orderBy,
	}).Exec(ctx)

	// Convert to a slice of pointers 
	cities := make([]*prisma.City, len(result))
	for i := range result {
		cities[i] = &result[i]
	}
	return cities, err

}
@malekjaroslav malekjaroslav changed the title Resolver returning array structs to return array of pointes Resolver returning type to return an array instead of a slice Jun 8, 2019
@malekjaroslav malekjaroslav changed the title Resolver returning type to return an array instead of a slice Resolver returning type to be an array instead of a slice Jun 8, 2019
@malekjaroslav malekjaroslav changed the title Resolver returning type to be an array instead of a slice Resolver returning type to be an slice of structs Jun 8, 2019
@malekjaroslav malekjaroslav changed the title Resolver returning type to be an slice of structs Resolver returning type to be a slice of structs Jun 8, 2019
@jszwedko
Copy link

I've run into this too. I think it might be nice to make this configurable behavior both globally and at the field resolver level.

@mathewbyrne
Copy link
Contributor

This was a major change in 0.9.0 and was the final piece of #375, so that all returned structs are expected to be pointers. We feel like this is the ideal interface for the current implementation of gqlgen.

That said, there has been a lot of people asking about this change, and we did have #719 submitted attempting to introduce this as a configuration option. We didn't feel like it was something we wanted to maintain in the library since it touches a lot of places so it was closed.

That said, our position has relaxed a little on this, so if there was a solution that didn't touch as many places in code and was simpler to maintain, it might be considered. @vektah might have more details on what this would look like, but it's not a priority for us as this stage.

@malekjaroslav
Copy link
Author

malekjaroslav commented Jun 14, 2019

@mathewbyrne Thank you for your reply! From what you say it might maybe make sense to report this to the Prisma team. Also, Prisma returns a struct pointer when returning one result so it would make sense for Prisma to also return pointers to structs when returning a slice. I will report this to Prisma.

@sagikazarmark
Copy link

I would like to second the request to revert/make it configurable. Unlike in case of protobuf, gqlgen lets us use domain models as return types. My domain services rarely return slices of pointers, so now using gqlgen is just about the same as using protobuf (which ironically sounds better, since it's more consistent, still feels worse from a developer experience point of view)

@stale
Copy link

stale bot commented Aug 28, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 28, 2019
@tv42
Copy link

tv42 commented Aug 28, 2019

Stalebots are a cancer of software engineering. Actual problems do not go away just because time has passed; either something is triaged as real, or it hasn't been triaged yet, and using a bot like that just hopes that things left untriaged fall into the "not a real problem" category.

@stale stale bot removed the stale label Aug 28, 2019
@vektah
Copy link
Collaborator

vektah commented Aug 28, 2019

Think of it as distributed triage. Triage takes time which I could better spend fixing issues and building features. It doesn't take much effort to leave a comment every 2 months if an issue is still relevant, does it?

@tv42
Copy link

tv42 commented Aug 29, 2019

Consider that the comment, with "commentary".

@sagikazarmark
Copy link

@vektah as an OSS maintainer I know the other side of the story (pressure from the community). Still, it would be nice to know if this is something you would consider (revert (I guess unlikely) or make it configurable) given enough feedback is provided.

If not, I propose to write the reasons down, close and lock this thread and refer to this issue if it comes up in the future.

@steebchen
Copy link
Contributor

My suggestion is to make it configurable in the long term. Personally I don't see any use for slices of pointers, but since it's already changed and it seems it mostly depends on what library you use for getting the data, I also suggest making it configurable, so I created #856.

@vektah
Copy link
Collaborator

vektah commented Sep 19, 2019

It can be hard to gauge the level of backpressure in some issues. Its a painful migration but most of our code ended up neater for it. I can understand that when working with prisma this is going the other way though.

So lets make it configurable (only for []*Thing vs []Thing, the other pointer changes would be really pervasive). Sorry its taken this long to come to a conclusion.

@steebchen
Copy link
Contributor

Thanks a lot @vektah!

@sagikazarmark
Copy link

Thanks! 🎉

@briskt
Copy link
Contributor

briskt commented Oct 25, 2019

Curious why this only affects field resolvers and not queryResolver methods. Seems inconsistent. I was happy to see this new config option, but I think I'll not use it for the sake of consistency.

By the way, the docs are incorrect at:

# Optional, set to true if you prefer []*Thing over []Thing
omit_slice_element_pointers: false

Should be

# Optional, set to true if you prefer []Thing over []*Thing
omit_slice_element_pointers: false

@vektah
Copy link
Collaborator

vektah commented Oct 26, 2019

Curious why this only affects field resolvers and not queryResolver methods. Seems inconsistent. I was happy to see this new config option, but I think I'll not use it for the sake of consistency.

That sounds like a bug, would you mind opening an issue so it doesn't get forgotten?

@briskt
Copy link
Contributor

briskt commented Oct 28, 2019

No problem.

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 a pull request may close this issue.

8 participants