Add Download API specification #1394
Conversation
jbygdell
force-pushed
the
feature/download_svc
branch
from
3ad0261 to
7daa6eb
Compare
jbygdell
force-pushed
the
feature/download_svc
branch
from
7daa6eb to
e02b66f
Compare
jbygdell
force-pushed
the
feature/download_svc
branch
from
cf3c27d to
b2398a3
Compare
| type: integer | ||
| responses: | ||
| "200": | ||
| content: |
There was a problem hiding this comment.
| content: | |
| headers: | |
| 'Content-Disposition: attachment; filename="filePath"' | |
| content: |
?
There was a problem hiding this comment.
Not really, if you request a file by stable ID that should be the filename in the header.
| description: Internal application error | ||
| security: | ||
| - bearerAuth: [] | ||
| /file/{fileId}: |
There was a problem hiding this comment.
| /file/{fileId}: | |
| /file/{datasetId}/{fileId}: |
? Or should it explicitly be possible to fetch a file when knowing the fileId?
There was a problem hiding this comment.
File stable ID is a unique entity and since the authorization step checks whether or not the user has permission to access the file or not this should be ok.
There was a problem hiding this comment.
I agree that it should be okay from a security standpoint and normally I'm all for not being lazy if it helps down the line, but here I'm a bit uncertain if there is a benefit in supporting this worth the added complexity.
There was a problem hiding this comment.
Adding the datasetID would make the authorization step easier/faster but then again is it really needed?
One problem that might arrive here is if the dataset ID is an URL.
There was a problem hiding this comment.
I think we agree that it's not strictly needed, but the flow gets easier (should not be overly complex but compared to other things in download it's a lot). We should maybe mull on it for a while.
(Values for datasetID can be managed through encoding.)
Co-authored-by: Pontus Freyhult <[email protected]>
| description: Internal application error | ||
| security: | ||
| - bearerAuth: [] | ||
| /ready: |
There was a problem hiding this comment.
Not /health?
There was a problem hiding this comment.
health or ready serve the same purpose
There was a problem hiding this comment.
I know, I was just wondering if it would make sense to keep the same endpoint name 🙃
There was a problem hiding this comment.
If there's no special reason, I agree with @MalinAhlberg that it is better to keep the same endpoint name.
Co-authored-by: Malin Klang <[email protected]>
jbygdell
force-pushed
the
feature/download_svc
branch
from
f2f809e to
83e67cd
Compare
|
In general, I think this looks very good!
true for the end of the range to? And exactly how should the range be formatted? |
Range headers has to adhere to the specification like what you wrote above. But also this is correct The query parameters on the other hand you can supply one or both, hence the logic about inferring the missing one. |
Co-authored-by: Malin Klang <[email protected]> Co-authored-by: Panos Chatzopoulos <[email protected]>
| openapi: 3.0.3 | ||
| info: | ||
| title: SDA file download API | ||
| version: "1.0" |
There was a problem hiding this comment.
Since this is premature, would it be better to use version 0.x?
| description: Returns the requested file to the user | ||
| parameters: | ||
| - in: header | ||
| description: "Public key used to re-encrypt the file (header) with. This should be supplied as the base64 encoding of the PEM (RFC 7468) encoding of the public key" |
There was a problem hiding this comment.
The description Public key used to re-encrypt the file (header) with. is ambiguous, unclear whether the public key is used to encrypt just the file header or the entire file.
| description: "Public key used to re-encrypt the file (header) with. This should be supplied as the base64 encoding of the PEM (RFC 7468) encoding of the public key" | |
| description: "Public key used to re-encrypt the file header server-side before transmission. This should be supplied as the base64 encoding of the PEM (RFC 7468) encoding of the public key" |
| schema: | ||
| type: string | ||
| required: false | ||
| description: Stable ID of the file to download. It is an error to supply both filePath and fileId |
There was a problem hiding this comment.
I think it is better to specify which error code to return when both filePath and fileId are specified. e.g. 400
Or maybe decide it later?
| format: binary | ||
| description: Successful partial file delivery | ||
| "400": | ||
| description: Bad request i.e start larger than end |
There was a problem hiding this comment.
Is start lager than end the only condition for an 400 error? If not, better to use e.g. instead of i.e..
If yes, change i.e -> i.e.
| "403": | ||
| description: The user does not have access to the dataset | ||
| "416": | ||
| description: Unsatisfiable range i.e. end larger than file size |
There was a problem hiding this comment.
Similar in this case, use e.g. if end larger than file size is not the only condition for 416.
| type: string | ||
| format: date-time | ||
| example: "2025-02-14T14:51:26.639Z" | ||
| files: |
There was a problem hiding this comment.
files represents number of files, right? Then maybe to use a clearer name, such as fileCount.
Related issue(s) and PR(s)
This PR closes #1378 .
Description
The
/file/{fileId}:is optional since the same functionality exists in the/fileendpoint. Although it provides a more convenient way to get a file out.Endpoints
/file- endpoint that takes query arguments to return a file by ID or path, the dataset name is a required parameter./file/{file_stable_id}- endpoint that returns the file withe the corresponding stable_id/info/datasets- returns a list of datasets the user has access to/info/datasets/{dataset_name}- returns information about the requested dataset/info/datasets/{dataset_name}/files- returns information about the files in the datasetHow to test
Paste the contents of the yaml file into: https://editor-next.swagger.io/ for a nicer view