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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix not to panic when struct contains an unexported field #35

Merged
merged 2 commits into from Dec 31, 2018

Conversation

2 participants
@shuheiktgw
Copy link
Contributor

shuheiktgw commented Dec 27, 2018

Thank you so much for a wonderful library!

I encountered the same problem with https://github.com/shurcooL/githubv4/issues/37, so I fixed it not to panic and return an error instead. As you mentioned in the issue, I think it would be better for the library to return error rather ignoring the panic, since that makes it easier for users to debug!

Thanks 馃憤

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Dec 29, 2018

Hi @shuheiktgw,

Thanks for the PR. In order to proceed here, we need a decision on #36. I've made one in #36 (comment) and it's compatible with the direction you've taken here.

I have a suggestion for how we can improve the implementation. Instead of modifying unmarshalValue to return an error if v isn't settable, we can detect the problem sooner. We should not add unexported fields to the d.vs stacks in the first place. That can be done by modifying fieldByGraphQLName to not consider unexported fields.

I'll push a modified version to this PR, please take a look and let me know what you think.

internal/jsonutil: Skip unexported fields.
This change makes jsonutil.UnmarshalGraphQL not consider unexported fields when looking for a matching field.

Document unmarshalValue precondition that v must be addressing and not obtained by the use of unexported fields. That makes it settable, which is a requirement. We want to arrange the internal jsonutil code so that unmarshalValue is never called on an unsettable reflect.Value.

Fixes #36.
@shuheiktgw

This comment has been minimized.

Copy link
Contributor

shuheiktgw commented Dec 30, 2018

@dmitshur That looks fantastic to me! 馃憤
Thanks for taking your time to fix it!

@dmitshur dmitshur merged commit d48a9a7 into shurcooL:master Dec 31, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment