fix(background): when list background is removed, delete file from file system and DB #1372

Merged
konrad merged 1 commits from :main into main 2023-02-01 11:38:24 +00:00
First-time contributor

Currently, if a list background is removed, the background file stays in disk and in the DB forever.

Fix this by deleting the file during background removal, similar to how it's done when a new background is uploaded: source

Test Plan:
A) Before these changes
A.1) Create a new list
A.2) Set a new background to the list
A.3) Check file is in disk and DB:

$ ls files/
1

$ sqlite3 vikunja.db 
SQLite version 3.40.1 2022-12-28 14:03:47
Enter ".help" for usage hints.
sqlite> select * from files;
1|20220410_180833.jpg||2822966|2023-01-25 21:25:28|1
sqlite> select * from lists;
1|Teste||||1|1|0|1|LeGlCmIqR*of_4R+WCoe4;s.oLjZ|65536.0|2023-01-25 21:25:21|2023-01-25 21:25:28

A.4) Remove background from list
A.5) Check file is still in disk and DB:

$ ls files/
1

$ sqlite3 vikunja.db 
SQLite version 3.40.1 2022-12-28 14:03:47
Enter ".help" for usage hints.
sqlite> select * from files;
1|20220410_180833.jpg||2822966|2023-01-25 21:25:28|1
sqlite> select * from lists;
1|Teste||||1|1|0|0||65536.0|2023-01-25 21:25:21|2023-01-25 21:32:39

B) After these changes
B.1) Set background again
B.2) Check files in disk and DB:

$ ls files/
1 2

$ sqlite3 vikunja.db 
SQLite version 3.40.1 2022-12-28 14:03:47
Enter ".help" for usage hints.
sqlite> select * from files;
1|20220410_180833.jpg||2822966|2023-01-25 21:25:28|1
2|20220410_180833.jpg||2822966|2023-01-25 21:34:33|1
sqlite> select * from lists;
1|Teste||||1|1|0|2|LeGlCmIqR*of_4R+WCoe4;s.oLjZ|65536.0|2023-01-25 21:25:21|2023-01-25 21:34:33

B.3) Remove background image
B.4) Check files and DB again, verify image 2 has been deleted:

$ ls files/
1

$ sqlite3 vikunja.db 
SQLite version 3.40.1 2022-12-28 14:03:47
Enter ".help" for usage hints.
sqlite> select * from files;
1|20220410_180833.jpg||2822966|2023-01-25 21:25:28|1
sqlite> select * from lists;
1|Teste||||1|1|0|0||65536.0|2023-01-25 21:25:21|2023-01-25 21:36:13
Currently, if a list background is removed, the background file stays in disk and in the DB forever. Fix this by deleting the file during background removal, similar to how it's done when a new background is uploaded: [source](https://kolaente.dev/vikunja/api/src/commit/726a517bec731f1af8e3186e280718fef02cadf7/pkg/modules/background/upload/upload.go#L56-L62) Test Plan: A) Before these changes A.1) Create a new list A.2) Set a new background to the list A.3) Check file is in disk and DB: ``` $ ls files/ 1 $ sqlite3 vikunja.db SQLite version 3.40.1 2022-12-28 14:03:47 Enter ".help" for usage hints. sqlite> select * from files; 1|20220410_180833.jpg||2822966|2023-01-25 21:25:28|1 sqlite> select * from lists; 1|Teste||||1|1|0|1|LeGlCmIqR*of_4R+WCoe4;s.oLjZ|65536.0|2023-01-25 21:25:21|2023-01-25 21:25:28 ``` A.4) Remove background from list A.5) Check file is still in disk and DB: ``` $ ls files/ 1 $ sqlite3 vikunja.db SQLite version 3.40.1 2022-12-28 14:03:47 Enter ".help" for usage hints. sqlite> select * from files; 1|20220410_180833.jpg||2822966|2023-01-25 21:25:28|1 sqlite> select * from lists; 1|Teste||||1|1|0|0||65536.0|2023-01-25 21:25:21|2023-01-25 21:32:39 ``` B) After these changes B.1) Set background again B.2) Check files in disk and DB: ``` $ ls files/ 1 2 $ sqlite3 vikunja.db SQLite version 3.40.1 2022-12-28 14:03:47 Enter ".help" for usage hints. sqlite> select * from files; 1|20220410_180833.jpg||2822966|2023-01-25 21:25:28|1 2|20220410_180833.jpg||2822966|2023-01-25 21:34:33|1 sqlite> select * from lists; 1|Teste||||1|1|0|2|LeGlCmIqR*of_4R+WCoe4;s.oLjZ|65536.0|2023-01-25 21:25:21|2023-01-25 21:34:33 ``` B.3) Remove background image B.4) Check files and DB again, verify image 2 has been deleted: ``` $ ls files/ 1 $ sqlite3 vikunja.db SQLite version 3.40.1 2022-12-28 14:03:47 Enter ".help" for usage hints. sqlite> select * from files; 1|20220410_180833.jpg||2822966|2023-01-25 21:25:28|1 sqlite> select * from lists; 1|Teste||||1|1|0|0||65536.0|2023-01-25 21:25:21|2023-01-25 21:36:13 ```
Ghost added 1 commit 2023-01-25 21:40:20 +00:00
continuous-integration/drone/pr Build is failing Details
01c9d2edb9
fix(background): when list background is removed, delete file from file system and DB
Currently, if a list background is removed, the background file stays in disk and in the DB forever.

Fix this by deleting the file during background removal, similar to how it's done when a new background is uploaded: [source](726a517bec/pkg/modules/background/upload/upload.go (L56-L62))

Test Plan:
A) Before these changes
A.1) Create a new list
A.2) Set a new background to the list
A.3) Check file is in disk and DB:
```
$ ls files/
1

$ sqlite3 vikunja.db 
SQLite version 3.40.1 2022-12-28 14:03:47
Enter ".help" for usage hints.
sqlite> select * from files;
1|20220410_180833.jpg||2822966|2023-01-25 21:25:28|1
sqlite> select * from lists;
1|Teste||||1|1|0|1|LeGlCmIqR*of_4R+WCoe4;s.oLjZ|65536.0|2023-01-25 21:25:21|2023-01-25 21:25:28
```

A.4) Remove background from list
A.5) Check file is still in disk and DB:
```
$ ls files/
1

$ sqlite3 vikunja.db 
SQLite version 3.40.1 2022-12-28 14:03:47
Enter ".help" for usage hints.
sqlite> select * from files;
1|20220410_180833.jpg||2822966|2023-01-25 21:25:28|1
sqlite> select * from lists;
1|Teste||||1|1|0|0||65536.0|2023-01-25 21:25:21|2023-01-25 21:32:39
```

B) After these changes
B.1) Set background again
B.2) Check files in disk and DB:
```
$ ls files/
1 2

$ sqlite3 vikunja.db 
SQLite version 3.40.1 2022-12-28 14:03:47
Enter ".help" for usage hints.
sqlite> select * from files;
1|20220410_180833.jpg||2822966|2023-01-25 21:25:28|1
2|20220410_180833.jpg||2822966|2023-01-25 21:34:33|1
sqlite> select * from lists;
1|Teste||||1|1|0|2|LeGlCmIqR*of_4R+WCoe4;s.oLjZ|65536.0|2023-01-25 21:25:21|2023-01-25 21:34:33
```

B.3) Remove background image
B.4) Check files and DB again, verify image 2 has been deleted:
```
$ ls files/
1

$ sqlite3 vikunja.db 
SQLite version 3.40.1 2022-12-28 14:03:47
Enter ".help" for usage hints.
sqlite> select * from files;
1|20220410_180833.jpg||2822966|2023-01-25 21:25:28|1
sqlite> select * from lists;
1|Teste||||1|1|0|0||65536.0|2023-01-25 21:25:21|2023-01-25 21:36:13
```
Owner

Good catch! Can you add a test for this?

Good catch! Can you add a test for this?
konrad reviewed 2023-01-26 09:45:11 +00:00
@ -332,1 +332,4 @@
}
// Remove the old background if one exists
if list.BackgroundFileID != 0 {
Owner

Can you move this to a function and use that function in 726a517bec/pkg/modules/background/upload/upload.go (L56-L62)
as well?

Can you move this to a function and use that function in https://kolaente.dev/vikunja/api/src/commit/726a517bec731f1af8e3186e280718fef02cadf7/pkg/modules/background/upload/upload.go#L56-L62 as well?
Owner

Could be a function for the List type

Could be a function for the `List` type
Member

Nice!

Would it make sense to add a command to the cli to clean up all the files that were already deleted before?

Nice! Would it make sense to add a command to the cli to clean up all the files that were already deleted before?
Author
First-time contributor

Nice!

Would it make sense to add a command to the cli to clean up all the files that were already deleted before?

I think so, or perhaps a migration?

> Nice! > > Would it make sense to add a command to the cli to clean up all the files that were already deleted before? I think so, or perhaps a migration?
Author
First-time contributor

I'll resubmit this on github if that's ok, since my local environment is set up that way. (The initial commit was simple enough that I just used the inline editor here, but as we're expanding it'll be faster if I do it over there)

I'll resubmit this on github if that's ok, since my local environment is set up that way. (The initial commit was simple enough that I just used the inline editor here, but as we're expanding it'll be faster if I do it over there)
Author
First-time contributor

I'll resubmit this on github if that's ok, since my local environment is set up that way. (The initial commit was simple enough that I just used the inline editor here, but as we're expanding it'll be faster if I do it over there)

Added some tests, refactored to a function for List and sent the new PR on Github:
https://github.com/go-vikunja/api/pull/64

> I'll resubmit this on github if that's ok, since my local environment is set up that way. (The initial commit was simple enough that I just used the inline editor here, but as we're expanding it'll be faster if I do it over there) Added some tests, refactored to a function for `List` and sent the new PR on Github: https://github.com/go-vikunja/api/pull/64
Owner

I'll resubmit this on github if that's ok, since my local environment is set up that way. (The initial commit was simple enough that I just used the inline editor here, but as we're expanding it'll be faster if I do it over there)

It's actually more complicated for us since the CI only runs here. What makes it easier to push your changes to GitHub instead of here?

> I'll resubmit this on github if that's ok, since my local environment is set up that way. (The initial commit was simple enough that I just used the inline editor here, but as we're expanding it'll be faster if I do it over there) It's actually more complicated for us since the CI only runs here. What makes it easier to push your changes to GitHub instead of here?
Author
First-time contributor

It's just that I had only cloned the github repo locally, and since the diff was simple enough I "migrated" it here just by using the web UI (cloned the repo and edited the file + create the commit inline). But if it's best for you guys to do it here, I'll try to bring the changes over then.

It's just that I had only cloned the github repo locally, and since the diff was simple enough I "migrated" it here just by using the web UI (cloned the repo and edited the file + create the commit inline). But if it's best for you guys to do it here, I'll try to bring the changes over then.
Ghost force-pushed main from 01c9d2edb9 to e8ab28f145 2023-01-27 23:56:54 +00:00 Compare
konrad reviewed 2023-01-29 14:35:00 +00:00
@ -260,6 +261,40 @@ func TestList_Delete(t *testing.T) {
})
}
func TestList_DeleteBackgroundFileIfExists(t *testing.T) {
Owner

Can you add a test where you remove a list with a background and then assert the file was removed?

(check the fixtures for a list with a background)

Can you add a test where you remove a list with a background and then assert the file was removed? (check [the fixtures](https://kolaente.dev/vikunja/api/src/branch/main/pkg/db/fixtures/lists.yml) for a list with a background)
konrad marked this conversation as resolved
Ghost force-pushed main from e8ab28f145 to 3101c5c4e1 2023-01-29 22:27:15 +00:00 Compare
Ghost reviewed 2023-01-29 22:30:59 +00:00
@ -815,12 +815,28 @@ func (l *List) Delete(s *xorm.Session, a web.Auth) (err error) {
}
}
err = l.DeleteBackgroundFileIfExists()
Author
First-time contributor

So, I added this here, but when I test deleting a list from the frontend UI, it seems like the files are not being deleted. I'll look more into it later, but I guess l doesn't have all the fields loaded in this Delete function? Any pointers to wherever the magic happens that loads a List object when a DELETE /lists/{id} call happens?

So, I added this here, but when I test deleting a list from the frontend UI, it seems like the files are not being deleted. I'll look more into it later, but I guess `l` doesn't have all the fields loaded in this `Delete` function? Any pointers to wherever the magic happens that loads a `List` object when a `DELETE /lists/{id}` call happens?
Owner

The CanDelete Method loads the list since it's the one doing the permission check (and it needs the full list anyway). At this point it should be loaded though, did you check with a debugger (or good ol' fmt.Println) if the list has the BackgroundFileID loaded?

The `CanDelete` Method loads the list since it's the one doing the permission check (and it needs the full list anyway). At this point it should be loaded though, did you check with a debugger (or good ol' `fmt.Println`) if the list has the `BackgroundFileID` loaded?
Author
First-time contributor

Yeah, I added a bunch of log.Infof here and BackgroundFileID is always 0.

Looks like CanDelete loads the list, but never copies the contents back into l. But I see that CanRead does do *l = *originalList. Should we be doing the same in CanDelete?

Yeah, I added a bunch of `log.Infof` here and `BackgroundFileID` is always 0. Looks like `CanDelete` loads the list, but never copies the contents back into `l`. But I see that `CanRead` does do `*l = *originalList`. Should we be doing the same in `CanDelete`?
Owner

Looks like CanDelete loads the list, but never copies the contents back into l.

That's entirely possible. In that case, can you just load the list in DeleteBackgroundFileIfExists with the GetListSimpleByID method?

> Looks like CanDelete loads the list, but never copies the contents back into l. That's entirely possible. In that case, can you just load the list in `DeleteBackgroundFileIfExists` with the `GetListSimpleByID` method?
Ghost force-pushed main from 3101c5c4e1 to bd458fe0b2 2023-01-31 21:23:43 +00:00 Compare
konrad reviewed 2023-01-31 21:28:30 +00:00
@ -795,6 +795,11 @@ func (l *List) Create(s *xorm.Session, a web.Auth) (err error) {
// @Router /lists/{id} [delete]
func (l *List) Delete(s *xorm.Session, a web.Auth) (err error) {
err = l.DeleteBackgroundFileIfExists(s)
Owner

Can you move this code block to after when the list itself was deleted? This could otherwise lead to weird errors where the file is gone, but the list isn't (when deleting the list fails but deleting the background succeeded)

Can you move this code block to after when the list itself was deleted? This could otherwise lead to weird errors where the file is gone, but the list isn't (when deleting the list fails but deleting the background succeeded)
Author
First-time contributor

I had it that way initially, but then GetListSimpleByID fails to load the List because it has been deleted.

I had it that way initially, but then `GetListSimpleByID` fails to load the List because it has been deleted.
Author
First-time contributor

I guess alternatively we could load the fullList here with GetListSimpleByID, remove the call to GetListSimpleByID from DeleteBackgroundFileIfExists, then down below we call fullList.DeleteBackgroundFileIfExists?

I guess alternatively we could load the `fullList` here with `GetListSimpleByID`, remove the call to `GetListSimpleByID` from `DeleteBackgroundFileIfExists`, then down below we call `fullList.DeleteBackgroundFileIfExists`?
Owner

I had it that way initially, but then GetListSimpleByID fails to load the List because it has been deleted.

That makes a lot of sense. I Must have missed that.

I guess alternatively we could load the fullList here with GetListSimpleByID, remove the call to GetListSimpleByID from DeleteBackgroundFileIfExists, then down below we call fullList.DeleteBackgroundFileIfExists?

I think that's the way to go. Can you check if this is required in the other place where DeleteBackgroundFileIfExists is called as well?

> I had it that way initially, but then `GetListSimpleByID` fails to load the List because it has been deleted. That makes a lot of sense. I Must have missed that. > I guess alternatively we could load the `fullList` here with `GetListSimpleByID`, remove the call to `GetListSimpleByID` from `DeleteBackgroundFileIfExists`, then down below we call `fullList.DeleteBackgroundFileIfExists`? I think that's the way to go. Can you check if this is required in the other place where `DeleteBackgroundFileIfExists` is called as well?
Author
First-time contributor

The other places were working as expected per my previous testings, but I just verified in code that ultimately they have the full list loaded by that point.

The two other places are:
/lists/{id}/backgrounds/upload [put]: already had the file deletion code, but the full list is coming from

UploadBackground -> setBackgroundPreparations -> GetListSimpleByID
                 -> Set -> DeleteBackgroundFileIfExists

/lists/{id}/background [delete]

RemoveListBackground -> checkListBackgroundRights -> list.CanRead -> GetListSimpleByID
                     -> DeleteBackgroundFileIfExists
The other places were working as expected per my previous testings, but I just verified in code that ultimately they have the full list loaded by that point. The two other places are: `/lists/{id}/backgrounds/upload [put]`: already had the file deletion code, but the full list is coming from ``` UploadBackground -> setBackgroundPreparations -> GetListSimpleByID -> Set -> DeleteBackgroundFileIfExists ``` `/lists/{id}/background [delete]` ``` RemoveListBackground -> checkListBackgroundRights -> list.CanRead -> GetListSimpleByID -> DeleteBackgroundFileIfExists ```
@ -260,0 +265,4 @@
db.LoadAndAssertFixtures(t)
files.InitTestFileFixtures(t)
s := db.NewSession()
list, err := GetListSimpleByID(s, 25)
Owner

Here you should not load the full list, instead create the list object like in the test before:

list := List{
	ID: 25,
}

That way it's the same as when the web handler calls the delete method.

Here you should not load the full list, instead create the list object like in the test before: ``` list := List{ ID: 25, } ``` That way it's the same as when the web handler calls the delete method.
Ghost force-pushed main from bd458fe0b2 to d74fdb35d1 2023-01-31 22:22:25 +00:00 Compare
Ghost changed title from WIP: fix(background): when list background is removed, delete file from file system and DB to fix(background): when list background is removed, delete file from file system and DB 2023-01-31 22:33:17 +00:00
konrad approved these changes 2023-02-01 11:38:03 +00:00
konrad left a comment
Owner

Thanks!

Thanks!
konrad merged commit afdceb0aff into main 2023-02-01 11:38:24 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/vikunja#1372
No description provided.