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

page iterator: reflect: call of reflect.Value.IsNil on zero Value #433

Closed
stephenwan-opal opened this issue Mar 8, 2023 · 9 comments
Closed
Assignees
Labels
Needs Attention 👋 question Further information is requested

Comments

@stephenwan-opal
Copy link

Hello, I started seeing this error starting 2 days ago:

panic: goroutine 500907 [running]:
panic({0x2f8ccc0, 0xc01dd3a0d8})
	/usr/local/go/src/runtime/panic.go:884 +0x212
reflect.Value.IsNil(...)
	/usr/local/go/src/reflect/value.go:1554
github.com/microsoftgraph/msgraph-sdk-go-core.convertToPage({0x37b7280, 0xc00fd4ba80})
	/go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go-core@v0.31.0/page_iterator.go:205 +0x51b
github.com/microsoftgraph/msgraph-sdk-go-core.(*PageIterator).next(0xc00e8ee120?, {0x3ea9fd0?, 0xc00d369140?})
	/go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go-core@v0.31.0/page_iterator.go:126 +0x4e
github.com/microsoftgraph/msgraph-sdk-go-core.(*PageIterator).Iterate(0xc00e8ee120, {0x3ea9fd0, 0xc00d369140}, 0xc00d270500?)
	/go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go-core@v0.31.0/page_iterator.go:92 +0x5e
	<...snip...>
: reflect: call of reflect.Value.IsNil on zero Value

This looks similar to #249 and #224 but it is a different line of code panicking.

code looks like this:

iter, err := msgraphcore.NewPageIterator(
	users,
	s.client.GetAdapter(),
	models.CreateUserFromDiscriminatorValue,
)
if err != nil {
	return nil, azuread.WrapErr(err)
}
if err := iter.Iterate(ctx, func(item interface{}) bool {
	// ...
}

panicking code:

value := ref.FieldByName("value")
if value.IsNil() { // this line
	return page, errors.New("value property missing in response object")
}

which is different from the issue addressed by https://github.com/microsoftgraph/msgraph-sdk-go-core/pull/105/files.

@ghost ghost added the Needs Triage 🔍 label Mar 8, 2023
@baywet baywet added question Further information is requested Needs Attention 👋 and removed Needs Triage 🔍 labels Mar 8, 2023
@rkodev
Copy link
Contributor

rkodev commented Mar 9, 2023

@stephenwan-opal could you kindly confirm if you are using v0.34.1 for package github.com/microsoftgraph/msgraph-sdk-go-core`, if not , could you upgrade and confirm if the error is still present?

@stephenwan-opal
Copy link
Author

we are using 0.31.0. I will upgrade and take a look, thanks.

Did the API response change? Why would this issue start happening 3 days ago?

@rkodev
Copy link
Contributor

rkodev commented Mar 10, 2023

@stephenwan-opal There isn't a change in the API response, this was a required change in the supporting library and was implement in core

@rkodev
Copy link
Contributor

rkodev commented Mar 10, 2023

You can also upgrade to SDK v0.57.0

@stephenwan-opal
Copy link
Author

Hi @rkodev, thanks - I guess I am confused on: why did I start seeing this issue 4 days ago, the same say microsoftgraph/msgraph-sdk-go-core#167 was put out? (is that coincidence? i.e. latent bug happens to show up on the same day?)

Doing this upgrade is unfortunately not straightforward for me. I tried to upgrade just the msgraph-sdk-go-core package but there are breaking changes up and downstream that require a bunch of things to be upgraded together. Because #129 is still an issue for me, the base msgraph-beta-sdk-go package is unusable for me, so I am doing this nasty hack to keep build times reasonable.

@baywet
Copy link
Member

baywet commented Mar 10, 2023

so I am doing this nasty hack to keep build times reasonable.

@stephenwan-opal the only part "nasty" about that solution is the fact that you're generating off a fork of this repo. You could perfectly generate your own client with only the API surface you care about in your project. In fact now that the tech to do so is mostly ready, we're working on lighting up experiences to make this easier to anybody who has stronger size/performance requirements than what the full SDK offers can use that option instead.

@stephenwan-opal
Copy link
Author

sorry - I'd like to de-emphasize the specific change linked there. I'm excited to hear that the self-generated kiota build should help; at the time it was tricky to figure out and the kiota generation did not work out of the box (i.e. missing graph_service_client.go and some other files).

coming back to the issue at hand, do you have a sense when this issue gets triggered? is there a specific api response that doesn't return the value field?

@rkodev
Copy link
Contributor

rkodev commented Mar 13, 2023

@stephenwan-opal /Perhaps a little context to help. This issue was brought about as a result of implementing the backing store. Values were no longer stored on the structs properties but were read from the backing store. The fix was to invoke the GetValue method via reflection as opposed to reading the value property. If it helps this the specific change that was done microsoftgraph/msgraph-sdk-go-core#167

@rkodev
Copy link
Contributor

rkodev commented Apr 12, 2023

Hi @stephenwan-opal, I'll be closing this ticket since its been a while. If you still need some more assistance, you can comment and re-open it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Attention 👋 question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants