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

Running genqlient on large schemas fails #340

Closed
benmai opened this issue Jun 12, 2024 · 4 comments
Closed

Running genqlient on large schemas fails #340

benmai opened this issue Jun 12, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@benmai
Copy link

benmai commented Jun 12, 2024

Describe the bug
In the last day running go run github.com/Khan/genqlient started failing with:

$ go run github.com/Khan/genqlient
invalid schema: exceeded token limit of 15000
exit status 1

To Reproduce
Run genqlient with a large schema.

Expected behavior
I would expect generation to succeed.

genqlient version
v0.7.0, with github.com/vektah/gqlparser/v2 v2.5.13 or v2.5.14

Additional context
I am fairly certain this is related to vektah/gqlparser#291, which I see @benjaminjkraft has already taken a look at and commented on the schema size limit. My workaround for now is to use a replace directive (since we're using Dependabot) to pin it to the old version:

replace github.com/vektah/gqlparser/v2 => github.com/vektah/gqlparser/v2 v2.5.12

If gqlparser is going to have this limitation I would like to also have that available for configuration via genqlient as well, although TBH I don't think schema size is a valid threat vector -- we always load it from disk.

@benmai benmai added the bug Something isn't working label Jun 12, 2024
@benjaminjkraft
Copy link
Collaborator

benjaminjkraft commented Jun 12, 2024

In theory you shouldn't need a replace? (Go picks the lowest version possible, so unless some other dep of yours has updated, and you've pulled that update, you should be fine.) But yeah we should just turn this off for genqlient (or better set gqlparser to actually default to disabling it, which I think was the intention but not actually what happened (per my comment on the PR). I see no need for genqlient to even expose this, unless someone can explain a plausible threat model where you'd want it. PRs welcome in either case!

@benjaminjkraft
Copy link
Collaborator

Fixed by #341

@benmai
Copy link
Author

benmai commented Jun 13, 2024

FWIW the latest gqlparser v2.5.16 works just fine for my case with genqlient! Thanks for the response and for being on top of genqlient's dependencies.

About not needing a replace: I tried removing the replace, manually setting gqlparser to an earlier version, and then updating genqlient with go get -u, and it does update the transitive dependency:

$ go get github.com/vektah/gqlparser/v2@v2.5.12
go: downgraded github.com/vektah/gqlparser/v2 v2.5.13 => v2.5.12
$ go get -u github.com/Khan/genqlient
go: upgraded github.com/vektah/gqlparser/v2 v2.5.12 => v2.5.16

I'm probably missing something but without an explicit replace directive I've not found a way to keep dependencies at a version other than the latest minor. Either way, in this case I'm grateful for the quick responses all around.

@benjaminjkraft
Copy link
Collaborator

Yeah, go get -u tends to update everything. I think it's intended behavior, although I'm too lazy to dig up the issue thread. If I want to update a specific package I just do it manually by editing go.mod. (rsc's post on the subject explains how and why the version-selection works.)

adrielp added a commit to adrielp/opentelemetry-collector-contrib that referenced this issue Jun 13, 2024
This is a byproduct of [this issue](Khan/genqlient#340)
posted in the Genqlient repository of the gqlparser dependency being
updated. This was patched in an updated version v2.5.16
evan-bradley pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Jun 13, 2024
…l generation (#33552)

This is a byproduct of [this
issue](Khan/genqlient#340) posted in the
Genqlient repository of the gqlparser dependency being updated. This was
patched in an updated version v2.5.16

**Description:** <Describe what has changed.>
Upgraded indirect dependency to take in patch fixing workflow errors. 

**Link to tracking Issue:**
#33551
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants