refactor!: disallow star rating < 1 (#27896)

Co-authored-by: Daniel Dietzler <mail@ddietzler.dev>
Co-authored-by: timonrieger <mail@timonrieger.de>

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}`)

mobile/lib/infrastructure/repositories/remote_asset.repository.dart

@@ -267,7 +267,7 @@ class RemoteAssetRepository extends DriftDatabaseRepository {
     );
   }
 
-  Future<void> updateRating(String assetId, int rating) async {
+  Future<void> updateRating(String assetId, int? rating) async {
     await (_db.remoteExifEntity.update()..where((row) => row.assetId.equals(assetId))).write(
       RemoteExifEntityCompanion(rating: Value(rating)),
     );

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()),

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<int?> rating;
+  SearchRatingFilter({this.rating = const Option.none()});
 
-  SearchRatingFilter copyWith({int? rating}) {
+  SearchRatingFilter copyWith({Option<int?>? rating}) {
     return SearchRatingFilter(rating: rating ?? this.rating);
   }
 
   Map<String, dynamic> toMap() {
-    return <String, dynamic>{'rating': rating};
+    if (rating.isNone) {
+      return <String, dynamic>{'active': false};
+    }
+    return <String, dynamic>{'active': true, 'value': rating.unwrapOrNull};
   }
 
   factory SearchRatingFilter.fromMap(Map<String, dynamic> 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;
   }
 

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));
       }

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);
             },
           ),
         ],

mobile/lib/presentation/widgets/asset_viewer/rating_bar.widget.dart

@@ -77,7 +77,11 @@ class _RatingBarState extends State<RatingBar> {
       setState(() {
         _currentRating = newRating;
       });
-      widget.onRatingUpdate?.call(newRating.round());
+      if (newRating == 0) {
+        widget.onClearRating?.call();
+      } else {
+        widget.onRatingUpdate?.call(newRating.round());
+      }
     }
   }
 

mobile/lib/providers/infrastructure/action.provider.dart

@@ -466,7 +466,7 @@ class ActionNotifier extends Notifier<void> {
     }
   }
 
-  Future<ActionResult> updateRating(ActionSource source, int rating) async {
+  Future<ActionResult> updateRating(ActionSource source, int? rating) async {
     final ids = _getRemoteIdsForSource(source);
     if (ids.length != 1) {
       _logger.warning('updateRating called with multiple assets, expected single asset');

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<void> updateRating(String assetId, int rating) {
+  Future<void> updateRating(String assetId, int? rating) {
     return _api.updateAsset(assetId, UpdateAssetDto(rating: Optional.present(rating)));
   }
 

mobile/lib/services/action.service.dart

@@ -231,7 +231,7 @@ class ActionService {
     return true;
   }
 
-  Future<bool> updateRating(String assetId, int rating) async {
+  Future<bool> updateRating(String assetId, int? rating) async {
     // update remote first, then local to ensure consistency
     await _assetApiRepository.updateRating(assetId, rating);
     await _remoteAssetRepository.updateRating(assetId, rating);

mobile/lib/utils/option.dart

@@ -1,3 +1,5 @@
+import 'package:openapi/api.dart' show Optional;
+
 sealed class Option<T> {
   const Option();
 
@@ -56,3 +58,10 @@ final class None<T> extends Option<T> {
 extension ObjectOptionExtension<T> on T? {
   Option<T> toOption() => Option.fromNullable(this);
 }
+
+extension OptionToOptional<T> on Option<T> {
+  Optional<T> toOptional() => switch (this) {
+    None() => const Optional.absent(),
+    Some(:final value) => Optional.present(value),
+  };
+}

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);
       },

mobile/test/services/action.service_test.dart

@@ -73,6 +73,32 @@ void main() {
     await Store.clear();
   });
 
+  group('ActionService.updateRating', () {
+    const assetId = 'asset_id_1';
+
+    test('calls both repositories with the given rating', () async {
+      when(() => 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);

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"

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);

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'),

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' });

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'),

server/src/schema/migrations/1780592070031-ConvertNegativeRatingToNull.ts

@@ -0,0 +1,9 @@
+import { Kysely, sql } from 'kysely';
+
+export async function up(db: Kysely<any>): Promise<void> {
+  await sql`UPDATE "asset_exif" SET "rating" = NULL WHERE "rating" = -1;`.execute(db);
+}
+
+export async function down(): Promise<void> {
+  // not supported
+}

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));

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,

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