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

NGP-916 #48

Merged
merged 2 commits into from
Jun 4, 2018
Merged

NGP-916 #48

merged 2 commits into from
Jun 4, 2018

Conversation

askurydzin
Copy link
Contributor

The root cause of the bug was runtime.Marshaller limited capabilities and it's inability to marshal nil values inside map, resulting in following error:

panic: reflect: call of reflect.Value.Interface on zero Value [recovered]
	panic: reflect: call of reflect.Value.Interface on zero Value

This fix implements logic for response map traversal and nil value removal.

$ make test
test -z `docker run --privileged --rm -v /home/askurydzin/go/src/github.com/infobloxopen/atlas-app-toolkit:/go/src/github.com/infobloxopen/atlas-app-toolkit -w /go/src/github.com/infobloxopen/atlas-app-toolkit infoblox/buildtool:v8 go fmt ./...`
docker run --privileged --rm -v /home/askurydzin/go/src/github.com/infobloxopen/atlas-app-toolkit:/go/src/github.com/infobloxopen/atlas-app-toolkit -w /go/src/github.com/infobloxopen/atlas-app-toolkit infoblox/buildtool:v8 go test ./...
ok  	github.com/infobloxopen/atlas-app-toolkit/auth	0.020s
ok  	github.com/infobloxopen/atlas-app-toolkit/gateway	0.020s
ok  	github.com/infobloxopen/atlas-app-toolkit/gorm	0.005s
ok  	github.com/infobloxopen/atlas-app-toolkit/health	1.049s
ok  	github.com/infobloxopen/atlas-app-toolkit/query	0.009s
ok  	github.com/infobloxopen/atlas-app-toolkit/requestid	0.005s
ok  	github.com/infobloxopen/atlas-app-toolkit/rpc/errdetails	0.001s
ok  	github.com/infobloxopen/atlas-app-toolkit/server	0.119s
?   	github.com/infobloxopen/atlas-app-toolkit/servertest	[no test files]

Copy link
Contributor

@amaskalenka amaskalenka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks as a temporary workaround.

@dmacthedestroyer
Copy link
Contributor

is there a formal fix pending in the grpc-gateway repo? I'd recommend at the very least opening an issue for this if the problem is with their marshaller, and possibly making a PR for the fix as well.

@askurydzin
Copy link
Contributor Author

i guess it will be hard to propagate the fix to grpc-gateway because it would inflame a holy war regarding returning nillable values as null or not return them at all. In our case, northstar would like to use wkt to distinguish between 0 and empty value, however their main purpose is nillable option. marshaller in grpc-gateway was not intended to do something more complicated than map of ints, strings and enums, and the main proposal would be to write our own marshaller.

@Evgeniy-L Evgeniy-L merged commit cde266f into infobloxopen:master Jun 4, 2018
@dmacthedestroyer
Copy link
Contributor

This PR seems to address this same issue, if I'm not mistaken. Looks like there's support for it but no one is willing to fix tests :)

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.

4 participants