vikunja/vikunja
5
49
Fork 88
Code Issues 132 Pull Requests 38 Packages Releases 37 Activity

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
Conversation 12 Commits 1 Files Changed 6 +112 -17
Ghost commented 2023-01-25 21:40:17 +00:00
First-time contributor
Copy Link

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 ```
konrad commented 2023-01-25 21:43:11 +00:00
Owner
Copy Link

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
pkg/modules/background/handler/background.go Outdated
@ -332,1 +332,4 @@
}
// Remove the old background if one exists
if list.BackgroundFileID != 0 {
konrad commented 2023-01-26 09:45:11 +00:00
Owner
Copy Link

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?
konrad commented 2023-01-26 09:45:45 +00:00
Owner
Copy Link

Could be a function for the List type

Could be a function for the `List` type
dpschen commented 2023-01-26 09:46:46 +00:00
Member
Copy Link

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?
Ghost commented 2023-01-26 19:12:47 +00:00
Author
First-time contributor
Copy Link

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?
Ghost commented 2023-01-26 19:14:23 +00:00
Author
First-time contributor
Copy Link

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)
Ghost commented 2023-01-26 19:28:45 +00:00
Author
First-time contributor
Copy Link

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
konrad commented 2023-01-27 11:45:33 +00:00
Owner
Copy Link

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?
Ghost commented 2023-01-27 23:37:10 +00:00
Author
First-time contributor
Copy Link

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.
👍 1
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
pkg/models/list_test.go Outdated
@ -260,6 +261,40 @@ func TestList_Delete(t *testing.T) {
})
}
func TestList_DeleteBackgroundFileIfExists(t *testing.T) {
konrad commented 2023-01-29 14:34:44 +00:00
Owner
Copy Link

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
pkg/models/list.go Outdated
@ -815,12 +815,28 @@ func (l *List) Delete(s *xorm.Session, a web.Auth) (err error) {
}
}
err = l.DeleteBackgroundFileIfExists()
Ghost commented 2023-01-29 22:30:59 +00:00
Author
First-time contributor
Copy Link

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?
konrad commented 2023-01-30 20:59:56 +00:00
Owner
Copy Link

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?
Ghost commented 2023-01-30 22:21:07 +00:00
Author
First-time contributor
Copy Link

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`?
konrad commented 2023-01-31 14:48:19 +00:00
Owner
Copy Link

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
pkg/models/list.go Outdated
@ -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)
konrad commented 2023-01-31 21:28:28 +00:00
Owner
Copy Link

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)
Ghost commented 2023-01-31 21:31:08 +00:00
Author
First-time contributor
Copy Link

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.
Ghost commented 2023-01-31 21:35:48 +00:00
Author
First-time contributor
Copy Link

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`?
konrad commented 2023-01-31 21:44:01 +00:00
Owner
Copy Link

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?
Ghost commented 2023-01-31 22:11:35 +00:00
Author
First-time contributor
Copy Link

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 ```
pkg/models/list_test.go Outdated
@ -260,0 +265,4 @@
db.LoadAndAssertFixtures(t)
files.InitTestFileFixtures(t)
s := db.NewSession()
list, err := GetListSimpleByID(s, 25)
konrad commented 2023-01-31 21:26:43 +00:00
Owner
Copy Link

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
Copy Link

Thanks!

Thanks!
konrad merged commit afdceb0aff into main 2023-02-01 11:38:24 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
konrad
Labels
Clear labels
dependencies
duplicate
This issue or pull request already exists
help wanted
Need some help
invalid
Something is wrong
kind/bug
Something is not working
kind/feature
New feature
needs reproduction
question
More information is needed
security
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vikunja/vikunja#1372
No description provided.
Delete Branch ":main"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?