From 650074c648d98f81674c6e2b0ebf052c473ada6e Mon Sep 17 00:00:00 2001 From: Evan Prodromou Date: Wed, 31 Mar 2010 15:54:35 -0400 Subject: [PATCH 01/11] don't insert the same notice twice into an inbox --- classes/Inbox.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/classes/Inbox.php b/classes/Inbox.php index 014ba3d829..0d807946d5 100644 --- a/classes/Inbox.php +++ b/classes/Inbox.php @@ -114,6 +114,16 @@ class Inbox extends Memcached_DataObject return false; } + $ids = unpack('N*', $inbox->notice_ids); + + // bulk inserts sometimes fail and get restarted. + // Skip if this one has been inserted before. + + if (in_array($notice_id, $ids)) { + // effectively successful + return true; + } + $result = $inbox->query(sprintf('UPDATE inbox '. 'set notice_ids = concat(cast(0x%08x as binary(4)), '. 'substr(notice_ids, 1, %d)) '. From 4b80ce0be89fe50eabec1a19dbf4a0c26a413423 Mon Sep 17 00:00:00 2001 From: Evan Prodromou Date: Thu, 1 Apr 2010 12:09:33 -0400 Subject: [PATCH 02/11] if user allows location sharing but turned off browser location use profile location --- actions/newnotice.php | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/actions/newnotice.php b/actions/newnotice.php index ed0fa1b2b5..748d104ff9 100644 --- a/actions/newnotice.php +++ b/actions/newnotice.php @@ -184,13 +184,21 @@ class NewnoticeAction extends Action $options = array('reply_to' => ($replyto == 'false') ? null : $replyto); - if ($user->shareLocation() && $this->arg('notice_data-geo')) { - - $locOptions = Notice::locationOptions($this->trimmed('lat'), - $this->trimmed('lon'), - $this->trimmed('location_id'), - $this->trimmed('location_ns'), - $user->getProfile()); + if ($user->shareLocation()) { + // use browser data if checked; otherwise profile data + if ($this->arg('notice_data-geo')) { + $locOptions = Notice::locationOptions($this->trimmed('lat'), + $this->trimmed('lon'), + $this->trimmed('location_id'), + $this->trimmed('location_ns'), + $user->getProfile()); + } else { + $locOptions = Notice::locationOptions(null, + null, + null, + null, + $user->getProfile()); + } $options = array_merge($options, $locOptions); } @@ -201,8 +209,6 @@ class NewnoticeAction extends Action $upload->attachToNotice($notice); } - - if ($this->boolean('ajax')) { header('Content-Type: text/xml;charset=utf-8'); $this->xw->startDocument('1.0', 'UTF-8'); From 8b24ad8a9c681585e95612084eb629df8b364b74 Mon Sep 17 00:00:00 2001 From: Evan Prodromou Date: Thu, 1 Apr 2010 12:52:12 -0400 Subject: [PATCH 03/11] Revert "if user allows location sharing but turned off browser location use profile location" This reverts commit 4b80ce0be89fe50eabec1a19dbf4a0c26a413423. --- actions/newnotice.php | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/actions/newnotice.php b/actions/newnotice.php index 748d104ff9..ed0fa1b2b5 100644 --- a/actions/newnotice.php +++ b/actions/newnotice.php @@ -184,21 +184,13 @@ class NewnoticeAction extends Action $options = array('reply_to' => ($replyto == 'false') ? null : $replyto); - if ($user->shareLocation()) { - // use browser data if checked; otherwise profile data - if ($this->arg('notice_data-geo')) { - $locOptions = Notice::locationOptions($this->trimmed('lat'), - $this->trimmed('lon'), - $this->trimmed('location_id'), - $this->trimmed('location_ns'), - $user->getProfile()); - } else { - $locOptions = Notice::locationOptions(null, - null, - null, - null, - $user->getProfile()); - } + if ($user->shareLocation() && $this->arg('notice_data-geo')) { + + $locOptions = Notice::locationOptions($this->trimmed('lat'), + $this->trimmed('lon'), + $this->trimmed('location_id'), + $this->trimmed('location_ns'), + $user->getProfile()); $options = array_merge($options, $locOptions); } @@ -209,6 +201,8 @@ class NewnoticeAction extends Action $upload->attachToNotice($notice); } + + if ($this->boolean('ajax')) { header('Content-Type: text/xml;charset=utf-8'); $this->xw->startDocument('1.0', 'UTF-8'); From a09b27ff41df41a86fdb0abae14239907d5ee6ec Mon Sep 17 00:00:00 2001 From: Evan Prodromou Date: Thu, 1 Apr 2010 12:52:26 -0400 Subject: [PATCH 04/11] Revert "don't insert the same notice twice into an inbox" This reverts commit 650074c648d98f81674c6e2b0ebf052c473ada6e. --- classes/Inbox.php | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/classes/Inbox.php b/classes/Inbox.php index 0d807946d5..014ba3d829 100644 --- a/classes/Inbox.php +++ b/classes/Inbox.php @@ -114,16 +114,6 @@ class Inbox extends Memcached_DataObject return false; } - $ids = unpack('N*', $inbox->notice_ids); - - // bulk inserts sometimes fail and get restarted. - // Skip if this one has been inserted before. - - if (in_array($notice_id, $ids)) { - // effectively successful - return true; - } - $result = $inbox->query(sprintf('UPDATE inbox '. 'set notice_ids = concat(cast(0x%08x as binary(4)), '. 'substr(notice_ids, 1, %d)) '. From 9efe5393ff3f6a23fae1e32e521c6afe69eef6f6 Mon Sep 17 00:00:00 2001 From: Evan Prodromou Date: Thu, 1 Apr 2010 12:57:52 -0400 Subject: [PATCH 05/11] Revert "Revert "don't insert the same notice twice into an inbox"" This reverts commit a09b27ff41df41a86fdb0abae14239907d5ee6ec. --- classes/Inbox.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/classes/Inbox.php b/classes/Inbox.php index 014ba3d829..0d807946d5 100644 --- a/classes/Inbox.php +++ b/classes/Inbox.php @@ -114,6 +114,16 @@ class Inbox extends Memcached_DataObject return false; } + $ids = unpack('N*', $inbox->notice_ids); + + // bulk inserts sometimes fail and get restarted. + // Skip if this one has been inserted before. + + if (in_array($notice_id, $ids)) { + // effectively successful + return true; + } + $result = $inbox->query(sprintf('UPDATE inbox '. 'set notice_ids = concat(cast(0x%08x as binary(4)), '. 'substr(notice_ids, 1, %d)) '. From d60c1f1a9f343f10d33800862c67839594607c8f Mon Sep 17 00:00:00 2001 From: Evan Prodromou Date: Thu, 1 Apr 2010 12:58:06 -0400 Subject: [PATCH 06/11] Revert "Revert "if user allows location sharing but turned off browser location use profile location"" This reverts commit 8b24ad8a9c681585e95612084eb629df8b364b74. --- actions/newnotice.php | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/actions/newnotice.php b/actions/newnotice.php index ed0fa1b2b5..748d104ff9 100644 --- a/actions/newnotice.php +++ b/actions/newnotice.php @@ -184,13 +184,21 @@ class NewnoticeAction extends Action $options = array('reply_to' => ($replyto == 'false') ? null : $replyto); - if ($user->shareLocation() && $this->arg('notice_data-geo')) { - - $locOptions = Notice::locationOptions($this->trimmed('lat'), - $this->trimmed('lon'), - $this->trimmed('location_id'), - $this->trimmed('location_ns'), - $user->getProfile()); + if ($user->shareLocation()) { + // use browser data if checked; otherwise profile data + if ($this->arg('notice_data-geo')) { + $locOptions = Notice::locationOptions($this->trimmed('lat'), + $this->trimmed('lon'), + $this->trimmed('location_id'), + $this->trimmed('location_ns'), + $user->getProfile()); + } else { + $locOptions = Notice::locationOptions(null, + null, + null, + null, + $user->getProfile()); + } $options = array_merge($options, $locOptions); } @@ -201,8 +209,6 @@ class NewnoticeAction extends Action $upload->attachToNotice($notice); } - - if ($this->boolean('ajax')) { header('Content-Type: text/xml;charset=utf-8'); $this->xw->startDocument('1.0', 'UTF-8'); From f1c01f9ead4a5f4da7c6964f0998831fa31f8a16 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Thu, 1 Apr 2010 10:15:40 -0700 Subject: [PATCH 07/11] Temporary hack until notice_profile_id_idx is updated to (profile_id, id) instead of (profile_id, created, id). It's been falling back to PRIMARY instead, which is really very inefficient for a profile that hasn't posted in a few months. Even though forcing the index will cause a filesort, it's usually going to be better. Even for large profiles it seems much faster than the badly-indexed query. --- classes/Profile.php | 63 +++++++++++++++++++++++++++++++++------------ 1 file changed, 47 insertions(+), 16 deletions(-) diff --git a/classes/Profile.php b/classes/Profile.php index 5de35c191a..54f557ea7c 100644 --- a/classes/Profile.php +++ b/classes/Profile.php @@ -225,31 +225,62 @@ class Profile extends Memcached_DataObject { $notice = new Notice(); - $notice->profile_id = $this->id; + // Temporary hack until notice_profile_id_idx is updated + // to (profile_id, id) instead of (profile_id, created, id). + // It's been falling back to PRIMARY instead, which is really + // very inefficient for a profile that hasn't posted in a few + // months. Even though forcing the index will cause a filesort, + // it's usually going to be better. + if (common_config('db', 'type') == 'mysql') { + $index = ''; + $query = + "select id from notice force index (notice_profile_id_idx) ". + "where profile_id=" . $notice->escape($this->id); - $notice->selectAdd(); - $notice->selectAdd('id'); + if ($since_id != 0) { + $query .= " and id > $since_id"; + } - if ($since_id != 0) { - $notice->whereAdd('id > ' . $since_id); - } + if ($max_id != 0) { + $query .= " and id < $max_id"; + } - if ($max_id != 0) { - $notice->whereAdd('id <= ' . $max_id); - } + $query .= ' order by id DESC'; - $notice->orderBy('id DESC'); + if (!is_null($offset)) { + $query .= " LIMIT $limit OFFSET $offset"; + } - if (!is_null($offset)) { - $notice->limit($offset, $limit); + $notice->query($query); + } else { + $index = ''; + + $notice->profile_id = $this->id; + + $notice->selectAdd(); + $notice->selectAdd('id'); + + if ($since_id != 0) { + $notice->whereAdd('id > ' . $since_id); + } + + if ($max_id != 0) { + $notice->whereAdd('id <= ' . $max_id); + } + + $notice->orderBy('id DESC'); + + if (!is_null($offset)) { + $notice->limit($offset, $limit); + } + + $notice->find(); } $ids = array(); - if ($notice->find()) { - while ($notice->fetch()) { - $ids[] = $notice->id; - } + while ($notice->fetch()) { + $ids[] = $notice->id; } return $ids; From b10ff031d9ba8c5e3b5267a469e8332011149fee Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Fri, 2 Apr 2010 09:32:03 -0700 Subject: [PATCH 08/11] Ticket 2271: extra whitespace in underlined link for username in notice lists Switching to a raw() output for the of the nickname removes the extra whitespace and fixes display. --- lib/noticelist.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/noticelist.php b/lib/noticelist.php index 0d4cd4dd91..83c8de9f6e 100644 --- a/lib/noticelist.php +++ b/lib/noticelist.php @@ -340,8 +340,9 @@ class NoticeListItem extends Widget function showNickname() { - $this->out->element('span', array('class' => 'nickname fn'), - $this->profile->nickname); + $this->out->raw('' . + htmlspecialchars($this->profile->nickname) . + ''); } /** From 6cd0637e552ed6ce35b4447aa280fb13cf7f65cb Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Fri, 2 Apr 2010 09:32:03 -0700 Subject: [PATCH 09/11] Ticket 2271: extra whitespace in underlined link for username in notice lists Switching to a raw() output for the of the nickname removes the extra whitespace and fixes display. --- lib/noticelist.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/noticelist.php b/lib/noticelist.php index 0d4cd4dd91..83c8de9f6e 100644 --- a/lib/noticelist.php +++ b/lib/noticelist.php @@ -340,8 +340,9 @@ class NoticeListItem extends Widget function showNickname() { - $this->out->element('span', array('class' => 'nickname fn'), - $this->profile->nickname); + $this->out->raw('' . + htmlspecialchars($this->profile->nickname) . + ''); } /** From 61394aa8ac3585c9b7f1e15b6c581af01854987d Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Fri, 2 Apr 2010 15:39:43 -0700 Subject: [PATCH 10/11] Don't save duplicate messages into a user's packed inbox. We've already got the packed box loaded at insert time, so we can simply unpack it and check before doing the update query. Should help with dupes that come in when inbox distrib jobs die and get restarted, etc. --- classes/Inbox.php | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/classes/Inbox.php b/classes/Inbox.php index 014ba3d829..2533210b73 100644 --- a/classes/Inbox.php +++ b/classes/Inbox.php @@ -96,12 +96,23 @@ class Inbox extends Memcached_DataObject $inbox = new Inbox(); $inbox->user_id = $user_id; - $inbox->notice_ids = call_user_func_array('pack', array_merge(array('N*'), $ids)); + $inbox->pack($ids); $inbox->fake = true; return $inbox; } + /** + * Append the given notice to the given user's inbox. + * Caching updates are managed for the inbox itself. + * + * If the notice is already in this inbox, the second + * add will be silently dropped. + * + * @param int @user_id + * @param int $notice_id + * @return boolean success + */ static function insertNotice($user_id, $notice_id) { $inbox = DB_DataObject::staticGet('inbox', 'user_id', $user_id); @@ -114,6 +125,13 @@ class Inbox extends Memcached_DataObject return false; } + $ids = $inbox->unpack(); + if (in_array(intval($notice_id), $ids)) { + // Already in there, we probably re-ran some inbox adds + // due to an error. Skip the dupe silently. + return true; + } + $result = $inbox->query(sprintf('UPDATE inbox '. 'set notice_ids = concat(cast(0x%08x as binary(4)), '. 'substr(notice_ids, 1, %d)) '. @@ -150,7 +168,7 @@ class Inbox extends Memcached_DataObject } } - $ids = unpack('N*', $inbox->notice_ids); + $ids = $inbox->unpack(); if (!empty($since_id)) { $newids = array(); @@ -229,4 +247,21 @@ class Inbox extends Memcached_DataObject } return new ArrayWrapper($items); } + + /** + * Saves a list of integer notice_ids into a packed blob in this object. + * @param array $ids list of integer notice_ids + */ + protected function pack(array $ids) + { + $this->notice_ids = call_user_func_array('pack', array_merge(array('N*'), $ids)); + } + + /** + * @return array of integer notice_ids + */ + protected function unpack() + { + return unpack('N*', $this->notice_ids); + } } From ec24f283dd6f1371125c042529f571645a5f13fa Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Fri, 2 Apr 2010 15:45:03 -0700 Subject: [PATCH 11/11] Don't save duplicate messages into a user's packed inbox. We've already got the packed box loaded at insert time, so we can simply unpack it and check before doing the update query. Should help with dupes that come in when inbox distrib jobs die and get restarted, etc. Conflicts: classes/Inbox.php Looks like this was implemented on master recently and not copied up to testing. Merging to my version on testing as I've added some doc comments and extracted a couple functions for future ease of use. --- classes/Inbox.php | 43 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/classes/Inbox.php b/classes/Inbox.php index 0d807946d5..2533210b73 100644 --- a/classes/Inbox.php +++ b/classes/Inbox.php @@ -96,12 +96,23 @@ class Inbox extends Memcached_DataObject $inbox = new Inbox(); $inbox->user_id = $user_id; - $inbox->notice_ids = call_user_func_array('pack', array_merge(array('N*'), $ids)); + $inbox->pack($ids); $inbox->fake = true; return $inbox; } + /** + * Append the given notice to the given user's inbox. + * Caching updates are managed for the inbox itself. + * + * If the notice is already in this inbox, the second + * add will be silently dropped. + * + * @param int @user_id + * @param int $notice_id + * @return boolean success + */ static function insertNotice($user_id, $notice_id) { $inbox = DB_DataObject::staticGet('inbox', 'user_id', $user_id); @@ -114,13 +125,10 @@ class Inbox extends Memcached_DataObject return false; } - $ids = unpack('N*', $inbox->notice_ids); - - // bulk inserts sometimes fail and get restarted. - // Skip if this one has been inserted before. - - if (in_array($notice_id, $ids)) { - // effectively successful + $ids = $inbox->unpack(); + if (in_array(intval($notice_id), $ids)) { + // Already in there, we probably re-ran some inbox adds + // due to an error. Skip the dupe silently. return true; } @@ -160,7 +168,7 @@ class Inbox extends Memcached_DataObject } } - $ids = unpack('N*', $inbox->notice_ids); + $ids = $inbox->unpack(); if (!empty($since_id)) { $newids = array(); @@ -239,4 +247,21 @@ class Inbox extends Memcached_DataObject } return new ArrayWrapper($items); } + + /** + * Saves a list of integer notice_ids into a packed blob in this object. + * @param array $ids list of integer notice_ids + */ + protected function pack(array $ids) + { + $this->notice_ids = call_user_func_array('pack', array_merge(array('N*'), $ids)); + } + + /** + * @return array of integer notice_ids + */ + protected function unpack() + { + return unpack('N*', $this->notice_ids); + } }