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

[BUG] fix disable suggestion bug #3456

Merged
merged 2 commits into from
Dec 25, 2024

Conversation

tomoikey
Copy link
Contributor

@tomoikey tomoikey commented Dec 25, 2024

@StevenACoffman
First, please allow me to offer my apologies. I have realized that a minor bug exists in the Pull Request I previously submitted.

The issue is that when disableSuggestion is set to true, and consecutive queries are made, the resulting error messages are duplicated.

Below, I’ve included a screenshot that demonstrates the bug, along with steps to reproduce it. Please feel free to try it out in your local playground environment:

  1. Set disableSuggestion to true.
  2. Execute a query that would trigger suggestions.
  3. Observe that the errors are outputted multiple times, creating duplicates.

Previous Pull Request

#3411

Screenshot

image

I have:

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

@coveralls
Copy link

coveralls commented Dec 25, 2024

Coverage Status

coverage: 73.891% (+0.03%) from 73.861%
when pulling 4366b32 on tomoikey:fix/disable-suggestion-bug
into 663d013 on 99designs:master.

Comment on lines +230 to +231
// remove the rule added when it was last executed
validator.RemoveRule(rule.Name)
Copy link
Contributor Author

@tomoikey tomoikey Dec 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If gqlparser has a function like Contains, it might be better to use it and add a conditional if check to determine whether the rule is already duplicated.

However, in terms of computational cost, this implementation should not cause any issues. Therefore, I think it would be sufficient to leave this as a future TODO for now.

@StevenACoffman
Copy link
Collaborator

I added ReplaceRule as an alternative to AddRule since you are not the first to request it.
vektah/gqlparser#338

@StevenACoffman
Copy link
Collaborator

Ok, I cut a new release for gqlparser v2.5.21

@StevenACoffman StevenACoffman merged commit dc565aa into 99designs:master Dec 25, 2024
17 checks passed
@tomoikey
Copy link
Contributor Author

@StevenACoffman
Thank you for your prompt response! I apologize for the inconvenience I caused you...

@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Dec 25, 2024

No worries.
Honestly, the ruleset being in a global variable is poor implementation for gqlparser that leads to problems like we discovered here.
Having the Init() setting the RuleSet in gqlgen makes the situation worse, because it is hard to override later.

Changing the rule set would be best to alter once when the config option is first read, rather than doing it repeatedly in the executor, but there would need to be a lot of changes to make that possible. So you were sort of set up to have a hard time.

I updated gqlparser and made the minor switch to use ReplaceRule
#3458

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.

3 participants