Conversation

dmivankov

Spanner client wants to have a projectId option set, by default it also tries to pick it up from lots of places
For emulator case none of those may be present as it could be that neither gcloud nor default application credentials are configured, but emulator is accessible nevertheless.

Set default project id in SpannerOptions, value doesn't matter so much as emulator would be happy with any value it seems.
Note that alternatively it could be improved in base ServiceOptions, but not sure about assumptions there.

failure example in case where emulatorHost is set without projectId

  java.lang.IllegalArgumentException: A project ID is required for this service but could not be determined from the builder or the environment.  Please set a project ID using the builder.
  at com.google.common.base.Preconditions.checkArgument(Preconditions.java:142)
  at com.google.cloud.ServiceOptions.<init>(ServiceOptions.java:304)
  at com.google.cloud.spanner.SpannerOptions.<init>(SpannerOptions.java:533)
  at com.google.cloud.spanner.SpannerOptions.<init>(SpannerOptions.java:77)
  at com.google.cloud.spanner.SpannerOptions$Builder.build(SpannerOptions.java:1029)

Note that in non-emulator case one usually doesn't need to set projectId as it could be extracted from credentials

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

@dmivankov
Set projectId to a default value in emulator case
d8e6320
Spanner client wants to have a projectId option set, by default it also tries to pick it up from [lots of places](https://.com/googleapis/java-core/blob/375983090b3700b3fb6a1953626db7efca49cc51/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java#L372)
For emulator case none of those may be present as it could be that neither gcloud nor default application credentials are configured, but emulator is accessible nevertheless.

Set default project id in SpannerOptions, value doesn't matter so much as emulator would be happy with any value it seems.
Note that alternatively it could be improved in base ServiceOptions, but not sure about assumptions there.
@dmivankovdmivankov requested a review from a team as a code owner August 12, 2021 11:40
@product-auto-labelproduct-auto-label bot added the api: spannerIssues related to the googleapis/java-spanner API.label Aug 12, 2021
@google-clagoogle-cla bot added the cla: yesThis human has signed the Contributor License Agreement.label Aug 12, 2021
@dmivankovdmivankov changed the title Set projectId to a default value in emulator case fix: set projectId to a default value in emulator case Aug 12, 2021
olavloite
olavloite reviewed Aug 12, 2021
View reviewed changes

Choose a reason for hiding this comment

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

Hi @dmivankov Thanks for your contribution. I think this is a good idea. I've left a couple of textual suggestions.
Also, there seems to be a compile error. Would you mind take a look at that?

Also, would you be able to add a test that verifies that it will actually use this, and that it does not fail before it gets this far? If not, then that's OK as well. I can take a look at a later moment. It might be quite tricky as such a test would depend on a lot of different environment variables.

@thiagotnunes Do you have any additional comments on this?

thiagotnunes and others added 2 commits August 13, 2021 14:46
thiagotnunes
thiagotnunes approved these changes Aug 13, 2021
View reviewed changes

Choose a reason for hiding this comment

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

There is a compilation error, but LGTM.

After fixing that we should be good to go.

dmivankov
dmivankov commented Aug 13, 2021
View reviewed changes
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
@@ -1134,6 +1134,11 @@ public SpannerOptions build() {
this.setChannelConfigurator(ManagedChannelBuilder::usePlaintext);
// As we are using plain text, we should never send any credentials.
this.setCredentials(NoCredentials.getInstance());
// Default project id resolution might fail, but the emulator accepts any
// project id, so we can safely use a dummy id.
if (this.getProjectId() == null) {
Copy link
Author

Choose a reason for hiding this comment

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

oops, may need more work as there's no such getter in ServiceOptions builder and the field is private https://.com/googleapis/java-core/blob/375983090b3700b3fb6a1953626db7efca49cc51/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java#L127

olavloite added a commit that referenced this pull request Aug 19, 2021
@olavloite
feat: use dummy emulator-project when no project is set
5068872
Use a dummy emulator-project when no default project is set for the environment
and the SPANNER_EMULATOR_HOST environment variable has been set.

Replaces #1345
@olavloiteolavloite mentioned this pull request Aug 19, 2021
Merged
@olavloite

@dmivankov I've created a new PR that uses a slightly different approach to achieve the same goal: #1363

thiagotnunes pushed a commit that referenced this pull request Aug 19, 2021
@olavloite
feat: use dummy emulator-project when no project is set (#1363)
673855e
Use a dummy emulator-project when no default project is set for the environment
and the SPANNER_EMULATOR_HOST environment variable has been set.

Replaces #1345
@thiagotnunes

Closed in favour of #1363

gcf-merge-on-green bot pushed a commit that referenced this pull request Aug 24, 2021
@release-please
chore: release 6.12.0 (#1364)
28df2d9
🤖 I have created a release \*beep\* \*boop\*
---
## [6.12.0](https://www..com/googleapis/java-spanner/compare/v6.11.1...v6.12.0) (2021-08-24)


### Features

* add support for JSON data type ([#872](https://www..com/googleapis/java-spanner/issues/872)) ([d7ff940](https://www..com/googleapis/java-spanner/commit/d7ff9409e974602dc9b18f82d6dbd11d96c956bf))
* use dummy emulator-project when no project is set ([#1363](https://www..com/googleapis/java-spanner/issues/1363)) ([673855e](https://www..com/googleapis/java-spanner/commit/673855eea8c244457ad4c8ac5abe3ad3a0a0cdde)), closes [#1345](https://www..com/googleapis/java-spanner/issues/1345)
---


This PR was generated with [Release Please](https://.com/googleapis/release-please). See [documentation](https://.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on . Already have an account? Sign in to comment

@olavloite olavloite olavloite left review comments

@thiagotnunes thiagotnunes thiagotnunes approved these changes

No one assigned
api: spannerIssues related to the googleapis/java-spanner API.cla: yesThis human has signed the Contributor License Agreement.
None yet
No milestone

Successfully merging this pull request may close these issues.