Skip to content
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

Add files api #360

Merged
merged 34 commits into from
Nov 30, 2023
Merged

Add files api #360

merged 34 commits into from
Nov 30, 2023

Conversation

MalinAhlberg
Copy link
Contributor

@MalinAhlberg MalinAhlberg commented Oct 17, 2023

This PR closes #435. It is based on neicnordic/sda-pipeline#540

  • merges api from repo sda-pipeline into sda
  • adds the endpoint files to the API service for retrieving information about the files a user has submitted
  • moves userauth from cmd to internal, for reuse in api
  • updates and refactoring of the api code to use the common code

Testing:
When running the containers from sda-s3-integration.yml, this should work:

 # call to the oidc server 
token="$(curl http://localhost:8080/tokens | jq -r '.[0]')" 
 # call to the api
curl -k -L "http://localhost:8090/files" -H "Authorization: Bearer $token" 

If the integration tests (docker compose -f ~/NBIS/sloth/sensitive-data-archive/.github/integration/sda-s3-integration.yml run integration_test) are run, the curl response should be
[{"inboxPath":"requester_demo.org/data/file1.c4gh","fileStatus":"uploaded","createAt":"2023-11-13T10:12:43.144242Z"}]

Updates of the status of the user's file in the psql db should be reflected in the output of the call, so that only the last event is shown.

fixes #444
fixes #436

@MalinAhlberg MalinAhlberg force-pushed the feat/files-api branch 3 times, most recently from bad25c0 to a35d564 Compare November 7, 2023 10:32
@MalinAhlberg MalinAhlberg force-pushed the feat/files-api branch 3 times, most recently from fe07924 to 7d77fb5 Compare November 13, 2023 10:17
@MalinAhlberg MalinAhlberg marked this pull request as ready for review November 13, 2023 11:03
@MalinAhlberg MalinAhlberg requested a review from a team November 13, 2023 11:03
sda/cmd/api/api.go Outdated Show resolved Hide resolved
sda/cmd/api/api.go Outdated Show resolved Hide resolved
sda/cmd/api/api.go Outdated Show resolved Hide resolved
sda/cmd/api/api.go Outdated Show resolved Hide resolved
sda/cmd/api/api.go Outdated Show resolved Hide resolved
sda/cmd/api/api_test.go Outdated Show resolved Hide resolved
sda/cmd/api/api_test.go Outdated Show resolved Hide resolved
sda/cmd/api/api_test.go Outdated Show resolved Hide resolved
sda/cmd/api/api_test.go Outdated Show resolved Hide resolved
sda/cmd/api/api_test.go Outdated Show resolved Hide resolved
@MalinAhlberg
Copy link
Contributor Author

Thanks for the very detailed review, @jbygdell 👍 ! I fixed almost all of the comments, except for the one in config.go, since the pattern

err = ....
if err != nil {

is used for all other cases in that section.

@jbygdell jbygdell requested a review from a team November 14, 2023 14:03
@jbygdell
Copy link
Collaborator

jbygdell commented Nov 14, 2023

A short Readme.md file needs to be created

# Api

short one line description

## Service Description

More in depth description

@MalinAhlberg
Copy link
Contributor Author

A short Readme.md file needs to be created

In sda/cmd/api?

@jbygdell
Copy link
Collaborator

A short Readme.md file needs to be created

In sda/cmd/api?

Yes, name it api.md so that it matches the other readme files

@MalinAhlberg
Copy link
Contributor Author

A short Readme.md file needs to be created

See be12ee5

jbygdell
jbygdell previously approved these changes Nov 15, 2023
Copy link
Contributor

@dbampalikis dbampalikis left a comment

Choose a reason for hiding this comment

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

Really nice! Tested it manually (without using the integration test) and it works great! Also tested with another user and the result is an empty json array, which feels correct. Not sure if we would like to add a message such as no files found or something, but that's just an extra thing I guess Updated the review after Alex's finding

sda/cmd/api/api.md Outdated Show resolved Hide resolved
sda/cmd/api/api.md Outdated Show resolved Hide resolved
@dbampalikis dbampalikis self-requested a review November 17, 2023 14:46
@aaperis
Copy link
Contributor

aaperis commented Nov 17, 2023

I made a test with two files. I uploaded the first one:

./sda-cli upload -config s3cfg --encrypt-with-key c4gh.pub.pem test_file_demo

curl -sk -L "http://localhost:8090/files" -H "Authorization: Bearer $token" | jq

and the files api responded nicely:

[
  {
    "inboxPath": "test_dummy.org/test_file_demo.c4gh",
    "fileStatus": "uploaded",
    "createAt": "2023-11-17T14:07:48.825563Z"
  }
]

Then, I uploaded another file:

./sda-cli upload -config s3cfg test_file_demo.c4gh -targetDir asd/testtest.c4gh
./sda-cli list -config s3cfg 
The provided token expires in less than 24 hours
Consider renewing the token.
1.00MB   asd/testtest.c4gh/test_file_demo.c4gh 
1.00MB   test_file_demo.c4gh 

Then

curl -sk -L "http://localhost:8090/files" -H "Authorization: Bearer $token" | jq

but the files api returns only info on the last uploaded file:

[
  {
    "inboxPath": "test_dummy.org/asd/testtest.c4gh/test_file_demo.c4gh",
    "fileStatus": "uploaded",
    "createAt": "2023-11-17T14:11:47.143238Z"
  }
]

We sat down with @dbampalikis and tested a correction to the db query, see suggestion in Dimi's comments :-)

Copy link
Contributor

@dbampalikis dbampalikis left a comment

Choose a reason for hiding this comment

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

Really nice, apart for the comment about the database.
Also tested with another user and the result is an empty json array, which feels correct. Not sure if we would like to add a message such as no files found or something, but that's just an extra thing.
Finally, maybe we should have tests for multiple files?

sda/internal/database/db_functions.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@b159870). Click here to learn what that means.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #360   +/-   ##
=======================================
  Coverage        ?   60.89%           
=======================================
  Files           ?       19           
  Lines           ?     3445           
  Branches        ?        0           
=======================================
  Hits            ?     2098           
  Misses          ?     1207           
  Partials        ?      140           
Flag Coverage Δ
unittests 60.89% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MalinAhlberg
Copy link
Contributor Author

Thanks @aaperis and @dbampalikis , really good that you spotted this 🙈 🙏 ! I commited your fix along with a test case for this, see dd0e91f.

@MalinAhlberg
Copy link
Contributor Author

(force pushed to rebase on main)

@MalinAhlberg MalinAhlberg requested a review from a team November 30, 2023 09:43
Copy link
Contributor

@aaperis aaperis left a comment

Choose a reason for hiding this comment

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

Nice!

@pontus
Copy link
Contributor

pontus commented Nov 30, 2023

Looks good.

I have some thoughts about the TLS-configuration but I think I'll make an issue for general overhaul at some point instead.

@MalinAhlberg MalinAhlberg merged commit f97240f into main Nov 30, 2023
27 checks passed
@MalinAhlberg MalinAhlberg deleted the feat/files-api branch November 30, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants