Add post endpoint for attachments #11

[joelvdavies]

Description

Adds an endpoint /attachments for creating new attachments.

Notes

• Creates the pre-signed url before the database insert to avoid needing transactions
• For the post response I had to create the response schema in the service layer rather than routers as is typically done in IMS
• I have attempted to implement a different style of error handling on the InvalidObjectId errors, although these are the trickiest to do and has resulted in the need for re-raising an error anyway. There might be a way to modify the validator to include the field_name in the exception rather than having to do this. I attempted it but could not get it to work at the time.
• We may wish to have another repo at some point with the following (copied from IMS and slightly modified due to slightly different error handling) - would probably have complications around versioning of that repo, but can be done with https://stackoverflow.com/questions/73274808/add-private-github-repo-to-pyproject-toml-as-new-dependency with tags)
  • core/custom_object_id.py
  • some of core/exceptions.py
  • models/custom_object_id_data_types.py
  • test/unit/core/test_custom_object.py
  • models/mixins.py
  • schemas/mixins.py
  • Any other future duplicate code - the auth middleware for example
• Only empty buckets may be deleted, also if a bucket is already in existence it will error on creation (like when a previous test run failed) so while I was initially going to get the e2e tests to create & delete the bucket, I have resorted to emptying its contents instead
• MinIO now uses host networking as avoids having to replace the docker hostname in presigned URLs to localhost in e2e tests

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

Agile board tracking

Closes #11

[joelvdavies]

@VKTB The core functionality is done, I will look at the tests next. Feel free to have a look - especially at the error handling.

[VKTB]

Some initial comments from me. Not sure if you want to keep the TODOs in or remove them, I will leave it to you to decide.

[VKTB]

I think we should ask ourselves here if it is clear enough that this endpoint (given its prefix and path) is used for getting a pre-signed URL rather than posting an actual attachment. It might help if we look from a perspective of how this router would look like if we also had another endpoint that allows for posting of actual attachments - i.e. how the endpoints would differ.

[joelvdavies]

Yeah good point. I would think

• /attachments for upload of metadata and data
• /attachments/metadata for submitting just the metadata about the attachment

We can also argue the same issue for GETs, we will not be downloading an attachment, only its metadata. There I would end up with

• /attachments/metadata for listing all attachment metadata
• /attachments/metadata/<id> for getting the information about an individual one (potentially with a presigned url?)

But I am not sure on that second point, nothing about it screams 'will return a presigned download url'. Equally I don't like /attachments/<id>/download as again its not downloading its just getting a presigned url. This would allow for an upload one if the initial upload failed some how (not something I think we need to worry about now though).

We could also have /download-url or something, but the upload doesn't make sense there. So I'm not sure there are any nice all round solutions unless you have any suggestions?

Right now I am essentially considering the attachment to be created when the metadata of it is posted, and I would expect it to be created when posting to /attachments.

[VKTB]

unless you have any suggestions?

I am not sure I do at this point, especially given your thoughts on this. I might do further down the line, I will let you know. Probably merge it as it is for now.

[joelvdavies]

Sure, I will make an issue to look at it again a bit later, the endpoints should be easy to change.

[joelvdavies]

#23