[api] delete file #1122

MalinAhlberg · 2024-11-07T15:38:50Z

Related issue(s) and PR(s)
This PR closes #1134 .

Description
This PR adds the delete file functionality to the api component. Specifically, it deletes the file from the inbox and it adds a new file log event, setting the file status to disabled.

Also, it adds the fileID to the list functionality of the api, since that field is needed in order to delete a file:

curl -H "Authorization: Bearer $token" "http://localhost:8090/users/[email protected]/files" | jq .
[
  {
    "fileID": "d32f7117-bb85-40e2-9c52-101bf9c1ca5a",
    "inboxPath": "test_dummy.org/race_file.c4gh",
    "fileStatus": "ready",
    "createAt": "2024-11-19T10:50:07.836513Z"
  },
  {...

How to test
make build-all then PR_NUMBER=$(date +%F) docker compose -f .github/integration/sda-s3-integration.yml run integration_test.
List the files (eg with http://localhost:8090/users/[email protected]/files) and make sure files in the inbox can be deleted, and that archived files can not be deleted.

kostas-kou

Good job. I have only some minor comments

sda/cmd/api/api.go

sda/internal/database/database.go

.github/integration/tests/sda/60_api_admin_test.sh

sda/cmd/api/api.md

MalinAhlberg · 2024-11-27T08:17:32Z

Great comments @kostas-kou ! Fixed most of them in 92903a6, ~~but left this one for others to see~~ <- solved during stand-up.

pahatz

Great work! Especially on the tests, very extensive.
I don't have any significant remark on the PR.

sda/cmd/api/api.go

sda/internal/database/db_functions.go

MalinAhlberg · 2024-11-29T08:54:10Z

@kostas-kou and @pahatz, thanks for your reviews! I have fixed the weird comment, rebased on main and also rebased to get rid of the fixup-commits. Only 3ac7e5e is new, the rest is the same as when you reviewed.

MalinAhlberg · 2024-11-29T09:39:59Z

...and added 8a745c4 for the rbac

aaperis

Haven't tested, but it looks good. Tiny minor comments :-)

aaperis · 2024-12-02T08:54:06Z

.github/integration/tests/sda/60_api_admin_test.sh

+last_event=$(psql -U postgres -h postgres -d sda -At -c "SELECT event FROM sda.file_event_log WHERE file_id='$fileid' order by started_at desc limit 1;")
+
+if [ "$last_event" != "disabled" ]; then
+   echo "The file $fileid does not have the expected las event 'disabled', but $last_event."
+fi


maybe it is too much, but shouldn't the test check at this point that the file was deleted from the s3 bucket as well?

Yes... it shoud. Fixed in 109e7e2

aaperis · 2024-12-02T09:22:34Z

sda/cmd/api/api.md

+- `/file/:username/*fileid`
+  - accepts `DELETE` requests
+  - marks the file as `disabled` in the database, and deletes it from the inbox.
+  - The file is identfied by its id, returned by `users/:username/:files`


Suggested change

- The file is identfied by its id, returned by `users/:username/:files`

- The file is identified by its id, returned by `users/:username/:files`

aaperis · 2024-12-02T09:26:22Z

sda/cmd/api/api.go

+	}
+
+	submissionUser := c.Param("username")
+	log.Warn("submission user:", submissionUser)


why log.Warn and not log.Debug or log.Info?

No reason 😬 ... Changed in 109e7e2

aaperis · 2024-12-02T09:26:45Z

sda/cmd/api/api.go

+
+	fileID := c.Param("file")
+	fileID = strings.TrimPrefix(fileID, "/")
+	log.Warn("submission file:", fileID)


why log.Warn?

No reason. Changed in 109e7e2

jbygdell

More specific comments are coming

jbygdell

There is a few things here that we need to think more about.

In FEGA the submitter can trigger ingestion of an uploaded file. That means we also needs to be able to remove a file both from the inbox and the archive during a cleaning operation. As well as the backup site if that is configured.
The API can also be used by the submitter and that means we can't use the same functions or structs sometimes as that might leak things the submitter should not be able to see.

jbygdell · 2024-12-02T13:59:46Z

.github/integration/sda/rbac.json

+       {
+          "role": "admin",
+          "path": "/file/*",
+          "action": "(DELETE)"


Suggested change

"action": "(DELETE)"

"action": "DELETE"

Single action doesn't require parentheses

jbygdell · 2024-12-02T14:03:35Z

sda/cmd/api/api.go

@@ -54,6 +56,10 @@ func main() {
 	if err != nil {
 		log.Fatal(err)
 	}
+	Conf.API.INBOX, err = storage.NewBackend(Conf.Inbox)
+	if err != nil {
+		log.Fatal(err)


log.Fatal is the equivalent of doing log.Error followed by os.Exit(1), something we shouldn't do when we have established connections to the MQ and DB.

The proper way is to have a shutdown function that will ensure that the external connections get's closed before the application closes

jbygdell · 2024-12-03T07:50:20Z

.github/integration/sda/rbac.json

@@ -20,6 +20,11 @@
          "path": "/file/accession",
          "action": "POST"
       },
+       {
+          "role": "admin",


Suggested change

"role": "admin",

"role": "submission",

Removing files from the inbox is something the helpdesk should be able to do.

jbygdell · 2024-12-03T08:09:04Z

sda/cmd/api/api.go

@@ -100,6 +106,7 @@ func setup(config *config.Config) *http.Server {
 	r.POST("/c4gh-keys/add", rbac(e), addC4ghHash)                      // Adds a key hash to the database
 	r.GET("/c4gh-keys/list", rbac(e), listC4ghHashes)                   // Lists key hashes in the database
 	r.POST("/c4gh-keys/deprecate/*keyHash", rbac(e), deprecateC4ghHash) // Deprecate a given key hash
+	r.DELETE("/file/:username/*file", rbac(e), deleteFile)              // Delete a file from inbox


The *file here will catch a full path i.e. /path/to/file.c4gh. If we want' that semantics we should probably call it filePath.

If we not intend to use the file path but instead use the file id (to cope with BPs expected upload structure) the *file should be :file or :fileID as we only expect a single value (no slashes).

jbygdell · 2024-12-03T08:11:22Z

sda/cmd/api/api.go

+	filePath := ""
+	// Get the file path from the fileID and submission user
+	if filePath, err = Conf.API.DB.GetInboxFilePathFromID(submissionUser, fileID); err != nil {


Suggested change

filePath := ""

// Get the file path from the fileID and submission user

if filePath, err = Conf.API.DB.GetInboxFilePathFromID(submissionUser, fileID); err != nil {

// Get the file path from the fileID and submission user

filePath, err := Conf.API.DB.GetInboxFilePathFromID(submissionUser, fileID)

if err != nil {

jbygdell · 2024-12-03T08:49:23Z

sda/cmd/api/api.go

+// The deleteFile function deletes files from the inbox and marks them as
+// discarded in the db. Files are identified by their ids and the user id.
+func deleteFile(c *gin.Context) {
+


Suggested change

jbygdell · 2024-12-03T09:09:58Z

sda/cmd/api/api.go

+	}
+
+	// Requires a filepath instead of fileID
+	// Note: The remove fails randomly sometimes


Suggested change

// Note: The remove fails randomly sometimes

File removal should never fail randomly.
If this is in the tests it's because of timing issues in the S3 backend (metadata not stored before deletion is initated).

jbygdell · 2024-12-03T09:14:41Z

.github/integration/tests/sda/60_api_admin_test.sh

+    exit 1
+fi
+# delete it
+resp="$(curl -s -k -L -o /dev/null -w "%{http_code}\n" -H "Authorization: Bearer $token" -H "Content-Type: application/json" -X DELETE "http://api:8080/file/[email protected]/$fileid")"


Suggested change

resp="$(curl -s -k -L -o /dev/null -w "%{http_code}\n" -H "Authorization: Bearer $token" -H "Content-Type: application/json" -X DELETE "http://api:8080/file/[email protected]/$fileid")"

resp="$(curl -s -k -L -o /dev/null -w "%{http_code}\n" -H "Authorization: Bearer $token" -X DELETE "http://api:8080/file/[email protected]/$fileid")"

Content type header not needed since we are not sending any payload.

jbygdell · 2024-12-03T09:18:19Z

sda/internal/database/database.go

@@ -46,6 +46,7 @@ type SyncData struct {
 }

 type SubmissionFileInfo struct {
+	FileID    string `json:"fileID"`


This struct is also used when the submission user wants to list the files that have been uploaded and the FileID is not something that the submitter needs to see.

jbygdell · 2024-12-03T09:36:29Z

sda/internal/database/db_functions.go

+		"AND EXISTS (SELECT 1 FROM " +
+		"(SELECT event from sda.file_event_log where file_id = $2 order by started_at desc limit 1) " +
+		"as subquery WHERE event in ('registered', 'uploaded', 'submitted', 'ingested', 'error'))"


This will be a bit iffy since in FEGA if the submission user adds a file to a run by mistake that file will be ingest, archived, backed up and given a stable ID (a case we actually will ned to be able to handle).
If we want to limit this to files that only have been uploaded to the inbox ingested can not be a valid event since then the file will also be in the archive.

Instead we probably need to check if the file has had a state of ingested, verified, archived or ready and then also remove it from the archive. If the file has had the state backed up we also need to remove the file from the backup site.

dbampalikis assigned MalinAhlberg, nanjiangshu and dbampalikis Nov 7, 2024

MalinAhlberg force-pushed the feature/api-delete-file branch 8 times, most recently from b31bc38 to 446a1ff Compare November 21, 2024 10:22

MalinAhlberg marked this pull request as ready for review November 21, 2024 10:22

MalinAhlberg requested a review from a team November 21, 2024 10:25

kostas-kou reviewed Nov 26, 2024

View reviewed changes

sda/cmd/api/api.go Outdated Show resolved Hide resolved

sda/internal/database/database.go Show resolved Hide resolved

.github/integration/tests/sda/60_api_admin_test.sh Outdated Show resolved Hide resolved

sda/cmd/api/api.md Show resolved Hide resolved

kostas-kou previously approved these changes Nov 28, 2024

View reviewed changes

pahatz previously approved these changes Nov 28, 2024

View reviewed changes

sda/cmd/api/api.go Outdated Show resolved Hide resolved

sda/internal/database/db_functions.go Show resolved Hide resolved

MalinAhlberg dismissed stale reviews from pahatz and kostas-kou via 353261a November 29, 2024 07:52

MalinAhlberg force-pushed the feature/api-delete-file branch from 353261a to 3ac7e5e Compare November 29, 2024 08:51

pahatz previously approved these changes Nov 29, 2024

View reviewed changes

aaperis previously approved these changes Dec 2, 2024

View reviewed changes

dbampalikis added 6 commits December 2, 2024 14:56

[postgres] Add api user priviledges

629e37d

[api] Add api configuration

4f9fcb9

[api] Add fileID to api struct

2c94aa0

[api] Change list to return id and skip disabled

6815248

[api] Add disable file event log function

8091441

[api] Add delete file functionality

842b5d3

MalinAhlberg added 11 commits December 2, 2024 14:56

[api] add integration test

b7bdca8

[api delete] only search for unarchived files

8aeb2c5

[tests] api admin tests

9cc1d89

[api delete] only search for unarchived files

edc3cf8

[tests] unit test for db function

11e6330

[api] allow deletion of files marked with error

d82442b

[charts] add inbox conf to api

2dba5ca

[api] add documentation

4e9efe8

[api] typos etc

ad71a98

[api] rephrase comment

fe0a1be

[tests] add rbac permissions for file deletion endpoint

1d73ae3

MalinAhlberg dismissed stale reviews from aaperis and pahatz via 0f445c8 December 2, 2024 13:57

MalinAhlberg force-pushed the feature/api-delete-file branch from 8a745c4 to 0f445c8 Compare December 2, 2024 13:57

jbygdell requested changes Dec 2, 2024

View reviewed changes

MalinAhlberg force-pushed the feature/api-delete-file branch from 0f445c8 to 109e7e2 Compare December 2, 2024 14:14

Apply suggestions from code review

d690e54

MalinAhlberg force-pushed the feature/api-delete-file branch from 109e7e2 to d690e54 Compare December 3, 2024 07:43

jbygdell requested changes Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api] delete file #1122

[api] delete file #1122

MalinAhlberg commented Nov 7, 2024 •

edited

Loading

kostas-kou left a comment

MalinAhlberg commented Nov 27, 2024 •

edited

Loading

pahatz left a comment

MalinAhlberg commented Nov 29, 2024

MalinAhlberg commented Nov 29, 2024

aaperis left a comment

aaperis Dec 2, 2024

MalinAhlberg Dec 2, 2024 •

edited

Loading

aaperis Dec 2, 2024

aaperis Dec 2, 2024

MalinAhlberg Dec 2, 2024 •

edited

Loading

aaperis Dec 2, 2024

MalinAhlberg Dec 2, 2024 •

edited

Loading

jbygdell left a comment

jbygdell left a comment

jbygdell Dec 2, 2024

jbygdell Dec 2, 2024

jbygdell Dec 3, 2024

jbygdell Dec 3, 2024

jbygdell Dec 3, 2024

jbygdell Dec 3, 2024

jbygdell Dec 3, 2024

jbygdell Dec 3, 2024

jbygdell Dec 3, 2024

jbygdell Dec 3, 2024

	- The file is identfied by its id, returned by `users/:username/:files`
	- The file is identified by its id, returned by `users/:username/:files`

	resp="$(curl -s -k -L -o /dev/null -w "%{http_code}\n" -H "Authorization: Bearer $token" -H "Content-Type: application/json" -X DELETE "http://api:8080/file/[email protected]/$fileid")"
	resp="$(curl -s -k -L -o /dev/null -w "%{http_code}\n" -H "Authorization: Bearer $token" -X DELETE "http://api:8080/file/[email protected]/$fileid")"

[api] delete file #1122

Are you sure you want to change the base?

[api] delete file #1122

Conversation

MalinAhlberg commented Nov 7, 2024 • edited Loading

kostas-kou left a comment

Choose a reason for hiding this comment

MalinAhlberg commented Nov 27, 2024 • edited Loading

pahatz left a comment

Choose a reason for hiding this comment

MalinAhlberg commented Nov 29, 2024

MalinAhlberg commented Nov 29, 2024

aaperis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MalinAhlberg Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MalinAhlberg Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MalinAhlberg Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

jbygdell left a comment

Choose a reason for hiding this comment

jbygdell 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

Choose a reason for hiding this comment

MalinAhlberg commented Nov 7, 2024 •

edited

Loading

MalinAhlberg commented Nov 27, 2024 •

edited

Loading

MalinAhlberg Dec 2, 2024 •

edited

Loading

MalinAhlberg Dec 2, 2024 •

edited

Loading

MalinAhlberg Dec 2, 2024 •

edited

Loading