Allow non-read-only external libraries that support asset deletion #8943

valpackett · 2024-04-20T08:20:19Z

Related: #5449 #8438
Sponsored by: @benmccann

This is an attempt at switching from hardcoded "external means cannot delete" to an explicit isReadOnly flag for libraries. (Not fully polished change yet, hence draft)

The per-asset isExternal and isReadOnly flags being stored in the database is making this difficult… I guess just using them and updating them while the library's read-onliness changes would be significantly easier than removing them and making everywhere query the library flags, but I'm looking for feedback re: the right way to do this.

…d-only

alextran1502 · 2024-04-20T08:30:43Z

I think the most natural change of this feature is to let the user know they can mount the library with ro or without that flag for docker mount; then, we can treat external library assets as normal upload assets to avoid introducing an additional boolean flag in the system. What do you think?

valpackett · 2024-04-20T08:55:12Z

Hm. That would mean checking mount info (only on boot? on rescan too?) and updating the flags based on that. Seems nice in terms of not adding any admin UI. I'm slightly worried about potential surprising behavior (e.g. adding a writable mount, possibly for multiple libraries inside of it, and adding libraries in the admin interface, not expecting them to be automatically writable) but on the other hand "it's a purely outside configured sysadmin level decision" does feel nice in a way.

Not sure we'd actually avoid the flag on the library, even if it would get set automatically based on the mount: it would definitely make sense to show in the admin interface whether it is writable, and have that fully consistent with what we've set for the assets…

alextran1502 · 2024-04-20T09:08:51Z

@etnoy I am pretty sure isReadOnly flag isn't used anymore after we move to your implementation of external library, correct? It was for the previous import mechanism

alextran1502 · 2024-04-20T09:10:57Z

Hm. That would mean checking mount info (only on boot? on rescan too?) and updating the flags based on that. Seems nice in terms of not adding any admin UI. I'm slightly worried about potential surprising behavior (e.g. adding a writable mount, possibly for multiple libraries inside of it, and adding libraries in the admin interface, not expecting them to be automatically writable) but on the other hand "it's a purely outside configured sysadmin level decision" does feel nice in a way.

Not sure we'd actually avoid the flag on the library, even if it would get set automatically based on the mount: it would definitely make sense to show in the admin interface whether it is writable, and have that fully consistent with what we've set for the assets…

I am thinking more of, if the user know that they use the ro flag in docker mount, the deletion call will error out expectedly, otherwise, the operation will happen as if it the assets are from the upload library. So we don't necessary need to do anything special here in term of checking the state of read/write of the mount

etnoy · 2024-04-20T09:10:58Z

@etnoy I am pretty sure isReadOnly flag isn't used anymore after we move to your implementation of external library, correct? It was for the previous import mechanism

That is my understanding, too.

I've argued for removing the field but I think @jrasm91 knew of something that still used it

jrasm91 · 2024-04-21T13:55:35Z

I would recommend keeping the per asset flags. Otherwise it will probably be a nightmare trying to load in the property everywhere it needs to be used. If a library is marked as read only, it seems easy enough to do a bulk asset update

jrasm91

This seems fine, but should be simplified. Furthermore, when a library is updated and readonly changes we should SQL update the affected assets.

jrasm91 · 2024-04-27T12:07:53Z

server/src/dtos/library.dto.ts

@@ -128,6 +131,8 @@ export class LibraryResponseDto {
  createdAt!: Date;
  updatedAt!: Date;
  refreshedAt!: Date | null;
+
+  isReadOnly!: boolean | null;


This isn't nullable.

jrasm91 · 2024-04-27T12:08:17Z

server/src/entities/library.entity.ts

@@ -53,6 +53,9 @@ export class LibraryEntity {

  @Column({ type: 'boolean', default: true })
  isVisible!: boolean;
+
+  @Column({ type: 'boolean', nullable: true })
+  isReadOnly!: boolean | null;


This should not be nullable and should default to true

jrasm91 · 2024-04-27T12:09:52Z

server/src/services/asset.service.ts

+    // Ignore requests that are not from external library job but is for an external read-only asset
+    const isReadOnlyLibrary = !asset.library || asset.library.isReadOnly ||
+      (asset.library.isReadOnly === null && asset.library.type === LibraryType.EXTERNAL);
+    if (!fromExternal && isReadOnlyLibrary) {


This should just check if from external and the asset is read only and skip if so. No reason to make it complicated.

jrasm91 · 2024-04-27T12:11:52Z

server/src/services/asset.service.ts

@@ -406,7 +408,7 @@ export class AssetService {
    }

    const files = [asset.thumbnailPath, asset.previewPath, asset.encodedVideoPath];
-    if (!(asset.isExternal || asset.isReadOnly)) {
+    if (!isReadOnlyLibrary) {


If asset isn't readonly

jrasm91 · 2024-04-27T12:12:24Z

web/src/lib/components/asset-viewer/asset-viewer-nav-bar.svelte

@@ -193,7 +193,7 @@
    {/if}

    {#if isOwner}
-      {#if !asset.isReadOnly || !asset.isExternal}
+      {#if !asset.isReadOnly || !asset.isExternal || (true /* XXX */)}


Just change this to asset not read only

jrasm91 · 2024-04-27T12:13:11Z

web/src/routes/admin/library-management/+page.svelte

+    const prevValue = library.isReadOnly == null ? (library.type === LibraryType.External) : library.isReadOnly;
+
+    try {
+      await updateLibrary({ id: library.id, updateLibraryDto: { ...library, isReadOnly: !prevValue } });


Please don't send the whole dto. Just send the read only setting.

jrasm91 · 2024-04-27T12:13:39Z

web/src/routes/admin/library-management/+page.svelte

@@ -397,6 +410,12 @@
                    <Portal target="body">
                      <ContextMenu {...contextMenuPosition} on:outclick={() => onMenuExit()}>
                        <MenuOption on:click={() => onRenameClicked()} text={`Rename`} />
+                        <MenuOption
+                          on:click={() => onToggleReadOnlyClicked()}
+                          text={(selectedLibrary?.isReadOnly ||


This should not be nullable

we could probably change the {#if showContextMenu} above to {#if showContextMenu && selectedLibrary} to simplify all the accesses to selectedLibrary within

benmccann · 2024-04-30T20:47:28Z

open-api/immich-openapi-specs.json

@@ -8778,6 +8778,10 @@
            },
            "type": "array"
          },
+          "isReadOnly": {
+            "nullable": true,


I think this shouldn't be nullable either

jrasm91 · 2024-05-03T01:20:20Z

Hey, thanks for working on this and sorry for the confusion/back and forth around this topic. Internally we have had a lot of discussions about the direction we want to take external libraries. Long story short, we decided to go ahead with the implementation laid out in #9235.

valpackett added 3 commits April 20, 2024 04:56

feat(server): add isReadOnly property to the Library model

11cade0

feat(server,web): allow deletion in external libraries marked not-rea…

c9f82db

…d-only

chore(server): add test for deleting external asset

49ac546

bo0tzz added 🗄️server external-library Issues related to external libraries labels Apr 20, 2024

jrasm91 reviewed Apr 27, 2024

View reviewed changes

jrasm91 mentioned this pull request Apr 27, 2024

Feature: Read-only property for External Libraries #8992

Closed

benmccann reviewed Apr 30, 2024

View reviewed changes

benmccann mentioned this pull request Apr 30, 2024

Option to disable readonly status of external libraries #9188

Closed

jrasm91 mentioned this pull request May 3, 2024

feat(server): read-write external assets #9235

Merged

jrasm91 closed this May 3, 2024

valpackett deleted the library-rdonly branch May 3, 2024 03:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow non-read-only external libraries that support asset deletion #8943

Allow non-read-only external libraries that support asset deletion #8943

valpackett commented Apr 20, 2024

alextran1502 commented Apr 20, 2024

valpackett commented Apr 20, 2024

alextran1502 commented Apr 20, 2024

alextran1502 commented Apr 20, 2024

etnoy commented Apr 20, 2024

jrasm91 commented Apr 21, 2024

jrasm91 left a comment

jrasm91 Apr 27, 2024

jrasm91 Apr 27, 2024

jrasm91 Apr 27, 2024

jrasm91 Apr 27, 2024

jrasm91 Apr 27, 2024

jrasm91 Apr 27, 2024

jrasm91 Apr 27, 2024

benmccann Apr 27, 2024

benmccann Apr 30, 2024

jrasm91 commented May 3, 2024

Allow non-read-only external libraries that support asset deletion #8943

Allow non-read-only external libraries that support asset deletion #8943

Conversation

valpackett commented Apr 20, 2024

alextran1502 commented Apr 20, 2024

valpackett commented Apr 20, 2024

alextran1502 commented Apr 20, 2024

alextran1502 commented Apr 20, 2024

etnoy commented Apr 20, 2024

jrasm91 commented Apr 21, 2024

jrasm91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrasm91 commented May 3, 2024