search

authzed / spicedb

Open Source, Google Zanzibar-inspired database for scalably storing and querying fine-grained authorization data
https://authzed.com/docs
Apache License 2.0
4.98k stars 267 forks source link

Fix empty value on optional credentialsJSON for Spanner #1948

Closed lexcao closed 3 months ago

lexcao commented 3 months ago

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

lexcao commented 3 months ago

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?

vroldanbet commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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

lexcao commented 3 months ago

Hi @vroldanbet I just raised a PR in the upstream. https://github.com/googleapis/google-api-go-client/pull/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.