Skip to content

Commit

Permalink
perf(core): Database access performance & edge case fixes (#2744)
Browse files Browse the repository at this point in the history
  • Loading branch information
monrostar authored Mar 20, 2024
1 parent cb03074 commit 48b239b
Show file tree
Hide file tree
Showing 10 changed files with 232 additions and 179 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ import { isEntityCreateOrUpdateMutation } from '../utils/is-entity-create-or-upd

@Injectable()
export class BaseDataService {
constructor(private apollo: Apollo, private serverConfigService: ServerConfigService) {}
constructor(
private apollo: Apollo,
private serverConfigService: ServerConfigService,
) {}

private get customFields(): Map<string, CustomFieldConfig[]> {
return this.serverConfigService.customFieldsMap;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ export class AssetsComponent {
@Input()
updatePermissions: string | string[] | Permission | Permission[];

constructor(private modalService: ModalService, private changeDetector: ChangeDetectorRef) {}
constructor(
private modalService: ModalService,
private changeDetector: ChangeDetectorRef,
) {}

selectAssets() {
this.modalService
Expand Down
37 changes: 16 additions & 21 deletions packages/core/e2e/shop-order.e2e-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,9 +161,8 @@ describe('Shop orders', () => {
},
);

const result = await shopClient.query<CodegenShop.GetAvailableCountriesQuery>(
GET_AVAILABLE_COUNTRIES,
);
const result =
await shopClient.query<CodegenShop.GetAvailableCountriesQuery>(GET_AVAILABLE_COUNTRIES);
expect(result.availableCountries.length).toBe(countries.items.length - 1);
expect(result.availableCountries.find(c => c.id === AT.id)).toBeUndefined();
});
Expand Down Expand Up @@ -1519,9 +1518,8 @@ describe('Shop orders', () => {
id: shippingMethods[1].id,
});

const activeOrderResult = await shopClient.query<CodegenShop.GetActiveOrderQuery>(
GET_ACTIVE_ORDER,
);
const activeOrderResult =
await shopClient.query<CodegenShop.GetActiveOrderQuery>(GET_ACTIVE_ORDER);

const order = activeOrderResult.activeOrder!;

Expand All @@ -1533,9 +1531,8 @@ describe('Shop orders', () => {
});

it('shipping method is preserved after adjustOrderLine', async () => {
const activeOrderResult = await shopClient.query<CodegenShop.GetActiveOrderQuery>(
GET_ACTIVE_ORDER,
);
const activeOrderResult =
await shopClient.query<CodegenShop.GetActiveOrderQuery>(GET_ACTIVE_ORDER);
activeOrder = activeOrderResult.activeOrder!;
const { adjustOrderLine } = await shopClient.query<
CodegenShop.AdjustItemQuantityMutation,
Expand Down Expand Up @@ -1709,9 +1706,10 @@ describe('Shop orders', () => {
expect(addPaymentToOrder.errorCode).toBe(ErrorCode.PAYMENT_FAILED_ERROR);
expect((addPaymentToOrder as any).paymentErrorMessage).toBe('Something went horribly wrong');

const result = await shopClient.query<CodegenShop.GetActiveOrderPaymentsQuery>(
GET_ACTIVE_ORDER_PAYMENTS,
);
const result =
await shopClient.query<CodegenShop.GetActiveOrderPaymentsQuery>(
GET_ACTIVE_ORDER_PAYMENTS,
);
const payment = result.activeOrder!.payments![1];
expect(result.activeOrder!.payments!.length).toBe(2);
expect(payment.method).toBe(testErrorPaymentMethod.code);
Expand Down Expand Up @@ -2059,9 +2057,8 @@ describe('Shop orders', () => {
},
});

const { activeOrder } = await shopClient.query<CodegenShop.GetCustomerAddressesQuery>(
GET_ACTIVE_ORDER_ADDRESSES,
);
const { activeOrder } =
await shopClient.query<CodegenShop.GetCustomerAddressesQuery>(GET_ACTIVE_ORDER_ADDRESSES);

expect(activeOrder!.customer!.addresses).toEqual([]);
});
Expand All @@ -2087,9 +2084,8 @@ describe('Shop orders', () => {
},
});

const { activeOrder } = await shopClient.query<CodegenShop.GetCustomerOrdersQuery>(
GET_ACTIVE_ORDER_ORDERS,
);
const { activeOrder } =
await shopClient.query<CodegenShop.GetCustomerOrdersQuery>(GET_ACTIVE_ORDER_ORDERS);

expect(activeOrder!.customer!.orders.items).toEqual([]);
});
Expand Down Expand Up @@ -2328,9 +2324,8 @@ describe('Shop orders', () => {

beforeAll(async () => {
// First we will remove all ShippingMethods and set up 2 specialized ones
const { shippingMethods } = await adminClient.query<Codegen.GetShippingMethodListQuery>(
GET_SHIPPING_METHOD_LIST,
);
const { shippingMethods } =
await adminClient.query<Codegen.GetShippingMethodListQuery>(GET_SHIPPING_METHOD_LIST);
for (const method of shippingMethods.items) {
await adminClient.query<
Codegen.DeleteShippingMethodMutation,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ export class CustomFieldRelationResolverService {
const translated: any = Array.isArray(result)
? result.map(r => (this.isTranslatable(r) ? this.translator.translate(r, ctx) : r))
: this.isTranslatable(result)
? this.translator.translate(result, ctx)
: result;
? this.translator.translate(result, ctx)
: result;

return translated;
}
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/api/resolvers/admin/collection.resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ export class CollectionResolver {
): Promise<Translated<Collection>> {
const { input } = args;
this.configurableOperationCodec.decodeConfigurableOperationIds(CollectionFilter, input.filters);
return this.collectionService.create(ctx, input);
const collection = await this.collectionService.create(ctx, input);
return collection;
}

@Transaction()
Expand Down
56 changes: 40 additions & 16 deletions packages/core/src/connection/transactional-connection.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,25 @@
import { Injectable } from '@nestjs/common';
import { InjectConnection } from '@nestjs/typeorm';
import { InjectDataSource } from '@nestjs/typeorm/dist/common/typeorm.decorators';
import { ID, Type } from '@vendure/common/lib/shared-types';
import {
Connection,
DataSource,
EntityManager,
EntitySchema,
FindOneOptions,
FindOptionsUtils,
FindManyOptions,
ObjectLiteral,
ObjectType,
Repository,
SelectQueryBuilder,
} from 'typeorm';
import { FindManyOptions } from 'typeorm/find-options/FindManyOptions';

import { RequestContext } from '../api/common/request-context';
import { TransactionIsolationLevel } from '../api/decorators/transaction.decorator';
import { TRANSACTION_MANAGER_KEY } from '../common/constants';
import { EntityNotFoundError } from '../common/error/errors';
import { ChannelAware, SoftDeletable } from '../common/types/common-types';
import { VendureEntity } from '../entity/base/base.entity';
import { joinTreeRelationsDynamically } from '../service/helpers/utils/tree-relations-qb-joiner';

import { TransactionWrapper } from './transaction-wrapper';
import { GetEntityOrThrowOptions } from './types';
Expand All @@ -38,7 +39,7 @@ import { GetEntityOrThrowOptions } from './types';
@Injectable()
export class TransactionalConnection {
constructor(
@InjectConnection() private connection: Connection,
@InjectDataSource() private dataSource: DataSource,
private transactionWrapper: TransactionWrapper,
) {}

Expand All @@ -48,8 +49,8 @@ export class TransactionalConnection {
* performed with this connection will not be performed within any outer
* transactions.
*/
get rawConnection(): Connection {
return this.connection;
get rawConnection(): DataSource {
return this.dataSource;
}

/**
Expand Down Expand Up @@ -275,16 +276,26 @@ export class TransactionalConnection {
channelId: ID,
options: FindOneOptions<T> = {},
) {
const qb = this.getRepository(ctx, entity).createQueryBuilder('entity').setFindOptions(options);
if (options.loadEagerRelations !== false) {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
FindOptionsUtils.joinEagerRelations(qb, qb.alias, qb.expressionMap.mainAlias!.metadata);
const qb = this.getRepository(ctx, entity).createQueryBuilder('entity');

if (Array.isArray(options.relations) && options.relations.length > 0) {
const joinedRelations = joinTreeRelationsDynamically(qb, entity, options.relations);
// Remove any relations which are related to the 'collection' tree, as these are handled separately
// to avoid duplicate joins.
options.relations = options.relations.filter(relationPath => !joinedRelations.has(relationPath));
}
qb.setFindOptions({
relationLoadStrategy: 'query', // default to query strategy for maximum performance
...options,
});

qb.leftJoin('entity.channels', '__channel')
.andWhere('entity.id = :id', { id })
.andWhere('__channel.id = :channelId', { channelId });

return qb.getOne().then(result => result ?? undefined);
return qb.getOne().then(result => {
return result ?? undefined;
});
}

/**
Expand All @@ -305,11 +316,24 @@ export class TransactionalConnection {
return Promise.resolve([]);
}

const qb = this.getRepository(ctx, entity).createQueryBuilder('entity').setFindOptions(options);
if (options.loadEagerRelations !== false) {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
FindOptionsUtils.joinEagerRelations(qb, qb.alias, qb.expressionMap.mainAlias!.metadata);
const qb = this.getRepository(ctx, entity).createQueryBuilder('entity');

if (Array.isArray(options.relations) && options.relations.length > 0) {
const joinedRelations = joinTreeRelationsDynamically(
qb as SelectQueryBuilder<VendureEntity>,
entity,
options.relations,
);
// Remove any relations which are related to the 'collection' tree, as these are handled separately
// to avoid duplicate joins.
options.relations = options.relations.filter(relationPath => !joinedRelations.has(relationPath));
}

qb.setFindOptions({
relationLoadStrategy: 'query', // default to query strategy for maximum performance
...options,
});

return qb
.leftJoin('entity.channels', 'channel')
.andWhere('entity.id IN (:...ids)', { ids })
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ import { InternalServerError } from '../../../common/error/errors';
import { TransactionalConnection } from '../../../connection/transactional-connection';
import { VendureEntity } from '../../../entity/base/base.entity';
import { ProductVariant } from '../../../entity/product-variant/product-variant.entity';
import { ListQueryBuilder } from '../list-query-builder/list-query-builder';
import { ProductPriceApplicator } from '../product-price-applicator/product-price-applicator';
import { TranslatorService } from '../translator/translator.service';
import { joinTreeRelationsDynamically } from '../utils/tree-relations-qb-joiner';

import { HydrateOptions } from './entity-hydrator-types';

Expand Down Expand Up @@ -80,7 +80,6 @@ export class EntityHydrator {
private connection: TransactionalConnection,
private productPriceApplicator: ProductPriceApplicator,
private translator: TranslatorService,
private listQueryBuilder: ListQueryBuilder,
) {}

/**
Expand Down Expand Up @@ -122,15 +121,15 @@ export class EntityHydrator {
const hydratedQb: SelectQueryBuilder<any> = this.connection
.getRepository(ctx, target.constructor)
.createQueryBuilder(target.constructor.name);
const processedRelations = this.listQueryBuilder.joinTreeRelationsDynamically(
const joinedRelations = joinTreeRelationsDynamically(
hydratedQb,
target.constructor,
missingRelations,
);
hydratedQb.setFindOptions({
relationLoadStrategy: 'query',
where: { id: target.id },
relations: missingRelations.filter(relationPath => !processedRelations.has(relationPath)),
relations: missingRelations.filter(relationPath => !joinedRelations.has(relationPath)),
});
const hydrated = await hydratedQb.getOne();
const propertiesToAdd = unique(missingRelations.map(relation => relation.split('.')[0]));
Expand Down
Loading

0 comments on commit 48b239b

Please sign in to comment.