fix: fixed converter issue

- fixed issue where the incorrect data converter would be used if a converterFactory was expected to be used. - for SystemState items, this meant that updates would use the pass through value instead of the proper converter
dereekb · Dec 31, 2022 · de8874d · de8874d
1 parent d34515c
commit de8874d
Show file tree

Hide file tree

Showing 7 changed files with 162 additions and 20 deletions.
diff --git a/packages/firebase/src/lib/common/firestore/accessor/converter.ts b/packages/firebase/src/lib/common/firestore/accessor/converter.ts
@@ -6,6 +6,13 @@ import { DocumentReference, FirestoreDataConverter } from '../types';
  */
 export type FirestoreDataConverterFactory<T> = FactoryWithInput<FirestoreDataConverter<T>, DocumentReference<T>>;
 
+/**
+ * Ref to a FirestoreDataConverterFactory.
+ */
+export interface FirestoreDataConverterFactoryRef<T> {
+  converterFactory: FirestoreDataConverterFactory<T>;
+}
+
 /**
  * Factory used to provide an optional custom FirestoreDataConverter based on the input reference.
  */

diff --git a/packages/firebase/src/lib/common/firestore/accessor/document.ts b/packages/firebase/src/lib/common/firestore/accessor/document.ts
@@ -13,7 +13,7 @@ import { build, Building, Maybe } from '@dereekb/util';
 import { FirestoreModelTypeRef, FirestoreModelIdentity, FirestoreModelTypeModelIdentityRef } from '../collection/collection';
 import { InterceptAccessorFactoryFunction } from './accessor.wrap';
 import { incrementUpdateWithAccessorFunction } from './increment';
-import { FirestoreDataConverterFactory, InterceptFirestoreDataConverterFactory } from './converter';
+import { FirestoreDataConverterFactory, FirestoreDataConverterFactoryRef, InterceptFirestoreDataConverterFactory } from './converter';
 
 export interface FirestoreDocument<T, I extends FirestoreModelIdentity = FirestoreModelIdentity> extends FirestoreDataConverterRef<T>, DocumentReferenceRef<T>, CollectionReferenceRef<T>, FirestoreModelIdentityRef<I>, FirestoreModelTypeRef<FirestoreModelIdentityModelType<I>>, FirestoreCollectionNameRef<FirestoreModelIdentityCollectionName<I>>, FirestoreModelKeyRef, FirestoreModelIdRef {
   readonly accessor: FirestoreDocumentDataAccessor<T>;
@@ -69,7 +69,7 @@ export abstract class AbstractFirestoreDocument<T, D extends AbstractFirestoreDo
   }
 
   get converter(): FirestoreDataConverter<T> {
-    return this.documentAccessor.converter;
+    return this.documentAccessor.converterFactory(this.documentRef);
   }
 
   /**
@@ -189,6 +189,11 @@ export interface LimitedFirestoreDocumentAccessor<T, D extends FirestoreDocument
    */
   documentRefForKey(fullPath: FirestoreModelKey): DocumentReference<T>;
 
+  /**
+   * This is the default converter. Be sure to use the converterFactory instead when attempting to get the accurate converter for a document.
+   */
+  readonly converter: FirestoreDataConverter<T>;
+
   /**
    * Returns the converter factory for this accessor.
    */
@@ -228,7 +233,7 @@ export type FirestoreDocumentFactoryFunction<T, D extends FirestoreDocument<T> =
 /**
  * Factory function used for creating a FirestoreDocumentAccessor.
  */
-export type LimitedFirestoreDocumentAccessorFactoryFunction<T, D extends FirestoreDocument<T> = FirestoreDocument<T>, A extends LimitedFirestoreDocumentAccessor<T, D> = LimitedFirestoreDocumentAccessor<T, D>> = (context?: FirestoreDocumentContext<T>) => A;
+export type LimitedFirestoreDocumentAccessorFactoryFunction<T, D extends FirestoreDocument<T> = FirestoreDocument<T>, A extends LimitedFirestoreDocumentAccessor<T, D> = LimitedFirestoreDocumentAccessor<T, D>> = ((context?: FirestoreDocumentContext<T>) => A) & FirestoreDataConverterFactoryRef<T>;
 
 /**
  * Factory type used for creating a FirestoreDocumentAccessor.
@@ -258,11 +263,11 @@ export interface LimitedFirestoreDocumentAccessorFactoryConfig<T, D extends Fire
 }
 
 export function limitedFirestoreDocumentAccessorFactory<T, D extends FirestoreDocument<T> = FirestoreDocument<T>>(config: LimitedFirestoreDocumentAccessorFactoryConfig<T, D>): LimitedFirestoreDocumentAccessorFactoryFunction<T, D> {
-  const { firestoreContext, firestoreAccessorDriver, makeDocument, accessorFactory: interceptAccessorFactory, converter: inputConverter, converterFactory: inputConverterFactory, modelIdentity } = config;
+  const { firestoreContext, firestoreAccessorDriver, makeDocument, accessorFactory: interceptAccessorFactory, converter: inputDefaultConverter, converterFactory: inputConverterFactory, modelIdentity } = config;
   const expectedCollectionName = firestoreAccessorDriver.fuzzedPathForPath ? firestoreAccessorDriver.fuzzedPathForPath(modelIdentity.collectionName) : modelIdentity.collectionName;
-  const converterFactory: FirestoreDataConverterFactory<T> = inputConverterFactory ? (ref) => (ref ? inputConverterFactory(ref) ?? inputConverter : inputConverter) : () => inputConverter;
+  const converterFactory: FirestoreDataConverterFactory<T> = inputConverterFactory ? (ref) => (ref ? inputConverterFactory(ref) ?? inputDefaultConverter : inputDefaultConverter) : () => inputDefaultConverter;
 
-  return (context?: FirestoreDocumentContext<T>) => {
+  const result = ((context?: FirestoreDocumentContext<T>) => {
     const databaseContext: FirestoreDocumentContext<T> = context ?? config.firestoreAccessorDriver.defaultContextFactory();
     const dataAccessorFactory = interceptAccessorFactory ? interceptAccessorFactory(databaseContext.accessorFactory) : databaseContext.accessorFactory;
 
@@ -293,7 +298,7 @@ export function limitedFirestoreDocumentAccessorFactory<T, D extends FirestoreDo
     }
 
     const documentAccessor: LimitedFirestoreDocumentAccessor<T, D> = {
-      converter: inputConverter,
+      converter: inputDefaultConverter,
       converterFactory,
       modelIdentity,
       loadDocumentFrom(document: FirestoreDocument<T>): D {
@@ -307,7 +312,9 @@ export function limitedFirestoreDocumentAccessorFactory<T, D extends FirestoreDo
     };
 
     return documentAccessor;
-  };
+  }) as Building<LimitedFirestoreDocumentAccessorFactoryFunction<T, D>>;
+  result.converterFactory = converterFactory;
+  return result as LimitedFirestoreDocumentAccessorFactoryFunction<T, D>;
 }
 
 // MARK: FirestoreDocumentAccessor
@@ -329,14 +336,16 @@ export interface FirestoreDocumentAccessorFactoryConfig<T, D extends FirestoreDo
 }
 
 export function firestoreDocumentAccessorFactory<T, D extends FirestoreDocument<T> = FirestoreDocument<T>>(config: FirestoreDocumentAccessorFactoryConfig<T, D>): FirestoreDocumentAccessorFactoryFunction<T, D> {
-  const { firestoreAccessorDriver, collection, converter } = config;
+  const { firestoreAccessorDriver, collection } = config;
   const limitedFirestoreDocumentAccessor = limitedFirestoreDocumentAccessorFactory(config);
 
   function documentRefForId(path: string): DocumentReference<T> {
-    return firestoreAccessorDriver.doc(collection, path).withConverter(converter);
+    const ref = firestoreAccessorDriver.doc(collection, path);
+    const converter = limitedFirestoreDocumentAccessor.converterFactory(ref);
+    return ref.withConverter(converter);
   }
 
-  return (context?: FirestoreDocumentContext<T>) => {
+  const result = ((context?: FirestoreDocumentContext<T>) => {
     const documentAccessor: FirestoreDocumentAccessor<T, D> = build<FirestoreDocumentAccessor<T, D>>({
       base: limitedFirestoreDocumentAccessor(context),
       build: (x) => {
@@ -360,7 +369,9 @@ export function firestoreDocumentAccessorFactory<T, D extends FirestoreDocument<
     });
 
     return documentAccessor;
-  };
+  }) as Building<FirestoreDocumentAccessorFactoryFunction<T, D>>;
+  result.converterFactory = limitedFirestoreDocumentAccessor.converterFactory;
+  return result as FirestoreDocumentAccessorFactoryFunction<T, D>;
 }
 
 // MARK: Extension

diff --git a/packages/firebase/src/lib/common/firestore/snapshot/snapshot.spec.ts b/packages/firebase/src/lib/common/firestore/snapshot/snapshot.spec.ts
@@ -2,23 +2,51 @@ import { asGetter } from '@dereekb/util';
 import { isDate } from 'date-fns';
 import { DocumentSnapshot } from '../types';
 import { snapshotConverterFunctions } from './snapshot';
-import { firestoreBoolean, firestoreDate, firestoreNumber, firestoreString, firestoreUniqueStringArray } from './snapshot.field';
+import { firestoreBoolean, firestoreDate, firestoreNumber, firestoreObjectArray, firestoreString, firestoreSubObject, firestoreUniqueStringArray } from './snapshot.field';
+
+export interface TestSnapshotDefaultsEmbeddedDefaults {
+  date: Date;
+  defaultDate: Date;
+  number: number;
+  numberWithStore: number;
+  uniqueStringArray: string[];
+  uniqueStringArrayWithDefaultValue: string[];
+}
+
+export const testSnapshotDefaultsEmbeddedDefaults = firestoreSubObject<TestSnapshotDefaultsEmbeddedDefaults>({
+  objectField: {
+    fields: {
+      date: firestoreDate({ saveDefaultAsNow: true }),
+      defaultDate: firestoreDate({}),
+      number: firestoreNumber({ default: 0 }),
+      numberWithStore: firestoreNumber({ default: 0, saveDefault: true }),
+      uniqueStringArray: firestoreUniqueStringArray(),
+      uniqueStringArrayWithDefaultValue: firestoreUniqueStringArray({ default: () => ['test'] })
+    }
+  }
+});
 
 export interface TestSnapshotDefaults {
   date: Date;
+  defaultDate: Date;
   number: number;
   numberWithStore: number;
   uniqueStringArray: string[];
   uniqueStringArrayWithDefaultValue: string[];
+  embedded: TestSnapshotDefaultsEmbeddedDefaults[];
 }
 
 export const testSnapshotDefaultsConverter = snapshotConverterFunctions<TestSnapshotDefaults>({
   fields: {
     date: firestoreDate({ saveDefaultAsNow: true }),
+    defaultDate: firestoreDate({}),
     number: firestoreNumber({ default: 0 }),
     numberWithStore: firestoreNumber({ default: 0, saveDefault: true }),
     uniqueStringArray: firestoreUniqueStringArray(),
-    uniqueStringArrayWithDefaultValue: firestoreUniqueStringArray({ default: () => ['test'] })
+    uniqueStringArrayWithDefaultValue: firestoreUniqueStringArray({ default: () => ['test'] }),
+    embedded: firestoreObjectArray({
+      objectField: testSnapshotDefaultsEmbeddedDefaults
+    })
   }
 });
 
@@ -103,11 +131,18 @@ describe('snapshotConverterFunctions()', () => {
         expect(x.b).not.toBeDefined();
         expect(x.c).not.toBeDefined();
 
-        expect(Object.keys(x).length).toBe(5);
+        expect(Object.keys(x).length).toBe(7);
       });
     });
 
     describe('to() - to data', () => {
+      it('should apply the default value from date if null values are passed', () => {
+        const result = testSnapshotDefaultsConverter.to({ date: null } as any);
+        expect(result.defaultDate).toBeNull(); // saves nothing
+        expect(result.date).not.toBeNull();
+        expect(typeof result.date).toBe('string');
+      });
+
       it('should apply the beforeSaveDefault from uniqueStringArray if an empty object is passed', () => {
         const result = testSnapshotDefaultsConverter.to({} as any);
 
@@ -122,6 +157,17 @@ describe('snapshotConverterFunctions()', () => {
         expect(typeof result.date).toBe('string');
         expect(result.uniqueStringArray).toBeNull();
       });
+
+      describe('embedded', () => {
+        const result = testSnapshotDefaultsConverter.to({ embedded: [{}] } as any);
+        expect(result.embedded.length).toBe(1);
+
+        const first = result.embedded[0];
+
+        expect(result.defaultDate).toBeNull();
+        expect(typeof first.date).toBe('string');
+        expect(first.uniqueStringArray).toBeNull();
+      });
     });
   });
 });
diff --git a/packages/firebase/test/src/lib/client/firestore.mock.item.spec.ts b/packages/firebase/test/src/lib/client/firestore.mock.item.spec.ts
@@ -1,4 +1,7 @@
+import { isDate } from 'date-fns';
+import { DocumentData, SystemState, SystemStateDocument } from '@dereekb/firebase';
 import { CollectionReference, doc, getDoc, setDoc } from '@firebase/firestore';
+import { MockSystemData, MOCK_SYSTEM_STATE_TYPE } from '../common/mock/mock.item';
 import { MockItemCollectionFixture, testWithMockItemCollectionFixture } from '../common/mock/mock.item.collection.fixture';
 import { authorizedFirebaseFactory } from './firebase.authorized';
 
@@ -18,5 +21,39 @@ describe('testWithMockItemFixture', () => {
       expect(snapshot).toBeDefined();
       expect(snapshot.exists()).toBe(true);
     });
+
+    describe('mock item system state', () => {
+      it('should save the system state type using the correct converter based off of the id type.', async () => {
+        const systemStateDocument = f.instance.mockItemSystemState.documentAccessor().loadDocumentForId(MOCK_SYSTEM_STATE_TYPE) as SystemStateDocument<MockSystemData>;
+        await systemStateDocument.create({ data: {} as MockSystemData });
+
+        let result = (await systemStateDocument.snapshotData()) as SystemState<MockSystemData>;
+
+        expect(result.data.lat).toBeDefined();
+        expect(isDate(result.data.lat)).toBe(true); // should set the default date
+
+        let rawData = (await systemStateDocument.accessor.getWithConverter(null).then((x) => x.data())) as DocumentData;
+        expect(rawData.data.lat).toBeDefined();
+        expect(typeof rawData.data.lat).toBe('string');
+
+        // then update it and pass the same data
+        await systemStateDocument.update({
+          data: {
+            ...result.data
+          }
+        });
+
+        result = (await systemStateDocument.snapshotData()) as SystemState<MockSystemData>;
+
+        expect(result.data.lat).toBeDefined();
+        expect(isDate(result.data.lat)).toBe(true); // should still be a date
+
+        // raw data should have saved a string still
+        rawData = (await systemStateDocument.accessor.getWithConverter(null).then((x) => x.data())) as DocumentData;
+
+        expect(rawData.data.lat).toBeDefined();
+        expect(typeof rawData.data.lat).toBe('string');
+      });
+    });
   });
 });
diff --git a/packages/firebase/test/src/lib/common/mock/mock.item.collection.fixture.ts b/packages/firebase/test/src/lib/common/mock/mock.item.collection.fixture.ts
@@ -47,6 +47,10 @@ export class MockItemCollectionFixtureInstance {
     return this.collections.mockItemSubItemDeepCollectionGroup;
   }
 
+  get mockItemSystemState() {
+    return this.collections.mockItemSystemStateCollection;
+  }
+
   constructor(readonly fixture: MockItemCollectionFixture) {}
 }
 

diff --git a/packages/firebase/test/src/lib/common/mock/mock.item.service.ts b/packages/firebase/test/src/lib/common/mock/mock.item.service.ts
@@ -32,9 +32,10 @@ import {
   MockItemUserFirestoreCollectionFactory,
   mockItemUserFirestoreCollectionGroup,
   MockItemUserFirestoreCollectionGroup,
-  MockItemUserRoles
+  MockItemUserRoles,
+  mockItemSystemStateStoredDataConverterMap
 } from './mock.item';
-import { FirebaseAppModelContext, FirebasePermissionServiceModel, firebaseModelServiceFactory, firebaseModelsService, FirestoreContext } from '@dereekb/firebase';
+import { FirebaseAppModelContext, FirebasePermissionServiceModel, firebaseModelServiceFactory, firebaseModelsService, FirestoreContext, SystemStateFirestoreCollection, systemStateFirestoreCollection, grantFullAccessIfAdmin, SystemState, SystemStateDocument, SystemStateRoles, SystemStateTypes } from '@dereekb/firebase';
 import { GrantedRoleMap } from '@dereekb/model';
 import { PromiseOrValue } from '@dereekb/util';
 
@@ -49,6 +50,7 @@ export abstract class MockItemCollections {
   abstract readonly mockItemSubItemCollectionGroup: MockItemSubItemFirestoreCollectionGroup;
   abstract readonly mockItemSubItemDeepCollectionFactory: MockItemSubItemDeepFirestoreCollectionFactory;
   abstract readonly mockItemSubItemDeepCollectionGroup: MockItemSubItemDeepFirestoreCollectionGroup;
+  abstract readonly mockItemSystemStateCollection: SystemStateFirestoreCollection;
 }
 
 export function makeMockItemCollections(firestoreContext: FirestoreContext): MockItemCollections {
@@ -61,7 +63,8 @@ export function makeMockItemCollections(firestoreContext: FirestoreContext): Moc
     mockItemSubItemCollectionFactory: mockItemSubItemFirestoreCollection(firestoreContext),
     mockItemSubItemCollectionGroup: mockItemSubItemFirestoreCollectionGroup(firestoreContext),
     mockItemSubItemDeepCollectionFactory: mockItemSubItemDeepFirestoreCollection(firestoreContext),
-    mockItemSubItemDeepCollectionGroup: mockItemSubItemDeepFirestoreCollectionGroup(firestoreContext)
+    mockItemSubItemDeepCollectionGroup: mockItemSubItemDeepFirestoreCollectionGroup(firestoreContext),
+    mockItemSystemStateCollection: systemStateFirestoreCollection(firestoreContext, mockItemSystemStateStoredDataConverterMap)
   };
 }
 
@@ -109,8 +112,15 @@ export const mockItemSubItemDeepFirebaseModelServiceFactory = firebaseModelServi
   getFirestoreCollection: (c) => c.app.mockItemSubItemDeepCollectionGroup
 });
 
+export const mockItemSystemStateFirebaseModelServiceFactory = firebaseModelServiceFactory<MockFirebaseContext, SystemState, SystemStateDocument, SystemStateRoles>({
+  roleMapForModel: function (output: FirebasePermissionServiceModel<SystemState, SystemStateDocument>, context: MockFirebaseContext, model: SystemStateDocument): PromiseOrValue<GrantedRoleMap<SystemStateRoles>> {
+    return grantFullAccessIfAdmin(context); // only sys-admin allowed
+  },
+  getFirestoreCollection: (c) => c.app.mockItemSystemStateCollection
+});
+
 // MARK: Model Service
-export type MockModelTypes = MockItemTypes;
+export type MockModelTypes = SystemStateTypes | MockItemTypes;
 
 export type MockFirebaseContextAppContext = MockItemCollections;
 
@@ -122,6 +132,7 @@ export type MockFirebaseBaseContext = FirebaseAppModelContext<MockFirebaseContex
 };
 
 export const MOCK_FIREBASE_MODEL_SERVICE_FACTORIES = {
+  systemState: mockItemSystemStateFirebaseModelServiceFactory,
   mockItem: mockItemFirebaseModelServiceFactory,
   mockItemPrivate: mockItemPrivateFirebaseModelServiceFactory,
   mockItemUser: mockItemUserFirebaseModelServiceFactory,

diff --git a/packages/firebase/test/src/lib/common/mock/mock.item.ts b/packages/firebase/test/src/lib/common/mock/mock.item.ts
@@ -24,7 +24,11 @@ import {
   firestoreUID,
   firestoreUniqueStringArray,
   optionalFirestoreArray,
-  optionalFirestoreDate
+  optionalFirestoreDate,
+  firestoreSubObject,
+  SystemStateStoredData,
+  SystemStateStoredDataConverterMap,
+  SystemStateStoredDataFieldConverterConfig
 } from '@dereekb/firebase';
 import { GrantedReadRole } from '@dereekb/model';
 
@@ -435,3 +439,25 @@ export function mockItemSubItemDeepFirestoreCollectionGroup(firestoreContext: Fi
     firestoreContext
   });
 }
+
+// MARK: Mock System Item
+export const MOCK_SYSTEM_STATE_TYPE = 'mockitemsystemstate';
+
+export interface MockSystemData extends SystemStateStoredData {
+  /**
+   * Last updated at
+   */
+  lat: Date;
+}
+
+export const mockItemSystemDataConverter: SystemStateStoredDataFieldConverterConfig<MockSystemData> = firestoreSubObject<MockSystemData>({
+  objectField: {
+    fields: {
+      lat: firestoreDate({ saveDefaultAsNow: true })
+    }
+  }
+});
+
+export const mockItemSystemStateStoredDataConverterMap: SystemStateStoredDataConverterMap = {
+  [MOCK_SYSTEM_STATE_TYPE]: mockItemSystemDataConverter
+};