fix: always reload entity after update since cascading changes may ha…

…ve changed it since commit (#233)

packages/entity-full-integration-tests/src/__integration-tests__/EntityIntegrity-test.ts

@@ -0,0 +1,149 @@
+import {
+  EntityPrivacyPolicy,
+  ViewerContext,
+  AlwaysAllowPrivacyPolicyRule,
+  Entity,
+  EntityCompanionDefinition,
+  EntityConfiguration,
+  UUIDField,
+} from '@expo/entity';
+import { GenericRedisCacheContext } from '@expo/entity-cache-adapter-redis';
+import Redis from 'ioredis';
+import { knex, Knex } from 'knex';
+import nullthrows from 'nullthrows';
+import { URL } from 'url';
+import { v4 as uuidv4 } from 'uuid';
+
+import { createFullIntegrationTestEntityCompanionProvider } from '../testfixtures/createFullIntegrationTestEntityCompanionProvider';
+
+interface TestFields {
+  id: string;
+}
+
+class TestEntityPrivacyPolicy extends EntityPrivacyPolicy<
+  TestFields,
+  string,
+  ViewerContext,
+  TestEntity
+> {
+  protected override readonly readRules = [
+    new AlwaysAllowPrivacyPolicyRule<TestFields, string, ViewerContext, TestEntity>(),
+  ];
+  protected override readonly createRules = [
+    new AlwaysAllowPrivacyPolicyRule<TestFields, string, ViewerContext, TestEntity>(),
+  ];
+  protected override readonly updateRules = [
+    new AlwaysAllowPrivacyPolicyRule<TestFields, string, ViewerContext, TestEntity>(),
+  ];
+  protected override readonly deleteRules = [
+    new AlwaysAllowPrivacyPolicyRule<TestFields, string, ViewerContext, TestEntity>(),
+  ];
+}
+
+class TestEntity extends Entity<TestFields, string, ViewerContext> {
+  static defineCompanionDefinition(): EntityCompanionDefinition<
+    TestFields,
+    string,
+    ViewerContext,
+    TestEntity,
+    TestEntityPrivacyPolicy
+  > {
+    return {
+      entityClass: TestEntity,
+      entityConfiguration: testEntityConfiguration,
+      privacyPolicyClass: TestEntityPrivacyPolicy,
+    };
+  }
+}
+
+const testEntityConfiguration = new EntityConfiguration<TestFields>({
+  idField: 'id',
+  tableName: 'testentities',
+  schema: {
+    id: new UUIDField({
+      columnName: 'id',
+      cache: true,
+    }),
+  },
+  databaseAdapterFlavor: 'postgres',
+  cacheAdapterFlavor: 'redis',
+});
+
+async function createOrTruncatePostgresTables(knex: Knex): Promise<void> {
+  await knex.schema.createTable('testentities', (table) => {
+    table.uuid('id').defaultTo(knex.raw('gen_random_uuid()')).primary();
+  });
+  await knex.into('testentities').truncate();
+}
+
+async function dropPostgresTable(knex: Knex): Promise<void> {
+  if (await knex.schema.hasTable('testentities')) {
+    await knex.schema.dropTable('testentities');
+  }
+}
+
+describe('Entity integrity', () => {
+  let knexInstance: Knex;
+  const redisClient = new Redis(new URL(process.env['REDIS_URL']!).toString());
+  let genericRedisCacheContext: GenericRedisCacheContext;
+
+  beforeAll(() => {
+    knexInstance = knex({
+      client: 'pg',
+      connection: {
+        user: nullthrows(process.env['PGUSER']),
+        password: nullthrows(process.env['PGPASSWORD']),
+        host: 'localhost',
+        port: parseInt(nullthrows(process.env['PGPORT']), 10),
+        database: nullthrows(process.env['PGDATABASE']),
+      },
+    });
+    genericRedisCacheContext = {
+      redisClient,
+      makeKeyFn(...parts: string[]): string {
+        const delimiter = ':';
+        const escapedParts = parts.map((part) =>
+          part.replace('\\', '\\\\').replace(delimiter, `\\${delimiter}`)
+        );
+        return escapedParts.join(delimiter);
+      },
+      cacheKeyPrefix: 'test-',
+      ttlSecondsPositive: 86400, // 1 day
+      ttlSecondsNegative: 600, // 10 minutes
+    };
+  });
+
+  beforeEach(async () => {
+    await createOrTruncatePostgresTables(knexInstance);
+    await redisClient.flushdb();
+  });
+
+  afterAll(async () => {
+    await dropPostgresTable(knexInstance);
+    await knexInstance.destroy();
+    redisClient.disconnect();
+  });
+
+  test('cannot update ID', async () => {
+    const viewerContext = new ViewerContext(
+      createFullIntegrationTestEntityCompanionProvider(knexInstance, genericRedisCacheContext)
+    );
+
+    const entity1 = await TestEntity.creator(viewerContext).enforceCreateAsync();
+
+    await expect(
+      TestEntity.updater(entity1).setField('id', uuidv4()).enforceUpdateAsync()
+    ).rejects.toThrow('id field updates not supported: (entityClass = TestEntity)');
+
+    // ensure cache consistency
+    const viewerContextLast = new ViewerContext(
+      createFullIntegrationTestEntityCompanionProvider(knexInstance, genericRedisCacheContext)
+    );
+
+    const loadedById = await TestEntity.loader(viewerContextLast)
+      .enforcing()
+      .loadByIDAsync(entity1.getID());
+
+    expect(loadedById.getID()).toEqual(entity1.getID());
+  });
+});

packages/entity/src/EntityMutator.ts

@@ -422,6 +422,7 @@ export class UpdateMutator<
     cascadingDeleteCause: EntityCascadingDeletionInfo | null
   ): Promise<Result<TEntity>> {
     this.validateFields(this.updatedFields);
+    this.ensureStableIDField(this.updatedFields);
 
     const entityLoader = this.entityLoaderFactory.forLoad(this.viewerContext, queryContext, {
       previousValue: this.originalEntity,
@@ -462,14 +463,14 @@ export class UpdateMutator<
     );
 
     // skip the database update when specified
-    const updateResult = skipDatabaseUpdate
-      ? null
-      : await this.databaseAdapter.updateAsync(
-          queryContext,
-          this.entityConfiguration.idField,
-          entityAboutToBeUpdated.getID(),
-          this.updatedFields
-        );
+    if (!skipDatabaseUpdate) {
+      await this.databaseAdapter.updateAsync(
+        queryContext,
+        this.entityConfiguration.idField,
+        entityAboutToBeUpdated.getID(),
+        this.updatedFields
+      );
+    }
 
     queryContext.appendPostCommitInvalidationCallback(
       entityLoader.invalidateFieldsAsync.bind(
@@ -481,13 +482,9 @@ export class UpdateMutator<
       entityLoader.invalidateFieldsAsync.bind(entityLoader, this.fieldsForEntity)
     );
 
-    // when the database update was skipped, assume it succeeded and use the optimistic
-    // entity to execute triggers
-    const updatedEntity = updateResult
-      ? await entityLoader
-          .enforcing()
-          .loadByIDAsync(entityLoader.constructEntity(updateResult).getID())
-      : entityAboutToBeUpdated;
+    const updatedEntity = await entityLoader
+      .enforcing()
+      .loadByIDAsync(entityAboutToBeUpdated.getID()); // ID is guaranteed to be stable by ensureStableIDField
 
     await this.executeMutationTriggersAsync(
       this.mutationTriggers.afterUpdate,
@@ -517,6 +514,14 @@ export class UpdateMutator<
 
     return result(updatedEntity);
   }
+
+  private ensureStableIDField(updatedFields: Partial<TFields>): void {
+    const originalId = this.originalEntity.getID();
+    const idField = this.entityConfiguration.idField;
+    if (updatedFields.hasOwnProperty(idField) && originalId !== updatedFields[idField]) {
+      throw new Error(`id field updates not supported: (entityClass = ${this.entityClass.name})`);
+    }
+  }
 }
 
 /**

packages/entity/src/__tests__/EntityEdges-test.ts

@@ -835,7 +835,7 @@ describe('EntityMutator.processEntityDeletionForInboundEdgesAsync', () => {
         ChildEntity: {
           [EntityAuthorizationAction.CREATE]: [],
 
-          // one READ auth action for child in order to update via cascade
+          // two READs auth action for child in order to update via cascade
           // no other entities are read since it is not cascaded past first entity
           [EntityAuthorizationAction.READ]: [
             {
@@ -844,6 +844,12 @@ describe('EntityMutator.processEntityDeletionForInboundEdgesAsync', () => {
                 cascadingDeleteCause: null,
               },
             },
+            {
+              cascadingDeleteCause: {
+                entity: expect.any(ParentEntity),
+                cascadingDeleteCause: null,
+              },
+            },
           ],
           // one UPDATE to set null
           [EntityAuthorizationAction.UPDATE]: [

packages/entity/src/__tests__/EntityMutator-test.ts

@@ -715,6 +715,7 @@ describe(EntityMutatorFactory, () => {
         }
       );
     });
+
     it('executes validators', async () => {
       const viewerContext = mock<ViewerContext>();
       const privacyPolicyEvaluationContext = instance(
@@ -771,6 +772,54 @@ describe(EntityMutatorFactory, () => {
         cascadingDeleteCause: null,
       });
     });
+
+    it('throws when id field is updated', async () => {
+      const viewerContext = mock<ViewerContext>();
+      const privacyPolicyEvaluationContext = instance(
+        mock<
+          EntityPrivacyPolicyEvaluationContext<
+            TestFields,
+            string,
+            ViewerContext,
+            TestEntity,
+            keyof TestFields
+          >
+        >()
+      );
+      const queryContext = StubQueryContextProvider.getQueryContext();
+
+      const id1 = uuidv4();
+      const { entityMutatorFactory, entityLoaderFactory } = createEntityMutatorFactory([
+        {
+          customIdField: id1,
+          stringField: 'huh',
+          testIndexedField: '4',
+          intField: 3,
+          dateField: new Date(),
+          nullableField: null,
+        },
+      ]);
+
+      const existingEntity = await enforceAsyncResult(
+        entityLoaderFactory
+          .forLoad(viewerContext, queryContext, privacyPolicyEvaluationContext)
+          .loadByIDAsync(id1)
+      );
+
+      await expect(
+        entityMutatorFactory
+          .forUpdate(existingEntity, queryContext)
+          .setField('customIdField', uuidv4())
+          .enforceUpdateAsync()
+      ).rejects.toThrow('id field updates not supported: (entityClass = TestEntity)');
+
+      const reloadedEntity = await enforceAsyncResult(
+        entityLoaderFactory
+          .forLoad(viewerContext, queryContext, privacyPolicyEvaluationContext)
+          .loadByIDAsync(id1)
+      );
+      expect(reloadedEntity.getAllFields()).toMatchObject(existingEntity.getAllFields());
+    });
   });
 
   describe('forDelete', () => {