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

Incorporate error handling in a few places #16

Merged
merged 4 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
"jsonwebtoken": "^9.0.2",
"mantine-datatable": "^7.12.4",
"multer": "^1.4.5-lts.1",
"next-swagger-doc": "^0.4"
"next-swagger-doc": "^0.4",
"uuid": "^11.0.3"
}
}
69 changes: 66 additions & 3 deletions src/app/api/run/[runId]/upload/route.test.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,20 @@
import { describe, expect, it } from 'vitest'
import { describe, expect, it, beforeEach } from 'vitest'
import { POST } from './route'
import { NextRequest } from 'next/server'
import path from 'path'
import fs from 'fs'
import { UPLOAD_DIR } from '@/app/utils'
import mockFs from 'mock-fs'
import { v4 as uuidv4 } from 'uuid'

beforeEach(() => {
fs.rmSync(UPLOAD_DIR, { recursive: true, force: true })
})

describe('POST /api/run/[runId]/upload', () => {
it('should upload a file successfully', async () => {
const mockFile = new Blob(['id,name\n1,John'], { type: 'text/csv' })
const mockRunId = '123'
const mockRunId = uuidv4()

const formData = new FormData()
formData.append('file', new File([mockFile], mockRunId))
Expand Down Expand Up @@ -38,9 +44,43 @@ describe('POST /api/run/[runId]/upload', () => {
const req = {
formData: async () => formData,
} as NextRequest
const params = { runId: '123' }
const params = { runId: uuidv4() }
const response = await POST(req, { params })
expect(response.status).toBe(400)
expect((await response.json()).error).toBe('Form data does not include expected file key')
})

it('should return failure if unexpected form data is included', async () => {
const mockFile = new Blob(['id,name\n1,John'], { type: 'text/csv' })
const mockRunId = uuidv4()

const formData = new FormData()
formData.append('file', new File([mockFile], mockRunId))
formData.append('file2', new File([mockFile], mockRunId))

const req = {
formData: async () => formData,
} as NextRequest
const params = { runId: uuidv4() }
const response = await POST(req, { params })
expect(response.status).toBe(400)
expect((await response.json()).error).toBe('Form data includes unexpected data keys')
})

it('should return failure if runId is not a UUID', async () => {
const mockRunId = '123'

const formData = new FormData()

const req = {
formData: async () => formData,
} as NextRequest

const params = { runId: mockRunId }

const response = await POST(req, { params })
expect(response.status).toBe(400)
expect((await response.json()).error).toBe('runId is not a UUID')
})

it('should return an error if no runID is provided', async () => {
Expand All @@ -55,4 +95,27 @@ describe('POST /api/run/[runId]/upload', () => {
const response = await POST(req, { params })
expect(response.status).toBe(400)
})

it('should return an error if the runID has results already', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

one "product" question i have around this is

  1. will they ever be uploading results for the same run? ie runs will always have different uuids right? so this is just a safeguard
  2. if the above is true, ie they can upload results for the same run, would we want to override and upload the newer results? ie overwrite vs error

just thinking out loud 🤔 💭

Copy link
Contributor Author

@rnathuji rnathuji Dec 12, 2024

Choose a reason for hiding this comment

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

Ah, I see. Yeah, my implicit assumption currently is it's a safeguard in case the researcher code (for some reason that was missed during code review 😅 ) attempts to submit multiple times for the same run.

I guess another case it could protect against is if (for whatever reason) the SA runs things twice, we ignore the erroneous re-submit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 -- FWIW, I think if we allow "override", we should version artifacts vs write over. That would definitely require some product definition, though, since it would have cascading impacts 😅 .

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah with your 2nd comment in mind, happy to just reject it and enforce a new runid upload :)

const mockRunId = uuidv4()

mockFs({
[UPLOAD_DIR]: {
[mockRunId]: '',
},
})

const formData = new FormData()
formData.append('file', '')

const req = {
formData: async () => formData,
} as NextRequest

const params = { runId: mockRunId }

const response = await POST(req, { params })
expect(response.status).toBe(400)
expect((await response.json()).error).toBe('Data already exists for runId')
Copy link
Collaborator

Choose a reason for hiding this comment

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

@philschatz had left a comment on my PR about testing for the error string of the 400 response and maybe having that not be necessary? im not sure if that applies here - i dont feel strongly either way, just pointing out for consistency

Copy link
Contributor Author

@rnathuji rnathuji Dec 12, 2024

Choose a reason for hiding this comment

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

Yeah -- I added it only to make sure we are getting the 400 from the expected code paths (because it can otherwise lead to false positive test passes). I'm open to a better suggestion on how to accomplish that, though 🤷‍♂️

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh yeah good point! Im happy keeping it in, doesn't matter much to me

})
})
21 changes: 20 additions & 1 deletion src/app/api/run/[runId]/upload/route.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { NextRequest, NextResponse } from 'next/server'
import { saveFile } from '@/app/utils'
import { saveFile, UPLOAD_DIR, isValidUUID } from '@/app/utils'
import path from 'path'
import fs from 'fs'

function isFile(obj: any): obj is File {
return obj instanceof File
Expand All @@ -14,6 +16,23 @@ export const POST = async (req: NextRequest, { params }: { params: { runId: stri
return NextResponse.json({ error: 'Missing runId' }, { status: 400 })
}

if (!isValidUUID(runId)) {
return NextResponse.json({ error: 'runId is not a UUID' }, { status: 400 })
}

if (!('file' in body)) {
return NextResponse.json({ error: 'Form data does not include expected file key' }, { status: 400 })
}

if (Object.keys(body).length !== 1) {
return NextResponse.json({ error: 'Form data includes unexpected data keys' }, { status: 400 })
}

const filePath = path.join(UPLOAD_DIR, runId)
if (fs.existsSync(filePath)) {
return NextResponse.json({ error: 'Data already exists for runId' }, { status: 400 })
}

if ('file' in body && isFile(body.file)) {
await saveFile(body.file, runId)
return NextResponse.json({}, { status: 200 })
Expand Down
19 changes: 18 additions & 1 deletion src/app/utils.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
import { createUploadDirIfNotExists, deleteFile, generateAuthorizationHeaders, saveFile, UPLOAD_DIR } from './utils'
import {
createUploadDirIfNotExists,
deleteFile,
generateAuthorizationHeaders,
isValidUUID,
saveFile,
UPLOAD_DIR,
} from './utils'
import mockFs from 'mock-fs'
import path from 'path'
import jwt from 'jsonwebtoken'
Expand Down Expand Up @@ -99,4 +106,14 @@ describe('Utils', () => {
})
})
})

describe('isValidUUID', () => {
it('should return false on an invalid UUID', () => {
expect(isValidUUID('123')).toBe(false)
})

it('should return true on a valid UUID', () => {
expect(isValidUUID('a9bcdef7-575e-4083-8fbf-e1a743f29f24')).toBe(true)
})
})
})
5 changes: 5 additions & 0 deletions src/app/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import path from 'path'
import fs from 'fs'
import os from 'os'
import jwt from 'jsonwebtoken'
import { validate as uuidValidate } from 'uuid'

export const UPLOAD_DIR = path.resolve(os.tmpdir(), 'public/uploads')

Expand Down Expand Up @@ -40,3 +41,7 @@ export const generateAuthorizationHeaders = () => {
Authorization: `Bearer ${token}`,
}
}

export const isValidUUID = (value: string): boolean => {
return uuidValidate(value)
}