From a6581ede0b7a50c6159eb5d36cf6be340a072609 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 21 Sep 2009 11:35:27 -0700 Subject: Fix Item_Model::get_position() so that our sort is stable when the comparison row has a null value in the sort field. Fix for #627 Note: this changes get_position() to take an Item_Model instead of an id! --- modules/gallery/controllers/albums.php | 15 +++++---- modules/gallery/controllers/photos.php | 2 +- modules/gallery/models/item.php | 61 +++++++++++++++++++++------------- 3 files changed, 47 insertions(+), 31 deletions(-) (limited to 'modules') diff --git a/modules/gallery/controllers/albums.php b/modules/gallery/controllers/albums.php index 183c26d0..3ea08538 100644 --- a/modules/gallery/controllers/albums.php +++ b/modules/gallery/controllers/albums.php @@ -39,12 +39,15 @@ class Albums_Controller extends Items_Controller { $show = $this->input->get("show"); if ($show) { - $index = $album->get_position($show); - $page = ceil($index / $page_size); - if ($page == 1) { - url::redirect($album->abs_url()); - } else { - url::redirect($album->abs_url("page=$page")); + $child = ORM::factory("item", $show); + $index = $album->get_position($child); + if ($index) { + $page = ceil($index / $page_size); + if ($page == 1) { + url::redirect($album->abs_url()); + } else { + url::redirect($album->abs_url("page=$page")); + } } } diff --git a/modules/gallery/controllers/photos.php b/modules/gallery/controllers/photos.php index 79ad674a..e6154535 100644 --- a/modules/gallery/controllers/photos.php +++ b/modules/gallery/controllers/photos.php @@ -25,7 +25,7 @@ class Photos_Controller extends Items_Controller { public function _show($photo) { access::required("view", $photo); - $position = $photo->parent()->get_position($photo->id); + $position = $photo->parent()->get_position($photo); if ($position > 1) { list ($previous_item, $ignore, $next_item) = $photo->parent()->children(3, $position - 2); diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index da1f6959..3cc9dd53 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -412,38 +412,51 @@ class Item_Model extends ORM_MPTT { * Find the position of the given child id in this album. The resulting value is 1-indexed, so * the first child in the album is at position 1. */ - public function get_position($child_id) { + public function get_position($child) { if ($this->sort_order == "DESC") { $comp = ">"; } else { $comp = "<"; } - $db = Database::instance(); - $position = $db->query(" - SELECT COUNT(*) AS position FROM {items} - WHERE `parent_id` = {$this->id} - AND `{$this->sort_column}` $comp (SELECT `{$this->sort_column}` - FROM {items} WHERE `id` = $child_id)") - ->current()->position; - - // We stopped short of our target value in the sort (notice that we're using a < comparator - // above) because it's possible that we have duplicate values in the sort column. An - // equality check would just arbitrarily pick one of those multiple possible equivalent - // columns, which would mean that if you choose a sort order that has duplicates, it'd pick - // any one of them as the child's "position". - // - // Fix this by doing a 2nd query where we iterate over the equivalent columns and add them to - // our base value. - $result = $db->query(" - SELECT id FROM {items} - WHERE `parent_id` = {$this->id} - AND `{$this->sort_column}` = (SELECT `{$this->sort_column}` - FROM {items} WHERE `id` = $child_id) - ORDER BY `id` ASC"); + + // We can't use isset() in this comparison because ORM::__get() confuses it. + if ($child->{$this->sort_column} !== null) { + $position = $db->query(" + SELECT COUNT(*) AS position FROM {items} + WHERE `parent_id` = {$this->id} + AND `{$this->sort_column}` $comp (SELECT `{$this->sort_column}` + FROM {items} WHERE `id` = $child->id)") + ->current()->position; + + // We stopped short of our target value in the sort (notice that we're using a < comparator + // above) because it's possible that we have duplicate values in the sort column. An + // equality check would just arbitrarily pick one of those multiple possible equivalent + // columns, which would mean that if you choose a sort order that has duplicates, it'd pick + // any one of them as the child's "position". + // + // Fix this by doing a 2nd query where we iterate over the equivalent columns and add them to + // our base value. + $result = $db->query(" + SELECT id FROM {items} + WHERE `parent_id` = {$this->id} + AND `{$this->sort_column}` = (SELECT `{$this->sort_column}` + FROM {items} WHERE `id` = $child->id) + ORDER BY `id` ASC"); + } else { + // If the sort value is null, then we can't take the approach of doing a comparison to get + // most of the way there. We have to iterate the entire data set. + $position = 0; + $result = $db->query(" + SELECT id FROM {items} + WHERE `parent_id` = {$this->id} + AND `{$this->sort_column}` IS NULL + ORDER BY `id` ASC"); + } + foreach ($result as $row) { $position++; - if ($row->id == $child_id) { + if ($row->id == $child->id) { break; } } -- cgit v1.2.3 From 9e6be40e31b06e5dffe7552928cb8b2d9ee7ad59 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 21 Sep 2009 20:47:55 -0700 Subject: Add viewable() protection to children() and children_count() calls. This is not currently necessary (nor is it a security hole) because we don't constrain permissions at the child level in the core, but it makes our security audits easier and will enable the scenario where somebody writes a module to add per-photo permissions. --- modules/gallery/controllers/photos.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'modules') diff --git a/modules/gallery/controllers/photos.php b/modules/gallery/controllers/photos.php index e6154535..3de9b3ee 100644 --- a/modules/gallery/controllers/photos.php +++ b/modules/gallery/controllers/photos.php @@ -31,7 +31,7 @@ class Photos_Controller extends Items_Controller { $photo->parent()->children(3, $position - 2); } else { $previous_item = null; - list ($next_item) = $photo->parent()->children(1, $position); + list ($next_item) = $photo->parent()->viewable()->children(1, $position); } $template = new Theme_View("page.html", "photo"); @@ -41,7 +41,7 @@ class Photos_Controller extends Items_Controller { $template->set_global("parents", $photo->parents()); $template->set_global("next_item", $next_item); $template->set_global("previous_item", $previous_item); - $template->set_global("sibling_count", $photo->parent()->children_count()); + $template->set_global("sibling_count", $photo->parent()->viewable()->children_count()); $template->set_global("position", $position); $template->content = new View("photo.html"); -- cgit v1.2.3 From 88350c5b88abb8bfc56b26177b64e049f6b7440f Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 21 Sep 2009 21:21:52 -0700 Subject: Update the next/previous item calculations to match what we do in photos.php Force the children_count to be zero, movies have no children. Rename $photo to $movie everywhere. --- modules/gallery/controllers/movies.php | 86 +++++++++++++++------------------- 1 file changed, 38 insertions(+), 48 deletions(-) (limited to 'modules') diff --git a/modules/gallery/controllers/movies.php b/modules/gallery/controllers/movies.php index 04e15315..fa07668e 100644 --- a/modules/gallery/controllers/movies.php +++ b/modules/gallery/controllers/movies.php @@ -22,42 +22,32 @@ class Movies_Controller extends Items_Controller { /** * @see REST_Controller::_show($resource) */ - public function _show($photo) { - access::required("view", $photo); + public function _show($movie) { + access::required("view", $movie); - // We sort by id ascending so for now, find sibling info by doing id based queries. - $next_item = ORM::factory("item") - ->viewable() - ->where("parent_id", $photo->parent_id) - ->where("id >", $photo->id) - ->orderby("id", "ASC") - ->find(); - $previous_item = ORM::factory("item") - ->viewable() - ->where("parent_id", $photo->parent_id) - ->where("id <", $photo->id) - ->orderby("id", "DESC") - ->find(); - $position = ORM::factory("item") - ->viewable() - ->where("parent_id", $photo->parent_id) - ->where("id <=", $photo->id) - ->count_all(); + $position = $movie->parent()->get_position($movie); + if ($position > 1) { + list ($previous_item, $ignore, $next_item) = + $movie->parent()->children(3, $position - 2); + } else { + $previous_item = null; + list ($next_item) = $movie->parent()->viewable()->children(1, $position); + } $template = new Theme_View("page.html", "movie"); - $template->set_global("item", $photo); + $template->set_global("item", $movie); $template->set_global("children", array()); - $template->set_global("children_count", $photo->children_count()); - $template->set_global("parents", $photo->parents()); - $template->set_global("next_item", $next_item->loaded ? $next_item : null); - $template->set_global("previous_item", $previous_item->loaded ? $previous_item : null); - $template->set_global("sibling_count", $photo->parent()->children_count()); + $template->set_global("children_count", 0); + $template->set_global("parents", $movie->parents()); + $template->set_global("next_item", $next_item); + $template->set_global("previous_item", $previous_item); + $template->set_global("sibling_count", $movie->parent()->viewable()->children_count()); $template->set_global("position", $position); $template->content = new View("movie.html"); - $photo->view_count++; - $photo->save(); + $movie->view_count++; + $movie->save(); print $template; } @@ -65,21 +55,21 @@ class Movies_Controller extends Items_Controller { /** * @see REST_Controller::_update($resource) */ - public function _update($photo) { + public function _update($movie) { access::verify_csrf(); - access::required("view", $photo); - access::required("edit", $photo); + access::required("view", $movie); + access::required("edit", $movie); - $form = photo::get_edit_form($photo); + $form = photo::get_edit_form($movie); if ($valid = $form->validate()) { - if ($form->edit_item->filename->value != $photo->name || - $form->edit_item->slug->value != $photo->slug) { + if ($form->edit_item->filename->value != $movie->name || + $form->edit_item->slug->value != $movie->slug) { // Make sure that there's not a name or slug conflict if ($row = Database::instance() ->select(array("name", "slug")) ->from("items") - ->where("parent_id", $photo->parent_id) - ->where("id <>", $photo->id) + ->where("parent_id", $movie->parent_id) + ->where("id <>", $movie->id) ->open_paren() ->where("name", $form->edit_item->filename->value) ->orwhere("slug", $form->edit_item->slug->value) @@ -98,16 +88,16 @@ class Movies_Controller extends Items_Controller { } if ($valid) { - $photo->title = $form->edit_item->title->value; - $photo->description = $form->edit_item->description->value; - $photo->slug = $form->edit_item->slug->value; - $photo->rename($form->edit_item->filename->value); - $photo->save(); - module::event("item_edit_form_completed", $photo, $form); + $movie->title = $form->edit_item->title->value; + $movie->description = $form->edit_item->description->value; + $movie->slug = $form->edit_item->slug->value; + $movie->rename($form->edit_item->filename->value); + $movie->save(); + module::event("item_edit_form_completed", $movie, $form); - log::success("content", "Updated movie", "url()}\">view"); + log::success("content", "Updated movie", "url()}\">view"); message::success( - t("Saved movie %movie_title", array("movie_title" => $photo->title))); + t("Saved movie %movie_title", array("movie_title" => $movie->title))); print json_encode( array("result" => "success")); @@ -121,9 +111,9 @@ class Movies_Controller extends Items_Controller { /** * @see REST_Controller::_form_edit($resource) */ - public function _form_edit($photo) { - access::required("view", $photo); - access::required("edit", $photo); - print photo::get_edit_form($photo); + public function _form_edit($movie) { + access::required("view", $movie); + access::required("edit", $movie); + print photo::get_edit_form($movie); } } -- cgit v1.2.3 From 123afc954281c1f924f851a33ae5016774e6d9f3 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 21 Sep 2009 21:22:07 -0700 Subject: Set children_count to 0, photos have no children. --- modules/gallery/controllers/photos.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules') diff --git a/modules/gallery/controllers/photos.php b/modules/gallery/controllers/photos.php index 3de9b3ee..81e7519e 100644 --- a/modules/gallery/controllers/photos.php +++ b/modules/gallery/controllers/photos.php @@ -37,7 +37,7 @@ class Photos_Controller extends Items_Controller { $template = new Theme_View("page.html", "photo"); $template->set_global("item", $photo); $template->set_global("children", array()); - $template->set_global("children_count", $photo->children_count()); + $template->set_global("children_count", 0); $template->set_global("parents", $photo->parents()); $template->set_global("next_item", $next_item); $template->set_global("previous_item", $previous_item); -- cgit v1.2.3 From 529ded3388673036314eefd5bfb1cfc0b76f7f9e Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 21 Sep 2009 21:28:00 -0700 Subject: 2nd attempt at gracefully dealing with sort columns that contain nulls. This time around, do a query to determine whether or not the sort column has nulls in it. If it doesn't, then use our comparators as before. There are NULLs in the sort column, so we can't use MySQL comparators. Fall back to iterating over every child row to get to the current one. This can be wildly inefficient for really large albums, but it should be a rare case that the user is sorting an album with null values in the sort column. Fixes #627 --- modules/gallery/models/item.php | 60 ++++++++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 15 deletions(-) (limited to 'modules') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 3cc9dd53..d1c6feb9 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -420,8 +420,15 @@ class Item_Model extends ORM_MPTT { } $db = Database::instance(); - // We can't use isset() in this comparison because ORM::__get() confuses it. - if ($child->{$this->sort_column} !== null) { + // If the comparison column has NULLs in it, we can't use comparators on it and will have to + // deal with it the hard way. + $count = $db->from("items") + ->where("parent_id", $this->id) + ->where($this->sort_column, NULL) + ->count_records(); + + if (empty($count)) { + // There are no NULLs in the sort column, so we can just use it directly. $position = $db->query(" SELECT COUNT(*) AS position FROM {items} WHERE `parent_id` = {$this->id} @@ -443,21 +450,36 @@ class Item_Model extends ORM_MPTT { AND `{$this->sort_column}` = (SELECT `{$this->sort_column}` FROM {items} WHERE `id` = $child->id) ORDER BY `id` ASC"); + foreach ($result as $row) { + $position++; + if ($row->id == $child->id) { + break; + } + } } else { - // If the sort value is null, then we can't take the approach of doing a comparison to get - // most of the way there. We have to iterate the entire data set. - $position = 0; - $result = $db->query(" - SELECT id FROM {items} - WHERE `parent_id` = {$this->id} - AND `{$this->sort_column}` IS NULL - ORDER BY `id` ASC"); - } + // There are NULLs in the sort column, so we can't use MySQL comparators. Fall back to + // iterating over every child row to get to the current one. This can be wildly inefficient + // for really large albums, but it should be a rare case that the user is sorting an album + // with null values in the sort column. + // + // Reproduce the children() functionality here using Database directly to avoid loading the + // whole ORM for each row. + $orderby = array($this->sort_column => $this->sort_order); + // Use id as a tie breaker + if ($this->sort_column != "id") { + $orderby["id"] = "ASC"; + } - foreach ($result as $row) { - $position++; - if ($row->id == $child->id) { - break; + $position = 0; + foreach ($db->select("id") + ->from("items") + ->where("parent_id", $this->id) + ->orderby($orderby) + ->get() as $row) { + $position++; + if ($row->id == $child->id) { + break; + } } } @@ -564,6 +586,10 @@ class Item_Model extends ORM_MPTT { function children($limit=null, $offset=0, $where=array(), $orderby=null) { if (empty($orderby)) { $orderby = array($this->sort_column => $this->sort_order); + // Use id as a tie breaker + if ($this->sort_column != "id") { + $orderby["id"] = "ASC"; + } } return parent::children($limit, $offset, $where, $orderby); } @@ -582,6 +608,10 @@ class Item_Model extends ORM_MPTT { function descendants($limit=null, $offset=0, $where=array(), $orderby=null) { if (empty($orderby)) { $orderby = array($this->sort_column => $this->sort_order); + // Use id as a tie breaker + if ($this->sort_column != "id") { + $orderby["id"] = "ASC"; + } } return parent::descendants($limit, $offset, $where, $orderby); } -- cgit v1.2.3