Incorporate error handling in a few places #16
Conversation
rnathuji
force-pushed
the
SHRMP-52/error-handling
branch
from
725cfe8 to
d87b3bc
Compare
| @@ -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 () => { | |||
There was a problem hiding this comment.
one "product" question i have around this is
- will they ever be uploading results for the same run? ie runs will always have different uuids right? so this is just a safeguard
- 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 🤔 💭
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🤔 -- 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 😅 .
There was a problem hiding this comment.
yeah with your 2nd comment in mind, happy to just reject it and enforce a new runid upload :)
|
|
||
| const response = await POST(req, { params }) | ||
| expect(response.status).toBe(400) | ||
| expect((await response.json()).error).toBe('Data already exists for runId') |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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 🤷♂️
There was a problem hiding this comment.
oh yeah good point! Im happy keeping it in, doesn't matter much to me
No description provided.