Skip to content

Commit 6ac66ba

Browse files
dconeybedconeybe
authored
Fix FirestoreDataConverter.fromFirestore() being called with QueryDocumentSnapshot from exp (#4284)
1 parent 818a961 commit 6ac66ba

File tree

6 files changed

+194
-19
lines changed

6 files changed

+194
-19
lines changed

‎.changeset/neat-yaks-kneel.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@firebase/firestore': patch
3+
---
4+
5+
Fixes FirestoreDataConverter.fromFirestore() being called with an incorrect "snapshot" object.

‎integration/firestore/firebase_export.ts

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,22 @@ export function usesFunctionalApi(): false {
5050
return false;
5151
}
5252

53-
const Firestore = firebase.firestore.Firestore;
53+
const Blob = firebase.firestore.Blob;
54+
const DocumentReference = firebase.firestore.DocumentReference;
5455
const FieldPath = firebase.firestore.FieldPath;
55-
const Timestamp = firebase.firestore.Timestamp;
56-
const GeoPoint = firebase.firestore.GeoPoint;
5756
const FieldValue = firebase.firestore.FieldValue;
58-
const Blob = firebase.firestore.Blob;
57+
const Firestore = firebase.firestore.Firestore;
58+
const GeoPoint = firebase.firestore.GeoPoint;
59+
const QueryDocumentSnapshot = firebase.firestore.QueryDocumentSnapshot;
60+
const Timestamp = firebase.firestore.Timestamp;
5961

60-
export { Firestore, FieldValue, FieldPath, Timestamp, Blob, GeoPoint };
62+
export {
63+
Blob,
64+
DocumentReference,
65+
FieldPath,
66+
FieldValue,
67+
Firestore,
68+
GeoPoint,
69+
QueryDocumentSnapshot,
70+
Timestamp
71+
};

‎integration/firestore/firebase_export_memory.ts

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,22 @@ export function usesFunctionalApi(): false {
5050
return false;
5151
}
5252

53-
const Firestore = firebase.firestore.Firestore;
53+
const Blob = firebase.firestore.Blob;
54+
const DocumentReference = firebase.firestore.DocumentReference;
5455
const FieldPath = firebase.firestore.FieldPath;
55-
const Timestamp = firebase.firestore.Timestamp;
56-
const GeoPoint = firebase.firestore.GeoPoint;
5756
const FieldValue = firebase.firestore.FieldValue;
58-
const Blob = firebase.firestore.Blob;
57+
const Firestore = firebase.firestore.Firestore;
58+
const GeoPoint = firebase.firestore.GeoPoint;
59+
const QueryDocumentSnapshot = firebase.firestore.QueryDocumentSnapshot;
60+
const Timestamp = firebase.firestore.Timestamp;
5961

60-
export { Firestore, FieldValue, FieldPath, Timestamp, Blob, GeoPoint };
62+
export {
63+
Blob,
64+
DocumentReference,
65+
FieldPath,
66+
FieldValue,
67+
Firestore,
68+
GeoPoint,
69+
QueryDocumentSnapshot,
70+
Timestamp
71+
};

‎packages/firestore/src/api/database.ts

Lines changed: 84 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ import {
9999
import {
100100
DocumentChange as ExpDocumentChange,
101101
DocumentSnapshot as ExpDocumentSnapshot,
102+
QueryDocumentSnapshot as ExpQueryDocumentSnapshot,
102103
QuerySnapshot as ExpQuerySnapshot,
103104
snapshotEqual,
104105
SnapshotMetadata
@@ -552,6 +553,86 @@ export class WriteBatch
552553
}
553554
}
554555

556+
/**
557+
* Wraps a `PublicFirestoreDataConverter` translating the types from the
558+
* experimental SDK into corresponding types from the Classic SDK before passing
559+
* them to the wrapped converter.
560+
*/
561+
class FirestoreDataConverter<U>
562+
extends Compat<PublicFirestoreDataConverter<U>>
563+
implements UntypedFirestoreDataConverter<U> {
564+
private static readonly INSTANCES = new WeakMap();
565+
566+
private constructor(
567+
private readonly _firestore: Firestore,
568+
private readonly _userDataWriter: UserDataWriter,
569+
delegate: PublicFirestoreDataConverter<U>
570+
) {
571+
super(delegate);
572+
}
573+
574+
fromFirestore(
575+
snapshot: ExpQueryDocumentSnapshot,
576+
options?: PublicSnapshotOptions
577+
): U {
578+
const expSnapshot = new ExpQueryDocumentSnapshot(
579+
this._firestore._delegate,
580+
this._userDataWriter,
581+
snapshot._key,
582+
snapshot._document,
583+
snapshot.metadata,
584+
/* converter= */ null
585+
);
586+
return this._delegate.fromFirestore(
587+
new QueryDocumentSnapshot(this._firestore, expSnapshot),
588+
options ?? {}
589+
);
590+
}
591+
592+
toFirestore(modelObject: U): PublicDocumentData;
593+
toFirestore(
594+
modelObject: Partial<U>,
595+
options: PublicSetOptions
596+
): PublicDocumentData;
597+
toFirestore(
598+
modelObject: U | Partial<U>,
599+
options?: PublicSetOptions
600+
): PublicDocumentData {
601+
if (!options) {
602+
return this._delegate.toFirestore(modelObject as U);
603+
} else {
604+
return this._delegate.toFirestore(modelObject, options);
605+
}
606+
}
607+
608+
// Use the same instance of `FirestoreDataConverter` for the given instances
609+
// of `Firestore` and `PublicFirestoreDataConverter` so that isEqual() will
610+
// compare equal for two objects created with the same converter instance.
611+
static getInstance<U>(
612+
firestore: Firestore,
613+
converter: PublicFirestoreDataConverter<U>
614+
): FirestoreDataConverter<U> {
615+
const converterMapByFirestore = FirestoreDataConverter.INSTANCES;
616+
let untypedConverterByConverter = converterMapByFirestore.get(firestore);
617+
if (!untypedConverterByConverter) {
618+
untypedConverterByConverter = new WeakMap();
619+
converterMapByFirestore.set(firestore, untypedConverterByConverter);
620+
}
621+
622+
let instance = untypedConverterByConverter.get(converter);
623+
if (!instance) {
624+
instance = new FirestoreDataConverter(
625+
firestore,
626+
new UserDataWriter(firestore),
627+
converter
628+
);
629+
untypedConverterByConverter.set(converter, instance);
630+
}
631+
632+
return instance;
633+
}
634+
}
635+
555636
/**
556637
* A reference to a particular document in a collection in the database.
557638
*/
@@ -753,7 +834,7 @@ export class DocumentReference<T = PublicDocumentData>
753834
return new DocumentReference<U>(
754835
this.firestore,
755836
this._delegate.withConverter(
756-
converter as UntypedFirestoreDataConverter<U>
837+
FirestoreDataConverter.getInstance(this.firestore, converter)
757838
)
758839
);
759840
}
@@ -1062,7 +1143,7 @@ export class Query<T = PublicDocumentData>
10621143
return new Query<U>(
10631144
this.firestore,
10641145
this._delegate.withConverter(
1065-
converter as UntypedFirestoreDataConverter<U>
1146+
FirestoreDataConverter.getInstance(this.firestore, converter)
10661147
)
10671148
);
10681149
}
@@ -1206,7 +1287,7 @@ export class CollectionReference<T = PublicDocumentData>
12061287
return new CollectionReference<U>(
12071288
this.firestore,
12081289
this._delegate.withConverter(
1209-
converter as UntypedFirestoreDataConverter<U>
1290+
FirestoreDataConverter.getInstance(this.firestore, converter)
12101291
)
12111292
);
12121293
}

‎packages/firestore/test/integration/api/database.test.ts

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ const newTestFirestore = firebaseExport.newTestFirestore;
3939
const Timestamp = firebaseExport.Timestamp;
4040
const FieldPath = firebaseExport.FieldPath;
4141
const FieldValue = firebaseExport.FieldValue;
42+
const DocumentReference = firebaseExport.DocumentReference;
43+
const QueryDocumentSnapshot = firebaseExport.QueryDocumentSnapshot;
4244

4345
const MEMORY_ONLY_BUILD =
4446
typeof process !== 'undefined' &&
@@ -1326,7 +1328,11 @@ apiDescribe('Database', (persistence: boolean) => {
13261328
// only to web.
13271329
apiDescribe('withConverter() support', (persistence: boolean) => {
13281330
class Post {
1329-
constructor(readonly title: string, readonly author: string) {}
1331+
constructor(
1332+
readonly title: string,
1333+
readonly author: string,
1334+
readonly ref: firestore.DocumentReference | null = null
1335+
) {}
13301336
byline(): string {
13311337
return this.title + ', by ' + this.author;
13321338
}
@@ -1340,8 +1346,9 @@ apiDescribe('Database', (persistence: boolean) => {
13401346
snapshot: firestore.QueryDocumentSnapshot,
13411347
options: firestore.SnapshotOptions
13421348
): Post {
1349+
expect(snapshot).to.be.an.instanceof(QueryDocumentSnapshot);
13431350
const data = snapshot.data(options);
1344-
return new Post(data.title, data.author);
1351+
return new Post(data.title, data.author, snapshot.ref);
13451352
}
13461353
};
13471354

@@ -1369,7 +1376,7 @@ apiDescribe('Database', (persistence: boolean) => {
13691376
options: firestore.SnapshotOptions
13701377
): Post {
13711378
const data = snapshot.data();
1372-
return new Post(data.title, data.author);
1379+
return new Post(data.title, data.author, snapshot.ref);
13731380
}
13741381
};
13751382

@@ -1552,7 +1559,7 @@ apiDescribe('Database', (persistence: boolean) => {
15521559
expect(options).to.deep.equal({ serverTimestamps: 'estimate' });
15531560

15541561
const data = snapshot.data(options);
1555-
return new Post(data.title, data.author);
1562+
return new Post(data.title, data.author, snapshot.ref);
15561563
}
15571564
});
15581565

@@ -1590,6 +1597,53 @@ apiDescribe('Database', (persistence: boolean) => {
15901597
expect(docRef.isEqual(docRef2)).to.be.false;
15911598
});
15921599
});
1600+
1601+
it('Correct snapshot specified to fromFirestore() when registered with DocumentReference', () => {
1602+
return withTestDb(persistence, async db => {
1603+
const untypedDocRef = db.collection('/models').doc();
1604+
const docRef = untypedDocRef.withConverter(postConverter);
1605+
await docRef.set(new Post('post', 'author'));
1606+
const docSnapshot = await docRef.get();
1607+
const ref = docSnapshot.data()!.ref!;
1608+
expect(ref).to.be.an.instanceof(DocumentReference);
1609+
expect(untypedDocRef.isEqual(ref)).to.be.true;
1610+
});
1611+
});
1612+
1613+
it('Correct snapshot specified to fromFirestore() when registered with CollectionReference', () => {
1614+
return withTestDb(persistence, async db => {
1615+
const untypedCollection = db
1616+
.collection('/models')
1617+
.doc()
1618+
.collection('sub');
1619+
const collection = untypedCollection.withConverter(postConverter);
1620+
const docRef = collection.doc();
1621+
await docRef.set(new Post('post', 'author', docRef));
1622+
const querySnapshot = await collection.get();
1623+
expect(querySnapshot.size).to.equal(1);
1624+
const ref = querySnapshot.docs[0].data().ref!;
1625+
expect(ref).to.be.an.instanceof(DocumentReference);
1626+
const untypedDocRef = untypedCollection.doc(docRef.id);
1627+
expect(untypedDocRef.isEqual(ref)).to.be.true;
1628+
});
1629+
});
1630+
1631+
it('Correct snapshot specified to fromFirestore() when registered with Query', () => {
1632+
return withTestDb(persistence, async db => {
1633+
const untypedCollection = db.collection('/models');
1634+
const untypedDocRef = untypedCollection.doc();
1635+
const docRef = untypedDocRef.withConverter(postConverter);
1636+
await docRef.set(new Post('post', 'author', docRef));
1637+
const query = untypedCollection
1638+
.where(FieldPath.documentId(), '==', docRef.id)
1639+
.withConverter(postConverter);
1640+
const querySnapshot = await query.get();
1641+
expect(querySnapshot.size).to.equal(1);
1642+
const ref = querySnapshot.docs[0].data().ref!;
1643+
expect(ref).to.be.an.instanceof(DocumentReference);
1644+
expect(untypedDocRef.isEqual(ref)).to.be.true;
1645+
});
1646+
});
15931647
});
15941648

15951649
it('can set and get data with auto detect long polling enabled', () => {

‎packages/firestore/test/integration/util/firebase_export.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,11 @@ import { FirebaseApp } from '@firebase/app-types';
2525
import * as firestore from '@firebase/firestore-types';
2626

2727
import { Blob } from '../../../src/api/blob';
28-
import { Firestore } from '../../../src/api/database';
28+
import {
29+
Firestore,
30+
DocumentReference,
31+
QueryDocumentSnapshot
32+
} from '../../../src/api/database';
2933
import { FieldPath } from '../../../src/api/field_path';
3034
import { FieldValue } from '../../../src/api/field_value';
3135
import { GeoPoint } from '../../../src/api/geo_point';
@@ -68,4 +72,13 @@ export function newTestFirestore(
6872
return firestore;
6973
}
7074

71-
export { Firestore, FieldValue, FieldPath, Timestamp, Blob, GeoPoint };
75+
export {
76+
Firestore,
77+
FieldValue,
78+
FieldPath,
79+
Timestamp,
80+
Blob,
81+
GeoPoint,
82+
DocumentReference,
83+
QueryDocumentSnapshot
84+
};

0 commit comments

Comments
 (0)