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

fix(datastore): Prevent panic on GetMulti failure #9656

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

bhshkh
Copy link
Contributor

@bhshkh bhshkh commented Mar 28, 2024

Issue:
When GetMulti fails, it causes below panic because response is nil and library is trying to access Transaction field of nil

=== RUN   TestIntegration_GetMulti
    integration_test.go:434: rpc error: code = Unauthenticated desc = transport: per-RPC creds failed due to error: oauth2: cannot fetch token: 400 Bad Request
        Response: {"error":"invalid_grant","error_description":"Invalid JWT Signature."}
--- FAIL: TestIntegration_GetMulti (0.09s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1b1e5f3]

goroutine 237 [running]:
testing.tRunner.func1.2({0x1ca3ca0, 0x24e14b0})
	/usr/local/go/src/testing/testing.go:1545 +0x3f7
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1548 +0x716
panic({0x1ca3ca0?, 0x24e14b0?})
	/usr/local/go/src/runtime/panic.go:920 +0x270
cloud.google.com/go/datastore.(*Client).get(0xc0002a8690, {0x1f3bee0, 0xc000701c20}, {0xc0002aeac0, 0x6, 0x10c9369?}, {0x1c450a0?, 0xc0001b25b8}, 0x0)
	/Users/bahaaiman/Documents/CNDB-Workspace-2/google-cloud-go/datastore/datastore.go:507 +0x1133
cloud.google.com/go/datastore.(*Client).GetMulti(0xc0002a8690, {0x1f3be70, 0x2537ba0}, {0xc0002aeac0, 0x6, 0x8}, {0x1c450a0, 0xc0001b25b8})
	/Users/bahaaiman/Documents/CNDB-Workspace-2/google-cloud-go/datastore/datastore.go:431 +0x51b
cloud.google.com/go/datastore.TestIntegration_GetMulti(0xc000376ea0)
	/Users/bahaaiman/Documents/CNDB-Workspace-2/google-cloud-go/datastore/integration_test.go:437 +0x12ad
testing.tRunner(0xc000376ea0, 0x1e08278)
	/usr/local/go/src/testing/testing.go:1595 +0x262
created by testing.(*T).Run in goroutine 1
	/usr/local/go/src/testing/testing.go:1648 +0x846
exit status 2
FAIL	cloud.google.com/go/datastore	1.099s

The above failure can be reproduced by using expired service account key while running integration tests.
Fix:
response will not be nil when err is nil.
So, access field after checking err is not nil
After fix:

=== RUN   TestIntegration_GetMulti
    integration_test.go:434: rpc error: code = Unauthenticated desc = transport: per-RPC creds failed due to error: oauth2: cannot fetch token: 400 Bad Request
        Response: {"error":"invalid_grant","error_description":"Invalid JWT Signature."}
    integration_test.go:443: client.GetMulti got *status.Error, expected MultiError
--- FAIL: TestIntegration_GetMulti (0.08s)

@bhshkh bhshkh requested review from a team as code owners March 28, 2024 00:43
@product-auto-label product-auto-label bot added the api: datastore Issues related to the Datastore API. label Mar 28, 2024
@bhshkh bhshkh added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 29, 2024
@bhshkh
Copy link
Contributor Author

bhshkh commented Mar 29, 2024

Do not merge until release freeze ends in mid April

@bhshkh bhshkh removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 15, 2024
@bhshkh bhshkh enabled auto-merge (squash) April 15, 2024 20:15
@bhshkh bhshkh merged commit 55845ad into googleapis:main Apr 15, 2024
8 checks passed
@bhshkh bhshkh deleted the fix/datastore-nil-check branch April 15, 2024 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants