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 Get Endpoint for Image metadata and presigned url #69

Conversation

asuresh-code
Copy link
Contributor

@asuresh-code asuresh-code commented Nov 21, 2024

Description

See #37.

Testing instructions

  • Review code
  • Check Actions build
  • Review changes to test coverage

Agile board tracking

closes #37

@asuresh-code asuresh-code self-assigned this Nov 21, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 97.43590% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.22%. Comparing base (005e416) to head (43bea90).
Report is 6 commits behind head on develop.

Files with missing lines Patch % Lines
object_storage_api/core/exceptions.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #69      +/-   ##
===========================================
+ Coverage    98.09%   98.22%   +0.13%     
===========================================
  Files           21       21              
  Lines          368      395      +27     
===========================================
+ Hits           361      388      +27     
  Misses           7        7              

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

@asuresh-code asuresh-code marked this pull request as draft November 21, 2024 11:06
@asuresh-code
Copy link
Contributor Author

asuresh-code commented Nov 21, 2024

One of the requirements for this issue listed here #37 (comment) , requires that the endpoint:

Must gracefully deal with a situation when the file object with the given object_key doesn't exist

When given an object key that doesn't exist, the presigned url is still generated without any error being thrown. The issue only arises when you open the url and are greeted with this response.

image

I wanted to check if this would still be a requirement for the back-end to handle or the front-end? S3 won't throw any exceptions when generating the url, so if the check has to be done in the back-end, it would require an additional call to S3 to get the file, implemeting something like this https://stackoverflow.com/questions/33842944/check-if-a-key-exists-in-a-bucket-in-s3-using-boto3

A similar questions was asked here https://stackoverflow.com/questions/78010875/how-to-directly-generate-presigned-url-if-key-exist-in-s3-or-upload-obj-and-cre, and the response was to add an explicit check referenced in my 1st link

@joelvdavies
Copy link
Collaborator

One of the requirements for this issue listed here #37 (comment) , requires that the endpoint:

Must gracefully deal with a situation when the file object with the given object_key doesn't exist

When given an object key that doesn't exist, the presigned url is still generated without any error being thrown. The issue only arises when you open the url and are greeted with this response.

image

I wanted to check if this would still be a requirement for the back-end to handle or the front-end? S3 won't throw any exceptions when generating the url, so if the check has to be done in the back-end, it would require an additional call to S3 to get the file, implemeting something like this https://stackoverflow.com/questions/33842944/check-if-a-key-exists-in-a-bucket-in-s3-using-boto3

A similar questions was asked here https://stackoverflow.com/questions/78010875/how-to-directly-generate-presigned-url-if-key-exist-in-s3-or-upload-obj-and-cre, and the response was to add an explicit check referenced in my 1st link

Yeah this behaviour is fine. I just checked the front end and it displays the right message when it fails to load from the URL so I it's fine to leave the URL in and ignore the check.

@asuresh-code asuresh-code marked this pull request as ready for review November 25, 2024 11:44
Copy link
Collaborator

@joelvdavies joelvdavies left a comment

Choose a reason for hiding this comment

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

Some initial comments, one I am not sure on but we can discuss, worked well on the front end when we tested it.

object_storage_api/stores/image.py Outdated Show resolved Hide resolved
object_storage_api/stores/image.py Outdated Show resolved Hide resolved
object_storage_api/services/image.py Outdated Show resolved Hide resolved
object_storage_api/services/image.py Outdated Show resolved Hide resolved
test/e2e/test_image.py Outdated Show resolved Hide resolved
object_storage_api/routers/image.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@joelvdavies joelvdavies left a comment

Choose a reason for hiding this comment

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

Just some minor comments and suggestions.

object_storage_api/repositories/image.py Outdated Show resolved Hide resolved
object_storage_api/repositories/image.py Outdated Show resolved Hide resolved
object_storage_api/routers/image.py Outdated Show resolved Hide resolved
object_storage_api/repositories/image.py Outdated Show resolved Hide resolved
object_storage_api/services/image.py Outdated Show resolved Hide resolved
test/unit/repositories/test_image.py Outdated Show resolved Hide resolved
test/unit/repositories/test_image.py Outdated Show resolved Hide resolved
test/unit/repositories/test_image.py Show resolved Hide resolved
object_storage_api/schemas/image.py Outdated Show resolved Hide resolved
test/e2e/test_image.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@joelvdavies joelvdavies left a comment

Choose a reason for hiding this comment

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

Looks good, final few minor comments. Will get Viktor to have a look over the error handling next week to see what he thinks.

object_storage_api/repositories/image.py Outdated Show resolved Hide resolved
test/unit/repositories/test_image.py Outdated Show resolved Hide resolved
test/unit/stores/test_image.py Outdated Show resolved Hide resolved
@joelvdavies joelvdavies requested a review from VKTB December 2, 2024 12:34
object_storage_api/repositories/image.py Show resolved Hide resolved
object_storage_api/repositories/image.py Outdated Show resolved Hide resolved
object_storage_api/routers/image.py Outdated Show resolved Hide resolved
@asuresh-code asuresh-code force-pushed the add-get-endpoint-for-image-metadata-with-presigned-url-#37 branch from 8a07c40 to 72374c9 Compare December 4, 2024 11:42
joelvdavies
joelvdavies previously approved these changes Dec 4, 2024
@asuresh-code asuresh-code requested a review from VKTB December 4, 2024 12:07
Copy link
Member

@rowan04 rowan04 left a comment

Choose a reason for hiding this comment

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

Minor docstring changes?

test/e2e/test_image.py Outdated Show resolved Hide resolved
test/e2e/test_image.py Outdated Show resolved Hide resolved
test/unit/repositories/test_image.py Outdated Show resolved Hide resolved
test/unit/services/test_image.py Outdated Show resolved Hide resolved
Copy link
Member

@rowan04 rowan04 left a comment

Choose a reason for hiding this comment

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

Some more minor docstring changes

object_storage_api/core/exceptions.py Outdated Show resolved Hide resolved
object_storage_api/schemas/image.py Outdated Show resolved Hide resolved
object_storage_api/schemas/image.py Outdated Show resolved Hide resolved
object_storage_api/stores/image.py Outdated Show resolved Hide resolved
test/unit/stores/test_image.py Outdated Show resolved Hide resolved
test/unit/stores/test_image.py Outdated Show resolved Hide resolved
VKTB
VKTB previously approved these changes Dec 4, 2024
@asuresh-code asuresh-code dismissed stale reviews from VKTB and joelvdavies via 7b85508 December 4, 2024 14:10
@asuresh-code asuresh-code requested a review from rowan04 December 4, 2024 14:18
@asuresh-code asuresh-code merged commit 0477879 into develop Dec 5, 2024
3 checks passed
@asuresh-code asuresh-code deleted the add-get-endpoint-for-image-metadata-with-presigned-url-#37 branch December 5, 2024 13:34
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.

Add an endpoint for getting an image with a download URL
5 participants