mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-11-14 00:01:03 +00:00
fix(storage): stop deleting covers, place images and journey photos another row may still show
Replacing a trip or list cover, changing or deleting a place or saved-place image and deleting a journey photo removed the bytes at once, without asking whether a trip copy, a journey, a saved place or another photo row still named them (#2924). Those paths now leave the object in place and the reclaim job removes it once no row names it. A journey photo the user deletes takes its local photo row with it when nothing else links it, so its bytes become reclaimable instead of being kept by a row nobody sees. A place image copied from a trip file is still dropped at once when the update that would name it fails: that copy was never anyone else's. Size and warning baselines follow the shrunk files.
This commit is contained in:
Maurice
@@ -147,6 +147,11 @@ export class TrekPhotosRepository extends TrekRepository<TrekPhotos> {
|
||||
return await this.nativeDelete({ id, provider: { $ne: 'local' } });
|
||||
}
|
||||
|
||||
/** `DELETE FROM trek_photos WHERE id = ?`, whatever the provider: a photo the user deleted. */
|
||||
async deleteById(id: number): Promise<number> {
|
||||
return await this.nativeDelete({ id });
|
||||
}
|
||||
|
||||
/**
|
||||
* TH1 — `SELECT file_path FROM trek_photos WHERE file_path LIKE 'journey/%'`.
|
||||
* The caller (`ThumbnailService.sweepOrphanThumbs`) always passes the
|
||||
|
||||
@@ -1,11 +1,9 @@
|
||||
import { DomainError } from '../common/domain-error';
|
||||
import { restTripEvents, type TripBroadcast, type TripEventSink } from '../common/trip-writer';
|
||||
import path from 'path';
|
||||
import { Injectable } from '@nestjs/common';
|
||||
import { InjectRepository } from '@mikro-orm/nestjs';
|
||||
import { PermissionsService } from '../permissions/permissions.service';
|
||||
import { RealtimeService } from '../realtime/realtime.service';
|
||||
import { isUploadedPlaceImage } from '../places/place-image';
|
||||
import { PlacesService } from '../places/places.service';
|
||||
import { StorageService } from '../storage/storage.service';
|
||||
import { Collections } from '../../db/entities/Collections.entity';
|
||||
@@ -132,21 +130,6 @@ export class CollectionsService {
|
||||
private readonly places: PlacesService, // the journey hook of every place copyToTrip adds
|
||||
) {}
|
||||
|
||||
/**
|
||||
* Reclaim a replaced cover object (mirrors tripService.deleteOldCover — kept
|
||||
* local so this service doesn't pull the trips import graph). Collection +
|
||||
* trip covers share the 'covers' category; basename() tolerates the stored
|
||||
* /uploads/covers/<name> URL form and any external URL the client saved
|
||||
* (central key validation rejects hostile values; the catch swallows them
|
||||
* like the old containment guard did).
|
||||
*/
|
||||
private async deleteOldCollectionCover(coverImage: string | null | undefined): Promise<void> {
|
||||
if (!coverImage) return;
|
||||
await this.storage.delete('covers', path.basename(coverImage)).catch(() => {
|
||||
/* external URL or already gone */
|
||||
});
|
||||
}
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
// Visibility — a user may see/edit a collection if they own it OR are an
|
||||
// accepted member. Every read/write goes through assertAccess.
|
||||
@@ -618,12 +601,13 @@ export class CollectionsService {
|
||||
return { ...col, is_owner: col.owner_id === userId };
|
||||
}
|
||||
|
||||
/** Set (or clear) a list's cover image, reclaiming the previous file. */
|
||||
/**
|
||||
* Set (or clear) a list's cover image. The previous file stays where another
|
||||
* row may name it; the storage reclaim job removes it once nothing does.
|
||||
*/
|
||||
async setCollectionCover(userId: number, id: number, coverUrl: string | null, socketId?: string): Promise<Collection> {
|
||||
await this.assertCanEdit(userId, id);
|
||||
const prev = (await this.collectionsRepo.coverImage(id)) ?? null;
|
||||
await this.collectionsRepo.setCoverImage(id, coverUrl);
|
||||
if (prev && prev !== coverUrl) await this.deleteOldCollectionCover(prev);
|
||||
await this.notifyCollectionUsers(id, socketId, 'collections:updated');
|
||||
const col = await this.getCollectionRow(id);
|
||||
return { ...col, is_owner: col.owner_id === userId };
|
||||
@@ -893,10 +877,6 @@ export class CollectionsService {
|
||||
const currentCollection = await this.collectionIdOfPlace(placeId);
|
||||
await this.assertCanEdit(userId, currentCollection);
|
||||
|
||||
// Capture the previous thumbnail so a replaced/cleared custom upload (#1136)
|
||||
// can be reclaimed once nothing references it any more.
|
||||
const prevImage = body.image_url !== undefined ? (await this.collectionPlaces.imageUrl(placeId)) ?? null : null;
|
||||
|
||||
const write: Parameters<CollectionPlacesRepository['updateFields']>[1] = {};
|
||||
if (body.name !== undefined) write.name = body.name;
|
||||
if (body.description !== undefined) write.description = body.description ?? null;
|
||||
@@ -934,10 +914,6 @@ export class CollectionsService {
|
||||
if (body.label_ids !== undefined) await this.setPlaceLabels(placeId, movedTo ?? currentCollection, body.label_ids);
|
||||
});
|
||||
|
||||
if (body.image_url !== undefined && prevImage !== (body.image_url ?? null)) {
|
||||
await this.reclaimPlaceImage(prevImage);
|
||||
}
|
||||
|
||||
await this.notifyCollectionUsers(currentCollection, socketId, 'collections:updated');
|
||||
if (movedTo) await this.notifyCollectionUsers(movedTo, socketId, 'collections:updated');
|
||||
return this.getPlaceById(placeId);
|
||||
@@ -968,35 +944,13 @@ export class CollectionsService {
|
||||
return this.getPlaceById(placeId);
|
||||
}
|
||||
|
||||
/**
|
||||
* Delete a custom saved-place-image object once nothing references it any
|
||||
* more (Plan 3h Task 6, replacing `place-image.ts`'s free function — see
|
||||
* that file's own doc comment). A trip place and a collection saved-place
|
||||
* can share the same uploaded file — save-to-collection and copy-to-trip
|
||||
* copy `image_url` by reference — so this ref-counts across both tables
|
||||
* before deleting, mirroring `PlacesService.reclaimPlaceImage`'s shape
|
||||
* exactly: `places` first via `PlacesRepository.existsByImageUrl`
|
||||
* (`tripPlaces`, already injected), `collection_places` second via
|
||||
* `CollectionPlacesRepository.existsByImageUrl` (`collectionPlaces`,
|
||||
* already injected), JS-level `||` short-circuit preserved by early
|
||||
* return. `path.basename()` keeps the storage name confined to the
|
||||
* 'places' category. Best-effort: never throws.
|
||||
*/
|
||||
private async reclaimPlaceImage(url: string | null | undefined): Promise<void> {
|
||||
if (!isUploadedPlaceImage(url)) return;
|
||||
if (await this.tripPlaces.existsByImageUrl(url)) return;
|
||||
if (await this.collectionPlaces.existsByImageUrl(url)) return;
|
||||
await this.storage.delete('places', path.basename(url)).catch(() => {
|
||||
/* best-effort */
|
||||
});
|
||||
}
|
||||
|
||||
// A saved place's uploaded image is never deleted here: copy-to-trip and
|
||||
// save-to-collection share it by reference, and other columns can name it
|
||||
// too. The storage reclaim job removes it once no row does (#2924).
|
||||
async deletePlace(userId: number, placeId: number, socketId?: string): Promise<void> {
|
||||
const collectionId = await this.collectionIdOfPlace(placeId);
|
||||
await this.assertCanDelete(userId, collectionId);
|
||||
const image = (await this.collectionPlaces.imageUrl(placeId)) ?? null;
|
||||
await this.collectionPlaces.deleteById(placeId); // CASCADE drops tags. NO photo-cache reclaim.
|
||||
await this.reclaimPlaceImage(image);
|
||||
await this.notifyCollectionUsers(collectionId, socketId, 'collections:updated');
|
||||
}
|
||||
|
||||
@@ -1007,11 +961,9 @@ export class CollectionsService {
|
||||
// and the deletes then run in one transaction.
|
||||
const deleted: number[] = [];
|
||||
const touched = new Set<number>();
|
||||
const images: (string | null)[] = [];
|
||||
for (const id of ids) {
|
||||
const collectionId = await this.collectionIdOfPlace(id);
|
||||
await this.assertCanDelete(userId, collectionId);
|
||||
images.push((await this.collectionPlaces.imageUrl(id)) ?? null);
|
||||
touched.add(collectionId);
|
||||
}
|
||||
await this.uow.transactional(async () => {
|
||||
@@ -1020,7 +972,6 @@ export class CollectionsService {
|
||||
deleted.push(id);
|
||||
}
|
||||
});
|
||||
for (const image of images) await this.reclaimPlaceImage(image);
|
||||
for (const cid of touched) await this.notifyCollectionUsers(cid, socketId, 'collections:updated');
|
||||
return deleted;
|
||||
}
|
||||
@@ -1108,13 +1059,11 @@ export class CollectionsService {
|
||||
return this.collectionPlaces.matchingByTripSource(collectionIds, tripId, place, COORD_DEDUP_TOLERANCE);
|
||||
}
|
||||
|
||||
/** Set (or clear) a saved place's custom thumbnail, reclaiming the previous upload. */
|
||||
/** Set (or clear) a saved place's custom thumbnail; the storage reclaim job takes the replaced upload. */
|
||||
async setPlaceImage(userId: number, placeId: number, imageUrl: string | null, socketId?: string): Promise<CollectionPlace> {
|
||||
const collectionId = await this.collectionIdOfPlace(placeId);
|
||||
await this.assertCanEdit(userId, collectionId);
|
||||
const prev = (await this.collectionPlaces.imageUrl(placeId)) ?? null;
|
||||
await this.collectionPlaces.setImageUrl(placeId, imageUrl);
|
||||
if (prev !== imageUrl) await this.reclaimPlaceImage(prev);
|
||||
await this.notifyCollectionUsers(collectionId, socketId, 'collections:updated');
|
||||
return this.getPlaceById(placeId);
|
||||
}
|
||||
|
||||
@@ -1496,7 +1496,7 @@ export class JourneyDomainService {
|
||||
// JG107 — cascade on journey_entry_photos.journey_photo_id handles junction cleanup.
|
||||
await this.uow.transactional(async () => {
|
||||
await this.photosRepo.deleteById(journeyPhotoId);
|
||||
await this.photos.deleteIfOrphan(row.photo_id);
|
||||
await this.photos.deleteIfOrphan(row.photo_id, { local: true });
|
||||
});
|
||||
|
||||
return { photo_id: row.photo_id, file_path: trekRow?.file_path ?? null, thumbnail_path: trekRow?.thumbnail_path ?? null };
|
||||
@@ -1558,7 +1558,7 @@ export class JourneyDomainService {
|
||||
// JG116 — same text as JG107.
|
||||
await this.uow.transactional(async () => {
|
||||
await this.photosRepo.deleteById(photoId);
|
||||
await this.photos.deleteIfOrphan(row.photo_id);
|
||||
await this.photos.deleteIfOrphan(row.photo_id, { local: true });
|
||||
});
|
||||
|
||||
return { id: row.id, photo_id: row.photo_id, file_path: trekRow?.file_path ?? null, thumbnail_path: trekRow?.thumbnail_path ?? null, journey_id: row.journey_id };
|
||||
|
||||
@@ -25,7 +25,6 @@ import type { User } from '../../types';
|
||||
import { RuntimeEnvService } from '../app-config/runtime-env.service';
|
||||
import { isDemoWriteBlocked, DEMO_WRITE_ERROR } from '../common/demo-write';
|
||||
import { StorageService } from '../storage/storage.service';
|
||||
import { journeyThumbName } from '../memories/thumbnail.service';
|
||||
import { JourneyService } from './journey.service';
|
||||
import { JourneyBookService } from './journey-book.service';
|
||||
import { JourneyPhotoCaptureService, type ProviderPhotoBatch, type ProviderPhotoTarget } from './journey-photo-capture.service';
|
||||
@@ -460,42 +459,16 @@ export class JourneyController {
|
||||
@Delete('photos/:photoId')
|
||||
@ResponseContract(successResponseSchema)
|
||||
async deletePhoto(@CurrentUser() user: User, @Param('photoId') photoId: string) {
|
||||
// The bytes stay: another journey, a trip or a cover may name the same
|
||||
// object. Once the photo row is gone the storage reclaim job takes the file
|
||||
// and its poster, and the thumbnail sweep its thumbnail (#2924).
|
||||
const photo = await this.journey.deletePhoto(Number(photoId), user.id);
|
||||
if (!photo) {
|
||||
throw new HttpException({ error: 'Photo not found' }, 404);
|
||||
}
|
||||
await this.deletePhotoObject(photo.file_path);
|
||||
await this.reclaimDerived(photo.file_path, photo.thumbnail_path);
|
||||
return { success: true };
|
||||
}
|
||||
|
||||
/**
|
||||
* Best-effort removal of a deleted photo's bytes. file_path rows are the
|
||||
* uploads-relative 'journey/<file>' form every writer stores; anything else
|
||||
* (provider rows have null) has no local object.
|
||||
*/
|
||||
private async deletePhotoObject(filePath: string | null | undefined): Promise<void> {
|
||||
if (!filePath) return;
|
||||
const name = filePath.startsWith('journey/') ? filePath.slice('journey/'.length) : null;
|
||||
if (!name) return;
|
||||
await this.storage.delete('journey', name).catch(() => { /* file already gone */ });
|
||||
}
|
||||
|
||||
/**
|
||||
* Delete a photo's derived artifacts with it (spec In-scope fix #2): the
|
||||
* hash-named thumbnail derivable from file_path, and whatever
|
||||
* thumbnail_path recorded — a generated thumb (usually the same object) or
|
||||
* a video's uploaded poster. Both best-effort, both idempotent.
|
||||
*/
|
||||
private async reclaimDerived(filePath: string | null | undefined, thumbnailPath: string | null | undefined): Promise<void> {
|
||||
if (filePath?.startsWith('journey/')) {
|
||||
await this.storage.delete('journey', journeyThumbName(filePath)).catch(() => { /* already gone */ });
|
||||
}
|
||||
if (thumbnailPath?.startsWith('journey/')) {
|
||||
await this.storage.delete('journey', thumbnailPath.slice('journey/'.length)).catch(() => { /* already gone */ });
|
||||
}
|
||||
}
|
||||
|
||||
// ── Gallery (prefix /:id/gallery — before /:id) ─────────────────────────
|
||||
@Post(':id/gallery/photos')
|
||||
@UseInterceptors(FilesInterceptor('photos'))
|
||||
@@ -573,12 +546,11 @@ export class JourneyController {
|
||||
@HttpCode(204)
|
||||
@ResponseContract(emptyResponseSchema)
|
||||
async deleteGalleryPhoto(@CurrentUser() user: User, @Param('journeyPhotoId') journeyPhotoId: string): Promise<void> {
|
||||
// The bytes stay, as for deletePhoto above.
|
||||
const photo = await this.journey.deleteGalleryPhoto(Number(journeyPhotoId), user.id);
|
||||
if (!photo) {
|
||||
throw new HttpException({ error: 'Photo not found or not allowed' }, 404);
|
||||
}
|
||||
await this.deletePhotoObject(photo.file_path);
|
||||
await this.reclaimDerived(photo.file_path, photo.thumbnail_path);
|
||||
}
|
||||
|
||||
// ── Journeys /:id ───────────────────────────────────────────────────────
|
||||
|
||||
@@ -145,8 +145,10 @@ export class TrekPhotoRegistrationService {
|
||||
|
||||
/**
|
||||
* PH10/PH11 — drop the row once no trip and no journey references it.
|
||||
* Local photos are kept: their bytes are ours, and the file would outlive
|
||||
* the row.
|
||||
* A local photo is kept unless the caller deletes it on purpose (`local`,
|
||||
* a journey photo the user deleted): its row is what keeps its bytes, and
|
||||
* once it is gone the storage reclaim job removes them when no other row
|
||||
* names the file (#2924). An unlink alone keeps a local photo.
|
||||
*
|
||||
* PH10's original statement was one `UNION ALL` existence check across
|
||||
* `trip_photos` and `journey_photos`; split into its two halves here
|
||||
@@ -156,9 +158,10 @@ export class TrekPhotoRegistrationService {
|
||||
* to run if the other already found a row), never a transaction (both are
|
||||
* reads).
|
||||
*/
|
||||
async deleteIfOrphan(photoId: number): Promise<void> {
|
||||
async deleteIfOrphan(photoId: number, options: { local?: boolean } = {}): Promise<void> {
|
||||
if (await this.tripPhotos.existsForPhoto(photoId)) return;
|
||||
if (await this.journeyPhotos.existsForPhoto(photoId)) return; // PH10 — converted (Plan 3g Task 4)
|
||||
await this.trekPhotos.deleteNonLocal(photoId);
|
||||
if (options.local) await this.trekPhotos.deleteById(photoId);
|
||||
else await this.trekPhotos.deleteNonLocal(photoId);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4,22 +4,7 @@ export function placeImageUrl(filename: string): string {
|
||||
return `${URL_PREFIX}${filename}`;
|
||||
}
|
||||
|
||||
export function isUploadedPlaceImage(url: string | null | undefined): url is string {
|
||||
return typeof url === 'string' && url.startsWith(URL_PREFIX);
|
||||
}
|
||||
|
||||
// `reclaimPlaceImage` (the free function this file used to export) is gone
|
||||
// (Plan 3h Task 6, closing out a 3-plan-old carve-out): `PlacesService` had
|
||||
// its own converted private `reclaimPlaceImage` since Plan 3c Task 4
|
||||
// (`places.service.ts`, PI1/PI2 through `PlacesRepository`/
|
||||
// `CollectionPlacesRepository.existsByImageUrl`); `collections.service.ts`'s
|
||||
// four call sites (`updatePlace`, `deletePlace`, `deletePlacesMany`,
|
||||
// `setPlaceImage`) now call a mirroring private `CollectionsService
|
||||
// .reclaimPlaceImage` method instead, through `PlacesRepository
|
||||
// .existsByImageUrl` (already injected there, `tripPlaces`) and
|
||||
// `CollectionPlacesRepository.existsByImageUrl` (already injected,
|
||||
// `collectionPlaces`) — a whole-tree grep for `from '.*place-image'` (run
|
||||
// before this deletion) confirmed zero remaining importers of this function;
|
||||
// `placeImageUrl`/`isUploadedPlaceImage` above still have real importers
|
||||
// (`collections.controller.ts`, `places.controller.ts`, `places.service.ts`)
|
||||
// and stay.
|
||||
// No place or saved-place write deletes a replaced image any more: copies
|
||||
// share the object by reference and other columns can name it, so the storage
|
||||
// reclaim job removes it once no row does (#2924, places-storage-refs.service.ts
|
||||
// and collections-storage-refs.service.ts answer for these columns).
|
||||
|
||||
@@ -20,7 +20,7 @@ import { ratingAggregate } from '../common/rowShape';
|
||||
import { UnsplashService } from '../unsplash/unsplash.service';
|
||||
import { PlacePhotoCacheService } from '../place-photos/place-photo-cache.service';
|
||||
import { type UpdateConflict, isUpdateConflict } from '../common/conflictResult';
|
||||
import { isUploadedPlaceImage, placeImageUrl } from './place-image';
|
||||
import { placeImageUrl } from './place-image';
|
||||
import { MAX_PLACE_IMAGE_SIZE, PLACE_IMAGE_EXTENSIONS } from '../common/place-image-upload';
|
||||
import { randomUUID } from 'node:crypto';
|
||||
import { JourneyDomainService } from '../journey/journey-domain.service';
|
||||
@@ -165,7 +165,8 @@ export class PlacesService {
|
||||
@InjectRepository(Trips) private readonly tripsRepo: TripsRepository,
|
||||
// Plan 3e Task 2 (budget) — additive, PL15/PL18/PL22 only.
|
||||
@InjectRepository(BudgetItems) private readonly budgetItemsRepo: BudgetItemsRepository,
|
||||
// Plan 3h Task 6 (survivors) — additive, SV-PI2's `reclaimPlaceImage` only.
|
||||
// Plan 3h Task 6 (survivors): read by nothing since the storage reclaim job
|
||||
// took over the image ref-count (#2924); kept so the constructor stays put.
|
||||
@InjectRepository(CollectionPlaces) private readonly collectionPlacesRepo: CollectionPlacesRepository,
|
||||
// Reading files and shared lists; this service only persists what it reads.
|
||||
private readonly placeImport: PlaceImportService,
|
||||
@@ -244,37 +245,6 @@ export class PlacesService {
|
||||
this.realtime.broadcast(tripId, event, payload, socketId);
|
||||
}
|
||||
|
||||
/**
|
||||
* Delete a custom place-image object once nothing references it any more
|
||||
* (moved in from `place-image.ts`'s free function `reclaimPlaceImage`,
|
||||
* option (a) — see `place-image.ts`'s own docstring for why that free
|
||||
* function stays, unconverted, for `collections.service.ts`'s own call
|
||||
* sites). A trip place and a collection saved-place can share the same
|
||||
* uploaded file — save-to-collection and copy-to-trip copy `image_url` by
|
||||
* reference — so this ref-counts across both tables before deleting.
|
||||
*
|
||||
* PI1 (`SELECT 1 FROM places WHERE image_url = ? LIMIT 1`) through
|
||||
* `PlacesRepository.existsByImageUrl`; PI2 (`SELECT 1 FROM collection_places
|
||||
* WHERE image_url = ? LIMIT 1`) through `CollectionPlacesRepository
|
||||
* .existsByImageUrl` (Plan 3h Task 6, closing out the carve-out), evaluated
|
||||
* only when PI1 is false — reproducing the legacy `UNION ALL … LIMIT 1`'s
|
||||
* short-circuit by evaluation order, the same pattern
|
||||
* `PlacePhotoCacheService.isReferenced` uses for the same two tables
|
||||
* (Plan 3c Task 1's PP6 ruling). `path.basename()` keeps the storage name
|
||||
* confined to the 'places' category. Best-effort: never throws (central
|
||||
* key validation rejects a hostile stored value; the catch swallows it
|
||||
* exactly like the legacy unlink guard).
|
||||
*/
|
||||
private async reclaimPlaceImage(url: string | null | undefined): Promise<void> {
|
||||
if (!isUploadedPlaceImage(url)) return;
|
||||
if (await this.placesRepo.existsByImageUrl(url)) return;
|
||||
// PI2 (Plan 3h Task 6) — `CollectionPlacesRepository.existsByImageUrl`.
|
||||
if (await this.collectionPlacesRepo.existsByImageUrl(url)) return;
|
||||
await this.storage.delete('places', path.basename(url)).catch(() => {
|
||||
/* best-effort */
|
||||
});
|
||||
}
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
// List places
|
||||
// -------------------------------------------------------------------------
|
||||
@@ -319,7 +289,7 @@ export class PlacesService {
|
||||
/**
|
||||
* Makes a picture already attached in the trip the place's own image (#1242). The file
|
||||
* is copied, not pointed at: the place keeps its picture when the attachment is deleted,
|
||||
* and the update path reclaims the copy like any other uploaded image. Returns a reason
|
||||
* and the storage reclaim job takes the copy like any other uploaded image. Returns a reason
|
||||
* string when the file cannot serve, the updated place otherwise.
|
||||
*/
|
||||
async setImageFromFile(tripId: string, placeId: string, fileId: number): Promise<'not_found' | 'not_image' | 'too_large' | Awaited<ReturnType<PlacesService['update']>>> {
|
||||
@@ -335,7 +305,8 @@ export class PlacesService {
|
||||
const { stream } = await this.storage.getStream('files', path.basename(file.filename));
|
||||
await this.storage.put('places', name, stream, { contentType: mime });
|
||||
const updated = await this.update(tripId, placeId, { image_url: placeImageUrl(name) } as never);
|
||||
if (!updated || isUpdateConflict(updated)) await this.reclaimPlaceImage(placeImageUrl(name));
|
||||
// The copy is this request's own until the update names it, so a failed update drops it here.
|
||||
if (!updated || isUpdateConflict(updated)) await this.storage.delete('places', name).catch(() => undefined);
|
||||
return updated;
|
||||
}
|
||||
|
||||
@@ -447,22 +418,20 @@ export class PlacesService {
|
||||
body: PlaceUpdateInput,
|
||||
ifMatch?: string,
|
||||
): Promise<PlaceWithTags | UpdateConflict | null> {
|
||||
const { result, reclaim } = await this.uow.transactional(() => this.applyUpdate(tripId, placeId, body, ifMatch));
|
||||
if (reclaim !== undefined) await this.reclaimPlaceImage(reclaim);
|
||||
const { result } = await this.uow.transactional(() => this.applyUpdate(tripId, placeId, body, ifMatch));
|
||||
return result;
|
||||
}
|
||||
|
||||
/**
|
||||
* The DB half of update(). Split out so updateMany() can run it inside the
|
||||
* same UnitOfWork transaction and settle the storage reclaims after the
|
||||
* transaction commits.
|
||||
* same UnitOfWork transaction.
|
||||
*/
|
||||
private async applyUpdate(
|
||||
tripId: string,
|
||||
placeId: string,
|
||||
body: PlaceUpdateInput,
|
||||
ifMatch?: string,
|
||||
): Promise<{ result: PlaceWithTags | UpdateConflict | null; reclaim?: string | null }> {
|
||||
): Promise<{ result: PlaceWithTags | UpdateConflict | null }> {
|
||||
// PL9 — the pre-image read every `!== undefined` fallback below needs.
|
||||
// `toRowId` first (Task 3 review H1, absorbed here): every write in
|
||||
// this method (`updatePlace`, `tagsRepo.deleteForPlace`/`insertIgnore`)
|
||||
@@ -555,14 +524,9 @@ export class PlacesService {
|
||||
if (tags.length > 0) await this.tagsRepo.insertIgnore(id, await this.tagsOnTrip(tid, tags)); // PL13
|
||||
}
|
||||
|
||||
// A custom uploaded thumbnail (#1136) that was just replaced or cleared leaves
|
||||
// an orphan file behind — reclaim it (in the caller, once any enclosing
|
||||
// transaction committed) if nothing references it any more.
|
||||
const reclaim = image_url !== undefined && image_url !== existingPlace.image_url
|
||||
? existingPlace.image_url
|
||||
: undefined;
|
||||
|
||||
return { result: await this.placesRepo.findWithTagsAndRatings(id), reclaim }; // PL14
|
||||
// A replaced or cleared custom upload (#1136) stays: a copy of this place or
|
||||
// a cover may name it, and the storage reclaim job removes it once none does.
|
||||
return { result: await this.placesRepo.findWithTagsAndRatings(id) }; // PL14
|
||||
}
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
@@ -654,7 +618,6 @@ export class PlacesService {
|
||||
await this.placesRepo.deleteById(id);
|
||||
});
|
||||
await reclaimPhotoCache(this.photoCache, place.google_place_id, place.image_url);
|
||||
await this.reclaimPlaceImage(place.image_url);
|
||||
return { deleted: true, deletedTourPlaceIds: wasTour ? [id] : [], cancelled };
|
||||
}
|
||||
|
||||
@@ -699,7 +662,6 @@ export class PlacesService {
|
||||
// Reclaim after the transaction commits so isReferenced() sees the final place set.
|
||||
for (const row of reclaimable) {
|
||||
await reclaimPhotoCache(this.photoCache, row.google_place_id, row.image_url);
|
||||
await this.reclaimPlaceImage(row.image_url);
|
||||
}
|
||||
return { deleted, deletedTourPlaceIds, cancelled };
|
||||
}
|
||||
@@ -728,19 +690,14 @@ export class PlacesService {
|
||||
async updateMany(tripId: string, ids: number[], body: PlaceUpdateInput): Promise<PlaceWithTags[]> {
|
||||
if (ids.length === 0) return [];
|
||||
const updated: PlaceWithTags[] = [];
|
||||
const reclaims: (string | null)[] = [];
|
||||
await this.uow.transactional(async () => {
|
||||
for (const id of ids) {
|
||||
// Bulk update sends no If-Match, so applyUpdate() never returns a
|
||||
// conflict here; the guard keeps the types honest.
|
||||
const { result: place, reclaim } = await this.applyUpdate(tripId, String(id), body);
|
||||
const { result: place } = await this.applyUpdate(tripId, String(id), body);
|
||||
if (place && !isUpdateConflict(place)) updated.push(place);
|
||||
if (reclaim !== undefined) reclaims.push(reclaim);
|
||||
}
|
||||
});
|
||||
// Settle reclaims after the transaction commits, so the refcount sees the
|
||||
// final image_url state.
|
||||
for (const reclaim of reclaims) await this.reclaimPlaceImage(reclaim);
|
||||
return updated;
|
||||
}
|
||||
|
||||
|
||||
@@ -200,15 +200,8 @@ export class TripsController {
|
||||
throw new HttpException({ error: 'Could not save the selected cover image' }, 502);
|
||||
}
|
||||
}
|
||||
const oldCover =
|
||||
body.cover_image !== undefined
|
||||
? ((await this.trips.getRaw(id)) as { cover_image: string | null } | undefined)?.cover_image
|
||||
: undefined;
|
||||
try {
|
||||
const result = await this.trips.update(id, user.id, body, user.role);
|
||||
if (body.cover_image !== undefined && body.cover_image !== oldCover) {
|
||||
await this.trips.deleteOldCover(oldCover);
|
||||
}
|
||||
if (Object.keys(result.changes).length > 0) {
|
||||
await this.audit.writeAudit({
|
||||
userId: user.id,
|
||||
@@ -264,8 +257,9 @@ export class TripsController {
|
||||
}
|
||||
// Commit the spooled upload to its final storage location (atomic
|
||||
// same-volume rename) before anything references the final path.
|
||||
// The replaced cover stays: a trip copy or a journey may show the same
|
||||
// object, and the storage reclaim job removes it once nothing names it.
|
||||
await this.storage.put('covers', file.filename, { tmpPath: file.path });
|
||||
await this.trips.deleteOldCover(trip.cover_image);
|
||||
const coverUrl = `/uploads/covers/${file.filename}`;
|
||||
await this.trips.updateCoverImage(id, coverUrl);
|
||||
return { cover_image: coverUrl };
|
||||
|
||||
@@ -1,5 +1,4 @@
|
||||
import { Injectable } from '@nestjs/common';
|
||||
import path from 'path';
|
||||
import { EntityManager } from '@mikro-orm/core';
|
||||
import { Trips } from '../../db/entities/Trips.entity';
|
||||
import { Days } from '../../db/entities/Days.entity';
|
||||
@@ -624,17 +623,6 @@ export class TripsService {
|
||||
|
||||
// ── Cover image ───────────────────────────────────────────────────────────
|
||||
|
||||
async deleteOldCover(coverImage: string | null | undefined): Promise<void> {
|
||||
if (!coverImage) return;
|
||||
// cover_image is client-supplied, so treat it as untrusted: covers are flat
|
||||
// filenames in the 'covers' category — basename() confines the delete to
|
||||
// it, and central key validation rejects anything hostile (swallowed like
|
||||
// the old containment guard; an external https URL is likewise a no-op).
|
||||
await this.storage.delete('covers', path.basename(coverImage)).catch(() => {
|
||||
/* external URL or already gone */
|
||||
});
|
||||
}
|
||||
|
||||
/** TP35 — `TripsRepository.setCoverImage`. */
|
||||
async updateCoverImage(tripId: string | number, coverUrl: string): Promise<void> {
|
||||
await this.tripsRepo.setCoverImage(Number(tripId), coverUrl); // safe: only called after `getRaw`/`getOwner` matched the same id through the raw-bind seam
|
||||
|
||||
Reference in New Issue
Block a user