From 2325a359a6f3f0d092e7f8683a1a790f8895d956 Mon Sep 17 00:00:00 2001 From: Mees Frensel Date: Fri, 17 Apr 2026 14:39:24 +0200 Subject: [PATCH] refactor!: disallow star rating of -1 --- e2e/src/specs/server/api/asset.e2e-spec.ts | 14 ----- mobile/openapi/lib/api/search_api.dart | 8 +-- .../lib/model/asset_bulk_update_dto.dart | 2 +- .../openapi/lib/model/exif_response_dto.dart | 9 +-- .../lib/model/metadata_search_dto.dart | 8 +-- .../openapi/lib/model/random_search_dto.dart | 8 +-- .../openapi/lib/model/smart_search_dto.dart | 8 +-- .../lib/model/statistics_search_dto.dart | 8 +-- .../openapi/lib/model/update_asset_dto.dart | 2 +- open-api/immich-openapi-specs.json | 63 +++++++++++++++---- .../src/controllers/asset.controller.spec.ts | 11 +--- server/src/dtos/asset.dto.ts | 5 +- server/src/dtos/exif.dto.ts | 2 +- server/src/dtos/search.dto.ts | 5 +- ...76429021231-ConvertNegativeRatingToNull.ts | 9 +++ server/src/services/metadata.service.spec.ts | 14 ----- server/src/services/metadata.service.ts | 2 +- server/src/utils/duplicate.spec.ts | 2 +- 18 files changed, 92 insertions(+), 88 deletions(-) create mode 100644 server/src/schema/migrations/1776429021231-ConvertNegativeRatingToNull.ts diff --git a/e2e/src/specs/server/api/asset.e2e-spec.ts b/e2e/src/specs/server/api/asset.e2e-spec.ts index 3fbacd5bf6..f99f697f26 100644 --- a/e2e/src/specs/server/api/asset.e2e-spec.ts +++ b/e2e/src/specs/server/api/asset.e2e-spec.ts @@ -566,20 +566,6 @@ describe('/asset', () => { expect(status).toEqual(200); }); - it('should set the negative rating', async () => { - const { status, body } = await request(app) - .put(`/assets/${user1Assets[0].id}`) - .set('Authorization', `Bearer ${user1.accessToken}`) - .send({ rating: -1 }); - expect(body).toMatchObject({ - id: user1Assets[0].id, - exifInfo: expect.objectContaining({ - rating: -1, - }), - }); - expect(status).toEqual(200); - }); - it('should return tagged people', async () => { const { status, body } = await request(app) .put(`/assets/${user1Assets[0].id}`) diff --git a/mobile/openapi/lib/api/search_api.dart b/mobile/openapi/lib/api/search_api.dart index 730627d4a1..a8afba68b6 100644 --- a/mobile/openapi/lib/api/search_api.dart +++ b/mobile/openapi/lib/api/search_api.dart @@ -404,7 +404,7 @@ class SearchApi { /// * [List] personIds: /// Filter by person IDs /// - /// * [num] rating: + /// * [int] rating: /// Filter by rating [1-5], or null for unrated /// /// * [num] size: @@ -443,7 +443,7 @@ class SearchApi { /// /// * [bool] withExif: /// Include EXIF data in response - Future searchLargeAssetsWithHttpInfo({ List? albumIds, String? city, String? country, DateTime? createdAfter, DateTime? createdBefore, bool? isEncoded, bool? isFavorite, bool? isMotion, bool? isNotInAlbum, bool? isOffline, String? lensModel, String? libraryId, String? make, int? minFileSize, String? model, String? ocr, List? personIds, num? rating, num? size, String? state, List? tagIds, DateTime? takenAfter, DateTime? takenBefore, DateTime? trashedAfter, DateTime? trashedBefore, AssetTypeEnum? type, DateTime? updatedAfter, DateTime? updatedBefore, AssetVisibility? visibility, bool? withDeleted, bool? withExif, }) async { + Future searchLargeAssetsWithHttpInfo({ List? albumIds, String? city, String? country, DateTime? createdAfter, DateTime? createdBefore, bool? isEncoded, bool? isFavorite, bool? isMotion, bool? isNotInAlbum, bool? isOffline, String? lensModel, String? libraryId, String? make, int? minFileSize, String? model, String? ocr, List? personIds, int? rating, num? size, String? state, List? tagIds, DateTime? takenAfter, DateTime? takenBefore, DateTime? trashedAfter, DateTime? trashedBefore, AssetTypeEnum? type, DateTime? updatedAfter, DateTime? updatedBefore, AssetVisibility? visibility, bool? withDeleted, bool? withExif, }) async { // ignore: prefer_const_declarations final apiPath = r'/search/large-assets'; @@ -619,7 +619,7 @@ class SearchApi { /// * [List] personIds: /// Filter by person IDs /// - /// * [num] rating: + /// * [int] rating: /// Filter by rating [1-5], or null for unrated /// /// * [num] size: @@ -658,7 +658,7 @@ class SearchApi { /// /// * [bool] withExif: /// Include EXIF data in response - Future?> searchLargeAssets({ List? albumIds, String? city, String? country, DateTime? createdAfter, DateTime? createdBefore, bool? isEncoded, bool? isFavorite, bool? isMotion, bool? isNotInAlbum, bool? isOffline, String? lensModel, String? libraryId, String? make, int? minFileSize, String? model, String? ocr, List? personIds, num? rating, num? size, String? state, List? tagIds, DateTime? takenAfter, DateTime? takenBefore, DateTime? trashedAfter, DateTime? trashedBefore, AssetTypeEnum? type, DateTime? updatedAfter, DateTime? updatedBefore, AssetVisibility? visibility, bool? withDeleted, bool? withExif, }) async { + Future?> searchLargeAssets({ List? albumIds, String? city, String? country, DateTime? createdAfter, DateTime? createdBefore, bool? isEncoded, bool? isFavorite, bool? isMotion, bool? isNotInAlbum, bool? isOffline, String? lensModel, String? libraryId, String? make, int? minFileSize, String? model, String? ocr, List? personIds, int? rating, num? size, String? state, List? tagIds, DateTime? takenAfter, DateTime? takenBefore, DateTime? trashedAfter, DateTime? trashedBefore, AssetTypeEnum? type, DateTime? updatedAfter, DateTime? updatedBefore, AssetVisibility? visibility, bool? withDeleted, bool? withExif, }) async { final response = await searchLargeAssetsWithHttpInfo( albumIds: albumIds, city: city, country: country, createdAfter: createdAfter, createdBefore: createdBefore, isEncoded: isEncoded, isFavorite: isFavorite, isMotion: isMotion, isNotInAlbum: isNotInAlbum, isOffline: isOffline, lensModel: lensModel, libraryId: libraryId, make: make, minFileSize: minFileSize, model: model, ocr: ocr, personIds: personIds, rating: rating, size: size, state: state, tagIds: tagIds, takenAfter: takenAfter, takenBefore: takenBefore, trashedAfter: trashedAfter, trashedBefore: trashedBefore, type: type, updatedAfter: updatedAfter, updatedBefore: updatedBefore, visibility: visibility, withDeleted: withDeleted, withExif: withExif, ); if (response.statusCode >= HttpStatus.badRequest) { throw ApiException(response.statusCode, await _decodeBodyBytes(response)); diff --git a/mobile/openapi/lib/model/asset_bulk_update_dto.dart b/mobile/openapi/lib/model/asset_bulk_update_dto.dart index f97300b19f..ee55e6fc06 100644 --- a/mobile/openapi/lib/model/asset_bulk_update_dto.dart +++ b/mobile/openapi/lib/model/asset_bulk_update_dto.dart @@ -94,7 +94,7 @@ class AssetBulkUpdateDto { /// Rating in range [1-5], or null for unrated /// - /// Minimum value: -1 + /// Minimum value: 1 /// Maximum value: 5 int? rating; diff --git a/mobile/openapi/lib/model/exif_response_dto.dart b/mobile/openapi/lib/model/exif_response_dto.dart index 64a5a73bed..f4e3ab3813 100644 --- a/mobile/openapi/lib/model/exif_response_dto.dart +++ b/mobile/openapi/lib/model/exif_response_dto.dart @@ -102,7 +102,10 @@ class ExifResponseDto { String? projectionType; /// Rating - num? rating; + /// + /// Minimum value: 1 + /// Maximum value: 5 + int? rating; /// State/province name String? state; @@ -321,9 +324,7 @@ class ExifResponseDto { modifyDate: mapDateTime(json, r'modifyDate', r''), orientation: mapValueOfType(json, r'orientation'), projectionType: mapValueOfType(json, r'projectionType'), - rating: json[r'rating'] == null - ? null - : num.parse('${json[r'rating']}'), + rating: mapValueOfType(json, r'rating'), state: mapValueOfType(json, r'state'), timeZone: mapValueOfType(json, r'timeZone'), ); diff --git a/mobile/openapi/lib/model/metadata_search_dto.dart b/mobile/openapi/lib/model/metadata_search_dto.dart index d49ea7a4e5..d3943905d8 100644 --- a/mobile/openapi/lib/model/metadata_search_dto.dart +++ b/mobile/openapi/lib/model/metadata_search_dto.dart @@ -237,9 +237,9 @@ class MetadataSearchDto { /// Filter by rating [1-5], or null for unrated /// - /// Minimum value: -1 + /// Minimum value: 1 /// Maximum value: 5 - num? rating; + int? rating; /// Number of results to return /// @@ -729,9 +729,7 @@ class MetadataSearchDto { ? (json[r'personIds'] as Iterable).cast().toList(growable: false) : const [], previewPath: mapValueOfType(json, r'previewPath'), - rating: json[r'rating'] == null - ? null - : num.parse('${json[r'rating']}'), + rating: mapValueOfType(json, r'rating'), size: num.parse('${json[r'size']}'), state: mapValueOfType(json, r'state'), tagIds: json[r'tagIds'] is Iterable diff --git a/mobile/openapi/lib/model/random_search_dto.dart b/mobile/openapi/lib/model/random_search_dto.dart index 3f33d8f850..06f3f1407c 100644 --- a/mobile/openapi/lib/model/random_search_dto.dart +++ b/mobile/openapi/lib/model/random_search_dto.dart @@ -145,9 +145,9 @@ class RandomSearchDto { /// Filter by rating [1-5], or null for unrated /// - /// Minimum value: -1 + /// Minimum value: 1 /// Maximum value: 5 - num? rating; + int? rating; /// Number of results to return /// @@ -549,9 +549,7 @@ class RandomSearchDto { personIds: json[r'personIds'] is Iterable ? (json[r'personIds'] as Iterable).cast().toList(growable: false) : const [], - rating: json[r'rating'] == null - ? null - : num.parse('${json[r'rating']}'), + rating: mapValueOfType(json, r'rating'), size: num.parse('${json[r'size']}'), state: mapValueOfType(json, r'state'), tagIds: json[r'tagIds'] is Iterable diff --git a/mobile/openapi/lib/model/smart_search_dto.dart b/mobile/openapi/lib/model/smart_search_dto.dart index bf1465223e..e6398a947b 100644 --- a/mobile/openapi/lib/model/smart_search_dto.dart +++ b/mobile/openapi/lib/model/smart_search_dto.dart @@ -185,9 +185,9 @@ class SmartSearchDto { /// Filter by rating [1-5], or null for unrated /// - /// Minimum value: -1 + /// Minimum value: 1 /// Maximum value: 5 - num? rating; + int? rating; /// Number of results to return /// @@ -589,9 +589,7 @@ class SmartSearchDto { : const [], query: mapValueOfType(json, r'query'), queryAssetId: mapValueOfType(json, r'queryAssetId'), - rating: json[r'rating'] == null - ? null - : num.parse('${json[r'rating']}'), + rating: mapValueOfType(json, r'rating'), size: num.parse('${json[r'size']}'), state: mapValueOfType(json, r'state'), tagIds: json[r'tagIds'] is Iterable diff --git a/mobile/openapi/lib/model/statistics_search_dto.dart b/mobile/openapi/lib/model/statistics_search_dto.dart index d0070e8e12..6f0170bf13 100644 --- a/mobile/openapi/lib/model/statistics_search_dto.dart +++ b/mobile/openapi/lib/model/statistics_search_dto.dart @@ -150,9 +150,9 @@ class StatisticsSearchDto { /// Filter by rating [1-5], or null for unrated /// - /// Minimum value: -1 + /// Minimum value: 1 /// Maximum value: 5 - num? rating; + int? rating; /// Filter by state/province name String? state; @@ -479,9 +479,7 @@ class StatisticsSearchDto { personIds: json[r'personIds'] is Iterable ? (json[r'personIds'] as Iterable).cast().toList(growable: false) : const [], - rating: json[r'rating'] == null - ? null - : num.parse('${json[r'rating']}'), + rating: mapValueOfType(json, r'rating'), state: mapValueOfType(json, r'state'), tagIds: json[r'tagIds'] is Iterable ? (json[r'tagIds'] as Iterable).cast().toList(growable: false) diff --git a/mobile/openapi/lib/model/update_asset_dto.dart b/mobile/openapi/lib/model/update_asset_dto.dart index 2c4c3352ea..927bb2c9f3 100644 --- a/mobile/openapi/lib/model/update_asset_dto.dart +++ b/mobile/openapi/lib/model/update_asset_dto.dart @@ -79,7 +79,7 @@ class UpdateAssetDto { /// Rating in range [1-5], or null for unrated /// - /// Minimum value: -1 + /// Minimum value: 1 /// Maximum value: 5 int? rating; diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index 68dc98e807..6c167297f2 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -9336,12 +9336,17 @@ "version": "v2.6.0", "state": "Updated", "description": "Using -1 as a rating is deprecated and will be removed in the next major version." + }, + { + "version": "v3", + "state": "Updated", + "description": "Using -1 as a rating is no longer valid." } ], "x-immich-state": "Stable", "schema": { - "type": "number", - "minimum": -1, + "type": "integer", + "minimum": 1, "maximum": 5, "nullable": true } @@ -15687,7 +15692,7 @@ "rating": { "description": "Rating in range [1-5], or null for unrated", "maximum": 5, - "minimum": -1, + "minimum": 1, "nullable": true, "type": "integer", "x-immich-history": [ @@ -15703,6 +15708,11 @@ "version": "v2.6.0", "state": "Updated", "description": "Using -1 as a rating is deprecated and will be removed in the next major version." + }, + { + "version": "v3", + "state": "Updated", + "description": "Using -1 as a rating is no longer valid." } ], "x-immich-state": "Stable" @@ -17735,8 +17745,10 @@ "rating": { "default": null, "description": "Rating", + "maximum": 5, + "minimum": 1, "nullable": true, - "type": "number" + "type": "integer" }, "state": { "default": null, @@ -18755,9 +18767,9 @@ "rating": { "description": "Filter by rating [1-5], or null for unrated", "maximum": 5, - "minimum": -1, + "minimum": 1, "nullable": true, - "type": "number", + "type": "integer", "x-immich-history": [ { "version": "v1", @@ -18771,6 +18783,11 @@ "version": "v2.6.0", "state": "Updated", "description": "Using -1 as a rating is deprecated and will be removed in the next major version." + }, + { + "version": "v3", + "state": "Updated", + "description": "Using -1 as a rating is no longer valid." } ], "x-immich-state": "Stable" @@ -20596,9 +20613,9 @@ "rating": { "description": "Filter by rating [1-5], or null for unrated", "maximum": 5, - "minimum": -1, + "minimum": 1, "nullable": true, - "type": "number", + "type": "integer", "x-immich-history": [ { "version": "v1", @@ -20612,6 +20629,11 @@ "version": "v2.6.0", "state": "Updated", "description": "Using -1 as a rating is deprecated and will be removed in the next major version." + }, + { + "version": "v3", + "state": "Updated", + "description": "Using -1 as a rating is no longer valid." } ], "x-immich-state": "Stable" @@ -21990,9 +22012,9 @@ "rating": { "description": "Filter by rating [1-5], or null for unrated", "maximum": 5, - "minimum": -1, + "minimum": 1, "nullable": true, - "type": "number", + "type": "integer", "x-immich-history": [ { "version": "v1", @@ -22006,6 +22028,11 @@ "version": "v2.6.0", "state": "Updated", "description": "Using -1 as a rating is deprecated and will be removed in the next major version." + }, + { + "version": "v3", + "state": "Updated", + "description": "Using -1 as a rating is no longer valid." } ], "x-immich-state": "Stable" @@ -22250,9 +22277,9 @@ "rating": { "description": "Filter by rating [1-5], or null for unrated", "maximum": 5, - "minimum": -1, + "minimum": 1, "nullable": true, - "type": "number", + "type": "integer", "x-immich-history": [ { "version": "v1", @@ -22266,6 +22293,11 @@ "version": "v2.6.0", "state": "Updated", "description": "Using -1 as a rating is deprecated and will be removed in the next major version." + }, + { + "version": "v3", + "state": "Updated", + "description": "Using -1 as a rating is no longer valid." } ], "x-immich-state": "Stable" @@ -25195,7 +25227,7 @@ "rating": { "description": "Rating in range [1-5], or null for unrated", "maximum": 5, - "minimum": -1, + "minimum": 1, "nullable": true, "type": "integer", "x-immich-history": [ @@ -25211,6 +25243,11 @@ "version": "v2.6.0", "state": "Updated", "description": "Using -1 as a rating is deprecated and will be removed in the next major version." + }, + { + "version": "v3", + "state": "Updated", + "description": "Using -1 as a rating is no longer valid." } ], "x-immich-state": "Stable" diff --git a/server/src/controllers/asset.controller.spec.ts b/server/src/controllers/asset.controller.spec.ts index 3c01e3d0a9..3e3b792cfa 100644 --- a/server/src/controllers/asset.controller.spec.ts +++ b/server/src/controllers/asset.controller.spec.ts @@ -214,23 +214,16 @@ describe(AssetController.name, () => { }); it('should reject invalid rating', async () => { - for (const test of [{ rating: 7 }, { rating: 3.5 }, { rating: -2 }]) { + for (const test of [{ rating: 0 }, { rating: 7 }, { rating: 3.5 }, { rating: -2 }]) { const { status, body } = await request(ctx.getHttpServer()).put(`/assets/${factory.uuid()}`).send(test); expect(status).toBe(400); expect(body).toEqual(factory.responses.badRequest()); } }); - it('should convert rating 0 to null', async () => { - const assetId = factory.uuid(); - const { status } = await request(ctx.getHttpServer()).put(`/assets/${assetId}`).send({ rating: 0 }); - expect(service.update).toHaveBeenCalledWith(undefined, assetId, { rating: null }); - expect(status).toBe(200); - }); - it('should leave correct ratings as-is', async () => { const assetId = factory.uuid(); - for (const test of [{ rating: -1 }, { rating: 1 }, { rating: 5 }]) { + for (const test of [{ rating: 1 }, { rating: 5 }]) { const { status } = await request(ctx.getHttpServer()).put(`/assets/${assetId}`).send(test); expect(service.update).toHaveBeenCalledWith(undefined, assetId, test); expect(status).toBe(200); diff --git a/server/src/dtos/asset.dto.ts b/server/src/dtos/asset.dto.ts index 1362a86ed7..7453ec3ad9 100644 --- a/server/src/dtos/asset.dto.ts +++ b/server/src/dtos/asset.dto.ts @@ -14,11 +14,9 @@ const UpdateAssetBaseSchema = z latitude: latitudeSchema.optional().describe('Latitude coordinate'), longitude: longitudeSchema.optional().describe('Longitude coordinate'), rating: z - .number() .int() - .min(-1) + .min(1) .max(5) - .transform((value) => (value === 0 ? null : value)) .nullish() .describe('Rating in range [1-5], or null for unrated') .meta({ @@ -26,6 +24,7 @@ const UpdateAssetBaseSchema = z .added('v1') .stable('v2') .updated('v2.6.0', 'Using -1 as a rating is deprecated and will be removed in the next major version.') + .updated('v3', 'Using -1 as a rating is no longer valid.') .getExtensions(), }), description: z.string().optional().describe('Asset description'), diff --git a/server/src/dtos/exif.dto.ts b/server/src/dtos/exif.dto.ts index c3e1ab36c8..aeeba5f5b6 100644 --- a/server/src/dtos/exif.dto.ts +++ b/server/src/dtos/exif.dto.ts @@ -29,7 +29,7 @@ export const ExifResponseSchema = z country: z.string().nullish().default(null).describe('Country name'), description: z.string().nullish().default(null).describe('Image description'), projectionType: z.string().nullish().default(null).describe('Projection type'), - rating: z.number().nullish().default(null).describe('Rating'), + rating: z.int().min(1).max(5).nullish().default(null).describe('Rating'), }) .describe('EXIF response') .meta({ id: 'ExifResponseDto' }); diff --git a/server/src/dtos/search.dto.ts b/server/src/dtos/search.dto.ts index c0362cdb5d..4fed130fa7 100644 --- a/server/src/dtos/search.dto.ts +++ b/server/src/dtos/search.dto.ts @@ -34,8 +34,8 @@ const BaseSearchSchema = z.object({ tagIds: z.array(z.uuidv4()).nullish().describe('Filter by tag IDs'), albumIds: z.array(z.uuidv4()).optional().describe('Filter by album IDs'), rating: z - .number() - .min(-1) + .int() + .min(1) .max(5) .nullish() .describe('Filter by rating [1-5], or null for unrated') @@ -44,6 +44,7 @@ const BaseSearchSchema = z.object({ .added('v1') .stable('v2') .updated('v2.6.0', 'Using -1 as a rating is deprecated and will be removed in the next major version.') + .updated('v3', 'Using -1 as a rating is no longer valid.') .getExtensions(), }), ocr: z.string().optional().describe('Filter by OCR text content'), diff --git a/server/src/schema/migrations/1776429021231-ConvertNegativeRatingToNull.ts b/server/src/schema/migrations/1776429021231-ConvertNegativeRatingToNull.ts new file mode 100644 index 0000000000..f54136dc2c --- /dev/null +++ b/server/src/schema/migrations/1776429021231-ConvertNegativeRatingToNull.ts @@ -0,0 +1,9 @@ +import { Kysely, sql } from 'kysely'; + +export async function up(db: Kysely): Promise { + await sql`UPDATE "asset_exif" SET "rating" = NULL WHERE "rating" = -1;`.execute(db); +} + +export async function down(): Promise { + // not supported +} diff --git a/server/src/services/metadata.service.spec.ts b/server/src/services/metadata.service.spec.ts index 245bb441a6..95f9415e77 100644 --- a/server/src/services/metadata.service.spec.ts +++ b/server/src/services/metadata.service.spec.ts @@ -1436,20 +1436,6 @@ describe(MetadataService.name, () => { ); }); - it('should handle valid negative rating value', async () => { - const asset = AssetFactory.create(); - mocks.assetJob.getForMetadataExtraction.mockResolvedValue(getForMetadataExtraction(asset)); - mockReadTags({ Rating: -1 }); - - await sut.handleMetadataExtraction({ id: asset.id }); - expect(mocks.asset.upsertExif).toHaveBeenCalledWith( - expect.objectContaining({ - rating: -1, - }), - { lockedPropertiesBehavior: 'skip' }, - ); - }); - it('should handle livePhotoCID not set', async () => { const asset = AssetFactory.create(); mocks.assetJob.getForMetadataExtraction.mockResolvedValue(getForMetadataExtraction(asset)); diff --git a/server/src/services/metadata.service.ts b/server/src/services/metadata.service.ts index c548d94c74..760b06d40b 100644 --- a/server/src/services/metadata.service.ts +++ b/server/src/services/metadata.service.ts @@ -304,7 +304,7 @@ export class MetadataService extends BaseService { // comments description: String(exifTags.ImageDescription || exifTags.Description || '').trim(), profileDescription: exifTags.ProfileDescription || null, - rating: exifTags.Rating === 0 ? null : validateRange(exifTags.Rating, -1, 5), + rating: exifTags.Rating === 0 ? null : validateRange(exifTags.Rating, 1, 5), // grouping livePhotoCID: (exifTags.ContentIdentifier || exifTags.MediaGroupUUID) ?? null, diff --git a/server/src/utils/duplicate.spec.ts b/server/src/utils/duplicate.spec.ts index d63f0d3e32..4ba8408d4b 100644 --- a/server/src/utils/duplicate.spec.ts +++ b/server/src/utils/duplicate.spec.ts @@ -78,7 +78,7 @@ describe('duplicate utils', () => { model: null, latitude: undefined, city: '', - rating: 0, + rating: null, }); // fileSizeInByte (1000) + make ('Canon') = 2 truthy values // model (null), latitude (undefined), city (''), rating (0) are all falsy