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

Using unexported field in query results in panic. #36

Closed
nodefortytwo opened this Issue Aug 7, 2018 · 5 comments

Comments

2 participants
@nodefortytwo
Copy link

nodefortytwo commented Aug 7, 2018

Whilst trying to use updatePullRequestReview mutation I get a panic when calling client.mutate

I suspect that I have made a mistake with my mutation structs but the library really shouldn't panic and should instead provide information on the error

NOTE: the mutation is successfully applied

Sample code below to aid in replicating.

func updateReview(ctx context.Context, client *githubv4.Client, reviewID, Body string) error {
	uuid, err := uuid.NewRandom()
	if err != nil {
		return err
	}
	var mut = githubv4.String(uuid.String())

	input := githubv4.UpdatePullRequestReviewInput{
		Body:                githubv4.String(Body),
		ClientMutationID:    &mut,
		PullRequestReviewID: reviewID,
	}

	var m struct {
		updatePullRequestReview struct {
			ClientMutationID string
		} `graphql:"updatePullRequestReview(input: $input)"`
	}

	return client.Mutate(context.Background(), &m, input, nil)
}

Full Panic

panic: reflect.Value.Interface: cannot return value obtained from unexported field or method

goroutine 1 [running]:
reflect.valueInterface(0x12374c0, 0xc4201d4370, 0x36, 0x1098601, 0x123ed20, 0x12374c0)
        /usr/local/Cellar/go/1.10.3/libexec/src/reflect/value.go:959 +0x1c1
reflect.Value.Interface(0x12374c0, 0xc4201d4370, 0x36, 0x12374c0, 0xc4201d4370)
        /usr/local/Cellar/go/1.10.3/libexec/src/reflect/value.go:948 +0x44
.../vendor/github.com/shurcooL/graphql/internal/jsonutil.unmarshalValue(0x123ed20, 0xc420010180, 0x123ed20, 0xc4201d4370, 0x1b8, 0xc420084480, 0x2)
       .../vendor/github.com/shurcooL/graphql/internal/jsonutil/graphql.go:307 +0xca
.../vendor/github.com/shurcooL/graphql/internal/jsonutil.(*decoder).decode(0xc420069bd0, 0xc42024a3e0, 0x16)
       .../vendor/github.com/shurcooL/graphql/internal/jsonutil/graphql.go:147 +0x45b
.../vendor/github.com/shurcooL/graphql/internal/jsonutil.(*decoder).Decode(0xc420069bd0, 0x1237ac0, 0xc4201d4370, 0xc420069bf8, 0x12375c0)
       .../vendor/github.com/shurcooL/graphql/internal/jsonutil/graphql.go:66 +0x13d
.../vendor/github.com/shurcooL/graphql/internal/jsonutil.UnmarshalGraphQL(0xc4204e2300, 0x57, 0x60, 0x1237ac0, 0xc4201d4370, 0x12a3fed, 0x10)
       .../vendor/github.com/shurcooL/graphql/internal/jsonutil/graphql.go:23 +0x12c
.../vendor/github.com/shurcooL/graphql.(*Client).do(0xc420114340, 0x12d5a80, 0xc4200a2008, 0x4e4e312d1f4d2b01, 0x1237ac0, 0xc4201d4370, 0xc42013c3c0, 0x0, 0x0)
       .../vendor/github.com/shurcooL/graphql/graphql.go:85 +0x393
.../vendor/github.com/shurcooL/graphql.(*Client).Mutate(0xc420114340, 0x12d5a80, 0xc4200a2008, 0x1237ac0, 0xc4201d4370, 0xc42013c3c0, 0xc42013c390, 0xc42013c390)
       .../vendor/github.com/shurcooL/graphql/graphql.go:43 +0x65
.../vendor/github.com/shurcooL/githubv4.(*Client).Mutate(0xc4200b0020, 0x12d5a80, 0xc4200a2008, 0x1237ac0, 0xc4201d4370, 0x1270140, 0xc42013c390, 0xc42013c3c0, 0x0, 0x0)
       .../vendor/github.com/shurcooL/githubv4/githubv4.go:55 +0xc9
main.updateReview(0x12d5a80, 0xc4200a2008, 0xc4200b0020, 0xc42001ed20, 0x28, 0x12a2613, 0x9, 0xc420014880, 0x11)
       .../github-commenter.go:101 +0x21a
main.main()
       .../github-commenter.go:39 +0x167
exit status 2
@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Aug 8, 2018

Thanks a lot for reporting. I suspect this issue is #11. I've already started a fix for it locally, I just need to finish it and push it. I'll confirm whether it fixes this issue too.

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Aug 8, 2018

I've investigated this, and it's not the same issue as what I thought it might be.

Your struct m where the results of the mutation get populated contains an unexported field:

var m struct {
	updatePullRequestReview struct {
		ClientMutationID string
	} `graphql:"updatePullRequestReview(input: $input)"`
}

As a result, it's not possible for another package (github.com/shurcooL/graphql/internal/jsonutil, in this case) to use reflect to access m.updatePullRequestReview.ClientMutationID and modify its contents. I haven't handled that edge case properly, hence the panic.

You should change your code to make updatePullRequestReview exported, then it'll work:

var m struct {
	UpdatePullRequestReview struct {
		ClientMutationID string
	} `graphql:"updatePullRequestReview(input: $input)"`
}

This is a valid bug. I'll need to think how to best handle such a situation:

  1. encoding/json package simply skips unexported fields, and I could do that here too.
  2. An alternative is to immediately return an error, which might make debugging easier (otherwise, it might be hard to understand why the value isn't being populated).

@dmitshur dmitshur changed the title Panic: reflect.Value.Interface: cannot return value obtained from unexported field or method Using unexported field in query results in panic. Aug 8, 2018

@nodefortytwo

This comment has been minimized.

Copy link

nodefortytwo commented Aug 8, 2018

Hi dmitshur,

My error is so obvious in hind sight. I am happy to close this given its purely user error but if there is anything that could be done to return an error gracefully without a panic that would be amazing.

thanks so much for your quick response

Rick

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Sep 10, 2018

If you don’t mind, I’ll reopen this issue and use it to track the fix to the panic when unexported fields are involved. Once resolved, future users won’t run into this panic either.

@dmitshur dmitshur reopened this Sep 10, 2018

@dmitshur dmitshur transferred this issue from shurcooL/githubv4 Dec 29, 2018

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Dec 29, 2018

This is a valid bug. I'll need to think how to best handle such a situation:

  1. encoding/json package simply skips unexported fields, and I could do that here too.
  2. An alternative is to immediately return an error, which might make debugging easier (otherwise, it might be hard to understand why the value isn't being populated).

Decision: We will skip unexported fields for now.

Rationale: It's a simple and risk-free change, since we'll essentially trade run-time panics for more descriptive errors. It mirrors encoding/json semantics. On the other hand, the choice of immediately returning errors is more risky and may interfere with unrelated fields. In any case, it should probably be done within the graphql layer and not in the internal/jsonutil package. We can consider that later, separately.

@dmitshur dmitshur added NeedsFix and removed NeedsDecision labels Dec 29, 2018

dmitshur added a commit to shuheiktgw/graphql that referenced this issue Dec 29, 2018

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 shurcooL#36.

@dmitshur dmitshur closed this in d48a9a7 Dec 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment