Skip to content

Relax fileExtension requirement in attachmentQueue #269

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class PhotoAttachmentQueue extends AbstractAttachmentQueue {
}

@override
StreamSubscription<void> watchIds({String fileExtension = 'jpg'}) {
StreamSubscription<void> watchIds({String? fileExtension}) {
log.info('Watching photos in $todosTable...');
return db.watch('''
SELECT photo_id FROM $todosTable
Expand Down
2 changes: 1 addition & 1 deletion demos/supabase-todolist/lib/attachments/queue.dart
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class PhotoAttachmentQueue extends AbstractAttachmentQueue {
}

@override
StreamSubscription<void> watchIds({String fileExtension = 'jpg'}) {
StreamSubscription<void> watchIds({String? fileExtension}) {
log.info('Watching photos in $todosTable...');
return db.watch('''
SELECT photo_id FROM $todosTable
Expand Down
2 changes: 1 addition & 1 deletion packages/powersync_attachments_helper/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class PhotoAttachmentQueue extends AbstractAttachmentQueue {
// This watcher will handle adding items to the queue based on
// a users table element receiving a photoId
@override
StreamSubscription<void> watchIds({String fileExtension = 'jpg'}) {
StreamSubscription<void> watchIds({String? fileExtension}) {
return db.watch('''
SELECT photo_id FROM users
WHERE photo_id IS NOT NULL
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class PhotoAttachmentQueue extends AbstractAttachmentQueue {
}

@override
StreamSubscription<void> watchIds({String fileExtension = 'jpg'}) {
StreamSubscription<void> watchIds({String? fileExtension}) {
return db.watch('''
SELECT photo_id FROM users
WHERE photo_id IS NOT NULL
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ abstract class AbstractAttachmentQueue {
/// when the attachment queue is initialized.
List<String>? subdirectories;

/// File extension to be used for the attachments queue
/// Can be left null if no extension is used or if extension is part of the filename
String? fileExtension;

AbstractAttachmentQueue(
{required this.db,
required this.remoteStorage,
Expand All @@ -49,7 +53,8 @@ abstract class AbstractAttachmentQueue {
this.onDownloadError,
this.onUploadError,
this.intervalInMinutes = 5,
this.subdirectories}) {
this.subdirectories,
this.fileExtension}) {
attachmentsService = AttachmentsService(
db, localStorage, attachmentDirectoryName, attachmentsQueueTableName);
syncingService = SyncingService(
Expand All @@ -59,7 +64,7 @@ abstract class AbstractAttachmentQueue {

/// Create watcher to get list of ID's from a table to be used for syncing in the attachment queue.
/// Set the file extension if you are using a different file type
StreamSubscription<void> watchIds({String fileExtension = 'jpg'});
StreamSubscription<void> watchIds({String? fileExtension});
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering that this is already a breaking change, I wonder if we should just remove the parameter entirely and transfer the responsibility to include file extensions in the id instead of supporting both modes.

That would also involve removing the fileExtension parameter from processIds.


/// Create a function to save files using the attachment queue
Future<Attachment> saveFile(String fileId, int size);
Expand All @@ -82,7 +87,7 @@ abstract class AbstractAttachmentQueue {
}
}

watchIds();
watchIds(fileExtension: fileExtension);
syncingService.watchAttachments();
syncingService.startPeriodicSync(intervalInMinutes);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import 'package:path_provider/path_provider.dart';
/// Storage adapter for local storage
class LocalStorageAdapter {
Future<File> saveFile(String fileUri, Uint8List data) async {
await makeDir(fileUri);
final file = File(fileUri);
return await file.writeAsBytes(data);
}
Expand All @@ -30,12 +31,13 @@ class LocalStorageAdapter {
Future<void> makeDir(String fileUri) async {
bool exists = await fileExists(fileUri);
if (!exists) {
Directory newDirectory = Directory(fileUri);
await newDirectory.create(recursive: true);
Directory parentDirectories = File(fileUri).parent;
await parentDirectories.create(recursive: true);
Comment on lines +34 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change necessary? Looking at how the method is called in init, it looks like it's really supposed to create a directory at fileUri. This only creates the parent directory.

I think creating nested directories automatically is the correct behavior, but then you could call await makeDir(dirname(targetUri)). The same thing could also be reasonable for saveFile.

}
}

Future<void> copyFile(String sourceUri, String targetUri) async {
await makeDir(targetUri);
File file = File(sourceUri);
await file.copy(targetUri);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,12 @@ class SyncingService {
}

/// Process ID's to be included in the attachment queue.
Future<void> processIds(List<String> ids, String fileExtension) async {
Future<void> processIds(List<String> ids, String? fileExtension) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

To follow up on Steven's suggestion, we could remove fileExtension here and replace List<String> ids with something like List<WatchedAttachmentItem>, where WatchedAttachmentItem is a simple class with a String id and a String? extension field.

Then here, we could take the extension of each item or fall back to the default extension set on the AbstractAttachmentQueue.

List<Attachment> attachments = List.empty(growable: true);

for (String id in ids) {
String path = await getLocalUri('$id.$fileExtension');
String filename = fileExtension != null ? '$id.$fileExtension' : id;
String path = await getLocalUri(filename);
File file = File(path);
bool fileExists = await file.exists();

Expand All @@ -180,7 +181,7 @@ class SyncingService {
log.info('Adding $id to queue');
attachments.add(Attachment(
id: id,
filename: '$id.$fileExtension',
filename: filename,
state: AttachmentState.queuedDownload.index));
}

Expand Down