Skip to content

[BUG] fix disable suggestion bug#3456

Merged
StevenACoffman merged 2 commits into99designs:masterfrom
tomoikey:fix/disable-suggestion-bug
Dec 25, 2024
Merged

[BUG] fix disable suggestion bug#3456
StevenACoffman merged 2 commits into99designs:masterfrom
tomoikey:fix/disable-suggestion-bug

Conversation

@tomoikey
Copy link
Copy Markdown
Collaborator

@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
Copy Markdown

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
Copy Markdown
Collaborator 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
Copy Markdown
Collaborator

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

@StevenACoffman
Copy link
Copy Markdown
Collaborator

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

@StevenACoffman StevenACoffman merged commit dc565aa into 99designs:master Dec 25, 2024
@tomoikey
Copy link
Copy Markdown
Collaborator Author

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

@StevenACoffman
Copy link
Copy Markdown
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

@tomoikey tomoikey self-assigned this Feb 11, 2026
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