Don't delete dataset in DB when annotations exist #7429

frcroth · 2023-11-08T10:56:41Z

URL of deployed dev instance (used for testing):

https://___.webknossos.xyz

Steps to test:

Create an annotation for a dataset
Delete that dataset
Download the annotation (it still works)
Opening annotation or dataset (via old URL) leads to error

TODOs:

Fix compact writes of annotations
do use the deleted dataset?
~~Frontend: Disable the view button for annotations with dataset status = "Deleted by user."~~

Issues:

fixes Deleting datasets triggers SQL foreign-key violation #6038

(Please delete unneeded items, merge only when none are left open)

Updated changelog
Needs datastore update after deployment

fm3 · 2023-11-11T13:30:08Z

I’ve given this some more thought, and I think maybe we should reuse the status field of datasets to store this information. Have a look at deactivateUnreportedDataSources, which is called when updating the dataset list as triggered by an update from a datastore server. If a datastore is deleted on disk (not by webknossos, but by someone with disk access), this is how webknossos "deactivates" it, implicitly setting isActive to false by setting a status message. That is a string (because it can also include different error messages from the datastore), but it has a couple of standard possible values, one of which is "No longer available on datastore." (Compare the uses of the status field throughout the code). We could add one status message like "Deleted by user" for this feature. What do you think?

Sorry that I haven’t said this sooner, I see you have already started implementing it with a new column. But maybe unifying this would be nice.

philippotto · 2023-11-14T14:50:37Z

I just looked into this, but I think two things are missing:

I don't see a dataset status in the annotation objects (only on the DS object) which is why I cannot change the UI when rendering annotations for deleted DS.
opening the annotation (with a deleted dataset) rightfully throws an error. however, opening the DS directly in view mode (e.g., via an old URL) yields a 200 status. rendering crashes later for other reasons, but I would expect an error response similar to the case where an annotation is opened.

fm3 · 2023-11-14T14:52:14Z

How do these things work for datasets that are "No longer available on the datastore"?

philippotto · 2023-11-14T15:06:33Z

Good point. I checked that now and the error is controlled by the front-end. It only cares for the existence of dataset.dataSource.dataLayers. If that is undefined, the error is shown. Now, the error is properly shown for the same DS for which it didn't work before. I assume that it was a caching issue? possibly bound to the rescan interval of the disk?

By the way, annotations with the "No longer available on the datastore" case are normally shown in the annotations list. The error is then shown when clicking "open" which is fine, I guess.

So, from my point of view, the front-end wouldn't need any change. Avoiding outdated dataset.dataSource.dataLayers would be nice for the back-end (if my theory from above is correct). Alternatively, the front-end could start checking the status string, but I'm not sure whether that is actually helps?

fm3 · 2023-11-14T15:10:47Z

Good to hear! Yes, I think that behavior is ok.

If I understood it correctly, the new delete action should immediately delete the layers in the postgres database, so there should be no backend-internal caching for dataset.dataSource.dataLayers. Maybe you could have another look at that Felix, and see if you can reproduce the issue Philipp describes

fm3

Great stuff, I think this help create less confusion for users :)

I added some comments, I think this needs to be integrated with the existing deactivateUnreported logic a bit more.

app/controllers/WKRemoteDataStoreController.scala

fm3 · 2023-11-16T08:36:03Z

app/models/dataset/Dataset.scala

+      uploaderIdOpt: Option[ObjectId],
+      searchQuery: Option[String],
+      includeSubfolders: Boolean,
+      excludeRemovedOnDisk: Boolean = true)(implicit ctx: DBAccessContext): Fox[SqlToken] =


This means the datasets marked as deleted are not included in the list query at all? I’d say they should still be included, just like the "no longer available on datastore" ones. I think there are already filters used by the frontend for active/inactive datasets. My guess is that the isUsable column is used for that. Also could you have another look at the deactivateUnreported logic in DatasetService? I’d guess that this will somehow interact. the deletedByUserStatus should probably be included in inactiveStatusList?

I think I implemented this now, but I noticed that the status "Deleted by user" is actually never (or only for a short amount of time) present in the DB, because I trigger the rechecking of the inbox which checks the directories and the dataset is actually deleted on disk, so it will be marked as "No longer available on datastore.". Is this sufficient or should there be a more explicit status? Otherwise I could delete the "Deleted by user" status again.

Hmm, I see. I think it would be valuable to preserve the information that it was in fact deleted by the user. Could you adapt the existing functions so that "Deleted by user" is not overwritten by "No longer available on datastore"?

Also, I do not yet fully see why this added check inbox trigger is needed now. Wouldn’t its effects already have happened on the wk side (delete layers, set status, etc)? Maybe I’m missing something though. It’s not like the new trigger should hurt much (it does potentially take some seconds, though, depending on IO speed)

Fixed it now. This behavior was caused by a typo in an SQL query, which meant the status was never actually assigned. This is fixed now. A problem that persists is that the deleted dataset is still displayed in the dataset list. This status should be probably filtered (via a modified request).

Cool! Hmm, I would have expected that these datasets have isActive=false, so the current query would skip them together with the "unreported" ones.

Maybe this has something to do with the isUsable bool? Just noticed that. Looks somewhat redundant to be honest, but I haven’t looked into it that much. Maybe you need to change that value when updating the status

Not certain what the best UX here would be. I would have said isUnreported (meaning show them if the "missing" filter is active) is good. That is at least some explanation why the name is still reserved. But maybe you prefer something different? cc @philippotto

I would have said isUnreported (meaning show them if the "missing" filter is active) is good. That is at least some explanation why the name is still reserved.

I agree with that (for "deleted" datasets where annotations exist). To avoid any potential misunderstandings, let me write out how I'd imagine this overall:

deleting a DS for which no annotation exists should completely delete the DS so that it cannot be found via the UI (also not via any filters)

deleting a DS for which annotations exist, should tell the user "we deleted the dataset, but since annotations exist for it, the dataset will still be listed under "unreported". if you want to delete the dataset completely, do ...".

the perceived behavior would be equal to when the user deletes a dataset from disk.

Does this make sense?

Sounds good to me. On "if you want to delete the dataset completely, do ..." – unfortunately there is currently no user-exposed way to do this. That would be a follow-up issue. So I’d omit that part of the message for the moment 😅

Ok, fair :)

Deleted by user is now also a cause for datasets to be unreported.

fm3

Looking good! As discussed in person, I’d ask for additional explanations in the frontend for the user,

before deletion: that the name may still be blocked
after deletion inside of "show error", that this was still referenced and is kept to not break things elsewhere

…shboard tab; also explain that the name is not guaranteed to be usable after deletion

philippotto · 2023-11-28T15:37:33Z

Looking good! As discussed in person, I’d ask for additional explanations in the frontend for the user,

before deletion: that the name may still be blocked

after deletion inside of "show error", that this was still referenced and is kept to not break things elsewhere

Done!

frontend/javascripts/dashboard/advanced_dataset/dataset_action_view.tsx

fm3

🎉

frcroth · 2023-12-06T08:21:42Z

@philippotto Does this need frontend review?

philippotto · 2023-12-06T10:47:07Z

@philippotto Does this need frontend review?

I don't think so. The front-end diff is mostly changing some words. @fm3 had a look at this, I think :)

Dont delete dataset row when annotations exist

e91669c

frcroth self-assigned this Nov 8, 2023

frcroth changed the title ~~Dont delete dataset row when annotations exist~~ WIP: Dont delete dataset row when annotations exist Nov 8, 2023

frcroth marked this pull request as draft November 8, 2023 10:57

frcroth and others added 6 commits November 8, 2023 11:58

Merge branch 'master' into delete-dataset-annotation

4bbe716

Fix compact writes annotation on deleted dataset

5710a22

Mark datasets as deleted on disk

5adab91

Fix missing view in sql evolutions

5ec15a5

Merge branch 'master' into delete-dataset-annotation

9b740eb

Update changelog

bc220c4

Use status instead of column

5349251

frcroth force-pushed the delete-dataset-annotation branch from 63b8801 to 5349251 Compare November 13, 2023 08:49

frcroth added the frontend label Nov 13, 2023

philippotto self-assigned this Nov 14, 2023

Refresh inbox after deleting dataset

38ac446

frcroth requested a review from fm3 November 15, 2023 09:01

frcroth changed the title ~~WIP: Dont delete dataset row when annotations exist~~ Don't delete dataset row when annotations exist Nov 15, 2023

frcroth changed the title ~~Don't delete dataset row when annotations exist~~ Don't delete dataset in DB when annotations exist Nov 15, 2023

frcroth marked this pull request as ready for review November 15, 2023 09:06

frcroth unassigned philippotto Nov 15, 2023

fm3 reviewed Nov 16, 2023

View reviewed changes

frcroth added 4 commits November 20, 2023 11:14

Set deleted datasets as not usable

a2b3d51

Merge branch 'master' into delete-dataset-annotation

92f9474

Fix typo in SQL query

f171a84

Undo check inbox change

7de30d2

Deleted datasets are unreported

e1d8386

fm3 reviewed Nov 27, 2023

View reviewed changes

explain to user why a dataset is still shown after deletion in the da…

418bafa

…shboard tab; also explain that the name is not guaranteed to be usable after deletion

fm3 reviewed Nov 28, 2023

View reviewed changes

frontend/javascripts/dashboard/advanced_dataset/dataset_action_view.tsx Outdated Show resolved Hide resolved

fix commas after even though

c08ff3a

frcroth commented Nov 29, 2023

View reviewed changes

frontend/javascripts/dashboard/advanced_dataset/dataset_action_view.tsx Outdated Show resolved Hide resolved

philippotto and others added 2 commits November 29, 2023 11:34

adapt explanation text from "is" to "was"

fb9a225

Merge branch 'master' into delete-dataset-annotation

fa280a5

frcroth requested a review from fm3 November 29, 2023 10:39

hotzenklotz mentioned this pull request Nov 29, 2023

Added a delete dataset action to dashboard #7448

Merged

7 tasks

fm3 approved these changes Nov 30, 2023

View reviewed changes

Merge branch 'master' into delete-dataset-annotation

d5d653c

Merge branch 'master' into delete-dataset-annotation

28550ee

frcroth merged commit bc835db into master Dec 6, 2023
2 checks passed

frcroth deleted the delete-dataset-annotation branch December 6, 2023 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't delete dataset in DB when annotations exist #7429

Don't delete dataset in DB when annotations exist #7429

frcroth commented Nov 8, 2023 •

edited

Loading

fm3 commented Nov 11, 2023

philippotto commented Nov 14, 2023

fm3 commented Nov 14, 2023

philippotto commented Nov 14, 2023

fm3 commented Nov 14, 2023

fm3 left a comment

fm3 Nov 16, 2023

frcroth Nov 20, 2023

fm3 Nov 20, 2023

frcroth Nov 22, 2023

fm3 Nov 22, 2023

fm3 Nov 22, 2023

philippotto Nov 22, 2023

fm3 Nov 22, 2023 •

edited

Loading

philippotto Nov 22, 2023

frcroth Nov 27, 2023

fm3 left a comment

philippotto commented Nov 28, 2023

fm3 left a comment

frcroth commented Dec 6, 2023

philippotto commented Dec 6, 2023

Don't delete dataset in DB when annotations exist #7429

Don't delete dataset in DB when annotations exist #7429

Conversation

frcroth commented Nov 8, 2023 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

TODOs:

Issues:

fm3 commented Nov 11, 2023

philippotto commented Nov 14, 2023

fm3 commented Nov 14, 2023

philippotto commented Nov 14, 2023

fm3 commented Nov 14, 2023

fm3 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

fm3 Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fm3 left a comment

Choose a reason for hiding this comment

philippotto commented Nov 28, 2023

fm3 left a comment

Choose a reason for hiding this comment

frcroth commented Dec 6, 2023

philippotto commented Dec 6, 2023

frcroth commented Nov 8, 2023 •

edited

Loading

fm3 Nov 22, 2023 •

edited

Loading