Firestore API #83
Firestore API #83
Conversation
…ase-admin-java into hkj-firestore-java
|
@mikelehen Code is pretty much the same as it was when you reviewed a while ago. It has just been updated/merged with the other changes that has been done recently in the master branch. |
pom.xml
Outdated
| </dependency> | ||
| <dependency> | ||
| <groupId>com.google.api</groupId> | ||
| <artifactId>gax</artifactId> |
There was a problem hiding this comment.
Why do we need this? I'm not really sure what it is, but I don't see any obvious references to it (e.g. com.google.api classes or anything)...
There was a problem hiding this comment.
This is a transitive dependency for Firestore/GRPC. Without it I get the following test failures:
testServiceAccountProjectId(com.google.firebase.cloud.FirestoreClientTest) Time elapsed: 0.001 sec <<< ERROR!
java.lang.NoClassDefFoundError: com/google/api/gax/rpc/ClientSettings
at com.google.firebase.cloud.FirestoreClientTest.testServiceAccountProjectId(FirestoreClientTest.java:42)
Caused by: java.lang.ClassNotFoundException: com.google.api.gax.rpc.ClientSettings
at com.google.firebase.cloud.FirestoreClientTest.testServiceAccountProjectId(FirestoreClientTest.java:42)
testAppDelete(com.google.firebase.cloud.FirestoreClientTest) Time elapsed: 0 sec <<< ERROR!
java.lang.NoClassDefFoundError: Could not initialize class com.google.cloud.firestore.FirestoreOptions
at com.google.firebase.cloud.FirestoreClientTest.testAppDelete(FirestoreClientTest.java:56)
testExplicitProjectId(com.google.firebase.cloud.FirestoreClientTest) Time elapsed: 0 sec <<< ERROR!
java.lang.NoClassDefFoundError: Could not initialize class com.google.cloud.firestore.FirestoreOptions
at com.google.firebase.cloud.FirestoreClientTest.testExplicitProjectId(FirestoreClientTest.java:30)
I think Firestore should declare this dependency. I'll report this for Firestore.
There was a problem hiding this comment.
Yeah, I don't think that should be necessary. FWIW, our docs don't mention needing it (https://firebase.google.com/docs/firestore/quickstart under the java tab just shows the one import)
There was a problem hiding this comment.
Reported at googleapis/google-cloud-java#2496
|
|
||
| private FirestoreClient(FirebaseApp app) { | ||
| checkNotNull(app, "FirebaseApp must not be null"); | ||
| // The following will be further simplified once we migrate to GoogleCredentials. |
|
|
||
| @Override | ||
| public void destroy() { | ||
| // NOTE: We don't explicitly tear down anything here, but public methods of StorageClient |
There was a problem hiding this comment.
Updated the comment as described below.
| @Override | ||
| public void destroy() { | ||
| // NOTE: We don't explicitly tear down anything here, but public methods of StorageClient | ||
| // will now fail because calls to getOptions() and getToken() will hit FirebaseApp, |
There was a problem hiding this comment.
"getOptions() and getToken()" => "getCredentials()" ?
There was a problem hiding this comment.
I think with the current design a Firestore instance can continue to work even after the FirebaseApp has been deleted. The user won't be able to call FirestoreClient.getFirestore(), but if the user already has a Firestore reference, that will continue to work. Firestore API will have to provide some way to clean it up/tear it down to properly support this.
I've updated the comment here accordingly.
Edit: I'm also thinking about controlling the thread pool provided to Firestore client. Perhaps there's some way to inject some cleanup logic there. I'll do it as a separate PR.
There was a problem hiding this comment.
Reported at googleapis/google-cloud-java#2497
| "population", 77846L, | ||
| "capital", false | ||
| ); | ||
| WriteResult result = reference.set(expected).get(); |
There was a problem hiding this comment.
Do we need to add instructions to CONTRIBUTING.md (or wherever) to open up your Firestore security rules?
|
Made the suggested changes. |
CONTRIBUTING.md
Outdated
| @@ -138,7 +138,9 @@ makes a large number of writes to the Firebase realtime database. Download the s | |||
| private key from the "Settings > Service Accounts" page of the project, and save it as | |||
| `integration_cert.json` at the root of the codebase. Also obtain the web API key of the project | |||
| from the "Settings > General" page, and save it as `integration_apikey.txt` at the root of the | |||
| codebase. Now run the following command to invoke the integration test suite: | |||
| codebase. Also make sure Firestore security rules configured in the project allow reads and writes | |||
| from any authenticated client. Finally, run the following command to invoke the integration test | |||
There was a problem hiding this comment.
Er, sorry. I think I was confused before. Since this is an admin SDK, you're using a service account, which overrides Firebase Rules, so I don't think they need to do anything special.
There was a problem hiding this comment.
Ok, sounds good. Good to know that Firestore rules can also be overridden in Admin SDK like RTDB rules.
Usage: