Orphaned entry updates break things

[mattoperry]

The following is verified in 1.6:

When an entry is deleted, a replaces entry is generated with reference to the deleted object, and the original comment itself is deleted:

https://github.com/Automattic/liveblog/blob/691d6c0667dc7bf419cfda49c8d9c931ea5d2c49/classes/class-wpcom-liveblog-entry.php#L276

However, any modifying entries with type update are not deleted, and become orphaned. This happens if an entry is first updated and then deleted.

Later, during lazy-loading, these updates are retrieved as if they were valid entries, and Liveblog attempts (and fails) to render them. This causes further breakage in lazyloading, and if "load more" is clicked again, the entire liveblog will break.

it would seem that any updates that refer to deleted entries should be deleted when the entry is deleted.

[no-sws]

I'm having trouble recreating this issue on master (v1.8) after manually testing the following scenarios:

• As an editor, I created, updated, and then deleted a single entry. No JavaScript errors were thrown and I was able to continue creating, updating, and deleting other entries. As an anonymous user (in another browser) I did not receive any errors; entries were added, updated, and removed as expected (based on my actions as editor).
• As an editor, I first created several (around ten) entries, and then performed an update and delete sequence on random entries. No errors were thrown for the editor or anonymous user (in a separate browser).

It looks like there are two main API endpoints for reading entries: the initial page load and pagination endpoint that grabs a list of entries (e.g., /wp-json/liveblog/v1/13/get-entries/1/30-1528590944) and another endpoint that returns updates and deletions (e.g., /wp-json/liveblog/v1/13/entries/1528594428/1528597527/).

For the former (i.e., list of entries), it looks like we avoid grabbing orphaned or deleted entries via the WPCOM_Liveblog_Entry_Query instance call to get_all_entries_asc in the WPCOM_Liveblog::get_entries_paged static method. As entries are grabbed in ascending order, entries should be returned in order from creation, to update, and then to deletion (e.g., for an entry Entry that has been updated once and then deleted, get_all_entries_asc should return in order [ Entry, Entry:updated, Entry:deleted ]). Finally the call to WPCOM_Liveblog::flatten_entries should only keep the the most recently updated entry or, in the case of deletion, remove the entry from the returned list.

For the latter endpoint (i.e., updates and deletions), it looks like the WPCOM_Liveblog_Entry_Query instance call to find_between_timestamps along with the client increment of the start timestamp should prevent orphaned or deleted entries from being returned. On initial page load, the client is localized with a start timestamp of the return value of WPCOM_Liveblog_Entry_Query#get_latest_timestamp which grabs the most recent entry and returns the timestamp value of its comment_date_gmt field. As the client increments this value, I think any orphaned updates (i.e., deleted but not "trash" for comment_approved field) or deleted entries should not be returned (as their timestamps will be less than the start timestamp).

Let me know if I'm missing a workflow to recreate this or if you think this was patched after v1.6. Thanks!

[mattoperry]

Hi @no-sws thanks for this ... the odd part here is that I never really had to reproduce this set of circumstances since it was reported by a client in the wild ... so I can't really help with how to recreate the data conditions I was seeing.

I think I follow what you're saying above, and looking at the code it makes sense. A couple of clarifying questions:

• can you find where this changed? In 1.7? 1.8? This is definitely broken in 1.6, so I'm wondering when this mechanism was introduced.
• does the lazy-load behavior ever load fewer than 5 (or the filtered number) of entries? For instance, if it selects 5 things, and one of them turns out to be deleted when flattened, does it return 4 instead? If that's the case, I think we may still have a problem here.

[no-sws]

Hi @mattoperry, reading through the CHANGELOG, it looks like the major changes to patch these issues were incorporated in 1.7. The Milestone/1.7 - React Front-End PR seems to confirm this.

For removing orphaned updates and deletions, it looks like WPCOM_Liveblog::flatten_entries was introduced and used in this PR. The WPCOM_Liveblog_Entry_Query#get_all_entries_asc was also made public so it could be used for grabbing entries prior to being "flattened".

For grabbing updates (e.g., during long polling), it looks like WPCOM_Liveblog_Entry_Query#find_between_timestamps was introduced (although only slightly different than the existing get_between_timestamps method). More importantly (from my perspective), the API call on the client was introduced which should omit older orphaned or deleted entries from being delivered to the client (via adding 1 to the newestEntryTimestamp argument, as this will exclude the last known entry timestamp; i.e., newestEntryTimestamp is not in [newestEntryTimestamp + 1, getCurrentTimestamp()]).

Re: your last question:

does the lazy-load behavior ever load fewer than 5...

I don't believe this is an issue:

• The $entry_query->get_all_entries_asc() call retrieves all liveblog comments for the post (as the eventual call to get_comments does not have a limit by default (see default number argument description here)
• The self::flatten_entries( $entries ) call only removes old created/updated entries (i.e., entries who have a more recent update) and deleted entries
• After this point array_slice is used on the complete list of active entries

As I'm seeing it, array_slice would only return less entries than the $per_page (e.g., 5) count if the page didn't have enough to be full (e.g., page 1 with $per_page = 5 may only return 3 entries if there are only 3 total entries; page 2 with $per_page = 5 may only return 3 entries if there are only 8 total entries). This seems like the correct/expected behavior to me.

Thanks!