-
Notifications
You must be signed in to change notification settings - Fork 13
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 an endpoint to download a file #818
Changes from 2 commits
a8cdd63
097b6bf
2bc73e5
c53e1fa
c2d497b
ebe81a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import { S3Client } from '@aws-sdk/client-s3' | ||
import { GetObjectCommand, GetObjectRequest, S3Client } from '@aws-sdk/client-s3' | ||
import { Upload } from '@aws-sdk/lib-storage' | ||
|
||
import config from '../utils/v2/config.js' | ||
|
@@ -31,3 +31,21 @@ export async function putObjectStream(bucket: string, key: string, body: Readabl | |
fileSize, | ||
} | ||
} | ||
|
||
export async function getObjectStream(bucket: string, key: string, range?: { start: number; end: number }) { | ||
const client = await getS3Client() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to create a new S3 Client for every S3 request? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our S3 credentials expire on some platforms relatively frequently. Tracking whether the current S3 client has expired and replacing it seems too complex for a function that's likely to only get called a few hundred - thousand times a day. |
||
|
||
const input: GetObjectRequest = { | ||
Bucket: bucket, | ||
Key: key, | ||
} | ||
|
||
if (range) { | ||
input.Range = `bytes=${range.start}-${range.end}` | ||
} | ||
|
||
const command = new GetObjectCommand(input) | ||
const response = await client.send(command) | ||
|
||
return response | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
import bodyParser from 'body-parser' | ||
import contentDisposition from 'content-disposition' | ||
import { Request, Response } from 'express' | ||
import { z } from 'zod' | ||
|
||
import { FileInterface } from '../../../../models/v2/File.js' | ||
import { downloadFile, getFileById } from '../../../../services/v2/file.js' | ||
import { BadReq, InternalError } from '../../../../utils/v2/error.js' | ||
import { parse } from '../../../../utils/validate.js' | ||
|
||
export const getDownloadFileSchema = z.object({ | ||
params: z.object({ | ||
modelId: z.string({ | ||
required_error: 'Must specify model id as param', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The standard Zod error here would be sufficient? |
||
}), | ||
fileId: z.string(), | ||
}), | ||
}) | ||
|
||
interface GetDownloadFileResponse { | ||
files: Array<FileInterface> | ||
} | ||
|
||
export const getDownloadFile = [ | ||
bodyParser.json(), | ||
async (req: Request, res: Response<GetDownloadFileResponse>) => { | ||
const { | ||
params: { fileId }, | ||
} = parse(req, getDownloadFileSchema) | ||
|
||
const file = await getFileById(req.user, fileId) | ||
|
||
// required to support utf-8 file names | ||
res.set('Content-Disposition', contentDisposition(file.name, { type: 'inline' })) | ||
res.set('Content-Type', file.mime) | ||
res.set('Cache-Control', 'public, max-age=604800, immutable') | ||
|
||
if (req.headers.range) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do this first before logic for response? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Range requests will include the same headers (Content-Type, Cache-Control and Content-Disposition) seen above. The idea behind placing it here is that when we do support it you can just write the implementation in here and not need to change anything else. I could move the error up (rule: do error checking first), but it feels like a waste for now. |
||
// TODO: support ranges | ||
throw BadReq('Ranges are not supported', { fileId }) | ||
} | ||
|
||
res.set('Content-Length', String(file.size)) | ||
// TODO: support ranges | ||
// res.set('Accept-Ranges', 'bytes') | ||
res.writeHead(200) | ||
|
||
const stream = await downloadFile(req.user, fileId) | ||
if (!stream.Body) { | ||
throw InternalError('We were not able to retrieve the body of this file', { fileId }) | ||
} | ||
|
||
// The AWS library doesn't seem to properly type 'Body' as being pipeable? | ||
;(stream.Body as any).pipe(res) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you raise this issue on their repo? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Existing issue here: aws/aws-sdk-js-v3#4720 Also narrowed type to |
||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I missing something, where is the method's return statement? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to return anything. The |
||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import { putObjectStream } from '../../clients/s3.js' | ||
import { ModelAction } from '../../connectors/v2/authorisation/Base.js' | ||
import { getObjectStream, putObjectStream } from '../../clients/s3.js' | ||
import { FileAction, ModelAction } from '../../connectors/v2/authorisation/Base.js' | ||
import authorisation from '../../connectors/v2/authorisation/index.js' | ||
import FileModel from '../../models/v2/File.js' | ||
import { UserDoc } from '../../models/v2/User.js' | ||
|
@@ -37,6 +37,18 @@ export async function uploadFile(user: UserDoc, modelId: string, name: string, m | |
return file | ||
} | ||
|
||
export async function downloadFile(user: UserDoc, fileId: string, range?: { start: number; end: number }) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests? |
||
const file = await getFileById(user, fileId) | ||
const model = await getModelById(user, file.modelId) | ||
|
||
const access = await authorisation.userFileAction(user, model, file, FileAction.Download) | ||
if (!access) { | ||
throw Forbidden(`You do not have permission to download this model.`, { user: user.dn, fileId }) | ||
} | ||
|
||
return getObjectStream(file.bucket, file.path, range) | ||
} | ||
|
||
export async function getFileById(user: UserDoc, fileId: string) { | ||
const file = await FileModel.findOne({ | ||
_id: fileId, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests for this method?