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

EntityResolver input type fix #2594

Merged

Conversation

erankor
Copy link
Contributor

@erankor erankor commented Mar 27, 2023

fixes #2326 - when using @entityResolver(multi: true) the InputType of the entity resolver may be in a different package (usually model). The fix is to pull the types.Type of the resolver input, and use templates.CurrentImports.LookupType in order to render it correctly (possibly adding another import)

Describe your PR and link to any relevant issues.

I have:

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

fixes 99designs#2326 - when using `@entityResolver(multi: true)` the InputType of
the entity resolver may be in a different package (usually `model`).
The fix is to pull the types.Type of the resolver input, and use
templates.CurrentImports.LookupType in order to render it correctly
(possibly adding another import)
@coveralls
Copy link

coveralls commented Mar 27, 2023

Coverage Status

Coverage: 78.611% (+3.2%) from 75.408% when pulling 89d6996 on erankor:entity-resolver-input-type-fix into 677d854 on 99designs:master.

change testdata/entityresolver/gqlgen.yml to use a dedicated package for
the model (as in the default sample yml), and run go generate.

before the input type fix, generation fails with errors like -
plugin/federation/testdata/entityresolver/generated/federation.go:338:17:
  undeclared name: MultiHelloByNamesInput
plugin/federation/testdata/entityresolver/generated/federation.go:354:21:
  undeclared name: MultiHelloMultipleRequiresByNamesInput
plugin/federation/testdata/entityresolver/generated/federation.go:362:17:
  undeclared name: MultiHelloMultipleRequiresByNamesInput
@StevenACoffman
Copy link
Collaborator

Thanks very much! This a nice improvement!

@StevenACoffman StevenACoffman merged commit db53479 into 99designs:master Mar 28, 2023
@erankor erankor deleted the entity-resolver-input-type-fix branch March 28, 2023 20:10
@erankor
Copy link
Contributor Author

erankor commented Mar 28, 2023

@StevenACoffman, thank you very much for the super-fast merge!
Any chance you can create a new tag/release? we want to start using this feature :)

@StevenACoffman
Copy link
Collaborator

I really wanted to look at the differences here and with the in progress #2593 and I was hoping that one of you would provide some test coverage before a new release.

@erankor
Copy link
Contributor Author

erankor commented Mar 29, 2023

@StevenACoffman, I don't understand... from what I see, #2593 aims to resolve the same bug as this one, so I guess it's no longer relevant, right?
About test coverage, in this PR, I updated the gqlgen.yml of the existing test for entity resolver to use a dedicated package for the model. So, the bug solved by this PR is already covered - running go generate on 'entityresolver' fails without the fix (and since this PR uses the existing framework for type lookup, I don't think we need to test also the case of model sharing package with federation.go).
What do you think is missing?

@StevenACoffman
Copy link
Collaborator

First, I try not to release a new version more than once a week unless it's entirely critical. Anyone who wants to can pin their gqlgen to a specific commit prior to that. go get -d github.com/99designs/gqlgen@VERSION where version is the commit sha1.

Second, In my volunteer OSS time in the mornings, I tend to only have enough time to see if new PRs fix legit bugs and pass the existing tests. Comparing competing solutions requires a deeper level of engagement. Drafting a new release also generally waits until I have more than the time to sip a single coffee.

I'd like to see if one of the solutions better sets us up to fix some of the other federation issues people have reported such as #2559

@erankor
Copy link
Contributor Author

erankor commented Mar 29, 2023

@StevenACoffman, no problem, we'll wait for the release, sorry if I was being pushy. I'm maintaining a couple of OSS repos myself, so I know how it's like... :)

RE #2593, as I wrote, I'm quite sure it's no longer relevant - it solves the exact same issue as this one and can be closed. @jclyons52, can you confirm?

RE #2559, it seems to be unrelated to either PR. If I'll have some free time, I'll try to give it a look.
Btw, on a similar topic to this issue, I also bumped into a problem with @requires - the entity resolver function (FindMany...ByIDs) doesn't get the 'required' fields, they are copied after the function returns. So, the function can't use these fields, which is, in my understanding, the main purpose of using @requires.
The workaround I found for this is to pull required fields from the context -

	fc := graphql.GetFieldContext(ctx)
	_reps := fc.Args["representations"].([]map[string]interface{})

It's not the most elegant solution, but it works... :)

@jclyons52
Copy link
Contributor

@erankor Yeah looks like we're solving the same issue! I'll close my PR

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.

Federation plugin does not import autobind and generated models
4 participants