Untangle Datastore #2971
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,113 +16,44 @@ | |
| */ | ||
|
|
||
| import { CredentialsProvider } from '../api/credentials'; | ||
| import { MaybeDocument } from '../model/document'; | ||
| import { DocumentKey } from '../model/document_key'; | ||
| import { Mutation, MutationResult } from '../model/mutation'; | ||
| import * as api from '../protos/firestore_proto_api'; | ||
| import { debugCast, hardAssert } from '../util/assert'; | ||
| import { Code, FirestoreError } from '../util/error'; | ||
| import { Connection } from './connection'; | ||
| import { JsonProtoSerializer } from './serializer'; | ||
| import { | ||
| PersistentListenStream, | ||
| PersistentWriteStream, | ||
| WatchStreamListener, | ||
| WriteStreamListener | ||
| } from './persistent_stream'; | ||
| import { AsyncQueue } from '../util/async_queue'; | ||
|
|
||
| /** | ||
| * Datastore and its related methods are a wrapper around the external Google | ||
| * Cloud Datastore grpc API, which provides an interface that is more convenient | ||
| * for the rest of the client SDK architecture to consume. | ||
| */ | ||
| export class Datastore {} | ||
|
|
||
| /** | ||
| * An implementation of Datastore that exposes additional state for internal | ||
| * consumption. | ||
| */ | ||
| class DatastoreImpl extends Datastore { | ||
| constructor( | ||
|
There was a problem hiding this comment. Rather than changing the object models and completely rearranging the client (so that it doesn't match the other ports), a better strategy would be to adopt tree-shakeable APIs internally. That is, make classes like This will greatly reduce the amount of churn in the relative arrangement of components. Indeed, you could apply this to There was a problem hiding this comment. One of the downsides is that it makes every member public, and I kind of wanted to avoid this. Your argument that we should pull out all methods does win here though. I updated the PR, which also greatly reduces the diff (a large part are now just test cleanup). There was a problem hiding this comment. That's what I meant by dumb structs. Tree-shakeable APIs make this code look like old-school C. Is it possible to adopt a convention of which members are actually public vs those which would be private, but are public by virtue of this kind of code structure? In C programs a way to handle this was to forward declare a private member of your struct to hide details: typedef struct DatastoreDetails DatastoreDetails;
typedef struct Datastore {
DatastoreDetails* details;
} Datastore;Then you only define A similar technique could be translated into typescript:
I'm fairly certain this isn't actually worth it, but this is how we implemented access control before it existed in the language. There was a problem hiding this comment. I pushed a commit with a TypeScript version of this hack: 0295cd5 It should be pretty easy to understand, but I wanted to point out a downside: With this change, at least IntelliJ now consider all members of There was a problem hiding this comment. That's a lot better than my suggestion. If we can get IntelliJ happy, I think this is the way to go. |
||
| public readonly connection: Connection, | ||
| public readonly credentials: CredentialsProvider, | ||
| public readonly serializer: JsonProtoSerializer | ||
| ) { | ||
| super(); | ||
| } | ||
|
|
||
| /** Gets an auth token and invokes the provided RPC. */ | ||
| invokeRPC<Req, Resp>(rpcName: string, request: Req): Promise<Resp> { | ||
| return this.credentials | ||
| .getToken() | ||
| .then(token => { | ||
|
|
@@ -137,7 +68,7 @@ export class Datastore { | |
| } | ||
|
|
||
| /** Gets an auth token and invokes the provided RPC with streamed results. */ | ||
| invokeStreamingRPC<Req, Resp>( | ||
| rpcName: string, | ||
| request: Req | ||
| ): Promise<Resp[]> { | ||
|
|
@@ -158,3 +89,88 @@ export class Datastore { | |
| }); | ||
| } | ||
| } | ||
|
|
||
| export function newDatastore( | ||
| connection: Connection, | ||
| credentials: CredentialsProvider, | ||
| serializer: JsonProtoSerializer | ||
| ): Datastore { | ||
| return new DatastoreImpl(connection, credentials, serializer); | ||
| } | ||
|
|
||
| export async function invokeCommitRpc( | ||
| datastore: Datastore, | ||
| mutations: Mutation[] | ||
| ): Promise<MutationResult[]> { | ||
| const datastoreImpl = debugCast(datastore, DatastoreImpl); | ||
| const params = { | ||
| database: datastoreImpl.serializer.encodedDatabaseId, | ||
| writes: mutations.map(m => datastoreImpl.serializer.toMutation(m)) | ||
| }; | ||
| const response = await datastoreImpl.invokeRPC< | ||
| api.CommitRequest, | ||
| api.CommitResponse | ||
| >('Commit', params); | ||
| return datastoreImpl.serializer.fromWriteResults( | ||
| response.writeResults, | ||
| response.commitTime | ||
| ); | ||
| } | ||
|
|
||
| export async function invokeBatchGetDocumentsRpc( | ||
| datastore: Datastore, | ||
| keys: DocumentKey[] | ||
| ): Promise<MaybeDocument[]> { | ||
| const datastoreImpl = debugCast(datastore, DatastoreImpl); | ||
| const params = { | ||
| database: datastoreImpl.serializer.encodedDatabaseId, | ||
| documents: keys.map(k => datastoreImpl.serializer.toName(k)) | ||
| }; | ||
| const response = await datastoreImpl.invokeStreamingRPC< | ||
| api.BatchGetDocumentsRequest, | ||
| api.BatchGetDocumentsResponse | ||
| >('BatchGetDocuments', params); | ||
|
|
||
| const docs = new Map<string, MaybeDocument>(); | ||
| response.forEach(proto => { | ||
| const doc = datastoreImpl.serializer.fromMaybeDocument(proto); | ||
| docs.set(doc.key.toString(), doc); | ||
| }); | ||
| const result: MaybeDocument[] = []; | ||
| keys.forEach(key => { | ||
| const doc = docs.get(key.toString()); | ||
| hardAssert(!!doc, 'Missing entity in write response for ' + key); | ||
| result.push(doc); | ||
| }); | ||
| return result; | ||
| } | ||
|
|
||
| export function newPersistentWriteStream( | ||
| datastore: Datastore, | ||
| queue: AsyncQueue, | ||
| listener: WriteStreamListener | ||
| ): PersistentWriteStream { | ||
| const datastoreImpl = debugCast(datastore, DatastoreImpl); | ||
| return new PersistentWriteStream( | ||
| queue, | ||
| datastoreImpl.connection, | ||
| datastoreImpl.credentials, | ||
| datastoreImpl.serializer, | ||
| listener | ||
| ); | ||
| } | ||
|
|
||
| export function newPersistentWatchStream( | ||
| datastore: Datastore, | ||
| queue: AsyncQueue, | ||
| listener: WatchStreamListener | ||
| ): PersistentListenStream { | ||
| const datastoreImpl = debugCast(datastore, DatastoreImpl); | ||
| return new PersistentListenStream( | ||
| queue, | ||
| datastoreImpl.connection, | ||
| datastoreImpl.credentials, | ||
| datastoreImpl.serializer, | ||
| listener | ||
| ); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you made this an
interfacethis wouldn't even materialize in the generated code.The downside is that empty interfaces match any object so we'd effectively lose type safety. Maybe that's why you chose class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, an empty interface matches all objects in TypeScript. It is even commonly used to do exactly that.