8000 ParseQueryWithTokenLimit's error is not processed correctly by gqlgen · Issue #3693 · 99designs/gqlgen · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

ParseQueryWithTokenLimit's error is not processed correctly by gqlgen #3693

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
Closed
hansod1 opened this issue May 7, 2025 · 1 comment · Fixed by #3695
Closed

ParseQueryWithTokenLimit's error is not processed correctly by gqlgen #3693

hansod1 opened this issue May 7, 2025 · 1 comment · Fixed by #3695

Comments

@hansod1
Copy link
hansod1 commented May 7, 2025

This concerns the token limit config option added last year: #3136

I was testing it and noticed that it doesn't send the correct error to the client because this part of the gqlgen codebase does not correctly interpret the token limit error as a parsing error because it is not returned from the parser as a *gqlerror.Error, and rather is an errors.errorString created with fmt.Errorf:

	doc, err := parser.ParseQueryWithTokenLimit(&ast.Source{Input: query}, e.parserTokenLimit)
	if err != nil {
		gqlErr, ok := err.(*gqlerror.Error)
		if ok {
			errcode.Set(gqlErr, errcode.ParseFailed)
			return nil, gqlerror.List{gqlErr}
		}
	}

For reference, here is the change in gqlparser that added the new error.

Is there a deeper reason why this function isn't designed to return all errors out of ParseQueryWithTokenLimit and is instead only checking for *gqlerror.Error?

I am unsure why they didn't create a gqlerror over in the parser, but I suspect that's the right place to fix this. That being said, let me know if you'd accept a PR returning all errors, not just gqlerror, from ParseQueryWithTokenLimit and I'll submit it.
This was referenced May 7, 2025
Merged
Merged
@StevenACoffman
Copy link
Collaborator

I super appreciate your bringing it to my attention and your willingness to make a PR! Honestly, any PRs (here or in gqlparser) are always welcome as both are unpaid community led volunteer efforts.

Any error from gqlparser should always be a gqlerror, and we should really have added a linter to ban the fmt.Errorf and just sprinkle a few nolint comments for any of the parser's Panic locations where that might cause a cyclical dependency. If you are willing to add a PR to gqlparser for that, I would really appreciate it!

The gqlerror.List takes a set of *gqlerror.Error because so it can output them more nicely if it has the extra information that those can contain (like Location).

We are trying to frontload the coercion to *gqlerror.Error at the point of error Origination because at the point of error Reporting we will no longer have important contextual information (like Location) and such.

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.

Update gqlparser to 2.5.27 to fix #3693 99designs/gqlgen
2F22
2 participants
@StevenACoffman @hansod1
0