From 7b0099fb4ee2be9be187ba0d2980cd272c1b4fa4 Mon Sep 17 00:00:00 2001 From: Scott Lamb Date: Thu, 21 Oct 2021 10:25:37 -0700 Subject: [PATCH] use typed errors in /view.mp4 path This fixes #178. Before, everything got translated to 5xx status; now it produces the correct type in several cases. Ideally I'd get rid of the untyped errors in all of web.rs; this is a small step. --- design/api.md | 2 +- server/db/db.rs | 23 ++++++++------- server/db/raw.rs | 74 +++++++++++++++++++++++++++-------------------- server/src/web.rs | 17 +++++++---- 4 files changed, 68 insertions(+), 48 deletions(-) diff --git a/design/api.md b/design/api.md index 99fe5fd..fc22e1f 100644 --- a/design/api.md +++ b/design/api.md @@ -462,7 +462,7 @@ Bugs and limitations: reference them. * The final recording in every "run" ends with a frame that has duration 0. It's not possible to append additional segments after such a frame; - the server will return an error like `Invalid argument: unable to append + the server will return a 400 error like `Invalid argument: unable to append recording 2/16672 after recording 2/16671 with trailing zero`. See also `hasTrailingZero` above, and [#178](https://github.com/scottlamb/moonfire-nvr/issues/178). diff --git a/server/db/db.rs b/server/db/db.rs index c6e087d..cba819e 100644 --- a/server/db/db.rs +++ b/server/db/db.rs @@ -33,6 +33,7 @@ use crate::raw; use crate::recording; use crate::schema; use crate::signal; +use base::bail_t; use base::clock::{self, Clocks}; use base::strutil::encode_size; use failure::{bail, format_err, Error, ResultExt}; @@ -1258,10 +1259,10 @@ impl LockedDatabase { &self, stream_id: i32, desired_time: Range, - f: &mut dyn FnMut(ListRecordingsRow) -> Result<(), Error>, - ) -> Result<(), Error> { + f: &mut dyn FnMut(ListRecordingsRow) -> Result<(), base::Error>, + ) -> Result<(), base::Error> { let s = match self.streams_by_id.get(&stream_id) { - None => bail!("no such stream {}", stream_id), + None => bail_t!(NotFound, "no such stream {}", stream_id), Some(s) => s, }; raw::list_recordings_by_time(&self.conn, stream_id, desired_time.clone(), f)?; @@ -1291,10 +1292,10 @@ impl LockedDatabase { &self, stream_id: i32, desired_ids: Range, - f: &mut dyn FnMut(ListRecordingsRow) -> Result<(), Error>, - ) -> Result<(), Error> { + f: &mut dyn FnMut(ListRecordingsRow) -> Result<(), base::Error>, + ) -> Result<(), base::Error> { let s = match self.streams_by_id.get(&stream_id) { - None => bail!("no such stream {}", stream_id), + None => bail_t!(NotFound, "no such stream {}", stream_id), Some(s) => s, }; if desired_ids.start < s.cum_recordings { @@ -1332,8 +1333,8 @@ impl LockedDatabase { stream_id: i32, desired_time: Range, forced_split: recording::Duration, - f: &mut dyn FnMut(&ListAggregatedRecordingsRow) -> Result<(), Error>, - ) -> Result<(), Error> { + f: &mut dyn FnMut(&ListAggregatedRecordingsRow) -> Result<(), base::Error>, + ) -> Result<(), base::Error> { // Iterate, maintaining a map from a recording_id to the aggregated row for the latest // batch of recordings from the run starting at that id. Runs can be split into multiple // batches for a few reasons: @@ -1371,7 +1372,8 @@ impl LockedDatabase { } else { // append. if a.time.end != row.start { - bail!( + bail_t!( + Internal, "stream {} recording {} ends at {} but {} starts at {}", stream_id, a.ids.end - 1, @@ -1381,7 +1383,8 @@ impl LockedDatabase { ); } if a.open_id != row.open_id { - bail!( + bail_t!( + Internal, "stream {} recording {} has open id {} but {} has {}", stream_id, a.ids.end - 1, diff --git a/server/db/raw.rs b/server/db/raw.rs index cbc4f62..cc5f90e 100644 --- a/server/db/raw.rs +++ b/server/db/raw.rs @@ -6,7 +6,8 @@ use crate::db::{self, CompositeId, FromSqlUuid}; use crate::recording; -use failure::{bail, Error, ResultExt}; +use base::{ErrorKind, ResultExt as _}; +use failure::{bail, Error, ResultExt as _}; use fnv::FnvHashSet; use rusqlite::{named_params, params}; use std::ops::Range; @@ -103,14 +104,18 @@ pub(crate) fn list_recordings_by_time( conn: &rusqlite::Connection, stream_id: i32, desired_time: Range, - f: &mut dyn FnMut(db::ListRecordingsRow) -> Result<(), Error>, -) -> Result<(), Error> { - let mut stmt = conn.prepare_cached(LIST_RECORDINGS_BY_TIME_SQL)?; - let rows = stmt.query(named_params! { - ":stream_id": stream_id, - ":start_time_90k": desired_time.start.0, - ":end_time_90k": desired_time.end.0, - })?; + f: &mut dyn FnMut(db::ListRecordingsRow) -> Result<(), base::Error>, +) -> Result<(), base::Error> { + let mut stmt = conn + .prepare_cached(LIST_RECORDINGS_BY_TIME_SQL) + .err_kind(ErrorKind::Internal)?; + let rows = stmt + .query(named_params! { + ":stream_id": stream_id, + ":start_time_90k": desired_time.start.0, + ":end_time_90k": desired_time.end.0, + }) + .err_kind(ErrorKind::Internal)?; list_recordings_inner(rows, false, f) } @@ -119,39 +124,46 @@ pub(crate) fn list_recordings_by_id( conn: &rusqlite::Connection, stream_id: i32, desired_ids: Range, - f: &mut dyn FnMut(db::ListRecordingsRow) -> Result<(), Error>, -) -> Result<(), Error> { - let mut stmt = conn.prepare_cached(LIST_RECORDINGS_BY_ID_SQL)?; - let rows = stmt.query(named_params! { - ":start": CompositeId::new(stream_id, desired_ids.start).0, - ":end": CompositeId::new(stream_id, desired_ids.end).0, - })?; + f: &mut dyn FnMut(db::ListRecordingsRow) -> Result<(), base::Error>, +) -> Result<(), base::Error> { + let mut stmt = conn + .prepare_cached(LIST_RECORDINGS_BY_ID_SQL) + .err_kind(ErrorKind::Internal)?; + let rows = stmt + .query(named_params! { + ":start": CompositeId::new(stream_id, desired_ids.start).0, + ":end": CompositeId::new(stream_id, desired_ids.end).0, + }) + .err_kind(ErrorKind::Internal)?; list_recordings_inner(rows, true, f) } fn list_recordings_inner( mut rows: rusqlite::Rows, include_prev: bool, - f: &mut dyn FnMut(db::ListRecordingsRow) -> Result<(), Error>, -) -> Result<(), Error> { - while let Some(row) = rows.next()? { - let wall_duration_90k = row.get(4)?; - let media_duration_delta_90k: i32 = row.get(5)?; + f: &mut dyn FnMut(db::ListRecordingsRow) -> Result<(), base::Error>, +) -> Result<(), base::Error> { + while let Some(row) = rows.next().err_kind(ErrorKind::Internal)? { + let wall_duration_90k = row.get(4).err_kind(ErrorKind::Internal)?; + let media_duration_delta_90k: i32 = row.get(5).err_kind(ErrorKind::Internal)?; f(db::ListRecordingsRow { - id: CompositeId(row.get(0)?), - run_offset: row.get(1)?, - flags: row.get(2)?, - start: recording::Time(row.get(3)?), + id: CompositeId(row.get(0).err_kind(ErrorKind::Internal)?), + run_offset: row.get(1).err_kind(ErrorKind::Internal)?, + flags: row.get(2).err_kind(ErrorKind::Internal)?, + start: recording::Time(row.get(3).err_kind(ErrorKind::Internal)?), wall_duration_90k, media_duration_90k: wall_duration_90k + media_duration_delta_90k, - sample_file_bytes: row.get(6)?, - video_samples: row.get(7)?, - video_sync_samples: row.get(8)?, - video_sample_entry_id: row.get(9)?, - open_id: row.get(10)?, + sample_file_bytes: row.get(6).err_kind(ErrorKind::Internal)?, + video_samples: row.get(7).err_kind(ErrorKind::Internal)?, + video_sync_samples: row.get(8).err_kind(ErrorKind::Internal)?, + video_sample_entry_id: row.get(9).err_kind(ErrorKind::Internal)?, + open_id: row.get(10).err_kind(ErrorKind::Internal)?, prev_media_duration_and_runs: match include_prev { false => None, - true => Some((recording::Duration(row.get(11)?), row.get(12)?)), + true => Some(( + recording::Duration(row.get(11).err_kind(ErrorKind::Internal)?), + row.get(12).err_kind(ErrorKind::Internal)?, + )), }, })?; } diff --git a/server/src/web.rs b/server/src/web.rs index b1dab35..08c4f86 100644 --- a/server/src/web.rs +++ b/server/src/web.rs @@ -11,7 +11,7 @@ use core::borrow::Borrow; use core::str::FromStr; use db::dir::SampleFileDir; use db::{auth, recording}; -use failure::{bail, format_err, Error}; +use failure::{format_err, Error}; use fnv::FnvHashMap; use futures::stream::StreamExt; use futures::{future::Either, sink::SinkExt}; @@ -860,7 +860,8 @@ impl Service { if let Some(o) = s.open_id { if r.open_id != o { - bail!( + bail_t!( + NotFound, "recording {} has open id {}, requested {}", r.id, r.open_id, @@ -872,9 +873,14 @@ impl Service { // Check for missing recordings. match prev { None if recording_id == s.ids.start => {} - None => bail!("no such recording {}/{}", stream_id, s.ids.start), + None => bail_t!( + NotFound, + "no such recording {}/{}", + stream_id, + s.ids.start + ), Some(id) if r.id.recording() != id + 1 => { - bail!("no such recording {}/{}", stream_id, id + 1); + bail_t!(NotFound, "no such recording {}/{}", stream_id, id + 1); } _ => {} }; @@ -913,8 +919,7 @@ impl Service { } cur_off += wd; Ok(()) - }) - .map_err(internal_server_err)?; + })?; // Check for missing recordings. match prev {