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.

Sign up for GitHub

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

Jump to bottom

ResolverImplementer has a break change that the implementor can't know a field status. #2856

Open
tsingsun opened this issue Dec 8, 2023 · 6 comments · May be fixed by #2886
Open

ResolverImplementer has a break change that the implementor can't know a field status. #2856

tsingsun opened this issue Dec 8, 2023 · 6 comments · May be fixed by #2886

Comments

@tsingsun
Copy link

tsingsun commented Dec 8, 2023 •
edited by StevenACoffman

What happened?

I implemented the plugin.ResolverImplementer to generate some CRUD method, but that breaks when bumping from v0.17.38 to v.0.17.41.
Those implemented and edited code will be overridden by the original template initialization code.

I found #2850:

in v0.17.38 /plugin/resolvergen/resolver.go

                          if implementation == "" {
				// Check for Implementer Plugin
				var resolver_implementer plugin.ResolverImplementer
				var exists bool
				for _, p := range data.Plugins {
					if p_cast, ok := p.(plugin.ResolverImplementer); ok {
						resolver_implementer = p_cast
						exists = true
						break
					}
				}

				if exists {
					implementation = resolver_implementer.Implement(f)
				} else {
					implementation = fmt.Sprintf("panic(fmt.Errorf(\"not implemented: %v - %v\"))", f.GoFieldName, f.Name)
				}

			}

in v0.17.41 /plugin/resolvergen/resolver.go

diff:

  • 0.17.38 if the implementation is empty, it is passed to the implementer.
  • 0.17.41 the Field is passed to the ResolverImplementer regardless of whether it is already implemented

What did you expect?

Let the ResolverImplementer can know the field status, whether the field needs to be implemented, field is new or has been resolved, if not , can you keep the v0.17.38 behavior the way it was

Minimal graphql.schema and models to reproduce

versions

  • go run github.com/99designs/gqlgen version 0.17.41
  • go version 1.21
@StevenACoffman
Copy link
Collaborator

@roneli I'm not quite clear on this problem. Can you please take a look?

@roneli
Copy link
Contributor

roneli commented Jan 19, 2024

Hi,

I am implemented #2850, I didn't break the original interface as I didn't want it to cause issues with backwards compatability. Basically, I concluded that not passing if it exists doesn't allow the implementer to decide / modify in future runs.

@StevenACoffman I can modify the interface to also include the prevDecl (will be empty if it was never declared etc') so the resolver implementor can decide if to change it or not. Question is are we okay with breaking the interface? I can make a new one that also accepts previous and switch case based on the provided implementation to keep backwards compatibility.

I'll dive deeper into the code and upload a proposed interface tomorrow.

@roneli roneli linked a pull request Jan 20, 2024 that will close this issue
Open
2 tasks
@roneli
Copy link
Contributor

roneli commented Jan 20, 2024

Hi, @tsingsun

I created a PR that gives the resolver implementor the previous method body, it will be empty if no previous body was created before.

It does break the interface, but I would assume its a minor inconvenience for the anyone who uses this interface to update.

@StevenACoffman would love for any comments on the interface change

@tsingsun
Copy link
Author

@roneli @StevenACoffman Sorry for the late reply. I added a rewriter to solve the interface changed in 17.41. Now I have updated to 17.45, the code generation is the same as before, thank you for your work.

@StevenACoffman
Copy link
Collaborator

@tsingsun Can you comment on #2886 ?

@tsingsun
Copy link
Author

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix #2856: resolver receive previous implementation on render roneli/gqlgen
3 participants
@StevenACoffman @tsingsun @roneli