Add patch endpoint for attachment metadata #27

[rowan04]

Description

Implements a PATCH endpoint /attachment/<attachment_id> that:

• Allows editing of
  • title
  • description
• Returns the updated attachment metadata
  • Without object_key and without a download url

Also prevents a file extension and content type mismatch when editing an attachment.

Testing instructions

Add a set up instructions describing how the reviewer should test the code

• Review code
• Check Actions build
• Review changes to test coverage
• {more steps here}

Agile board tracking

Resolves #27
Resolves #96

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.59%. Comparing base (ba67c36) to head (e222a9d).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #95      +/-   ##
===========================================
+ Coverage    99.57%   99.59%   +0.02%     
===========================================
  Files           19       19              
  Lines          468      492      +24     
===========================================
+ Hits           466      490      +24     
  Misses           2        2              

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

[asuresh-code]

All looks good, I actually do have 1 minor suggestion, which I realised after approving

[asuresh-code]

This PR addressed an issue where the file extension could be edited to not match the content type of an image, making it unopenable. I think the same problem exists for attachments, and could be addressed with the same implementation. The update portion of it could be covered in this PR, or be made a separate issue?

[rowan04]

This PR addressed an issue where the file extension could be edited to not match the content type of an image, making it unopenable. I think the same problem exists for attachments, and could be addressed with the same implementation. The update portion of it could be covered in this PR, or be made a separate issue?

yeah the same problem does exist for attachments, I'll look into it.

[rowan04]

My last commit checks for file extension and content type mismatch when an attachment is being edited, but not when its being created. I don't think I can follow the same implementation as images (as seen at https://github.com/ral-facilities/object-storage-api/pull/82/files#diff-89e412be5e05de57abb22dd8b7385de050592624f92bef53c8447c019f2fa3faR59), as attachments doesn't have an upload_file.content_type (please correct me if I'm wrong!). I think it would be possible to check the actual file and take the file type from that using magic (https://pypi.org/project/python-magic/), but that requires installing separately. So I'd appreciate advice, should this even be tackled in a separate PR? @asuresh-code @joelvdavies

[joelvdavies]

My last commit checks for file extension and content type mismatch when an attachment is being edited, but not when its being created. I don't think I can follow the same implementation as images (as seen at https://github.com/ral-facilities/object-storage-api/pull/82/files#diff-89e412be5e05de57abb22dd8b7385de050592624f92bef53c8447c019f2fa3faR59), as attachments doesn't have an upload_file.content_type (please correct me if I'm wrong!). I think it would be possible to check the actual file and take the file type from that using magic (https://pypi.org/project/python-magic/), but that requires installing separately. So I'd appreciate advice, should this even be tackled in a separate PR? @asuresh-code @joelvdavies

Yeah I would personally do it as a separate PR linking #96. Yes the content type is irrelevant in this case as it is always treated as binary data. My personal thought would be that the same logic preventing changing of the file extension on edit should still be fine - you can compare the old and new extension. But for upload there is nothing to prevent as we don't know the content type so its purely up to the front end instead.

Good spot with the library, that would also allow invalid images to be detected even if the content type/extension was manually manipulated. The only issue with it is it doesn't look like its had an update for 3 years so I would prefer to avoid using it if possible.

[rowan04]

Yeah I would personally do it as a separate PR linking #96. Yes the content type is irrelevant in this case as it is always treated as binary data. My personal thought would be that the same logic preventing changing of the file extension on edit should still be fine - you can compare the old and new extension. But for upload there is nothing to prevent as we don't know the content type so its purely up to the front end instead.

Good spot with the library, that would also allow invalid images to be detected even if the content type/extension was manually manipulated. The only issue with it is it doesn't look like its had an update for 3 years so I would prefer to avoid using it if possible.

ok thanks. So can this PR stay as it is, and the check when uploading attachments can be done in a different issue/pr? Or would you like the check when patching an attachment in a separate pr as well?

[joelvdavies]

Yeah I would personally do it as a separate PR linking #96. Yes the content type is irrelevant in this case as it is always treated as binary data. My personal thought would be that the same logic preventing changing of the file extension on edit should still be fine - you can compare the old and new extension. But for upload there is nothing to prevent as we don't know the content type so its purely up to the front end instead.
Good spot with the library, that would also allow invalid images to be detected even if the content type/extension was manually manipulated. The only issue with it is it doesn't look like its had an update for 3 years so I would prefer to avoid using it if possible.

ok thanks. So can this PR stay as it is, and the check when uploading attachments can be done in a different issue/pr? Or would you like the check when patching an attachment in a separate pr as well?

Ignore the upload attachments on the backend (the front end issue should prevent it instead), only add the check for editing attachments, I think you have already done it in this PR? In which case that is fine, it can be left as part of this PR and just link the issue in the description by adding another resolves/closes on it.

[asuresh-code]

Works well, and testing and implementation looks good.

[joelvdavies]

Looks good, worked well for me.