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

Provide a setter for emulator host #319

Closed
Gerschtli opened this issue Aug 6, 2020 · 4 comments · Fixed by #336
Closed

Provide a setter for emulator host #319

Gerschtli opened this issue Aug 6, 2020 · 4 comments · Fixed by #336
Assignees
Labels
api: firestore Issues related to the googleapis/java-firestore API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@Gerschtli
Copy link
Contributor

Is your feature request related to a problem? Please describe.

When I write an integration test and I use a new emulator instance every test with different port numbers, I need to copy the setup logic from FirestoreOptions.Builder.build to configure the connection.

Describe the solution you'd like

I would like to see a setter for the emulator host value, which overrides the value of FIRESTORE_EMULATOR_HOST.
I tried to implement it to clarify my intention:

commit d5e6586e3ba78f3cda6dec0b6a0fb9dcec807cfb
Author: Tobias Happ <tobias.happ@gmx.de>
Date:   2020-08-06 21:31:12 +0200

    feat: add getEmulatorHost for FirestoreOptions
---
 .../google/cloud/firestore/FirestoreOptions.java   | 23 +++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreOptions.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreOptions.java
index a65abca..4542504 100644
--- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreOptions.java
+++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreOptions.java
@@ -63,6 +63,7 @@ public final class FirestoreOptions extends ServiceOptions<Firestore, FirestoreO
   private final boolean timestampsInSnapshotsEnabled;
   private final TransportChannelProvider channelProvider;
   private final CredentialsProvider credentialsProvider;
+  private final String emulatorHost;
 
   public static class DefaultFirestoreFactory implements FirestoreFactory {
 
@@ -117,12 +118,17 @@ public final class FirestoreOptions extends ServiceOptions<Firestore, FirestoreO
     return channelProvider;
   }
 
+  public String getEmulatorHost() {
+    return emulatorHost;
+  }
+
   public static class Builder extends ServiceOptions.Builder<Firestore, FirestoreOptions, Builder> {
 
     @Nullable private String databaseId = null;
     @Nullable private Boolean timestampsInSnapshotsEnabled = null;
     @Nullable private TransportChannelProvider channelProvider = null;
     @Nullable private CredentialsProvider credentialsProvider = null;
+    @Nullable private String emulatorHost = null;
 
     private Builder() {}
 
@@ -132,6 +138,7 @@ public final class FirestoreOptions extends ServiceOptions<Firestore, FirestoreO
       this.timestampsInSnapshotsEnabled = options.timestampsInSnapshotsEnabled;
       this.channelProvider = options.channelProvider;
       this.credentialsProvider = options.credentialsProvider;
+      this.emulatorHost = options.emulatorHost;
     }
 
     /**
@@ -189,6 +196,16 @@ public final class FirestoreOptions extends ServiceOptions<Firestore, FirestoreO
       return this;
     }
 
+    /**
+     * Sets the emulator host to use with this Firestore client.
+     *
+     * @param emulatorHost The Firestore emulator host to use with this client.
+     */
+    public Builder setEmulatorHost(@Nonnull String emulatorHost) {
+      this.emulatorHost = emulatorHost;
+      return this;
+    }
+
     @Override
     @Nonnull
     public FirestoreOptions build() {
@@ -201,7 +218,9 @@ public final class FirestoreOptions extends ServiceOptions<Firestore, FirestoreO
       }
 
       // Override credentials and channel provider if we are using the emulator.
-      String emulatorHost = System.getenv(FIRESTORE_EMULATOR_SYSTEM_VARIABLE);
+      if (emulatorHost == null) {
+        emulatorHost = System.getenv(FIRESTORE_EMULATOR_SYSTEM_VARIABLE);
+      }
       if (emulatorHost != null) {
         // Try creating a host in order to validate that the host name is valid.
         try {
@@ -290,6 +309,8 @@ public final class FirestoreOptions extends ServiceOptions<Firestore, FirestoreO
         builder.credentialsProvider != null
             ? builder.credentialsProvider
             : GrpcTransportOptions.setUpCredentialsProvider(this);
+
+    this.emulatorHost = builder.emulatorHost;;
   }
 
   private static class FirestoreDefaults implements ServiceDefaults<Firestore, FirestoreOptions> {

Describe alternatives you've considered

The alternative is to copy the setup logic used in FirestoreOptions.Builder.build but this does unnecessarily increase code duplication and complexity.

Additional information

I would like to contribute to help this feature request get implemented. I am looking forward for your feedback!

@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Aug 6, 2020
@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Aug 7, 2020
@wu-hui wu-hui self-assigned this Aug 12, 2020
@wu-hui wu-hui removed the triage me I really want to be triaged. label Aug 12, 2020
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Aug 12, 2020
@wu-hui
Copy link
Contributor

wu-hui commented Aug 12, 2020

Hi @Gerschtli

Thank you so much for helping to make our SDK better!

I think having a host settings on top of env variable is very helpful for integration tests, can you open up a PR if that is OK with you?

Also, changing public API surface requires to be reviewed by our API team (across all SDKs), so the actual approval for you PR might come a bit slow.

Thank you again!

@BenWhitehead BenWhitehead added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Aug 12, 2020
@BenWhitehead
Copy link
Collaborator

One thing to keep in mind as you are working on this. When an emulator is connected to there is additional setup that has to take place. Hopefully, adding the parameter will be enough and the rest of the code will be just fine, however if not; things to look into:

  1. Ensure that the connection to the emulator is plaintext since the emulator only supports http
  2. Ensure that the credentials pass the necessary headers to allow the emulator to perform admin actions see EmulatorCredentials

@wu-hui
Copy link
Contributor

wu-hui commented Aug 14, 2020

FWIW, you can also just use below snippet to setup tests against emulator:

    FirestoreOptions firestoreOptions = FirestoreOptions.newBuilder()
            .setChannelProvider(InstantiatingGrpcChannelProvider.newBuilder()
                    .setEndpoint("localhost:8080")
                    .setChannelConfigurator(
                            new ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder>() {
                              @Override
                              public ManagedChannelBuilder apply(ManagedChannelBuilder input) {
                                return input.usePlaintext();
                              }
                            })
                    .build())
            .setCredentials(new FirestoreOptions.EmulatorCredentials())
            .build();
    Firestore firestore = firestoreOptions.getService();

@Gerschtli
Copy link
Contributor Author

Thank you for your responses. I will try creating a PR for this.

@wu-hui Yes this works, but why should a user of the library copy-paste this snippet over and over? A more concise API would be easier to understand and maintain.

Gerschtli added a commit to Gerschtli/java-firestore that referenced this issue Aug 15, 2020
Gerschtli added a commit to Gerschtli/java-firestore that referenced this issue Aug 27, 2020
gcf-merge-on-green bot pushed a commit that referenced this issue Aug 27, 2020
gcf-merge-on-green bot pushed a commit that referenced this issue Sep 15, 2020
🤖 I have created a release \*beep\* \*boop\* 
---
## [2.1.0](https://www.github.com/googleapis/java-firestore/compare/v2.0.0...v2.1.0) (2020-09-10)


### Features

* add method to set emulator host programmatically ([#319](https://www.github.com/googleapis/java-firestore/issues/319)) ([#336](https://www.github.com/googleapis/java-firestore/issues/336)) ([97037f4](https://www.github.com/googleapis/java-firestore/commit/97037f42f76e9df3ae458d4e2b04336e64b834c3)), closes [#210](https://www.github.com/googleapis/java-firestore/issues/210) [#190](https://www.github.com/googleapis/java-firestore/issues/190)
* add opencensus tracing support ([#360](https://www.github.com/googleapis/java-firestore/issues/360)) ([edaa539](https://www.github.com/googleapis/java-firestore/commit/edaa5395be0353fb261d954429c624623bc4e346))
* add support for != and NOT_IN queries ([#350](https://www.github.com/googleapis/java-firestore/issues/350)) ([68aff5b](https://www.github.com/googleapis/java-firestore/commit/68aff5b406fb2732951750f3d5f9768df6ee12b5))
* generate protos to add NOT_EQUAL, NOT_IN, IS_NOT_NAN, IS_NOT_NULL query operators ([#343](https://www.github.com/googleapis/java-firestore/issues/343)) ([3fb1b63](https://www.github.com/googleapis/java-firestore/commit/3fb1b631f8dd087f0f3e1c43363e9642f497993a))


### Bug Fixes

* **samples:** re-add maven exec config for Quickstart sample ([#347](https://www.github.com/googleapis/java-firestore/issues/347)) ([4c2329b](https://www.github.com/googleapis/java-firestore/commit/4c2329bf89ffab4bd3060e16e1cf231b7caf4653))
* add support to deserialize to custom Lists and Maps ([#337](https://www.github.com/googleapis/java-firestore/issues/337)) ([dc897e0](https://www.github.com/googleapis/java-firestore/commit/dc897e00a85e745f57f615460b29d17b7dd247c6))


### Dependencies

* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.9.0 ([#352](https://www.github.com/googleapis/java-firestore/issues/352)) ([783d41e](https://www.github.com/googleapis/java-firestore/commit/783d41e167c7c79957faeeebd7a76ab72b5b157d))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/java-firestore API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants