From 99281de6ab9cc465d60a4b16b2eee3a15a04fa9f Mon Sep 17 00:00:00 2001 From: Mees Frensel <33722705+meesfrensel@users.noreply.github.com> Date: Thu, 4 Jun 2026 19:06:28 +0200 Subject: [PATCH] refactor!: disallow star rating < 1 (#27896) Co-authored-by: Daniel Dietzler Co-authored-by: timonrieger --- e2e/src/specs/server/api/asset.e2e-spec.ts | 14 ----- .../repositories/remote_asset.repository.dart | 2 +- .../repositories/search_api.repository.dart | 5 +- .../models/search/search_filter.model.dart | 20 +++++-- .../pages/search/drift_search.page.dart | 9 ++- .../asset_details/rating_details.widget.dart | 2 +- .../asset_viewer/rating_bar.widget.dart | 6 +- .../infrastructure/action.provider.dart | 2 +- .../repositories/asset_api.repository.dart | 2 +- mobile/lib/services/action.service.dart | 2 +- mobile/lib/utils/option.dart | 9 +++ .../search_filter/star_rating_picker.dart | 5 +- .../lib/model/asset_bulk_update_dto.dart | Bin 11065 -> 11064 bytes .../openapi/lib/model/exif_response_dto.dart | Bin 13642 -> 13611 bytes .../lib/model/metadata_search_dto.dart | Bin 39352 -> 39351 bytes .../openapi/lib/model/random_search_dto.dart | Bin 30878 -> 30877 bytes .../openapi/lib/model/smart_search_dto.dart | Bin 32460 -> 32459 bytes .../lib/model/statistics_search_dto.dart | Bin 27624 -> 27623 bytes .../openapi/lib/model/update_asset_dto.dart | Bin 8876 -> 8875 bytes mobile/test/services/action.service_test.dart | 26 +++++++++ open-api/immich-openapi-specs.json | 53 +++++++++++++++--- .../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 | 3 +- ...80592070031-ConvertNegativeRatingToNull.ts | 9 +++ server/src/services/metadata.service.spec.ts | 16 ------ server/src/services/metadata.service.ts | 2 +- server/src/utils/duplicate.spec.ts | 2 +- 29 files changed, 133 insertions(+), 74 deletions(-) create mode 100644 server/src/schema/migrations/1780592070031-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 010b096c4d..7f89cb515d 100644 --- a/e2e/src/specs/server/api/asset.e2e-spec.ts +++ b/e2e/src/specs/server/api/asset.e2e-spec.ts @@ -492,20 +492,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/lib/infrastructure/repositories/remote_asset.repository.dart b/mobile/lib/infrastructure/repositories/remote_asset.repository.dart index 7d4e23c22b..2e4a239a0b 100644 --- a/mobile/lib/infrastructure/repositories/remote_asset.repository.dart +++ b/mobile/lib/infrastructure/repositories/remote_asset.repository.dart @@ -267,7 +267,7 @@ class RemoteAssetRepository extends DriftDatabaseRepository { ); } - Future updateRating(String assetId, int rating) async { + Future updateRating(String assetId, int? rating) async { await (_db.remoteExifEntity.update()..where((row) => row.assetId.equals(assetId))).write( RemoteExifEntityCompanion(rating: Value(rating)), ); diff --git a/mobile/lib/infrastructure/repositories/search_api.repository.dart b/mobile/lib/infrastructure/repositories/search_api.repository.dart index e2f2af17eb..395d4045cf 100644 --- a/mobile/lib/infrastructure/repositories/search_api.repository.dart +++ b/mobile/lib/infrastructure/repositories/search_api.repository.dart @@ -1,6 +1,7 @@ import 'package:immich_mobile/domain/models/asset/base_asset.model.dart' hide AssetVisibility; import 'package:immich_mobile/infrastructure/repositories/api.repository.dart'; import 'package:immich_mobile/models/search/search_filter.model.dart'; +import 'package:immich_mobile/utils/option.dart'; import 'package:openapi/api.dart'; class SearchApiRepository extends ApiRepository { @@ -37,7 +38,7 @@ class SearchApiRepository extends ApiRepository { ? const Optional.absent() : Optional.present(filter.date.takenBefore!), visibility: Optional.present(filter.display.isArchive ? AssetVisibility.archive : AssetVisibility.timeline), - rating: filter.rating.rating == null ? const Optional.absent() : Optional.present(filter.rating.rating!), + rating: filter.rating.rating.toOptional(), isFavorite: filter.display.isFavorite ? const Optional.present(true) : const Optional.absent(), isNotInAlbum: filter.display.isNotInAlbum ? const Optional.present(true) : const Optional.absent(), personIds: Optional.present(filter.people.map((e) => e.id).toList()), @@ -70,7 +71,7 @@ class SearchApiRepository extends ApiRepository { ? const Optional.absent() : Optional.present(filter.date.takenBefore!), visibility: Optional.present(filter.display.isArchive ? AssetVisibility.archive : AssetVisibility.timeline), - rating: filter.rating.rating == null ? const Optional.absent() : Optional.present(filter.rating.rating!), + rating: filter.rating.rating.toOptional(), isFavorite: filter.display.isFavorite ? const Optional.present(true) : const Optional.absent(), isNotInAlbum: filter.display.isNotInAlbum ? const Optional.present(true) : const Optional.absent(), personIds: Optional.present(filter.people.map((e) => e.id).toList()), diff --git a/mobile/lib/models/search/search_filter.model.dart b/mobile/lib/models/search/search_filter.model.dart index cf1a1dcdaf..825acb32c6 100644 --- a/mobile/lib/models/search/search_filter.model.dart +++ b/mobile/lib/models/search/search_filter.model.dart @@ -3,6 +3,7 @@ import 'dart:convert'; import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; import 'package:immich_mobile/domain/models/person.model.dart'; +import 'package:immich_mobile/utils/option.dart'; class SearchLocationFilter { String? country; @@ -133,19 +134,26 @@ class SearchDateFilter { } class SearchRatingFilter { - int? rating; - SearchRatingFilter({this.rating}); + /// none = no filter; some(null) = filter for unrated; some(1-5) = filter for that rating + Option rating; + SearchRatingFilter({this.rating = const Option.none()}); - SearchRatingFilter copyWith({int? rating}) { + SearchRatingFilter copyWith({Option? rating}) { return SearchRatingFilter(rating: rating ?? this.rating); } Map toMap() { - return {'rating': rating}; + if (rating.isNone) { + return {'active': false}; + } + return {'active': true, 'value': rating.unwrapOrNull}; } factory SearchRatingFilter.fromMap(Map map) { - return SearchRatingFilter(rating: map['rating'] != null ? map['rating'] as int : null); + if (!(map['active'] as bool? ?? false)) { + return SearchRatingFilter(); + } + return SearchRatingFilter(rating: Option.some(map['value'] as int?)); } String toJson() => json.encode(toMap()); @@ -270,7 +278,7 @@ class SearchFilter { display.isNotInAlbum == false && display.isArchive == false && display.isFavorite == false && - rating.rating == null && + rating.rating.isNone && mediaType == AssetType.other; } diff --git a/mobile/lib/presentation/pages/search/drift_search.page.dart b/mobile/lib/presentation/pages/search/drift_search.page.dart index 2a40556641..2ae6371f6a 100644 --- a/mobile/lib/presentation/pages/search/drift_search.page.dart +++ b/mobile/lib/presentation/pages/search/drift_search.page.dart @@ -404,12 +404,15 @@ class DriftSearchPage extends HookConsumerWidget { handleClear() { ratingCurrentFilterWidget.value = null; - search(filter.value.copyWith(rating: SearchRatingFilter(rating: null))); + search(filter.value.copyWith(rating: SearchRatingFilter())); } handleApply() { - ratingCurrentFilterWidget.value = rating.rating != null - ? Text('rating_count'.t(args: {'count': rating.rating!}), style: context.textTheme.labelLarge) + ratingCurrentFilterWidget.value = rating.rating.isSome + ? Text( + 'rating_count'.t(args: {'count': rating.rating.unwrapOrNull ?? 0}), + style: context.textTheme.labelLarge, + ) : null; search(filter.value.copyWith(rating: rating)); } diff --git a/mobile/lib/presentation/widgets/asset_viewer/asset_details/rating_details.widget.dart b/mobile/lib/presentation/widgets/asset_viewer/asset_details/rating_details.widget.dart index 1056626119..e501c2ee3e 100644 --- a/mobile/lib/presentation/widgets/asset_viewer/asset_details/rating_details.widget.dart +++ b/mobile/lib/presentation/widgets/asset_viewer/asset_details/rating_details.widget.dart @@ -44,7 +44,7 @@ class RatingDetails extends ConsumerWidget { await ref.read(actionProvider.notifier).updateRating(ActionSource.viewer, rating.round()); }, onClearRating: () async { - await ref.read(actionProvider.notifier).updateRating(ActionSource.viewer, 0); + await ref.read(actionProvider.notifier).updateRating(ActionSource.viewer, null); }, ), ], diff --git a/mobile/lib/presentation/widgets/asset_viewer/rating_bar.widget.dart b/mobile/lib/presentation/widgets/asset_viewer/rating_bar.widget.dart index bd4935e41f..b956ef103c 100644 --- a/mobile/lib/presentation/widgets/asset_viewer/rating_bar.widget.dart +++ b/mobile/lib/presentation/widgets/asset_viewer/rating_bar.widget.dart @@ -77,7 +77,11 @@ class _RatingBarState extends State { setState(() { _currentRating = newRating; }); - widget.onRatingUpdate?.call(newRating.round()); + if (newRating == 0) { + widget.onClearRating?.call(); + } else { + widget.onRatingUpdate?.call(newRating.round()); + } } } diff --git a/mobile/lib/providers/infrastructure/action.provider.dart b/mobile/lib/providers/infrastructure/action.provider.dart index 6fdd9fc5c9..426c028822 100644 --- a/mobile/lib/providers/infrastructure/action.provider.dart +++ b/mobile/lib/providers/infrastructure/action.provider.dart @@ -466,7 +466,7 @@ class ActionNotifier extends Notifier { } } - Future updateRating(ActionSource source, int rating) async { + Future updateRating(ActionSource source, int? rating) async { final ids = _getRemoteIdsForSource(source); if (ids.length != 1) { _logger.warning('updateRating called with multiple assets, expected single asset'); diff --git a/mobile/lib/repositories/asset_api.repository.dart b/mobile/lib/repositories/asset_api.repository.dart index 4a23ced504..0fdfb7e21b 100644 --- a/mobile/lib/repositories/asset_api.repository.dart +++ b/mobile/lib/repositories/asset_api.repository.dart @@ -97,7 +97,7 @@ class AssetApiRepository extends ApiRepository { return _api.updateAsset(assetId, UpdateAssetDto(description: Optional.present(description))); } - Future updateRating(String assetId, int rating) { + Future updateRating(String assetId, int? rating) { return _api.updateAsset(assetId, UpdateAssetDto(rating: Optional.present(rating))); } diff --git a/mobile/lib/services/action.service.dart b/mobile/lib/services/action.service.dart index b22c6680a4..88f33395f2 100644 --- a/mobile/lib/services/action.service.dart +++ b/mobile/lib/services/action.service.dart @@ -231,7 +231,7 @@ class ActionService { return true; } - Future updateRating(String assetId, int rating) async { + Future updateRating(String assetId, int? rating) async { // update remote first, then local to ensure consistency await _assetApiRepository.updateRating(assetId, rating); await _remoteAssetRepository.updateRating(assetId, rating); diff --git a/mobile/lib/utils/option.dart b/mobile/lib/utils/option.dart index 3470e8489e..1d73ddfbcc 100644 --- a/mobile/lib/utils/option.dart +++ b/mobile/lib/utils/option.dart @@ -1,3 +1,5 @@ +import 'package:openapi/api.dart' show Optional; + sealed class Option { const Option(); @@ -56,3 +58,10 @@ final class None extends Option { extension ObjectOptionExtension on T? { Option toOption() => Option.fromNullable(this); } + +extension OptionToOptional on Option { + Optional toOptional() => switch (this) { + None() => const Optional.absent(), + Some(:final value) => Optional.present(value), + }; +} diff --git a/mobile/lib/widgets/search/search_filter/star_rating_picker.dart b/mobile/lib/widgets/search/search_filter/star_rating_picker.dart index 917d56e802..32d1ab5bd4 100644 --- a/mobile/lib/widgets/search/search_filter/star_rating_picker.dart +++ b/mobile/lib/widgets/search/search_filter/star_rating_picker.dart @@ -2,6 +2,7 @@ import 'package:flutter/material.dart'; import 'package:flutter_hooks/flutter_hooks.dart'; import 'package:immich_mobile/extensions/translate_extensions.dart'; import 'package:immich_mobile/models/search/search_filter.model.dart'; +import 'package:immich_mobile/utils/option.dart'; class StarRatingPicker extends HookWidget { const StarRatingPicker({super.key, required this.onSelect, this.filter}); @@ -13,12 +14,12 @@ class StarRatingPicker extends HookWidget { final selectedRating = useState(filter); return RadioGroup( - groupValue: selectedRating.value?.rating, + groupValue: selectedRating.value?.rating.fold((v) => v ?? 0, () => null), onChanged: (int? newValue) { if (newValue == null) { return; } - final newFilter = SearchRatingFilter(rating: newValue); + final newFilter = SearchRatingFilter(rating: Option.some(newValue == 0 ? null : newValue)); selectedRating.value = newFilter; onSelect(newFilter); }, diff --git a/mobile/openapi/lib/model/asset_bulk_update_dto.dart b/mobile/openapi/lib/model/asset_bulk_update_dto.dart index 4ba1546c441552c3babcc3440c77c5f3c1d52eaa..f5e679274f8c3ab5382a96914ad4b50f747e8cdd 100644 GIT binary patch delta 12 TcmdlPwj*pqAK&JFzEv^+C#wa3 delta 14 VcmdlHwli!)A0MOc=03hvG5{}*1+4%8 diff --git a/mobile/openapi/lib/model/exif_response_dto.dart b/mobile/openapi/lib/model/exif_response_dto.dart index 8a0ac0909ce4be28bf62479154c3e8b0f47dd369..2cec6f516162059123b2cc0bb7fce29f1dd58129 100644 GIT binary patch delta 20 ccmX?=wK{9VQ7%Tq$pY+RjHa9aaQSHg09CRFA^-pY delta 16 YcmZ3Tbt-GaQLf2rI6ODM=d#fP07ml%YXATM diff --git a/mobile/openapi/lib/model/metadata_search_dto.dart b/mobile/openapi/lib/model/metadata_search_dto.dart index 0d93ed3173eb8aa279a19e9a12549464b7802703..2fd717b4f05d7e97cc3488c3ee307a973957bfd0 100644 GIT binary patch delta 18 acmdn7nQ8lGrVZcJCTl5+Z{|~9RRjP}+y|im delta 16 YcmdnKnQ6ynrVZcJ7i~#tk{*n{&mhLjXC^2DtzL delta 16 XcmaEUo$ assetApiRepository.updateRating(assetId, 3)).thenAnswer((_) async {}); + when(() => remoteAssetRepository.updateRating(assetId, 3)).thenAnswer((_) async {}); + + final result = await sut.updateRating(assetId, 3); + + expect(result, isTrue); + verify(() => assetApiRepository.updateRating(assetId, 3)).called(1); + verify(() => remoteAssetRepository.updateRating(assetId, 3)).called(1); + }); + + test('calls both repositories with null to clear rating', () async { + when(() => assetApiRepository.updateRating(assetId, null)).thenAnswer((_) async {}); + when(() => remoteAssetRepository.updateRating(assetId, null)).thenAnswer((_) async {}); + + final result = await sut.updateRating(assetId, null); + + expect(result, isTrue); + verify(() => assetApiRepository.updateRating(assetId, null)).called(1); + verify(() => remoteAssetRepository.updateRating(assetId, null)).called(1); + }); + }); + group('ActionService.deleteLocal', () { test('routes deleted ids to trashed repository when Android trash handling is enabled', () async { await Store.put(StoreKey.manageLocalMediaAndroid, true); diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index d8dd7b1a1b..434079e6a1 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -9885,12 +9885,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": "integer", - "minimum": -1, + "minimum": 1, "maximum": 5, "nullable": true } @@ -16330,7 +16335,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": [ @@ -16346,6 +16351,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" @@ -18336,8 +18346,8 @@ "rating": { "default": null, "description": "Rating", - "maximum": 9007199254740991, - "minimum": -9007199254740991, + "maximum": 5, + "minimum": 1, "nullable": true, "type": "integer" }, @@ -19368,7 +19378,7 @@ "rating": { "description": "Filter by rating [1-5], or null for unrated", "maximum": 5, - "minimum": -1, + "minimum": 1, "nullable": true, "type": "integer", "x-immich-history": [ @@ -19384,6 +19394,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" @@ -21041,7 +21056,7 @@ "rating": { "description": "Filter by rating [1-5], or null for unrated", "maximum": 5, - "minimum": -1, + "minimum": 1, "nullable": true, "type": "integer", "x-immich-history": [ @@ -21057,6 +21072,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" @@ -22505,7 +22525,7 @@ "rating": { "description": "Filter by rating [1-5], or null for unrated", "maximum": 5, - "minimum": -1, + "minimum": 1, "nullable": true, "type": "integer", "x-immich-history": [ @@ -22521,6 +22541,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" @@ -22765,7 +22790,7 @@ "rating": { "description": "Filter by rating [1-5], or null for unrated", "maximum": 5, - "minimum": -1, + "minimum": 1, "nullable": true, "type": "integer", "x-immich-history": [ @@ -22781,6 +22806,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" @@ -25960,7 +25990,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": [ @@ -25976,6 +26006,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 acdcb84403..b0191f7217 100644 --- a/server/src/controllers/asset.controller.spec.ts +++ b/server/src/controllers/asset.controller.spec.ts @@ -240,7 +240,7 @@ describe(AssetController.name, () => { for (const [test, errors] of [ [{ rating: 7 }, [{ path: ['rating'], message: 'Too big: expected number to be <=5' }]], [{ rating: 3.5 }, [{ path: ['rating'], message: 'Invalid input: expected int, received number' }]], - [{ rating: -2 }, [{ path: ['rating'], message: 'Too small: expected number to be >=-1' }]], + [{ rating: -2 }, [{ path: ['rating'], message: 'Too small: expected number to be >=1' }]], ] as const) { const { status, body } = await request(ctx.getHttpServer()).put(`/assets/${factory.uuid()}`).send(test); expect(status).toBe(400); @@ -248,16 +248,9 @@ describe(AssetController.name, () => { } }); - 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 1f6124b7db..5072b3aa17 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 d07bbcfb0e..b918c4abd1 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.int().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 39276bffe0..ec4d58dae3 100644 --- a/server/src/dtos/search.dto.ts +++ b/server/src/dtos/search.dto.ts @@ -35,7 +35,7 @@ const BaseSearchSchema = z.object({ albumIds: z.array(z.uuidv4()).optional().describe('Filter by album IDs'), rating: z .int() - .min(-1) + .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/1780592070031-ConvertNegativeRatingToNull.ts b/server/src/schema/migrations/1780592070031-ConvertNegativeRatingToNull.ts new file mode 100644 index 0000000000..f54136dc2c --- /dev/null +++ b/server/src/schema/migrations/1780592070031-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 f5ffe52375..ffb92f3e00 100644 --- a/server/src/services/metadata.service.spec.ts +++ b/server/src/services/metadata.service.spec.ts @@ -1610,22 +1610,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({ - exif: 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 a3e9c19472..70e059d4bf 100644 --- a/server/src/services/metadata.service.ts +++ b/server/src/services/metadata.service.ts @@ -305,7 +305,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 155438f1bd..0c353aedcb 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