Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(mobile): DB repository for asset, backup, sync service #12953

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fyfrey
Copy link
Contributor

@fyfrey fyfrey commented Sep 26, 2024

This PR removes the direct dependency on Isar for the last services: asset, backup and most difficult sync.
To do so, this PR introduces new methods to existing database repositories.

Now, only providers are left to refactor to use the services (or repositories) instead of directly performing database operations.

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Hopefully we're at a point where we can add more unit tests now.

mobile/lib/repositories/asset.repository.dart Show resolved Hide resolved
mobile/lib/services/sync.service.dart Show resolved Hide resolved
mobile/lib/services/sync.service.dart Show resolved Hide resolved
mobile/lib/services/sync.service.dart Outdated Show resolved Hide resolved
@alextran1502
Copy link
Contributor

Hello Fynn, thank you for the PR. Below are some results of my testing

Assertion trigger 🟠

When first signing in an instance, I am seeing this assertion gets triggered, even signing out and singing back in trigger this condition.

flutter: FlutterError - Catch all: 'package:immich_mobile/services/sync.service.dart': Failed assertion: line 141 pos 12: 'dbUsers.isSortedBy((u) => u.id)': dbUsers not sorted!
flutter: [SEVERE] [2024-09-30 11:39:43.479103] PlatformDispatcher - Catch all
flutter: FlutterError - Catch all: 'package:immich_mobile/services/sync.service.dart': Failed assertion: line 141 pos 12: 'dbUsers.isSortedBy((u) => u.id)': dbUsers not sorted!
flutter: [SEVERE] [2024-09-30 11:39:43.479411] PlatformDispatcher - Catch all
flutter: FlutterError - Catch all: 'package:immich_mobile/services/sync.service.dart': Failed assertion: line 141 pos 12: 'dbUsers.isSortedBy((u) => u.id)': dbUsers not sorted!
flutter: [SEVERE] [2024-09-30 11:39:43.479893] PlatformDispatcher - Catch all

Here is the list of users in Isar database

Users json
[
  {
    "avatarColor": 4,
    "email": "[email protected]",
    "hasQuota": false,
    "id": "66950e9c-3a26-4735-9427-ac4480dc5152",
    "inTimeline": false,
    "isAdmin": true,
    "isPartnerSharedBy": false,
    "isPartnerSharedWith": false,
    "isarId": -8267335366296161000,
    "memoryEnabled": true,
    "name": "Admin User",
    "profileImagePath": "",
    "quotaSizeInBytes": 0,
    "quotaUsageInBytes": 11238730230,
    "updatedAt": 1727663393072000
  },
  {
    "avatarColor": 0,
    "email": "[email protected]",
    "hasQuota": false,
    "id": "3ae26239-a033-4b3a-9c89-7684fc598b90",
    "inTimeline": false,
    "isAdmin": false,
    "isPartnerSharedBy": true,
    "isPartnerSharedWith": false,
    "isarId": -7096415854335082000,
    "memoryEnabled": false,
    "name": "Alex Tran",
    "profileImagePath": "",
    "quotaSizeInBytes": 0,
    "quotaUsageInBytes": 0,
    "updatedAt": 1727671173473359
  },
  {
    "avatarColor": 5,
    "email": "[email protected]",
    "hasQuota": false,
    "id": "90505521-71bd-49f9-9650-96dd33ffab5c",
    "inTimeline": false,
    "isAdmin": false,
    "isPartnerSharedBy": false,
    "isPartnerSharedWith": false,
    "isarId": -5032370629639494000,
    "memoryEnabled": false,
    "name": "Test User 3",
    "profileImagePath": "",
    "quotaSizeInBytes": 0,
    "quotaUsageInBytes": 0,
    "updatedAt": 1727671173473360
  },
  {
    "avatarColor": 3,
    "email": "[email protected]",
    "hasQuota": false,
    "id": "d1a02b32-9d95-4ed7-8cf4-26f8710b10be",
    "inTimeline": false,
    "isAdmin": false,
    "isPartnerSharedBy": false,
    "isPartnerSharedWith": false,
    "isarId": 1214268050783358200,
    "memoryEnabled": false,
    "name": "Test User1",
    "profileImagePath": "",
    "quotaSizeInBytes": 0,
    "quotaUsageInBytes": 0,
    "updatedAt": 1727671173473363
  },
  {
    "avatarColor": 4,
    "email": "[email protected]",
    "hasQuota": false,
    "id": "39535b22-fc87-41d9-bfec-d4991f232603",
    "inTimeline": false,
    "isAdmin": false,
    "isPartnerSharedBy": false,
    "isPartnerSharedWith": false,
    "isarId": 5924345563143171000,
    "memoryEnabled": false,
    "name": "Test User 2",
    "profileImagePath": "",
    "quotaSizeInBytes": 0,
    "quotaUsageInBytes": 0,
    "updatedAt": 1727671173473363
  }
]

Asset sync 🟢

After commenting out the assertion condition, the sync process was completed successfully to get all current and partner users' assets.

image image

Album sync 🟢

Album sync also works correctly.

import 'package:immich_mobile/repositories/database.repository.dart';
import 'package:isar/isar.dart';

final etagRepositoryProvider =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we invalidate this in invalidateAllApiRepositoryProviders() as well?

import 'package:immich_mobile/interfaces/database.interface.dart';
import 'package:isar/isar.dart';

const Symbol _zoneTxn = #zoneTxn;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

Future<Album?> getByName(
String name, {
bool? shared,
bool? remote,
});

Future<List<Album>> getAll({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the upcoming layout, I will need to get all albums, both shared and owned. Can it be done with this interface in a single invocation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants