Skip to content

Commit b9d8b2d

Browse files
fwalfwal
committed
fix: properly encode array field values
1 parent 5f7169a commit b9d8b2d

9 files changed

Lines changed: 147 additions & 96 deletions

File tree

‎packages/admin/src/lib/firestore/converter.spec.ts‎

Lines changed: 43 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,15 @@ import {
55
GeoPoint as AdminGeoPoint,
66
Timestamp as AdminTimestamp,
77
} from 'firebase-admin/firestore';
8-
import {
9-
fromFirestoreDocumentData,
10-
toFirestoreDocumentData,
11-
} from './converter.js';
8+
import { firestoreDecode, firestoreEncode } from './converter.js';
129
import { FirestoreSchema, FirestoreField } from 'effect-firebase';
1310

1411
describe('Firestore Converter', () => {
15-
describe('fromFirestoreDocumentData', () => {
12+
describe('firestoreDecode', () => {
1613
it('should convert Firestore Timestamp to FirestoreSchema.Timestamp', () => {
1714
const adminTimestamp = new AdminTimestamp(1705315800, 123000000);
1815

19-
const result = fromFirestoreDocumentData({
16+
const result = firestoreDecode({
2017
title: 'Test Post',
2118
createdAt: adminTimestamp,
2219
});
@@ -30,7 +27,7 @@ describe('Firestore Converter', () => {
3027
it('should handle nested timestamps', () => {
3128
const adminTimestamp = new AdminTimestamp(1705315800, 0);
3229

33-
const result = fromFirestoreDocumentData({
30+
const result = firestoreDecode({
3431
post: {
3532
createdAt: adminTimestamp,
3633
title: 'Nested',
@@ -43,7 +40,7 @@ describe('Firestore Converter', () => {
4340
it('should handle timestamps in arrays', () => {
4441
const adminTimestamp = new AdminTimestamp(1705315800, 0);
4542

46-
const result = fromFirestoreDocumentData({
43+
const result = firestoreDecode({
4744
timestamps: [adminTimestamp, adminTimestamp],
4845
});
4946

@@ -52,7 +49,7 @@ describe('Firestore Converter', () => {
5249
});
5350

5451
it('should preserve null and undefined values', () => {
55-
const result = fromFirestoreDocumentData({
52+
const result = firestoreDecode({
5653
nullValue: null,
5754
undefinedValue: undefined,
5855
stringValue: 'test',
@@ -64,18 +61,18 @@ describe('Firestore Converter', () => {
6461
});
6562

6663
it('should NOT convert null to Timestamp', () => {
67-
const result = fromFirestoreDocumentData({
64+
const result = firestoreDecode({
6865
createdAt: null,
6966
});
7067

7168
expect(result.createdAt).toBeNull();
7269
});
7370
});
7471

75-
describe('toFirestoreDocumentData', () => {
72+
describe('firestoreEncode', () => {
7673
it('should convert FirestoreSchema.Timestamp to Firestore Timestamp', () => {
7774
const fakeFirestore = {} as unknown as Firestore;
78-
const result = toFirestoreDocumentData(
75+
const result = firestoreEncode(
7976
fakeFirestore,
8077
FirestoreSchema.Timestamp.fromMillis(1705315800123)
8178
);
@@ -87,7 +84,7 @@ describe('Firestore Converter', () => {
8784

8885
it('should convert FirestoreSchema.GeoPoint to Firestore GeoPoint', () => {
8986
const fakeFirestore = {} as unknown as Firestore;
90-
const result = toFirestoreDocumentData(
87+
const result = firestoreEncode(
9188
fakeFirestore,
9289
new FirestoreSchema.GeoPoint({
9390
latitude: 55.6761,
@@ -110,7 +107,7 @@ describe('Firestore Converter', () => {
110107
},
111108
} as unknown as Firestore;
112109

113-
const result = toFirestoreDocumentData(
110+
const result = firestoreEncode(
114111
fakeFirestore,
115112
FirestoreSchema.Reference.makeFromPath('posts/post-1')
116113
);
@@ -121,7 +118,7 @@ describe('Firestore Converter', () => {
121118

122119
it('should convert ServerTimestamp to Firestore field value', () => {
123120
const fakeFirestore = {} as unknown as Firestore;
124-
const result = toFirestoreDocumentData(
121+
const result = firestoreEncode(
125122
fakeFirestore,
126123
FirestoreSchema.ServerTimestamp.make()
127124
);
@@ -131,17 +128,14 @@ describe('Firestore Converter', () => {
131128

132129
it('should convert Delete to Firestore field value', () => {
133130
const fakeFirestore = {} as unknown as Firestore;
134-
const result = toFirestoreDocumentData(
135-
fakeFirestore,
136-
FirestoreField.delete()
137-
);
131+
const result = firestoreEncode(fakeFirestore, FirestoreField.delete());
138132

139133
expect(result).toStrictEqual(FieldValue.delete());
140134
});
141135

142136
it('should convert ArrayUnion to arrayUnion FieldValue', () => {
143137
const fakeFirestore = {} as unknown as Firestore;
144-
const result = toFirestoreDocumentData(
138+
const result = firestoreEncode(
145139
fakeFirestore,
146140
FirestoreField.arrayUnion(['a', 'b'])
147141
);
@@ -150,19 +144,45 @@ describe('Firestore Converter', () => {
150144

151145
it('should convert ArrayRemove to arrayRemove FieldValue', () => {
152146
const fakeFirestore = {} as unknown as Firestore;
153-
const result = toFirestoreDocumentData(
147+
const result = firestoreEncode(
154148
fakeFirestore,
155149
FirestoreField.arrayRemove(['a'])
156150
);
157151
expect(result).toStrictEqual(FieldValue.arrayRemove('a'));
158152
});
159153

154+
it('should recursively encode values inside ArrayUnion', () => {
155+
const fakeFirestore = {} as unknown as Firestore;
156+
const ts = FirestoreSchema.Timestamp.fromMillis(1705315800000);
157+
const result = firestoreEncode(
158+
fakeFirestore,
159+
FirestoreField.arrayUnion([ts])
160+
);
161+
const expected = FieldValue.arrayUnion(
162+
AdminTimestamp.fromMillis(1705315800000)
163+
);
164+
expect(result).toStrictEqual(expected);
165+
});
166+
167+
it('should recursively encode values inside ArrayRemove', () => {
168+
const fakeFirestore = {} as unknown as Firestore;
169+
const ts = FirestoreSchema.Timestamp.fromMillis(1705315800000);
170+
const result = firestoreEncode(
171+
fakeFirestore,
172+
FirestoreField.arrayRemove([ts])
173+
);
174+
const expected = FieldValue.arrayRemove(
175+
AdminTimestamp.fromMillis(1705315800000)
176+
);
177+
expect(result).toStrictEqual(expected);
178+
});
179+
160180
it('should recursively convert nested objects and arrays', () => {
161181
const fakeFirestore = {
162182
doc: (path: string) => ({ path, __tag: 'fake-doc-ref' }),
163183
} as unknown as Firestore;
164184

165-
const result = toFirestoreDocumentData(fakeFirestore, {
185+
const result = firestoreEncode(fakeFirestore, {
166186
createdAt: FirestoreSchema.Timestamp.fromMillis(1705315800000),
167187
metadata: {
168188
location: new FirestoreSchema.GeoPoint({ latitude: 1, longitude: 2 }),
@@ -212,7 +232,7 @@ describe('Firestore Converter', () => {
212232
const adminGeoPoint = new AdminGeoPoint(10, 20);
213233
const adminDelete = FieldValue.delete();
214234

215-
const result = toFirestoreDocumentData(fakeFirestore, {
235+
const result = firestoreEncode(fakeFirestore, {
216236
timestamp: adminTimestamp,
217237
geoPoint: adminGeoPoint,
218238
delete: adminDelete,

‎packages/admin/src/lib/firestore/converter.ts‎

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,13 @@ import {
99
} from 'firebase-admin/firestore';
1010
import { FirestoreSchema, FirestoreField } from 'effect-firebase';
1111

12-
export const toFirestoreDocumentData = (
13-
db: Firestore,
14-
data: DocumentData
15-
): DocumentData => {
12+
/**
13+
* Encode a value to Firestore admin sdk format.
14+
* @param db The Firestore instance.
15+
* @param data The value to encode.
16+
* @returns The encoded value.
17+
*/
18+
export const firestoreEncode = (db: Firestore, data: unknown): unknown => {
1619
if (
1720
data === null ||
1821
data instanceof Timestamp ||
@@ -39,23 +42,32 @@ export const toFirestoreDocumentData = (
3942
return FieldValue.delete();
4043
}
4144
if (data instanceof FirestoreField.ArrayUnion) {
42-
return FieldValue.arrayUnion(...data.values);
45+
return FieldValue.arrayUnion(
46+
...data.values.map((v) => firestoreEncode(db, v))
47+
);
4348
}
4449
if (data instanceof FirestoreField.ArrayRemove) {
45-
return FieldValue.arrayRemove(...data.values);
50+
return FieldValue.arrayRemove(
51+
...data.values.map((v) => firestoreEncode(db, v))
52+
);
4653
}
4754
if (Array.isArray(data)) {
48-
return data.map((item) => toFirestoreDocumentData(db, item));
55+
return data.map((item) => firestoreEncode(db, item));
4956
}
5057
if (typeof data === 'object' && data !== null) {
5158
return Object.fromEntries(
52-
Object.entries(data).map(([k, v]) => [k, toFirestoreDocumentData(db, v)])
59+
Object.entries(data).map(([k, v]) => [k, firestoreEncode(db, v)])
5360
);
5461
}
5562
return data;
5663
};
5764

58-
export const fromFirestoreDocumentData = (data: DocumentData): DocumentData => {
65+
/**
66+
* Decode a value from Firestore admin sdk format.
67+
* @param data The value to decode.
68+
* @returns The decoded value.
69+
*/
70+
export const firestoreDecode = (data: DocumentData): DocumentData => {
5971
if (data instanceof Timestamp) {
6072
return FirestoreSchema.Timestamp.fromMillis(data.toMillis());
6173
}
@@ -69,11 +81,11 @@ export const fromFirestoreDocumentData = (data: DocumentData): DocumentData => {
6981
return FirestoreSchema.Reference.makeFromPath(data.path);
7082
}
7183
if (Array.isArray(data)) {
72-
return data.map(fromFirestoreDocumentData);
84+
return data.map(firestoreDecode);
7385
}
7486
if (typeof data === 'object' && data !== null) {
7587
return Object.fromEntries(
76-
Object.entries(data).map(([k, v]) => [k, fromFirestoreDocumentData(v)])
88+
Object.entries(data).map(([k, v]) => [k, firestoreDecode(v)])
7789
);
7890
}
7991
return data;
@@ -82,6 +94,7 @@ export const fromFirestoreDocumentData = (data: DocumentData): DocumentData => {
8294
export const makeConverter = (
8395
db: Firestore
8496
): FirestoreDataConverter<DocumentData, DocumentData> => ({
85-
toFirestore: (modelObject) => toFirestoreDocumentData(db, modelObject),
86-
fromFirestore: (snapshot) => fromFirestoreDocumentData(snapshot.data()),
97+
toFirestore: (modelObject) =>
98+
firestoreEncode(db, modelObject) as DocumentData,
99+
fromFirestore: (snapshot) => firestoreDecode(snapshot.data()),
87100
});

‎packages/admin/src/lib/firestore/firestore-service.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,10 @@ import type { Snapshot } from 'effect-firebase';
99
import { UnknownException } from 'effect/Cause';
1010
import { getFirestore, type Firestore } from 'firebase-admin/firestore';
1111
import { App } from '../app.js';
12-
import { fromFirestoreDocumentData, makeConverter } from './converter.js';
12+
import { firestoreDecode, makeConverter } from './converter.js';
1313
import { buildQuery } from './query-builder.js';
1414

15-
const packSnapshot = makeSnapshotPacker(fromFirestoreDocumentData);
15+
const packSnapshot = makeSnapshotPacker(firestoreDecode);
1616

1717
const mapError = (error: unknown) =>
1818
error instanceof Error

‎packages/admin/src/lib/firestore/query-builder.ts‎

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,10 @@ import {
22
Query,
33
CollectionReference,
44
Filter,
5-
DocumentData,
65
Firestore,
76
} from 'firebase-admin/firestore';
87
import type { QueryConstraint } from 'effect-firebase';
9-
import { toFirestoreDocumentData } from './converter.js';
10-
11-
/**
12-
* Helper to convert unknown value to DocumentData for Firestore.
13-
*/
14-
const convertValue = (db: Firestore, value: unknown): unknown =>
15-
toFirestoreDocumentData(db, value as DocumentData);
8+
import { firestoreEncode } from './converter.js';
169

1710
/**
1811
* Convert a single query constraint to Firebase Admin SDK query constraint.
@@ -27,7 +20,7 @@ const applyConstraint = (
2720
return query.where(
2821
constraint.field,
2922
constraint.op,
30-
convertValue(db, constraint.value)
23+
firestoreEncode(db, constraint.value)
3124
);
3225
case 'OrderBy':
3326
return query.orderBy(constraint.field, constraint.direction);
@@ -37,19 +30,19 @@ const applyConstraint = (
3730
return query.limitToLast(constraint.count);
3831
case 'StartAt':
3932
return query.startAt(
40-
...constraint.values.map((value) => convertValue(db, value))
33+
...constraint.values.map((value) => firestoreEncode(db, value))
4134
);
4235
case 'StartAfter':
4336
return query.startAfter(
44-
...constraint.values.map((value) => convertValue(db, value))
37+
...constraint.values.map((value) => firestoreEncode(db, value))
4538
);
4639
case 'EndAt':
4740
return query.endAt(
48-
...constraint.values.map((value) => convertValue(db, value))
41+
...constraint.values.map((value) => firestoreEncode(db, value))
4942
);
5043
case 'EndBefore':
5144
return query.endBefore(
52-
...constraint.values.map((value) => convertValue(db, value))
45+
...constraint.values.map((value) => firestoreEncode(db, value))
5346
);
5447
case 'And':
5548
return query.where(buildCompositeFilter(db, constraint));
@@ -84,7 +77,7 @@ const buildFilter = (db: Firestore, constraint: QueryConstraint): Filter => {
8477
return Filter.where(
8578
constraint.field,
8679
constraint.op,
87-
convertValue(db, constraint.value)
80+
firestoreEncode(db, constraint.value)
8881
);
8982
case 'And':
9083
return Filter.and(

‎packages/admin/src/lib/functions/decode-document-data.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { Effect, Schema } from 'effect';
2-
import { fromFirestoreDocumentData } from '../firestore/converter.js';
2+
import { firestoreDecode } from '../firestore/converter.js';
33

44
/**
55
* Decodes raw Firestore document data into a typed schema.
@@ -17,7 +17,7 @@ export const decodeDocumentData = <S extends Schema.Schema.Any>(
1717
schema: S,
1818
idField?: string
1919
): Effect.Effect<Schema.Schema.Type<S>, never, Schema.Schema.Context<S>> => {
20-
const convertedData = fromFirestoreDocumentData(rawData ?? {});
20+
const convertedData = firestoreDecode(rawData ?? {});
2121
const dataWithId = idField
2222
? { ...convertedData, [idField]: docId }
2323
: convertedData;

0 commit comments

Comments
 (0)