From 3e01a92481ec200246fd70aec73aab9318166397 Mon Sep 17 00:00:00 2001 From: Will Schurman Date: Mon, 3 Jun 2024 11:59:01 -0700 Subject: [PATCH 1/3] fix: always reload entity after update since cascading changes may have changed it since commit --- .../EntityIntegrity-test.ts | 149 ++++++++++++++++++ packages/entity/src/EntityMutator.ts | 35 ++-- .../src/__tests__/EntityMutator-test.ts | 49 ++++++ 3 files changed, 218 insertions(+), 15 deletions(-) create mode 100644 packages/entity-full-integration-tests/src/__integration-tests__/EntityIntegrity-test.ts diff --git a/packages/entity-full-integration-tests/src/__integration-tests__/EntityIntegrity-test.ts b/packages/entity-full-integration-tests/src/__integration-tests__/EntityIntegrity-test.ts new file mode 100644 index 000000000..fa4e43ee2 --- /dev/null +++ b/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(), + ]; + protected override readonly createRules = [ + new AlwaysAllowPrivacyPolicyRule(), + ]; + protected override readonly updateRules = [ + new AlwaysAllowPrivacyPolicyRule(), + ]; + protected override readonly deleteRules = [ + new AlwaysAllowPrivacyPolicyRule(), + ]; +} + +class TestEntity extends Entity { + static defineCompanionDefinition(): EntityCompanionDefinition< + TestFields, + string, + ViewerContext, + TestEntity, + TestEntityPrivacyPolicy + > { + return { + entityClass: TestEntity, + entityConfiguration: testEntityConfiguration, + privacyPolicyClass: TestEntityPrivacyPolicy, + }; + } +} + +const testEntityConfiguration = new EntityConfiguration({ + idField: 'id', + tableName: 'testentities', + schema: { + id: new UUIDField({ + columnName: 'id', + cache: true, + }), + }, + databaseAdapterFlavor: 'postgres', + cacheAdapterFlavor: 'redis', +}); + +async function createOrTruncatePostgresTables(knex: Knex): Promise { + 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 { + 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()); + }); +}); diff --git a/packages/entity/src/EntityMutator.ts b/packages/entity/src/EntityMutator.ts index 7f6cac117..0e6b2cadd 100644 --- a/packages/entity/src/EntityMutator.ts +++ b/packages/entity/src/EntityMutator.ts @@ -422,6 +422,7 @@ export class UpdateMutator< cascadingDeleteCause: EntityCascadingDeletionInfo | null ): Promise> { 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): void { + const originalId = this.originalEntity.getID(); + const idField = this.entityConfiguration.idField; + if (idField in updatedFields && originalId !== updatedFields[idField]) { + throw new Error(`id field updates not supported: (entityClass = ${this.entityClass.name})`); + } + } } /** diff --git a/packages/entity/src/__tests__/EntityMutator-test.ts b/packages/entity/src/__tests__/EntityMutator-test.ts index ac2a97769..ce445c20e 100644 --- a/packages/entity/src/__tests__/EntityMutator-test.ts +++ b/packages/entity/src/__tests__/EntityMutator-test.ts @@ -715,6 +715,7 @@ describe(EntityMutatorFactory, () => { } ); }); + it('executes validators', async () => { const viewerContext = mock(); const privacyPolicyEvaluationContext = instance( @@ -771,6 +772,54 @@ describe(EntityMutatorFactory, () => { cascadingDeleteCause: null, }); }); + + it('throws when id field is updated', async () => { + const viewerContext = mock(); + 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', () => { From b5a2f1b63079e953b5e5c9c986fd765869c439eb Mon Sep 17 00:00:00 2001 From: Will Schurman Date: Mon, 3 Jun 2024 12:19:13 -0700 Subject: [PATCH 2/3] Fix test --- packages/entity/src/__tests__/EntityEdges-test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/entity/src/__tests__/EntityEdges-test.ts b/packages/entity/src/__tests__/EntityEdges-test.ts index 5419a0e71..046dcc51a 100644 --- a/packages/entity/src/__tests__/EntityEdges-test.ts +++ b/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]: [ From 1d969ed618ab26c8720d5ee213ff10072a3b6d92 Mon Sep 17 00:00:00 2001 From: Will Schurman Date: Tue, 4 Jun 2024 11:15:11 -0700 Subject: [PATCH 3/3] Address comment --- packages/entity/src/EntityMutator.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/entity/src/EntityMutator.ts b/packages/entity/src/EntityMutator.ts index 0e6b2cadd..e6c15f8d2 100644 --- a/packages/entity/src/EntityMutator.ts +++ b/packages/entity/src/EntityMutator.ts @@ -518,7 +518,7 @@ export class UpdateMutator< private ensureStableIDField(updatedFields: Partial): void { const originalId = this.originalEntity.getID(); const idField = this.entityConfiguration.idField; - if (idField in updatedFields && originalId !== updatedFields[idField]) { + if (updatedFields.hasOwnProperty(idField) && originalId !== updatedFields[idField]) { throw new Error(`id field updates not supported: (entityClass = ${this.entityClass.name})`); } }