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.

Sign up for GitHub

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

Jump to bottom

spanner: third party dependency exception for github.com/json-iterator/go #9380

Closed
noahdietz opened this issue Feb 6, 2024 · 2 comments · Fixed by #10113
Closed

spanner: third party dependency exception for github.com/json-iterator/go #9380

noahdietz opened this issue Feb 6, 2024 · 2 comments · Fixed by #10113
Assignees
@rahul2393
Labels
api: spanner Issues related to the Spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: process A process-related concern. May include testing, release, or the like.

Comments

@noahdietz
Copy link
Contributor

Module: spanner
Usage(s): spanner/value.go

For the time being, it will be exempt from the third party dep check, but please do one of the following:

A. Remove the dependency by switching to a package own directly by Google, handwriting necessary functionality, or using stdlib
B. Justify the exception and ack the risks, maintaining the exception indefinitely
@noahdietz noahdietz added the type: process A process-related concern. May include testing, release, or the like. label Feb 6, 2024
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Feb 6, 2024
@noahdietz noahdietz mentioned this issue Feb 6, 2024
Merged
@rahul2393 rahul2393 added the priority: p3 Desirable enhancement or fix. May not be included in next release. label Feb 12, 2024
@egonelbre
Copy link
Contributor

There is a significant concern from my point of view with json-iterator dependency due to it's heavy reliance on unsafe and modern-go/reflect2. See golang/go#54766 (comment) for issues that happen from it -- the lucky case is when your compilation or tests fail.

@rahul2393
Copy link
Contributor

Thanks @egonelbre for pointing this out, bumping the priority for now.

@rahul2393 rahul2393 added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed priority: p3 Desirable enhancement or fix. May not be included in next release. labels Apr 10, 2024
elek added a commit to elek/google-cloud-go that referenced this issue Apr 11, 2024
@elek
disable json support
3d855a3 
This commit removes the JSON support to avoid using json-iter as dependency.

See: googleapis#9380
egonelbre added a commit to egonelbre/google-cloud-go that referenced this issue May 3, 2024
@egonelbre
fix(spanner): remove json-iterator dependency
f86ca15 
json-iterator heavily relies on an unmaintained modern-go/reflect2
that pokes at the internals of Go runtime. This was shown to cause
failures in golang/go#54766 (comment).

Switching to encoding/json does come with a performance penalty.

Updates googleapis#9380
@egonelbre egonelbre mentioned this issue May 3, 2024
Merged
rahul2393 pushed a commit that referenced this issue May 3, 2024
@egonelbre
fix(spanner): remove json-iterator dependency (#10099)
3917cca 
json-iterator heavily relies on an unmaintained modern-go/reflect2
that pokes at the internals of Go runtime. This was shown to cause
failures in golang/go#54766 (comment).

Switching to encoding/json does come with a performance penalty.

Updates #9380
@release-please release-please bot mentioned this issue May 3, 2024
Merged
egonelbre added a commit to egonelbre/google-cloud-go that referenced this issue May 4, 2024
@egonelbre
chore(thirdparty): remove json-iterator exemption
6608f2e 
Fixes googleapis#9380
@egonelbre egonelbre mentioned this issue May 4, 2024
Merged
gcf-merge-on-green bot pushed a commit that referenced this issue May 4, 2024
@egonelbre
chore(thirdparty): remove json-iterator exemption (#10113)
b9b55e0 
Fixes #9380
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rahul2393 rahul2393

Labels
api: spanner Issues related to the Spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: process A process-related concern. May include testing, release, or the like.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore(thirdparty): remove json-iterator exemption egonelbre/google-cloud-go
3 participants
@egonelbre @noahdietz @rahul2393