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

Ability to set emulator host/port from .setHost rather than env var #210

Open
saturnism opened this issue May 8, 2020 · 8 comments
Open
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

@saturnism
Copy link

Environment details

  1. OSX, Java

Steps to reproduce

  1. To use Testcontainer + Firestore emulator is extremely difficult.

Code example

Firestore client library can only connect to a firestore emulator by using an env var. This is not idiomatic for Java unit tests.

it's impossible to set the emulator host/port simply from .setHost(..)

FirestoreOptions firestoreOptions =
   FirestoreOptions.newBuilder().setHost(...).

The real code is this:

// duplicate the FakeCredentials class because it's package protected and not publicly accessible.
static class FakeCredentials extends Credentials {
 private final Map<String, List<String>> HEADERS =
     ImmutableMap.of("Authorization", Arrays.asList("Bearer owner"));
...
}

// then, duplicate the code that otherwise executes from an env var:
FirestoreOptions firestoreOptions =
   FirestoreOptions.newBuilder()
       .setProjectId(FIRESTORE_PROJECT_ID)
       .setChannelProvider(
           InstantiatingGrpcChannelProvider.newBuilder()
               .setEndpoint(
                   String.format(
                       "%s:%d",
                       FIRESTORE_CONTAINER.getContainerIpAddress(),
                       FIRESTORE_CONTAINER.getMappedPort(8080)))
               .setChannelConfigurator(
                   (managedChannelBuilder -> managedChannelBuilder.usePlaintext()))
               .build())
       .setCredentialsProvider(FixedCredentialsProvider.create(new FakeCredentials()))
       .build();

Any additional information below

It'd be MUCH easier if emulator host/port can simply be set via .setHost(...), OR, a convenience method, such as .useEmulator(host, port)

@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label May 8, 2020
@saturnism
Copy link
Author

/cc @eddumelendez who's also working with TestContainer integration for GCP. https://github.com/eddumelendez/testcontainers-java/tree/gcloud

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label May 9, 2020
@BenWhitehead BenWhitehead added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed triage me I really want to be triaged. labels May 11, 2020
@BenWhitehead
Copy link
Collaborator

In the same vein of #190.

Something I'm hoping to have time for soon, but no specific estimate.

@saturnism
Copy link
Author

looks like the same issue. shared an internal friction log. thnx!

@schmidt-sebastian
Copy link
Contributor

@BenWhitehead I am not opposed to this, but there seems to be a much more generic problem here: It is very difficult (even for me) to figure out which of the sheer endless options I have to enable to get change even simple settings.

If we add a setHost here (and not in the main settings provider), how should we deal with conflicting settings?

@saturnism
Copy link
Author

saturnism commented May 12, 2020

I don't think that'll be any worse than now? Which is exposing all of these surfaces anyways. I feel that's a different set of problems - we need to minimize the surface, identify what's really necessary, and expose them via proper API surface. But setHost should just work. It's already there.

@schmidt-sebastian
Copy link
Contributor

What you really need is a setHost(String hostname, boolean ssl, boolean useFakeCredentials). A single setHost does not help, since it still leaves a bunch of settings unset. If we need a short term fix, we could add a setEmulatorHost that does most of this for you.

@BenWhitehead
Copy link
Collaborator

I don't think a short term setEmulatorHost would necessarily be better for us maintenance wise. There is also the question about how difficult it is to bootstrap the emulator for use in java code. In datastore there is an emulator runner which allows the emulator to be started from java code, I think there is a desire to have the same thing for Firestore. With that in mind I think it'd be better for us to figure out how those will work together and then provide a good api for all emulator functionality rather than having two related but disconnected things our users have to figure out how to connect.

If we're wanting to push out a fix to help out the fact that env variables can't be modified in process, we could add a fallback java property firestore.emulator.host and try reading that. This would allow in-process configuration of that flag for users.

    String envVar = System.getenv("FIRESTORE_EMULATOR_HOST");
    String sysProp = System.getProperty("firestore.emulator.host");
    String emulatorHost = envVar != null ? envVar : sysProp;
    
    if (emulatorHost != null) {
      // use fake creds
      // no ssl
    }

@saturnism
Copy link
Author

why do we expose setHost if it's not to be used to set it to any arbitrary hosts?

does setHost take in a protocol? it actually does respect the dns:// protocol. tbh; ideally the api should've accepted a full url prefix, e.g. https://.... or http://.... emulators would only be using http

the system property would work, but no one would find that in the api or .setHost javadoc popup. we should add it to the java doc.

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

No branches or pull requests

4 participants