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

fix #1876: Optional Any type should allow nil values #2231

Merged
merged 3 commits into from
Jun 8, 2022

Conversation

vediatoni
Copy link
Contributor

@vediatoni vediatoni commented Jun 7, 2022

This PR addresses and fixes this open issue #1876

Short TLDR of the issue (quoted from the above link):

When trying to use Any as an optional input parameter, if you omit it or pass in null, it panics.

Error it produces:
interface conversion: interface is nil, not interface {}

I added a simple solution that checks whether the argument is of an interface type and if it is, then instead of appending the standard string code fc.Args[...].(...), it will append a stringified anonymous function that will either return an interface{} or nil. The value returned is decided based on the value of the argument. So if the argument value is nil, it won't try to do type assertion to interface{} (which makes Go panic), rather it will return nil.

This anonymous function can also be converted into a helper function.

I have:

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

@vediatoni vediatoni changed the title Anonymous func that checks value of arg type interface for nil fix: Optional Any type should allow nil values https://github.com/99designs/gqlgen/issues/1876 Jun 7, 2022
@vediatoni vediatoni changed the title fix: Optional Any type should allow nil values https://github.com/99designs/gqlgen/issues/1876 fix: Optional Any type should allow nil values #1876 Jun 7, 2022
@vediatoni vediatoni changed the title fix: Optional Any type should allow nil values #1876 fix #1876: Optional Any type should allow nil values Jun 7, 2022
@coveralls
Copy link

coveralls commented Jun 7, 2022

Coverage Status

Coverage increased (+0.1%) to 75.128% when pulling 4da7b31 on vediatoni:master into 65e6810 on 99designs:master.

@StevenACoffman
Copy link
Collaborator

This seems ok, but I would like to see a unit test for it, which might be easier to do if it was a helper function. What do you think about adding a test?

@vediatoni
Copy link
Contributor Author

This seems ok, but I would like to see a unit test for it, which might be easier to do if it was a helper function. What do you think about adding a test?

I added a little unit test that tests the Field.CallArgs() function as a whole. Please tell me if this is alright, if it's not I'd love some basic pointers you'd be able to provide me. As you can see this is my first time contributing to this project and I don't have too much time to spend to fully dig in into the code. But we do need this issue fixed since it does impact our project a bit.
Thanks!

@StevenACoffman StevenACoffman merged commit c355df9 into 99designs:master Jun 8, 2022
@StevenACoffman
Copy link
Collaborator

Thanks! This is great for now! If you get time to dive into the code for more improvements, we are always happy to get PRs!

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.

None yet

3 participants