diff --git a/db/AI_Models.sql b/db/AI_Models.sql index e515bf8b0..4128b1478 100644 --- a/db/AI_Models.sql +++ b/db/AI_Models.sql @@ -62,7 +62,9 @@ CREATE TABLE IF NOT EXISTS `AI_Detection_Settings` ( CREATE TABLE IF NOT EXISTS `AI_Detections` ( `Id` BIGINT unsigned NOT NULL auto_increment, `EventId` BIGINT unsigned NOT NULL, - `FrameId` BIGINT unsigned, + /* Frames.FrameId within EventId. Not a foreign key: Frames rows are + written in batches, so a detection can be recorded before its frame. */ + `FrameId` int(10) unsigned, `ObjectClassId` int(10) unsigned NOT NULL, `Confidence` decimal(5,4) NOT NULL, `BoundingBoxX` int(10) unsigned, @@ -71,10 +73,8 @@ CREATE TABLE IF NOT EXISTS `AI_Detections` ( `BoundingBoxHeight` int(10) unsigned, `DetectedAt` TIMESTAMP(3) DEFAULT CURRENT_TIMESTAMP(3), PRIMARY KEY (`Id`), - KEY `AI_Detections_EventId_idx` (`EventId`), - KEY `AI_Detections_FrameId_idx` (`FrameId`), + KEY `AI_Detections_EventId_FrameId_idx` (`EventId`,`FrameId`), KEY `AI_Detections_ObjectClassId_idx` (`ObjectClassId`), FOREIGN KEY (`EventId`) REFERENCES `Events` (`Id`) ON DELETE CASCADE, - FOREIGN KEY (`FrameId`) REFERENCES `Frames` (`Id`) ON DELETE SET NULL, FOREIGN KEY (`ObjectClassId`) REFERENCES `AI_Object_Classes` (`Id`) ON DELETE CASCADE ) ENGINE=InnoDB; diff --git a/db/zm_create.sql.in b/db/zm_create.sql.in index bd273f6b8..feba2f0a4 100644 --- a/db/zm_create.sql.in +++ b/db/zm_create.sql.in @@ -443,12 +443,11 @@ CREATE TABLE `Filters` ( DROP TABLE IF EXISTS `Frames`; CREATE TABLE `Frames` ( - `Id` BIGINT UNSIGNED NOT NULL AUTO_INCREMENT, `EventId` BIGINT UNSIGNED NOT NULL default '0', /* FOREIGN KEY (`EventId`) REFERENCES `Events` (`Id`) ON DELETE CASCADE,*/ `FrameId` int(10) unsigned NOT NULL default '0', `Type` enum('Normal','Bulk','Alarm') NOT NULL default 'Normal', - `TimeStamp` timestamp NOT NULL default CURRENT_TIMESTAMP on update CURRENT_TIMESTAMP, + `TimeStamp` timestamp NOT NULL default CURRENT_TIMESTAMP, `Delta` decimal(8,2) NOT NULL default '0.00', `Score` smallint(5) unsigned NOT NULL default '0', /* Peak audio level since the previous row, 0-100 on the same dBFS-derived @@ -457,11 +456,11 @@ CREATE TABLE `Frames` ( chosen by looking at the graph. 0 means silence or no audio, which is how the event view knows not to draw an audio line. */ `AudioLevel` tinyint(3) unsigned NOT NULL default '0', - PRIMARY KEY (`Id`), /* Every query on Frames is anchored on EventId and orders or ranges by - FrameId within the event, so one composite index serves them all. Nothing - reads Frames by Type or TimeStamp without EventId. See zm_update-1.39.27. */ - INDEX `EventId_FrameId_idx` (`EventId`,`FrameId`) + FrameId within the event, so the clustered key serves them all and keeps + an event's rows together. Nothing reads Frames by Type or TimeStamp + without EventId. See zm_update-1.39.27 and zm_update-1.39.36. */ + PRIMARY KEY (`EventId`,`FrameId`) ) ENGINE=@ZM_MYSQL_ENGINE@; -- diff --git a/db/zm_update-1.39.36.sql b/db/zm_update-1.39.36.sql new file mode 100644 index 000000000..303943be2 --- /dev/null +++ b/db/zm_update-1.39.36.sql @@ -0,0 +1,73 @@ +-- +-- This updates a 1.39.35 database to 1.39.36 +-- +-- Make (EventId, FrameId) the primary key of Frames and drop the surrogate Id. +-- +-- Frames is the largest table in the schema. Every query against it is +-- anchored on EventId and orders or ranges by FrameId (see zm_update-1.39.27), +-- and nothing looks a frame up by Id. Keeping Id meant a clustered index that +-- nothing reads plus a secondary EventId_FrameId_idx that every read uses, +-- each secondary entry also carrying a copy of Id. On a copy of a real table +-- the secondary index was half the table's size. With (EventId, FrameId) as +-- the clustered key that index goes away, and an event's rows sit together on +-- disk, so deleting an event is a range delete rather than scattered writes. +-- +-- FrameId is the per-event frame counter, so (EventId, FrameId) is unique for +-- anything zmc writes. Duplicates are removed first anyway, keeping the +-- earliest row, because a single duplicate would make ADD PRIMARY KEY fail. +-- +-- TimeStamp also loses ON UPDATE CURRENT_TIMESTAMP. It is the time the frame +-- was captured, and any UPDATE of a frame row was silently replacing it with +-- the time of the update. +-- +-- AI_Detections.FrameId was a foreign key to Frames.Id. It becomes the +-- per-event frame number, the same thing Frames.FrameId, Stats.FrameId and +-- Event_Data.FrameId hold, converted from the old Id before Id is dropped. +-- It cannot be a composite foreign key to Frames: ON DELETE SET NULL would +-- have to null the NOT NULL EventId, and Frames rows are written in batches, +-- so a detection can be recorded before its frame row exists. +-- +-- Note for large installs: this rebuilds the table and will take a while on a +-- Frames table with hundreds of millions of rows. +-- + +set @exist := (select count(*) from information_schema.columns where table_schema = database() and table_name = 'Frames' and column_name = 'Id'); + +set @fk := (select constraint_name from information_schema.referential_constraints where constraint_schema = database() and table_name = 'AI_Detections' and referenced_table_name = 'Frames' limit 1); +set @sqlstmt := if( @fk is not null, concat('ALTER TABLE `AI_Detections` DROP FOREIGN KEY `', @fk, '`'), "SELECT 'AI_Detections has no foreign key to Frames.'"); +PREPARE stmt FROM @sqlstmt; +EXECUTE stmt; + +set @sqlstmt := if( @exist > 0, 'UPDATE `AI_Detections` D JOIN `Frames` F ON F.`Id` = D.`FrameId` SET D.`FrameId` = F.`FrameId`', "SELECT 'AI_Detections.FrameId already converted.'"); +PREPARE stmt FROM @sqlstmt; +EXECUTE stmt; + +set @idx := (select count(*) from information_schema.statistics where table_schema = database() and table_name = 'AI_Detections' and index_name = 'AI_Detections_EventId_FrameId_idx'); +set @sqlstmt := if( @idx = 0, 'ALTER TABLE `AI_Detections` MODIFY `FrameId` int(10) unsigned, ADD INDEX `AI_Detections_EventId_FrameId_idx` (`EventId`,`FrameId`)', "SELECT 'AI_Detections_EventId_FrameId_idx already exists.'"); +PREPARE stmt FROM @sqlstmt; +EXECUTE stmt; + +set @idx := (select count(*) from information_schema.statistics where table_schema = database() and table_name = 'AI_Detections' and index_name = 'AI_Detections_FrameId_idx'); +set @sqlstmt := if( @idx > 0, 'DROP INDEX `AI_Detections_FrameId_idx` ON `AI_Detections`', "SELECT 'AI_Detections_FrameId_idx already removed.'"); +PREPARE stmt FROM @sqlstmt; +EXECUTE stmt; + +set @idx := (select count(*) from information_schema.statistics where table_schema = database() and table_name = 'AI_Detections' and index_name = 'AI_Detections_EventId_idx'); +set @sqlstmt := if( @idx > 0, 'DROP INDEX `AI_Detections_EventId_idx` ON `AI_Detections`', "SELECT 'AI_Detections_EventId_idx already removed.'"); +PREPARE stmt FROM @sqlstmt; +EXECUTE stmt; + +SELECT IF(@exist > 0, 'Removing duplicate (EventId, FrameId) rows from Frames.', 'Frames.Id already removed.'); +set @sqlstmt := if( @exist > 0, 'DELETE F1 FROM `Frames` F1 JOIN `Frames` F2 ON F1.`EventId` = F2.`EventId` AND F1.`FrameId` = F2.`FrameId` AND F1.`Id` > F2.`Id`', "SELECT 1"); +PREPARE stmt FROM @sqlstmt; +EXECUTE stmt; + +SELECT IF(@exist > 0, 'Rebuilding Frames with (EventId, FrameId) as primary key. On a large Frames table this will take some time.', ''); +set @sqlstmt := if( @exist > 0, 'ALTER TABLE `Frames` + DROP PRIMARY KEY, + DROP COLUMN `Id`, + ADD PRIMARY KEY (`EventId`, `FrameId`), + DROP INDEX `EventId_FrameId_idx`, + MODIFY `TimeStamp` timestamp NOT NULL default CURRENT_TIMESTAMP', "SELECT 1"); +PREPARE stmt FROM @sqlstmt; +EXECUTE stmt; diff --git a/scripts/ZoneMinder/lib/ZoneMinder/Event.pm b/scripts/ZoneMinder/lib/ZoneMinder/Event.pm index 9f23bbc58..7110c473c 100644 --- a/scripts/ZoneMinder/lib/ZoneMinder/Event.pm +++ b/scripts/ZoneMinder/lib/ZoneMinder/Event.pm @@ -929,14 +929,14 @@ sub recover_timestamps { my $file = $path.'/'.$jpg; ( $file ) = $file =~ /^(.*)$/; my $timestamp = (stat($file))[9]; - my $Frame = new ZoneMinder::Frame(); - $Frame->save({ + my $Frame = new ZoneMinder::Frame(\@ZoneMinder::Frame::identified_by, { EventId=>$$Event{Id}, FrameId=>$id, TimeStamp=>Date::Format::time2str('%Y-%m-%d %H:%M:%S',$timestamp), Delta => $timestamp - $first_timestamp, Type=>'Normal', Score=>0, }); + $Frame->save(); } # end if Frame not found } # end foreach capture jpg $ZoneMinder::Database::dbh->commit(); diff --git a/scripts/ZoneMinder/lib/ZoneMinder/Frame.pm b/scripts/ZoneMinder/lib/ZoneMinder/Frame.pm index c2a4e5167..e24906182 100644 --- a/scripts/ZoneMinder/lib/ZoneMinder/Frame.pm +++ b/scripts/ZoneMinder/lib/ZoneMinder/Frame.pm @@ -33,12 +33,11 @@ require ZoneMinder::Object; use parent qw(ZoneMinder::Object); -use vars qw/ $table $primary_key %fields /; +use vars qw/ $table @identified_by %fields /; $table = 'Frames'; -$primary_key = 'Id'; +@identified_by = ('EventId', 'FrameId'); %fields = ( - Id => 'Id', EventId => 'EventId', FrameId => 'FrameId', Type => 'Type', diff --git a/tests/zm_db_schema.cpp b/tests/zm_db_schema.cpp index 535ab207c..4e9296aaf 100644 --- a/tests/zm_db_schema.cpp +++ b/tests/zm_db_schema.cpp @@ -97,3 +97,41 @@ TEST_CASE("Frames carries the audio level the event graph plots") { REQUIRE(block.find("'AudioLevel' => true") != std::string::npos); } } + +TEST_CASE("Frames is keyed by (EventId, FrameId)") { + const auto repo_root = std::filesystem::path(ZM_SOURCE_DIR); + const auto schema = ReadFile(repo_root / "db" / "zm_create.sql.in"); + const auto frames_at = schema.find("CREATE TABLE `Frames`"); + REQUIRE(frames_at != std::string::npos); + const auto frames = schema.substr(frames_at, schema.find("ENGINE", frames_at) - frames_at); + + SECTION("fresh schema has the composite primary key and no surrogate Id") { + REQUIRE(frames.find("PRIMARY KEY (`EventId`,`FrameId`)") != std::string::npos); + REQUIRE(frames.find("`Id` BIGINT") == std::string::npos); + REQUIRE(frames.find("AUTO_INCREMENT") == std::string::npos); + REQUIRE(frames.find("EventId_FrameId_idx") == std::string::npos); + } + + SECTION("TimeStamp is not rewritten by updates") { + REQUIRE(frames.find("`TimeStamp` timestamp NOT NULL default CURRENT_TIMESTAMP,") != std::string::npos); + REQUIRE(frames.find("on update") == std::string::npos); + } + + SECTION("nothing references Frames.Id") { + // A foreign key to Frames.Id makes the migration's DROP COLUMN fail. + const auto ai_models = ReadFile(repo_root / "db" / "AI_Models.sql"); + REQUIRE(ai_models.find("REFERENCES `Frames`") == std::string::npos); + REQUIRE(schema.find("REFERENCES `Frames`") == std::string::npos); + } + + SECTION("upgrade migration converts AI_Detections before dropping Id, and is re-runnable") { + const auto migration = ReadFile(repo_root / "db" / "zm_update-1.39.36.sql"); + const auto convert = migration.find("SET D.`FrameId` = F.`FrameId`"); + const auto drop = migration.find("DROP COLUMN `Id`"); + REQUIRE(convert != std::string::npos); + REQUIRE(drop != std::string::npos); + REQUIRE(convert < drop); + REQUIRE(migration.find("ADD PRIMARY KEY (`EventId`, `FrameId`)") != std::string::npos); + REQUIRE(migration.find("column_name = 'Id'") != std::string::npos); + } +} diff --git a/version.txt b/version.txt index a8dd0a21f..5c0799ee5 100644 --- a/version.txt +++ b/version.txt @@ -1 +1 @@ -1.39.35 +1.39.36 diff --git a/web/api/app/Controller/FramesController.php b/web/api/app/Controller/FramesController.php index a21714308..de4aaa6d8 100644 --- a/web/api/app/Controller/FramesController.php +++ b/web/api/app/Controller/FramesController.php @@ -27,17 +27,22 @@ class FramesController extends AppController { } } - # Frames are addressed by their own Id, so the parent Event's per-monitor ACL - # has to be resolved explicitly. Without this a user denied a monitor can - # reach that monitor's frames by guessing frame Ids. - private function eventForFrame($id) { + # A single frame is addressed by (EventId, FrameId), its primary key. + private function findFrame($eventId, $frameId) { $this->Frame->recursive = -1; $frame = $this->Frame->find('first', array( - 'conditions' => array('Frame.' . $this->Frame->primaryKey => $id) + 'conditions' => array('Frame.EventId' => $eventId, 'Frame.FrameId' => $frameId) )); if (!$frame) { throw new NotFoundException(__('Invalid frame')); } + return $frame; + } + + # Frames carry no MonitorId, so the parent Event's per-monitor ACL has to be + # resolved explicitly. + private function eventForFrame($eventId, $frameId) { + $frame = $this->findFrame($eventId, $frameId); $this->loadModel('Event'); $this->Event->recursive = -1; $event = $this->Event->find('first', array( @@ -49,18 +54,16 @@ class FramesController extends AppController { return new ZM\Event($event['Event']); } - # A frame being added or re-pointed names its event in the request data. Require - # edit on that event too, or a user could attach frames to a denied monitor's event. - private function requireRequestEventEdit($required) { - $data = $this->request->data; - if (isset($data['Frame']) and is_array($data['Frame'])) $data = $data['Frame']; - if (!isset($data['EventId'])) { - if ($required) throw new BadRequestException(__('EventId is required')); - return; + # A frame being added names its event in the request data. Require edit on + # that event too, or a user could attach frames to a denied monitor's event. + private function requireRequestEventEdit() { + $eventId = $this->requestField('Frame', 'EventId'); + if ($eventId === null) { + throw new BadRequestException(__('EventId is required')); } $this->loadModel('Event'); $this->Event->recursive = -1; - $event = $this->Event->find('first', array('conditions' => array('Event.Id' => $data['EventId']))); + $event = $this->Event->find('first', array('conditions' => array('Event.Id' => $eventId))); if (!$event) { throw new NotFoundException(__('Invalid event')); } @@ -72,16 +75,29 @@ class FramesController extends AppController { # Frame mutation is an Event mutation, so require Events=Edit as well as the # per-monitor ACL. beforeFilter() only guarantees Events != None. - private function requireFrameEdit($id) { + private function requireFrameEdit($eventId, $frameId) { global $user; if ($user and ($user->Events() != 'Edit')) { throw new UnauthorizedException(__('Insufficient Privileges')); } - if (!$this->eventForFrame($id)->canEdit()) { + if (!$this->eventForFrame($eventId, $frameId)->canEdit()) { throw new UnauthorizedException(__('Insufficient Privileges')); } } + # The request's Frame fields that are real columns. Model save() cannot be + # used to write them: it would match rows on the single-column primaryKey. + private function requestColumns($exclude = array()) { + $data = $this->request->data; + if (isset($data['Frame']) and is_array($data['Frame'])) $data = $data['Frame']; + $columns = array(); + foreach (array_keys($this->Frame->schema()) as $field) { + if (in_array($field, $exclude) or !array_key_exists($field, $data)) continue; + $columns[$field] = $data[$field]; + } + return $columns; + } + /** * index method * @return void @@ -125,21 +141,16 @@ class FramesController extends AppController { * view method * * @throws NotFoundException - * @param string $id + * @param string $eventId + * @param string $frameId * @return void */ - public function view($id = null) { - $this->Frame->recursive = -1; - if (!$this->Frame->exists($id)) { - throw new NotFoundException(__('Invalid frame')); - } - if (!$this->eventForFrame($id)->canView()) { + public function view($eventId = null, $frameId = null) { + if (!$this->eventForFrame($eventId, $frameId)->canView()) { throw new UnauthorizedException(__('Insufficient Privileges')); } - $options = array('conditions' => array('Frame.' . $this->Frame->primaryKey => $id)); - $frame = $this->Frame->find('first', $options); $this->set(array( - 'frame' => $frame, + 'frame' => $this->findFrame($eventId, $frameId), '_serialize' => array('frame') )); } @@ -155,10 +166,11 @@ class FramesController extends AppController { if ($user and ($user->Events() != 'Edit')) { throw new UnauthorizedException(__('Insufficient Privileges')); } - $this->requireRequestEventEdit(true); - $this->pinRequestId($this->Frame, null); - $this->Frame->create(); - if ($this->Frame->save($this->request->data)) { + $this->requireRequestEventEdit(); + $columns = $this->requestColumns(); + $this->Frame->set($columns); + if ($this->Frame->validates() and + $this->Frame->getDataSource()->create($this->Frame, array_keys($columns), array_values($columns))) { return $this->flash(__('The frame has been saved.'), array('action' => 'index')); } } @@ -169,24 +181,28 @@ class FramesController extends AppController { /** * edit method * + * EventId and FrameId are the key and cannot be changed. + * * @throws NotFoundException - * @param string $id + * @param string $eventId + * @param string $frameId * @return void */ - public function edit($id = null) { - if (!$this->Frame->exists($id)) { - throw new NotFoundException(__('Invalid frame')); - } - $this->requireFrameEdit($id); + public function edit($eventId = null, $frameId = null) { + $this->requireFrameEdit($eventId, $frameId); if ($this->request->is(array('post', 'put'))) { - $this->pinRequestId($this->Frame, $id); - $this->requireRequestEventEdit(false); - if ($this->Frame->save($this->request->data)) { + # updateAll() takes SQL expressions, so the values must be quoted here. + $db = $this->Frame->getDataSource(); + $columns = array(); + foreach ($this->requestColumns(array('EventId', 'FrameId')) as $field => $value) { + $columns[$field] = $db->value($value, $this->Frame->getColumnType($field)); + } + if (count($columns) and $this->Frame->updateAll($columns, + array('Frame.EventId' => $eventId, 'Frame.FrameId' => $frameId))) { return $this->flash(__('The frame has been saved.'), array('action' => 'index')); } } else { - $options = array('conditions' => array('Frame.' . $this->Frame->primaryKey => $id)); - $this->request->data = $this->Frame->find('first', $options); + $this->request->data = $this->findFrame($eventId, $frameId); } $events = $this->Frame->Event->find('list'); $this->set(compact('events')); @@ -196,17 +212,14 @@ class FramesController extends AppController { * delete method * * @throws NotFoundException - * @param string $id + * @param string $eventId + * @param string $frameId * @return void */ - public function delete($id = null) { - $this->Frame->id = $id; - if (!$this->Frame->exists()) { - throw new NotFoundException(__('Invalid frame')); - } + public function delete($eventId = null, $frameId = null) { $this->request->allowMethod('post', 'delete'); - $this->requireFrameEdit($id); - if ($this->Frame->delete()) { + $this->requireFrameEdit($eventId, $frameId); + if ($this->Frame->deleteAll(array('Frame.EventId' => $eventId, 'Frame.FrameId' => $frameId), false)) { return $this->flash(__('The frame has been deleted.'), array('action' => 'index')); } else { return $this->flash(__('The frame could not be deleted. Please, try again.'), array('action' => 'index')); diff --git a/web/api/app/Model/Frame.php b/web/api/app/Model/Frame.php index f1c46bbb1..35e1f5d47 100644 --- a/web/api/app/Model/Frame.php +++ b/web/api/app/Model/Frame.php @@ -17,9 +17,15 @@ class Frame extends AppModel { /** * Primary key field * + * The real key is (EventId, FrameId), which CakePHP 2 cannot express. EventId + * is its leftmost column, and is what Event's dependent hasMany cascade + * resolves Frames by, so deleting an event deletes exactly its frames. + * FramesController addresses single frames by both columns and never relies + * on save(), exists() or delete() by this key. + * * @var string */ - public $primaryKey = 'Id'; + public $primaryKey = 'EventId'; /** * Validation rules diff --git a/web/includes/Frame.php b/web/includes/Frame.php index 486f1ff35..b10b52976 100644 --- a/web/includes/Frame.php +++ b/web/includes/Frame.php @@ -7,7 +7,6 @@ require_once('Object.php'); class Frame extends ZM_Object { protected static $table = 'Frames'; protected $defaults = array( - 'Id' => null, 'EventId' => 0, 'FrameId' => 0, 'Type' => 'Normal', diff --git a/web/views/image.php b/web/views/image.php index e550fc287..2f469b50e 100644 --- a/web/views/image.php +++ b/web/views/image.php @@ -498,28 +498,10 @@ if ( empty($_REQUEST['path']) ) { } # if special frame (snapshot, alarm etc) or identified by id } else { -# If we are only specifying fid, then the fid must be the primary key into the frames table. But when the event is specified, then it is the frame # - $Frame = ZM\Frame::find_one(array('Id'=>$_REQUEST['fid'])); - if ( !$Frame ) { - header('HTTP/1.0 404 Not Found'); - ZM\Error('Frame ' . $_REQUEST['fid'] . ' Not Found'); - return; - } - - $Event = ZM\Event::find_one(array('Id'=>$Frame->EventId())); - if ( !$Event ) { - header('HTTP/1.0 404 Not Found'); - ZM\Error('Event ' . $Frame->EventId() . ' Not Found'); - return; - } - // Per-event ACL: see GHSA-vj5r-pc2v-gfwv. The frame id is user-supplied so the - // event/monitor it resolves to may be one the user is denied from viewing. - if (!$Event->canView()) { - header('HTTP/1.0 404 Not Found'); - ZM\Warning('Event '.$Frame->EventId().' access denied via frame '.$_REQUEST['fid']); - return; - } - $path = $Event->Path().'/'.sprintf('%0'.ZM_EVENT_IMAGE_DIGITS.'d',$Frame->FrameId()).'-'.$show.'.jpg'; + # A frame is only identified by its event and its number within the event. + header('HTTP/1.0 404 Not Found'); + ZM\Error('No Event ID specified for frame '.validInt($_REQUEST['fid'])); + return; } # end if have eid if ( !file_exists($path) ) {