handlers: exit if a bad request or error is encountered

[emranemran]

No description provided.

[codecov]

Codecov Report

Merging #168 (10dd863) into main (bda4324) will decrease coverage by 0.10408%.
The diff coverage is 0.00000%.

@@                 Coverage Diff                 @@
##                main        #168         +/-   ##
===================================================
- Coverage   44.75524%   44.65116%   -0.10408%     
===================================================
  Files             24          24                 
  Lines           1716        1720          +4     
===================================================
  Hits             768         768                 
- Misses           858         862          +4     
  Partials          90          90                 

Impacted Files	Coverage Δ
handlers/upload.go	52.67857% <0.00000%> (-1.95106%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bda4324...10dd863. Read the comment docs.

Impacted Files	Coverage Δ
handlers/upload.go	52.67857% <0.00000%> (-1.95106%)	⬇️

[red-0ne]

There is a missing one here

catalyst-api/handlers/upload.go

Line 147 in 10dd863

errors.WriteHTTPInternalServerError(w, "Cannot send transcode status", err)

if err := clients.DefaultCallbackClient.SendTranscodeStatus(uploadVODRequest.CallbackUrl, clients.TranscodeStatusPreparing, 0.0); err != nil {
	errors.WriteHTTPInternalServerError(w, "Cannot send transcode status", err)
}

[emranemran]

There is a missing one here

catalyst-api/handlers/upload.go

Line 147 in 10dd863

errors.WriteHTTPInternalServerError(w, "Cannot send transcode status", err)

if err := clients.DefaultCallbackClient.SendTranscodeStatus(uploadVODRequest.CallbackUrl, clients.TranscodeStatusPreparing, 0.0); err != nil {
	errors.WriteHTTPInternalServerError(w, "Cannot send transcode status", err)
}

@red-0ne I left that one out on purpose. That's the callback to studio so it didn't make sense to bail early if it happens to fail to notify studio - we should continue with everything anyways.

[thomshutt]

LGTM, good job catching this @red-0ne!

[red-0ne]

There is a missing one here

catalyst-api/handlers/upload.go

Line 147 in 10dd863

errors.WriteHTTPInternalServerError(w, "Cannot send transcode status", err)

if err := clients.DefaultCallbackClient.SendTranscodeStatus(uploadVODRequest.CallbackUrl, clients.TranscodeStatusPreparing, 0.0); err != nil {
	errors.WriteHTTPInternalServerError(w, "Cannot send transcode status", err)
}

@red-0ne I left that one out on purpose. That's the callback to studio so it didn't make sense to bail early if it happens to fail to notify studio - we should continue with everything anyways.

You are right. Thanks for the clarification. Shouldn't we have this in a goroutine to save up some transcoding time then?

[red-0ne]

Also, we shouldn't write the error to the response then proceed to write response bytes coming out of transcoding. IMHO we should not do anything with responses from SendTranscodeStatus.

[thomshutt]

I think you're right on both counts @red-0ne - feel free to put in a followup PR to fix. I think the goroutine should be spawned intside the callback client and then it should just log and not return anything on errors

[red-0ne]

I think you're right on both counts @red-0ne - feel free to put in a followup PR to fix. I think the goroutine should be spawned intside the callback client and then it should just log and not return anything on errors

I've worked on this followup. It seems that it is easier to have the goroutine outside of the callback if we want to keep testing against errors of the sendTranscodeStatus request; without adding some mechanism to capture the error from outside. Otherwise we be removing 4 assertions from 2 tests of the callback client. WDYT @thomshutt ?