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(server): optimize getByIds query #7918

Merged
merged 9 commits into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions server/src/domain/download/download.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ describe(DownloadService.name, () => {
const assetIds = ['asset-1', 'asset-2'];
await expect(sut.getDownloadInfo(authStub.admin, { assetIds })).resolves.toEqual(downloadResponse);

expect(assetMock.getByIds).toHaveBeenCalledWith(['asset-1', 'asset-2']);
expect(assetMock.getByIds).toHaveBeenCalledWith(['asset-1', 'asset-2'], { exifInfo: true });
});

it('should return a list of archives (albumId)', async () => {
Expand Down Expand Up @@ -228,9 +228,15 @@ describe(DownloadService.name, () => {

accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(assetIds));
when(assetMock.getByIds)
.calledWith([assetStub.livePhotoStillAsset.id])
.calledWith([assetStub.livePhotoStillAsset.id], { exifInfo: true })
.mockResolvedValue([assetStub.livePhotoStillAsset]);
when(assetMock.getByIds)
.calledWith([assetStub.livePhotoMotionAsset.id], { exifInfo: true })
.mockResolvedValue([assetStub.livePhotoMotionAsset]);
when(assetMock.getByIdsWithAllRelations)
.calledWith([assetStub.livePhotoStillAsset.id])
.mockResolvedValue([assetStub.livePhotoStillAsset]);
when(assetMock.getByIdsWithAllRelations)
.calledWith([assetStub.livePhotoMotionAsset.id])
.mockResolvedValue([assetStub.livePhotoMotionAsset]);

Expand Down
4 changes: 2 additions & 2 deletions server/src/domain/download/download.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export class DownloadService {
// motion part of live photos
const motionIds = assets.map((asset) => asset.livePhotoVideoId).filter<string>((id): id is string => !!id);
if (motionIds.length > 0) {
assets.push(...(await this.assetRepository.getByIds(motionIds)));
assets.push(...(await this.assetRepository.getByIdsWithAllRelations(motionIds)));
mertalev marked this conversation as resolved.
Show resolved Hide resolved
}

for (const asset of assets) {
Expand Down Expand Up @@ -114,7 +114,7 @@ export class DownloadService {
if (dto.assetIds) {
const assetIds = dto.assetIds;
await this.access.requirePermission(auth, Permission.ASSET_DOWNLOAD, assetIds);
const assets = await this.assetRepository.getByIds(assetIds);
const assets = await this.assetRepository.getByIds(assetIds, { exifInfo: true });
return usePagination(PAGINATION_SIZE, () => ({ hasNextPage: false, items: assets }));
}

Expand Down
2 changes: 0 additions & 2 deletions server/src/domain/job/job.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -330,8 +330,6 @@ describe(JobService.name, () => {
} else {
assetMock.getByIds.mockResolvedValue([assetStub.livePhotoMotionAsset]);
}
} else {
assetMock.getByIds.mockResolvedValue([]);
}

await sut.init(makeMockHandlers(true));
Expand Down
4 changes: 2 additions & 2 deletions server/src/domain/job/job.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ export class JobService {

case JobName.METADATA_EXTRACTION: {
if (item.data.source === 'sidecar-write') {
const [asset] = await this.assetRepository.getByIds([item.data.id]);
const [asset] = await this.assetRepository.getByIdsWithAllRelations([item.data.id]);
if (asset) {
this.communicationRepository.send(ClientEvent.ASSET_UPDATE, asset.ownerId, mapAsset(asset));
}
Expand Down Expand Up @@ -272,7 +272,7 @@ export class JobService {
break;
}

const [asset] = await this.assetRepository.getByIds([item.data.id]);
const [asset] = await this.assetRepository.getByIdsWithAllRelations([item.data.id]);

// Only live-photo motion part will be marked as not visible immediately on upload. Skip notifying clients
if (asset && asset.isVisible) {
Expand Down
4 changes: 2 additions & 2 deletions server/src/domain/media/media.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ export class MediaService {
}

async handleGenerateJpegThumbnail({ id }: IEntityJob) {
const [asset] = await this.assetRepository.getByIds([id]);
const [asset] = await this.assetRepository.getByIds([id], { exifInfo: true });
if (!asset) {
return false;
}
Expand Down Expand Up @@ -215,7 +215,7 @@ export class MediaService {
}

async handleGenerateWebpThumbnail({ id }: IEntityJob) {
const [asset] = await this.assetRepository.getByIds([id]);
const [asset] = await this.assetRepository.getByIds([id], { exifInfo: true });
if (!asset) {
return false;
}
Expand Down
10 changes: 5 additions & 5 deletions server/src/domain/metadata/metadata.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ describe(MetadataService.name, () => {
describe('handleLivePhotoLinking', () => {
it('should handle an asset that could not be found', async () => {
await expect(sut.handleLivePhotoLinking({ id: assetStub.image.id })).resolves.toBe(false);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id]);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id], { exifInfo: true });
expect(assetMock.findLivePhotoMatch).not.toHaveBeenCalled();
expect(assetMock.save).not.toHaveBeenCalled();
expect(albumMock.removeAsset).not.toHaveBeenCalled();
Expand All @@ -124,7 +124,7 @@ describe(MetadataService.name, () => {
assetMock.getByIds.mockResolvedValue([{ ...assetStub.image, exifInfo: undefined }]);

await expect(sut.handleLivePhotoLinking({ id: assetStub.image.id })).resolves.toBe(false);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id]);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id], { exifInfo: true });
expect(assetMock.findLivePhotoMatch).not.toHaveBeenCalled();
expect(assetMock.save).not.toHaveBeenCalled();
expect(albumMock.removeAsset).not.toHaveBeenCalled();
Expand All @@ -134,7 +134,7 @@ describe(MetadataService.name, () => {
assetMock.getByIds.mockResolvedValue([{ ...assetStub.image }]);

await expect(sut.handleLivePhotoLinking({ id: assetStub.image.id })).resolves.toBe(true);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id]);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id], { exifInfo: true });
expect(assetMock.findLivePhotoMatch).not.toHaveBeenCalled();
expect(assetMock.save).not.toHaveBeenCalled();
expect(albumMock.removeAsset).not.toHaveBeenCalled();
Expand All @@ -149,7 +149,7 @@ describe(MetadataService.name, () => {
]);

await expect(sut.handleLivePhotoLinking({ id: assetStub.livePhotoMotionAsset.id })).resolves.toBe(true);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.livePhotoMotionAsset.id]);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.livePhotoMotionAsset.id], { exifInfo: true });
expect(assetMock.findLivePhotoMatch).toHaveBeenCalledWith({
livePhotoCID: assetStub.livePhotoStillAsset.id,
ownerId: assetStub.livePhotoMotionAsset.ownerId,
Expand All @@ -170,7 +170,7 @@ describe(MetadataService.name, () => {
assetMock.findLivePhotoMatch.mockResolvedValue(assetStub.livePhotoMotionAsset);

await expect(sut.handleLivePhotoLinking({ id: assetStub.livePhotoStillAsset.id })).resolves.toBe(true);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.livePhotoStillAsset.id]);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.livePhotoStillAsset.id], { exifInfo: true });
expect(assetMock.findLivePhotoMatch).toHaveBeenCalledWith({
livePhotoCID: assetStub.livePhotoMotionAsset.id,
ownerId: assetStub.livePhotoStillAsset.ownerId,
Expand Down
2 changes: 1 addition & 1 deletion server/src/domain/metadata/metadata.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ export class MetadataService {

async handleLivePhotoLinking(job: IEntityJob) {
const { id } = job;
const [asset] = await this.assetRepository.getByIds([id]);
const [asset] = await this.assetRepository.getByIds([id], { exifInfo: true });
if (!asset?.exifInfo) {
return false;
}
Expand Down
1 change: 1 addition & 0 deletions server/src/domain/repositories/asset.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ export interface IAssetRepository {
relations?: FindOptionsRelations<AssetEntity>,
select?: FindOptionsSelect<AssetEntity>,
): Promise<AssetEntity[]>;
getByIdsWithAllRelations(ids: string[]): Promise<AssetEntity[]>;
getByDayOfYear(ownerId: string, monthDay: MonthDay): Promise<AssetEntity[]>;
getByChecksum(userId: string, checksum: Buffer): Promise<AssetEntity | null>;
getByAlbumId(pagination: PaginationOptions, albumId: string): Paginated<AssetEntity>;
Expand Down
2 changes: 1 addition & 1 deletion server/src/domain/search/search.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ describe(SearchService.name, () => {
fieldName: 'smartInfo.tags',
items: [{ value: 'train', data: assetStub.imageFrom2015.id }],
});
assetMock.getByIds.mockResolvedValueOnce([assetStub.image, assetStub.imageFrom2015]);
assetMock.getByIdsWithAllRelations.mockResolvedValueOnce([assetStub.image, assetStub.imageFrom2015]);
const expectedResponse = [
{ fieldName: 'exifInfo.city', items: [{ value: 'Paris', data: mapAsset(assetStub.image) }] },
{ fieldName: 'smartInfo.tags', items: [{ value: 'train', data: mapAsset(assetStub.imageFrom2015) }] },
Expand Down
2 changes: 1 addition & 1 deletion server/src/domain/search/search.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export class SearchService {
this.assetRepository.getAssetIdByTag(auth.user.id, options),
]);
const assetIds = new Set<string>(results.flatMap((field) => field.items.map((item) => item.data)));
const assets = await this.assetRepository.getByIds([...assetIds]);
const assets = await this.assetRepository.getByIdsWithAllRelations([...assetIds]);
const assetMap = new Map<string, AssetResponseDto>(assets.map((asset) => [asset.id, mapAsset(asset)]));

return results.map(({ fieldName, items }) => ({
Expand Down
4 changes: 4 additions & 0 deletions server/src/domain/smart-info/smart-info.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ export class SmartInfoService {
}

const [asset] = await this.assetRepository.getByIds([id]);
if (!asset) {
return false;
}

if (!asset.resizePath) {
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,11 @@ describe(StorageTemplateService.name, () => {
.mockResolvedValue(assetStub.livePhotoMotionAsset);

when(assetMock.getByIds)
.calledWith([assetStub.livePhotoStillAsset.id])
.calledWith([assetStub.livePhotoStillAsset.id], { exifInfo: true })
.mockResolvedValue([assetStub.livePhotoStillAsset]);

when(assetMock.getByIds)
.calledWith([assetStub.livePhotoMotionAsset.id])
.calledWith([assetStub.livePhotoMotionAsset.id], { exifInfo: true })
.mockResolvedValue([assetStub.livePhotoMotionAsset]);

when(moveMock.create)
Expand Down Expand Up @@ -140,8 +140,8 @@ describe(StorageTemplateService.name, () => {

await expect(sut.handleMigrationSingle({ id: assetStub.livePhotoStillAsset.id })).resolves.toBe(true);

expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.livePhotoStillAsset.id]);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.livePhotoMotionAsset.id]);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.livePhotoStillAsset.id], { exifInfo: true });
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.livePhotoMotionAsset.id], { exifInfo: true });
expect(storageMock.checkFileExists).toHaveBeenCalledTimes(2);
expect(assetMock.save).toHaveBeenCalledWith({
id: assetStub.livePhotoStillAsset.id,
Expand Down Expand Up @@ -172,7 +172,9 @@ describe(StorageTemplateService.name, () => {
.calledWith({ id: assetStub.image.id, originalPath: newPath })
.mockResolvedValue(assetStub.image);

when(assetMock.getByIds).calledWith([assetStub.image.id]).mockResolvedValue([assetStub.image]);
when(assetMock.getByIds)
.calledWith([assetStub.image.id], { exifInfo: true })
.mockResolvedValue([assetStub.image]);

when(moveMock.update)
.calledWith({
Expand All @@ -190,7 +192,7 @@ describe(StorageTemplateService.name, () => {

await expect(sut.handleMigrationSingle({ id: assetStub.image.id })).resolves.toBe(true);

expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id]);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id], { exifInfo: true });
expect(storageMock.checkFileExists).toHaveBeenCalledTimes(3);
expect(storageMock.rename).toHaveBeenCalledWith(assetStub.image.originalPath, newPath);
expect(moveMock.update).toHaveBeenCalledWith({
Expand Down Expand Up @@ -227,7 +229,9 @@ describe(StorageTemplateService.name, () => {
.calledWith({ id: assetStub.image.id, originalPath: newPath })
.mockResolvedValue(assetStub.image);

when(assetMock.getByIds).calledWith([assetStub.image.id]).mockResolvedValue([assetStub.image]);
when(assetMock.getByIds)
.calledWith([assetStub.image.id], { exifInfo: true })
.mockResolvedValue([assetStub.image]);

when(moveMock.update)
.calledWith({
Expand All @@ -245,7 +249,7 @@ describe(StorageTemplateService.name, () => {

await expect(sut.handleMigrationSingle({ id: assetStub.image.id })).resolves.toBe(true);

expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id]);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id], { exifInfo: true });
expect(storageMock.checkFileExists).toHaveBeenCalledTimes(3);
expect(storageMock.stat).toHaveBeenCalledWith(previousFailedNewPath);
expect(storageMock.rename).toHaveBeenCalledWith(previousFailedNewPath, newPath);
Expand Down Expand Up @@ -275,7 +279,9 @@ describe(StorageTemplateService.name, () => {
.calledWith({ id: assetStub.image.id, originalPath: newPath })
.mockResolvedValue(assetStub.image);

when(assetMock.getByIds).calledWith([assetStub.image.id]).mockResolvedValue([assetStub.image]);
when(assetMock.getByIds)
.calledWith([assetStub.image.id], { exifInfo: true })
.mockResolvedValue([assetStub.image]);

when(moveMock.create)
.calledWith({
Expand All @@ -294,7 +300,7 @@ describe(StorageTemplateService.name, () => {

await expect(sut.handleMigrationSingle({ id: assetStub.image.id })).resolves.toBe(true);

expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id]);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id], { exifInfo: true });
expect(storageMock.checkFileExists).toHaveBeenCalledTimes(1);
expect(storageMock.stat).toHaveBeenCalledWith(newPath);
expect(moveMock.create).toHaveBeenCalledWith({
Expand Down Expand Up @@ -340,7 +346,9 @@ describe(StorageTemplateService.name, () => {
.calledWith({ id: assetStub.image.id, originalPath: newPath })
.mockResolvedValue(assetStub.image);

when(assetMock.getByIds).calledWith([assetStub.image.id]).mockResolvedValue([assetStub.image]);
when(assetMock.getByIds)
.calledWith([assetStub.image.id], { exifInfo: true })
.mockResolvedValue([assetStub.image]);

when(moveMock.update)
.calledWith({
Expand All @@ -358,7 +366,7 @@ describe(StorageTemplateService.name, () => {

await expect(sut.handleMigrationSingle({ id: assetStub.image.id })).resolves.toBe(true);

expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id]);
expect(assetMock.getByIds).toHaveBeenCalledWith([assetStub.image.id], { exifInfo: true });
expect(storageMock.checkFileExists).toHaveBeenCalledTimes(3);
expect(storageMock.stat).toHaveBeenCalledWith(previousFailedNewPath);
expect(storageMock.rename).not.toHaveBeenCalled();
Expand Down
10 changes: 8 additions & 2 deletions server/src/domain/storage-template/storage-template.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,10 @@ export class StorageTemplateService {
return true;
}

const [asset] = await this.assetRepository.getByIds([id]);
const [asset] = await this.assetRepository.getByIds([id], { exifInfo: true });
if (!asset) {
return false;
}

const user = await this.userRepository.get(asset.ownerId, {});
const storageLabel = user?.storageLabel || null;
Expand All @@ -101,7 +104,10 @@ export class StorageTemplateService {

// move motion part of live photo
if (asset.livePhotoVideoId) {
const [livePhotoVideo] = await this.assetRepository.getByIds([asset.livePhotoVideoId]);
const [livePhotoVideo] = await this.assetRepository.getByIds([asset.livePhotoVideoId], { exifInfo: true });
if (!livePhotoVideo) {
return false;
}
const motionFilename = getLivePhotoMotionFilename(filename, livePhotoVideo.originalPath);
await this.moveAsset(livePhotoVideo, { storageLabel, filename: motionFilename });
}
Expand Down
1 change: 1 addition & 0 deletions server/src/infra/entities/asset-face.entity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { AssetEntity } from './asset.entity';
import { PersonEntity } from './person.entity';

@Entity('asset_faces', { synchronize: false })
@Index('IDX_asset_faces_assetId_personId', ['assetId', 'personId'])
@Index(['personId', 'assetId'])
export class AssetFaceEntity {
@PrimaryGeneratedColumn('uuid')
Expand Down
2 changes: 1 addition & 1 deletion server/src/infra/entities/asset-stack.entity.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Column, Entity, JoinColumn, OneToMany, OneToOne, PrimaryGeneratedColumn } from 'typeorm';
import { AssetEntity } from './asset.entity';

@Entity('asset_stack')
@Entity('asset_stack', { synchronize: false })
mertalev marked this conversation as resolved.
Show resolved Hide resolved
export class AssetStackEntity {
@PrimaryGeneratedColumn('uuid')
id!: string;
Expand Down
3 changes: 2 additions & 1 deletion server/src/infra/entities/asset.entity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export const ASSET_CHECKSUM_CONSTRAINT = 'UQ_assets_owner_library_checksum';
@Index('IDX_day_of_month', { synchronize: false })
@Index('IDX_month', { synchronize: false })
@Index('IDX_originalPath_libraryId', ['originalPath', 'libraryId'])
@Index('IDX_asset_id_stackId', ['id', 'stackId'])
@Index('idx_originalFileName_trigram', { synchronize: false })
// For all assets, each originalpath must be unique per user and library
export class AssetEntity {
Expand Down Expand Up @@ -145,7 +146,7 @@ export class AssetEntity {
smartSearch?: SmartSearchEntity;

@ManyToMany(() => TagEntity, (tag) => tag.assets, { cascade: true })
@JoinTable({ name: 'tag_asset' })
@JoinTable({ name: 'tag_asset', synchronize: false })
tags!: TagEntity[];

@ManyToMany(() => SharedLinkEntity, (link) => link.assets, { cascade: true })
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { MigrationInterface, QueryRunner } from "typeorm";

export class AddAssetRelationIndices1710293990203 implements MigrationInterface {
public async up(queryRunner: QueryRunner): Promise<void> {
queryRunner.query(`CREATE INDEX "IDX_asset_id_stackId" on assets ("id", "stackId")`);
queryRunner.query(`CREATE INDEX "IDX_tag_asset_assetsId_tagsId" on tag_asset ("assetsId", "tagsId")`);
queryRunner.query(`CREATE INDEX "IDX_asset_faces_assetId_personId" on asset_faces ("assetId", "personId")`);
}

public async down(queryRunner: QueryRunner): Promise<void> {
queryRunner.query(`DROP INDEX "IDX_asset_id_stackId" on assets ("id", "stackId")`);
queryRunner.query(`DROP INDEX "IDX_tag_asset_assetsId_tagsId" on tag_asset ("assetsId", "tagsId")`);
queryRunner.query(`DROP INDEX "IDX_asset_faces_assetId_personId" on asset_faces ("assetId", "personId")`);
}
}
24 changes: 15 additions & 9 deletions server/src/infra/repositories/asset.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,20 @@ export class AssetRepository implements IAssetRepository {
relations?: FindOptionsRelations<AssetEntity>,
select?: FindOptionsSelect<AssetEntity>,
): Promise<AssetEntity[]> {
if (!relations) {
relations = {
return this.repository.find({
where: { id: In(ids) },
relations,
select,
withDeleted: true,
});
}

@GenerateSql({ params: [[DummyValue.UUID]] })
@ChunkedArray()
getByIdsWithAllRelations(ids: string[]): Promise<AssetEntity[]> {
return this.repository.find({
where: { id: In(ids) },
relations: {
exifInfo: true,
smartInfo: true,
tags: true,
Expand All @@ -148,13 +160,7 @@ export class AssetRepository implements IAssetRepository {
stack: {
assets: true,
},
};
}

return this.repository.find({
where: { id: In(ids) },
relations,
select,
},
withDeleted: true,
});
}
Expand Down