Avoid problems with val being undefined in the federation template.
#1760
Conversation
When running gqlgen over our schema, we were seeing errors like:
```
assignments/generated/graphql/service.go:300:4: val declared but not used
```
The generated code looks like this:
```
func entityResolverNameForMobileNavigation(ctx context.Context, rep map[string]interface{}) (string, error) {
for {
var (
m map[string]interface{}
val interface{}
ok bool
)
m = rep
if _, ok = m["kaid"]; !ok {
break
}
m = rep
if _, ok = m["language"]; !ok {
break
}
return "findMobileNavigationByKaidAndLanguage", nil
}
return "", fmt.Errorf("%w for MobileNavigation", ErrTypeNotFound)
}
```
Looking at the code, it's pretty clear that this happens when there
are multiple key-fields, but each of them has only one keyField.Field
entry. This is because the old code looked at `len(keyFields)` to
decide whether to declare the `val` variable, but looks at
`len(keyField.Field)` for each keyField to decide whether to use the
`val` variable.
The easiest solution, and the one I do in this PR, is to just declare
`val` all the time, and use a null-assignment to quiet the compiler
when it's not used.
|
I don't seem to able to suggest reviewers, but @carldunham you're probably the best person to review this! Also cc-ing @StevenACoffman and @MiguelCastillo for landing this when the time comes. |
|
Good catch @csilvers! Could you add some unit tests that would fail without this fix? |
It shouldn't be too bad. We can create a unit test based on how @csilvers found it in the first place. Let me see if I can add one. But happy to do after in a separate PR if it turns out to be too painful. |
From `plugins/federation`, run the following command and verify that no errors are produced ``` go run github.com/99designs/gqlgen --config testdata/entityresolver/gqlgen.yml ```
|
Ok I added a test. I verified the issue happens without these changes and that the issue is fixed with these changes. Also - this is a very clever solution, Craig! I would have definitely tried adjusting the logic. This seems good enough but may circle back if a more clear fix comes to mind. |
| val interface{} | ||
| ok bool | ||
| ) | ||
| _ = val |
There was a problem hiding this comment.
This is where the fix does its thing.
|
Thanks for the quick review everyone! And extra thank you for adding the test @MiguelCastillo ! |
When running gqlgen over our schema, we were seeing errors like:
The generated code looks like this:
Looking at the code, it's pretty clear that this happens when there
are multiple key-fields, but each of them has only one keyField.Field
entry. This is because the old code looked at
len(keyFields)todecide whether to declare the
valvariable, but looks atlen(keyField.Field)for each keyField to decide whether to use thevalvariable.The easiest solution, and the one I do in this PR, is to just declare
valall the time, and use a null-assignment to quiet the compilerwhen it's not used.
We have added tests to ensure that this fixes the issue, which you can run:
plugins/federationgo run github.com/99designs/gqlgen --config testdata/entityresolver/gqlgen.ymlDescribe your PR and link to any relevant issues.
I have: