Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(core): optimization for assignToChannels method #2743

Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 73 additions & 19 deletions packages/core/src/service/services/channel.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,12 @@ import {
import { DEFAULT_CHANNEL_CODE } from '@vendure/common/lib/shared-constants';
import { ID, PaginatedList, Type } from '@vendure/common/lib/shared-types';
import { unique } from '@vendure/common/lib/unique';
import { FindOneOptions } from 'typeorm';
import { FindOptionsWhere } from 'typeorm';

import { RelationPaths } from '../../api';
import { RequestContext } from '../../api/common/request-context';
import { ErrorResultUnion, isGraphQlErrorResult } from '../../common/error/error-result';
import {
ChannelNotFoundError,
EntityNotFoundError,
InternalServerError,
UserInputError,
} from '../../common/error/errors';
import { ChannelNotFoundError, EntityNotFoundError, InternalServerError, UserInputError } from '../../common/error/errors';
import { LanguageNotAvailableError } from '../../common/error/generated-graphql-admin-errors';
import { createSelfRefreshingCache, SelfRefreshingCache } from '../../common/self-refreshing-cache';
import { ChannelAware, ListQueryOptions } from '../../common/types/common-types';
Expand Down Expand Up @@ -124,6 +119,42 @@ export class ChannelService {
return entity;
}

/**
* This method is used to bypass a bug with Typeorm when working with ManyToMany relationships.
* For some reason, a regular query does not return all the channels that an entity has.
* This is a most optimized way to get all the channels that an entity has.
*
* @param ctx - The RequestContext object.
* @param entityType - The type of the entity.
* @param entityId - The ID of the entity.
* @returns A promise that resolves to an array of objects, each containing a channel ID.
* @private
*/
private async getAssignedEntityChannels<T extends ChannelAware & VendureEntity>(ctx: RequestContext, entityType: Type<T>, entityId: T['id']): Promise<{ channelId: ID }[]> {
const repository = this.connection.getRepository(ctx, entityType);

const metadata = repository.metadata;
const channelsRelation = metadata.findRelationWithPropertyPath('channels');

if (!channelsRelation) {
throw new InternalServerError(`Could not find the channels relation for entity ${metadata.name}`);
}

const junctionTableName = channelsRelation.junctionEntityMetadata?.tableName;
const junctionColumnName = channelsRelation.junctionEntityMetadata?.columns[0].databaseName;
const inverseJunctionColumnName = channelsRelation.junctionEntityMetadata?.inverseColumns[0].databaseName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the latest updates we explicitly added inverse relations to the Channel entity.

I'm wondering what would happen if someone defines a custom channel-aware entity which does not have the inverse relation on the Channel? Would this break?

Copy link
Contributor Author

@monrostar monrostar Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite normal, there should be no errors because even without reverse side entity is still available
But if there are problems we replace them with pure SQL

With the latest updates we explicitly added inverse relations to the Channel entity.

I'm wondering what would happen if someone defines a custom channel-aware entity which does not have the inverse relation on the Channel? Would this break?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or just manually parse the column names in the table. This can also be done. We just need to test it on a real project


if (!junctionTableName || !junctionColumnName || !inverseJunctionColumnName) {
throw new InternalServerError(`Could not find necessary join table information for the channels relation of entity ${metadata.name}`);
}

return await this.connection.getRepository(ctx, entityType).createQueryBuilder()
.select(`channel.${inverseJunctionColumnName}`, 'channelId')
.from(junctionTableName, 'channel')
.where(`channel.${junctionColumnName} = :entityId`, { entityId })
.execute();
}

/**
* @description
* Assigns the entity to the given Channels and saves.
Expand All @@ -134,7 +165,7 @@ export class ChannelService {
entityId: ID,
channelIds: ID[],
): Promise<T> {
const relations = ['channels'];
const relations = [];
// This is a work-around for https://github.com/vendure-ecommerce/vendure/issues/1391
// A better API would be to allow the consumer of this method to supply an entity instance
// so that this join could be done prior to invoking this method.
Expand All @@ -143,13 +174,24 @@ export class ChannelService {
relations.push('lines', 'shippingLines');
}
const entity = await this.connection.getEntityOrThrow(ctx, entityType, entityId, {
loadEagerRelations: false,
relationLoadStrategy: 'query',
where: {
id: entityId,
} as FindOptionsWhere<T>,
relations,
});
for (const id of channelIds) {
const channel = await this.connection.getEntityOrThrow(ctx, Channel, id);
entity.channels.push(channel);
}
await this.connection.getRepository(ctx, entityType).save(entity as any, { reload: false });
const assignedChannels = await this.getAssignedEntityChannels(ctx, entityType, entityId);

const newChannelIds = channelIds.filter(id => !assignedChannels.some(ec => idsAreEqual(ec.channelId, id)))

await this.connection
.getRepository(ctx, entityType)
.createQueryBuilder()
.relation('channels')
.of(entity.id)
.add(newChannelIds);

this.eventBus.publish(new ChangeChannelEvent(ctx, entity, channelIds, 'assigned', entityType));
return entity;
}
Expand All @@ -165,16 +207,28 @@ export class ChannelService {
channelIds: ID[],
): Promise<T | undefined> {
const entity = await this.connection.getRepository(ctx, entityType).findOne({
where: { id: entityId },
relations: ['channels'],
} as FindOneOptions<T>);
loadEagerRelations: false,
relationLoadStrategy: 'query',
where: {
id: entityId,
} as FindOptionsWhere<T>,
})
if (!entity) {
return;
}
for (const id of channelIds) {
entity.channels = entity.channels.filter(c => !idsAreEqual(c.id, id));
const assignedChannels = await this.getAssignedEntityChannels(ctx, entityType, entityId);

const existingChannelIds = channelIds.filter(id => assignedChannels.some(ec => idsAreEqual(ec.channelId, id)));

if (!existingChannelIds.length) {
return
}
await this.connection.getRepository(ctx, entityType).save(entity as any, { reload: false });
await this.connection
.getRepository(ctx, entityType)
.createQueryBuilder()
.relation('channels')
.of(entity.id)
.remove(existingChannelIds);
this.eventBus.publish(new ChangeChannelEvent(ctx, entity, channelIds, 'removed', entityType));
return entity;
}
Expand Down
Loading