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 empty value on optional credentialsJSON for Spanner #1948

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

lexcao
Copy link
Contributor

@lexcao lexcao commented Jun 18, 2024

Closes #1941
This is the follow-up PR for #1942, which fix the optional value issue.

@lexcao lexcao requested a review from a team June 18, 2024 16:15
@github-actions github-actions bot added the area/datastore Affects the storage system label Jun 18, 2024
Copy link
Contributor

@vroldanbet vroldanbet left a comment

Choose a reason for hiding this comment

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

The change makes sense to me, but concerned it has taken 3 PRs to get this right. Can we add a test?

@lexcao lexcao force-pushed the fix/spanner-empty-json branch 2 times, most recently from 3645316 to 3877e9e Compare June 18, 2024 17:41
@lexcao
Copy link
Contributor Author

lexcao commented Jun 18, 2024

Hi @vroldanbet
I just thought it is options forwarding thing but I am not familiar with the code base at the beginning.
Is there any test case I can add?

@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Jun 18, 2024
@vroldanbet
Copy link
Contributor

@lexcao I've made client initialization simpler by making the option handle empty credentials JSON, which Spanner client does not handle well. I also added a test. Please have a look and make sure this works for you.

@lexcao
Copy link
Contributor Author

lexcao commented Jun 19, 2024

Hi @vroldanbet
Thanks for your suggestions, but it would not resolve the issue.
The key point is option.WithCredentialsJSON(config.credentialsJSON) from Spanner,
which should skip adding that option if the credentialsJSON is nil.

The internal operation in the WithCredentialsJSON method is making nil to empty slice of bytes,
which would cause initialization error for Spanner with empty slice

https://github.com/googleapis/google-api-go-client/blob/e732ee3983555c5b0ca708033d9b8ec675f99019/option/option.go#L61-L70

@vroldanbet
Copy link
Contributor

@lexcao, you are right. I'd suggest removing my commit, and if anything adding a new commit only with the test. I'd also suggest opening a PR upstream spanner SDK to fix this annoying behaviour

@lexcao
Copy link
Contributor Author

lexcao commented Jun 19, 2024

Thanks, let me try to raise a PR for Spanner.

Closes authzed#1941
This is the follow-up PR for authzed#1942, which fix the optional value issue.

Signed-off-by: Lex Cao <[email protected]>
@lexcao
Copy link
Contributor Author

lexcao commented Jun 19, 2024

Hi @vroldanbet
I just raised a PR in the upstream.
googleapis/google-api-go-client#2643
Waiting for review.

For now, I think we should add the fix for this, since it could break the use of CredentialsFile for Spanner.
I have updated my code, you can review now.

I will make it more ergonomic after the upstream accepts my PR then.

@github-actions github-actions bot removed the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Jun 19, 2024
@vroldanbet vroldanbet added this pull request to the merge queue Jun 19, 2024
Merged via the queue into authzed:main with commit cbbb5b0 Jun 19, 2024
22 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/datastore Affects the storage system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow credential JSON for Spanner options
3 participants