Skip to content

[s3inbox] Send remove message on overwrite #1543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 8, 2025

Conversation

MalinAhlberg
Copy link
Contributor

@MalinAhlberg MalinAhlberg commented Apr 1, 2025

Related issue(s) and PR(s)

This PR closes #1480 .

Description

When uploading a file to s3inbox, a check is done to see if the file is already there. If it is, a delete message is sent before the upload message, so that the FEGA portal will know that the new version of the file exists.

Note: The remove message is formatted according to this spec. However, our messages will have two extra fields, "encrypted_checksums":null and "filesize":0, since it shares the go struct with upload messages. Hopefully, that won't be any problem. Please let me know if you think it will.

How to test

Run the integration tests, and check that after the tests in .github/integration/tests/sda/10_upload_test.sh are run, there are 6 messages for upload and one for remove in rabbitmq messages in the inbox queue.

You can also manually upload a file twice, and check the queue and the logs to make sure the remove message is only sent the second time:
s3cmd -c /tmp/shared/s3cfg put dummyfile.c4gh s3://test_dummy.org/

@MalinAhlberg MalinAhlberg force-pushed the feat/send-remove-message-on-overwrite branch 3 times, most recently from f6c34d0 to a13a000 Compare April 1, 2025 13:19
@MalinAhlberg MalinAhlberg requested a review from a team April 1, 2025 13:28
@jbygdell
Copy link
Collaborator

jbygdell commented Apr 1, 2025

Note: The remove message is formatted according to this spec. However, our messages will have two extra fields, "encrypted_checksums":null and "filesize":0, since it shares the go struct with upload messages. Hopefully, that won't be any problem. Please let me know if you think it will.

We have the schema for the inbox-remove event here. As well in internal/schemas´ as InboxRemove`

Copy link
Collaborator

@jbygdell jbygdell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have all the JSON schemas available so validation of the outgoing message can be done.

We should also not only check for the existence of the file but also handle any errors in a way so that the user gets the proper notification.

@MalinAhlberg MalinAhlberg requested review from jbygdell and a team April 2, 2025 07:41
@MalinAhlberg MalinAhlberg self-assigned this Apr 2, 2025
Copy link
Collaborator

@jbygdell jbygdell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checkFileExists should have a static test so we can validate the two expected results.

@MalinAhlberg MalinAhlberg force-pushed the feat/send-remove-message-on-overwrite branch 2 times, most recently from e2fab24 to 6b88b02 Compare April 4, 2025 14:49
@MalinAhlberg MalinAhlberg force-pushed the feat/send-remove-message-on-overwrite branch from 6b88b02 to 000c8ad Compare April 4, 2025 14:49
@MalinAhlberg
Copy link
Contributor Author

After a lot of rebasing, bug fixing and improved test setup (mainly from @jbygdell) this is now ready for review again!
Sorry for the messy history here on this comment page. The commits are less messy!

@MalinAhlberg MalinAhlberg requested review from jbygdell and a team April 4, 2025 15:00
@MalinAhlberg
Copy link
Contributor Author

I have fixed the things you comment on @jbygdell . Please have a look and resolve the comments.

Personally, I think our linters should be responsible for things like newlines. They should complain on everything we want to enforce. If they do not enforce a standard, it should be optional :) .

@jbygdell
Copy link
Collaborator

jbygdell commented Apr 7, 2025

Personally, I think our linters should be responsible for things like newlines. They should complain on everything we want to enforce. If they do not enforce a standard, it should be optional :) .

The new version of the linter (that caused all the failures) will catch the empty lines among many other things in the codbase. So I'm just being preemptive here.

Copy link
Contributor

@nanjiangshu nanjiangshu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well when I tested. Great job!

@MalinAhlberg MalinAhlberg added this pull request to the merge queue Apr 8, 2025
Merged via the queue into main with commit ae33000 Apr 8, 2025
29 checks passed
@MalinAhlberg MalinAhlberg deleted the feat/send-remove-message-on-overwrite branch April 8, 2025 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[s3inbox] send remove message if a file is overwritten.
3 participants