-
Notifications
You must be signed in to change notification settings - Fork 194
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
chore: bump go to 1.23.4 and gqlgen to v0.17.51 #607
Conversation
3af58ab
to
ea64e7e
Compare
gqlgen v0.17.52 breaks field collection see 99designs/gqlgen@f81239e
ea64e7e
to
159f393
Compare
Hey @michaelcaulley are you able to get this merged, please? I have another dep on 1.23.4 and code-gen is breaking for me. |
@a8m , is the failing lint rule unrelated? It looks like everything passed except two lint issues. |
cf60dc8
to
44c4de5
Compare
44c4de5
to
769c44e
Compare
@a8m I bumped the golangci-lint version, and the checks are passing. However, I did have two errors related to a potential int overflow during a cast to int32. I added a check and returned an error if an overflow occurred. This change is in the second commit. Can you please check it before merging? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution, @michaelcaulley.
Thank you @michaelcaulley for working on this and thanks @a8m for reviewing and approving it! |
@a8m @michaelcaulley @vitorfalcaor Thanks for your work! I see that this PR has a comment that says that gqlgen version v0.17.52 has an issue with ent field collection. It blames commit: 99designs/gqlgen@f81239e The blamed gqlgen commit was merged 99designs/gqlgen#3287 Currently gqlgen v0.17.61 is the latest. It is not clear to me if there remains any problem with gqlgen and ent field collection. |
I opened #609 to see if there's anything amiss in the current v0.17.61 version. |
I was just able to test this and I am actually still running into |
@vitorfalcaor If you can provide something I can reproduce locally, that would be very helpful. The two biggest changes that have happened in gqlgen and gqlparser recently are that we no longer allow the invalid empty GraphQL arguments ( (The GraphQL specification does not allow empty arguments, but the specification's wording of that restriction was originally confusing, and the reference test suite did not require it. After they clarified the wording and tightened the specification tests, gqlparser was no longer compliant.) The error message you are reporting is not consistent with either one of these big changes, so something I can reproduce would be needed for me to help more. |
Hi, @StevenACoffman. I'll create an example in the next week or two. I initially tried the latest version of gqlgen but saw n+1 queries instead of ent properly doing field collection to use a SQL join. I worked backward from the latest version to find one that worked ok, which led me to use version 0.17.51. Edit: I just tried 0.17.61, and I no longer see any issues with it. I can try to find the problematic version sometime later; perhaps I had a mismatch in my go.mod, and what contrib included? For now, I see no reason not to bump the version all the way to the latest, 0.17.61. |
Ok, that's great news. Thanks for checking. So hopefuly #609 is approved and can get merged then. |
@michaelcaulley Can you look to see what is wrong with my build of #609 ? |
Hey @StevenACoffman, Happy New Year! I was able to repro the issue of
Expected output:
After diving deeper into this issue I found these two insights:
Please let me know if you prefer that I open an issue, but it seems that this is an issue with |
I made a8m/ent-graphql-example#12 to avoid any miscommunication.
That is the state of where that PR is. However, in this state if I run:
I get these errors:
|
Also commenting here to keep everyone in the loop. Thanks for creating this @StevenACoffman! I got the same result as you, but if I run go run -mod=mod github.com/99designs/gqlgen@latest --config gqlgen.yml (note the @latest) gives me:
Can you try repro'ing this, please? |
Yeah, I can reproduce this, but I'm not sure what has changed in x/tools that causes this. Do we need to add there in gqlgen https://github.com/99designs/gqlgen/blob/master/internal/code/packages.go#L111 a new config option to the packages Load? |
Hey @StevenACoffman I don't think so. This is the first time I am dealing with this code, so I am learning as we go. I tested adding other options to the Config, but I wasn't successful. I did run one interesting test. I added a
This code prints:
I then tested this code with an actual name for a local package and it did work just fine. This indicates that what's happening in the gqlgen instance is that |
Hmm, I tried setting the toolchain directive in the go.mod, as I wondered if it was something to do with golang/go#62114 Can you try doing:
and try it again to see if it is different? I can look again on Monday, but if you figure it out before hand, let me know. |
@StevenACoffman AMAZING! This fixed it! Please note that it didn't work when setting Thank you so much for the help! |
Glad you have a workaround, but Ugh, this is going to cause other people problems. @vitorfalcaor, can you dump out your I think if you used:
it would work, but only if you are using that specific version of Go for that line. Go 1.23.3 will have loaded the packages and then it will download and use a local Go 1.23.4 toolchain that doesn't have it. You got:
So I think when you are using Forcing both the runtime and toolchain to match for every place fixes it, but figuring out how to detect what that is and set it is tricky. |
Hey @StevenACoffman just to complement what I shared earlier, I also had to clone Here's the output of what you requested. Command:
Output:
|
Thanks! And that output was with or without the local environment variable I cannot force gqlgen to use a later x/tools as I can't move gqlgen to require Go 1.23 until Go 1.24 is released in February (or really until Go AppEngine supports Go 1.23 so my work can use it in their prod) |
That output was with Got it, thank you for providing this context. In this case, what would you recommend I do? Keep a local copy of gqlgen and bump |
Can you run
If you If that doesn't work, updating your local Go to 1.23.4 and then repeating the above (compiling gqlgen, then executing the binary gqlgen) should work since then there's only one Go toolchain involved. If that doesn't work, then locally updating your copy of gqlgen to the latest |
The first options worked well: compile gqlgen into a local binary (go build -o Here's the test: Setup:
Output:
Thanks for all the help! |
Summary