Skip to content

Add ProtoJS types to generated proto types #2749

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Mar 18, 2020
Merged

Add ProtoJS types to generated proto types #2749

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b887735
Add ProtoJS types to generated proto types
schmidt-sebastian Mar 17, 2020
22ec7de
Review
schmidt-sebastian Mar 18, 2020
009ae20
Merge
schmidt-sebastian Mar 18, 2020
e9fe7ff
Fix unit test
schmidt-sebastian Mar 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/firestore/src/api/user_data_writer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ export class UserDataWriter<T> {
private convertServerTimestamp(value: ServerTimestampValue): unknown {
switch (this.serverTimestampBehavior) {
case 'previous':
return value.previousValue ? this.convertValue(value.previousValue) : null;
return value.previousValue
? this.convertValue(value.previousValue)
: null;
case 'estimate':
return this.convertTimestamp(value.localWriteTime);
default:
Expand Down
14 changes: 5 additions & 9 deletions packages/firestore/src/model/proto_values.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,13 @@ import {
numericEquals,
primitiveComparator
} from '../util/misc';
import { TimestampValue } from '../remote/serializer';

// A RegExp matching ISO 8601 UTC timestamps with optional fraction.
const ISO_TIMESTAMP_REG_EXP = new RegExp(
/^\d{4}-\d\d-\d\dT\d\d:\d\d:\d\d(?:\.(\d+))?Z$/
);

// Denotes the possible representations for timestamps in the Value type.
type ProtoTimestampValue =
| string
| { seconds?: string | number; nanos?: number };

/** Extracts the backend's type order for the provided value. */
export function typeOrder(value: api.Value): TypeOrder {
if ('nullValue' in value) {
Expand Down Expand Up @@ -236,8 +232,8 @@ function compareNumbers(left: api.Value, right: api.Value): number {
}

function compareTimestamps(
left: ProtoTimestampValue,
right: ProtoTimestampValue
left: TimestampValue,
right: TimestampValue
): number {
if (typeof left === 'string' && typeof right === 'string') {
// Use string ordering for ISO 8601 timestamps, but strip the timezone
Expand Down Expand Up @@ -376,7 +372,7 @@ function canonifyByteString(byteString: string | Uint8Array): string {
return normalizeByteString(byteString).toBase64();
}

function canonifyTimestamp(timestamp: ProtoTimestampValue): string {
function canonifyTimestamp(timestamp: TimestampValue): string {
const normalizedTimestamp = normalizeTimestamp(timestamp);
return `time(${normalizedTimestamp.seconds},${normalizedTimestamp.nanos})`;
}
Expand Down Expand Up @@ -478,7 +474,7 @@ function estimateArrayByteSize(arrayValue: api.ArrayValue): number {
* nanos" representation.
*/
export function normalizeTimestamp(
date: ProtoTimestampValue
date: TimestampValue
): { seconds: number; nanos: number } {
assert(!!date, 'Cannot normalize null or undefined timestamp.');

Expand Down
20 changes: 10 additions & 10 deletions packages/firestore/src/protos/firestore_proto_api.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ export declare namespace firestoreV1ApiClientInterfaces {
name?: string;
fields?: ApiClientObjectMap<Value>;
createTime?: string;
updateTime?: string;
updateTime?: string | { seconds?: string | number; nanos?: number };
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this generated or hand edited?

If hand edited, would it be worthwhile to define the timestamp type in here and reuse it for all these declarations?

Then we could just call it Timestamp and refer to it as api.Timestamp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to be generated, but has been hand edited many times since then. As such, I added api.Timestamp.

}
interface DocumentChange {
document?: Document;
Expand All @@ -190,7 +190,7 @@ export declare namespace firestoreV1ApiClientInterfaces {
interface DocumentDelete {
document?: string;
removedTargetIds?: number[];
readTime?: string;
readTime?: string | { seconds?: string | number; nanos?: number };
}
interface DocumentMask {
fieldPaths?: string[];
Expand Down Expand Up @@ -290,7 +290,7 @@ export declare namespace firestoreV1ApiClientInterfaces {
}
interface Precondition {
exists?: boolean;
updateTime?: string;
updateTime?: string | { seconds?: string | number; nanos?: number };
}
interface Projection {
fields?: FieldReference[];
Expand Down Expand Up @@ -333,12 +333,12 @@ export declare namespace firestoreV1ApiClientInterfaces {
startAt?: Cursor;
endAt?: Cursor;
offset?: number;
limit?: number;
limit?: number | { value: number };
}
interface Target {
query?: QueryTarget;
documents?: DocumentsTarget;
resumeToken?: string;
resumeToken?: string | Uint8Array;
readTime?: string;
targetId?: number;
once?: boolean;
Expand All @@ -347,8 +347,8 @@ export declare namespace firestoreV1ApiClientInterfaces {
targetChangeType?: TargetChangeTargetChangeType;
targetIds?: number[];
cause?: Status;
resumeToken?: string;
readTime?: string;
resumeToken?: string | Uint8Array;
readTime?: string | { seconds?: string | number; nanos?: number };
}
interface TransactionOptions {
readOnly?: ReadOnly;
Expand Down Expand Up @@ -382,17 +382,17 @@ export declare namespace firestoreV1ApiClientInterfaces {
interface WriteRequest {
streamId?: string;
writes?: Write[];
streamToken?: string;
streamToken?: string | Uint8Array;
labels?: ApiClientObjectMap<string>;
}
interface WriteResponse {
streamId?: string;
streamToken?: string;
writeResults?: WriteResult[];
commitTime?: string;
commitTime?: string | { seconds?: string | number; nanos?: number };
}
interface WriteResult {
updateTime?: string;
updateTime?: string | { seconds?: string | number; nanos?: number };
transformResults?: Value[];
}
}
Expand Down
63 changes: 22 additions & 41 deletions packages/firestore/src/remote/serializer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ import { assert, fail } from '../util/assert';
import { Code, FirestoreError } from '../util/error';
import * as obj from '../util/obj';
import { ByteString } from '../util/byte_string';
import * as typeUtils from '../util/types';
import { isNullOrUndefined } from '../util/types';

import {
ArrayRemoveTransformOperation,
Expand Down Expand Up @@ -99,15 +99,13 @@ const OPERATORS = (() => {
})();

function assertPresent(value: unknown, description: string): asserts value {
assert(!typeUtils.isNullOrUndefined(value), description + ' is missing');
assert(!isNullOrUndefined(value), description + ' is missing');
}

// This is a supplement to the generated proto interfaces, which fail to account
// for the fact that a timestamp may be encoded as either a string OR this.
interface TimestampProto {
seconds?: string | number;
nanos?: number;
}
// Denotes the possible representations for timestamps in the Value type.
export type TimestampValue =
Copy link
Contributor

Choose a reason for hiding this comment

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

The Value proto can contain a timestamp and the name TimestampValue suggests that this is one of those, when it's really just the timestamp itself. Also, this is somewhat confusing because historically FooValue has been one of our model types and not a proto. In this regard the old TimestampProto name was better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed it to Timestamp, which aligns with LatLng and GeoPoint. I also moved it to the proto d.ts file.

| string
| { seconds?: string | number; nanos?: number };

export interface SerializerOptions {
/**
Expand Down Expand Up @@ -148,46 +146,33 @@ export class JsonProtoSerializer {
* our generated proto interfaces say Int32Value must be. But GRPC actually
* expects a { value: <number> } struct.
*/
private toInt32Value(val: number | null): number | null {
if (this.options.useProto3Json || typeUtils.isNullOrUndefined(val)) {
private toInt32Value(val: number | null): number | { value: number } | null {
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: toInt32Value doesn't really do much to indicate that it's converting to a proto message. What do you think of renaming these (and related items) to names like toInt32Proto?

The Android/iOS convention of naming these encode/decode helps somewhat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed this instance to toInt32Proto but did not convert the methods to the encode/decode pattern. I am personally also confused by the naming in the file and constantly try to use the wrong methods (toValue instead of fromValue etc)

if (this.options.useProto3Json || isNullOrUndefined(val)) {
return val;
} else {
// ProtobufJS requires that we wrap Int32Values.
// Use any because we need to match generated Proto types.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return { value: val } as any;
return { value: val };
}
}

/**
* Returns a number (or null) from a google.protobuf.Int32Value proto.
* DO NOT USE THIS FOR ANYTHING ELSE.
* This method cheats. It's typed as accepting "number" because that's what
* our generated proto interfaces say Int32Value must be, but it actually
* accepts { value: number } to match our serialization in toInt32Value().
*/
private fromInt32Value(val: number | undefined): number | null {
private fromInt32Value(
val: number | { value: number } | undefined
): number | null {
let result;
if (typeof val === 'object') {
// Use any because we need to match generated Proto types.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
result = (val as any).value;
result = val.value;
} else {
// We accept raw numbers (without the {value: ... } wrapper) for
// compatibility with legacy persisted data.
result = val;
}
return typeUtils.isNullOrUndefined(result) ? null : result;
return isNullOrUndefined(result) ? null : result;
}

/**
* Returns a value for a Date that's appropriate to put into a proto.
* DO NOT USE THIS FOR ANYTHING ELSE.
* This method cheats. It's typed as returning "string" because that's what
* our generated proto interfaces say dates must be. But it's easier and safer
* to actually return a Timestamp proto.
*/
private toTimestamp(timestamp: Timestamp): string {
private toTimestamp(timestamp: Timestamp): TimestampValue {
if (this.options.useProto3Json) {
// Serialize to ISO-8601 date format, but with full nano resolution.
// Since JS Date has only millis, let's only use it for the seconds and
Expand All @@ -208,25 +193,21 @@ export class JsonProtoSerializer {
}
}

private fromTimestamp(date: string | TimestampProto): Timestamp {
private fromTimestamp(date: TimestampValue): Timestamp {
const timestamp = normalizeTimestamp(date);
return new Timestamp(timestamp.seconds, timestamp.nanos);
}

/**
* Returns a value for bytes that's appropriate to put in a proto.
* DO NOT USE THIS FOR ANYTHING ELSE.
* This method cheats. It's typed as returning "string" because that's what
* our generated proto interfaces say bytes must be. But it should return
* an Uint8Array in Node.
*
* Visible for testing.
*/
toBytes(bytes: Blob | ByteString): string {
toBytes(bytes: Blob | ByteString): string | Uint8Array {
if (this.options.useProto3Json) {
return bytes.toBase64();
} else {
return (bytes.toUint8Array() as unknown) as string;
return bytes.toUint8Array();
}
}

Expand All @@ -249,11 +230,11 @@ export class JsonProtoSerializer {
}
}

toVersion(version: SnapshotVersion): string {
toVersion(version: SnapshotVersion): TimestampValue {
return this.toTimestamp(version.toTimestamp());
}

fromVersion(version: string): SnapshotVersion {
fromVersion(version: TimestampValue): SnapshotVersion {
assert(!!version, "Trying to deserialize version that isn't set");
return SnapshotVersion.fromTimestamp(this.fromTimestamp(version));
}
Expand Down Expand Up @@ -844,7 +825,7 @@ export class JsonProtoSerializer {

private fromWriteResult(
proto: api.WriteResult,
commitTime: string
commitTime: TimestampValue
): MutationResult {
// NOTE: Deletes don't have an updateTime.
let version = proto.updateTime
Expand All @@ -871,7 +852,7 @@ export class JsonProtoSerializer {

fromWriteResults(
protos: api.WriteResult[] | undefined,
commitTime?: string
commitTime?: TimestampValue
): MutationResult[] {
if (protos && protos.length > 0) {
assert(
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/util/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export const isInteger: (value: unknown) => boolean =
/**
* Returns whether a variable is either undefined or null.
*/
export function isNullOrUndefined(value: unknown): boolean {
export function isNullOrUndefined(value: unknown): value is null | undefined {
return value === null || value === undefined;
}

Expand Down
27 changes: 20 additions & 7 deletions packages/firestore/test/unit/specs/spec_test_runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import { DocumentOptions } from '../../../src/model/document';
import { DocumentKey } from '../../../src/model/document_key';
import { JsonObject } from '../../../src/model/field_value';
import { Mutation } from '../../../src/model/mutation';
import { normalizeByteString } from '../../../src/model/proto_values';
import { PlatformSupport } from '../../../src/platform/platform';
import * as api from '../../../src/protos/firestore_proto_api';
import { Connection, Stream } from '../../../src/remote/connection';
Expand All @@ -69,7 +70,10 @@ import { ExistenceFilter } from '../../../src/remote/existence_filter';
import { WriteRequest } from '../../../src/remote/persistent_stream';
import { RemoteStore } from '../../../src/remote/remote_store';
import { mapCodeFromRpcCode } from '../../../src/remote/rpc_error';
import { JsonProtoSerializer } from '../../../src/remote/serializer';
import {
JsonProtoSerializer,
TimestampValue
} from '../../../src/remote/serializer';
import { StreamBridge } from '../../../src/remote/stream_bridge';
import {
DocumentWatchChange,
Expand Down Expand Up @@ -196,7 +200,10 @@ class MockConnection implements Connection {
return this.watchOpen.promise;
}

ackWrite(commitTime?: string, mutationResults?: api.WriteResult[]): void {
ackWrite(
commitTime?: TimestampValue,
mutationResults?: api.WriteResult[]
): void {
this.writeStream!.callOnMessage({
// Convert to base64 string so it can later be parsed into ByteString.
streamToken: PlatformSupport.getPlatform().btoa(
Expand Down Expand Up @@ -1151,11 +1158,17 @@ abstract class TestRunner {
expect(actualTarget.query).to.deep.equal(expectedTarget.query);
expect(actualTarget.targetId).to.equal(expectedTarget.targetId);
expect(actualTarget.readTime).to.equal(expectedTarget.readTime);
// actualTarget's resumeToken is a string, but the serialized
// resumeToken will be a base64 string, so we need to convert it back.
expect(actualTarget.resumeToken || '').to.equal(
this.platform.atob(expectedTarget.resumeToken || '')
);
if (expectedTarget.resumeToken !== undefined) {
// actualTarget's resumeToken is a string, but the serialized
// resumeToken will be a base64 string, so we need to convert it back.
expect(actualTarget.resumeToken).to.equal(
this.platform.atob(
normalizeByteString(expectedTarget.resumeToken).toBase64()
)
);
} else {
expect(actualTarget.resumeToken).to.be.undefined;
}
delete actualTargets[targetId];
});
expect(obj.size(actualTargets)).to.equal(
Expand Down