Skip to content

Commit 79d0e31

Browse files
fgiannarfgiannar
authored
Sync: Stop sending redundant actions when Dedicated Sync fails to enable (#46700)
* Sync: Stop sending redundant actions when Dedicated Sync fails to enable
1 parent 191517e commit 79d0e31

File tree

8 files changed

+168
-68
lines changed

8 files changed

+168
-68
lines changed

‎projects/packages/sync/changelog/update-stop-sending-redundant-dedicated_sync_enabled-actions‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Significance: patch
2+
Type: changed
3+
4+
Sync: Stop sending redundant actions when Dedicated Sync fails to enable.

‎projects/packages/sync/src/class-queue.php‎

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ public function add_all( $items ) {
121121
}
122122

123123
/**
124-
* Get the front-most item on the queue without checking it out.
124+
* Get the front-most items on the queue without checking them out.
125125
*
126126
* @param int $count Number of items to return when looking at the items.
127127
*
@@ -136,6 +136,22 @@ public function peek( $count = 1 ) {
136136
return array();
137137
}
138138

139+
/**
140+
* Get the last-added items on the queue without checking them out.
141+
*
142+
* @param int $count Number of items to return when looking at the items.
143+
*
144+
* @return array
145+
*/
146+
public function peek_newest( $count = 1 ) {
147+
$items = $this->fetch_items( $count, 'DESC' );
148+
if ( $items ) {
149+
return Utils::get_item_values( $items );
150+
}
151+
152+
return array();
153+
}
154+
139155
/**
140156
* Gets items with particular IDs.
141157
*
@@ -245,15 +261,40 @@ function ( $item ) {
245261
}
246262

247263
/**
248-
* Pop elements from the queue.
264+
* Remove the oldest items from the queue.
249265
*
250-
* @param int $limit Number of items to pop from the queue.
266+
* @param int $limit Number of items to remove from the queue.
251267
*
252268
* @return array|object|null
253269
*/
254270
public function pop( $limit ) {
255271
$items = $this->fetch_items( $limit );
256272

273+
if ( ! $items ) {
274+
return array();
275+
}
276+
277+
$ids = $this->get_ids( $items );
278+
279+
$this->delete( $ids );
280+
281+
return $items;
282+
}
283+
284+
/**
285+
* Remove the newest items from the queue.
286+
*
287+
* @param int $limit Number of items to remove from the queue.
288+
*
289+
* @return array|object|null
290+
*/
291+
public function pop_newest( $limit ) {
292+
$items = $this->fetch_items( $limit, 'DESC' );
293+
294+
if ( ! $items ) {
295+
return array();
296+
}
297+
257298
$ids = $this->get_ids( $items );
258299

259300
$this->delete( $ids );
@@ -609,11 +650,15 @@ private function get_next_data_row_option_name() {
609650
* Return the items in the queue.
610651
*
611652
* @param null|int $limit Limit to the number of items we fetch at once.
653+
* @param string $order Sort direction for the items. Accepts 'ASC' or 'DESC'.
654+
* Any other value will be treated as 'ASC'.
612655
*
613656
* @return array|object|null
614657
*/
615-
private function fetch_items( $limit = null ) {
616-
$items = $this->queue_storage->fetch_items( $limit );
658+
private function fetch_items( $limit = null, $order = 'ASC' ) {
659+
$order = 'DESC' === $order ? 'DESC' : 'ASC';
660+
661+
$items = $this->queue_storage->fetch_items( $limit, $order );
617662

618663
return $this->unserialize_values( $items );
619664
}

‎projects/packages/sync/src/class-settings.php‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,21 @@ public static function update_settings( $new_settings ) {
304304
if ( 'dedicated_sync_enabled' === $setting && $updated && (bool) $value ) {
305305
if ( ! Dedicated_Sender::can_spawn_dedicated_sync_request() ) {
306306
update_option( self::SETTINGS_OPTION_PREFIX . $setting, 0, true );
307+
$listener = Listener::get_instance();
308+
// Remove the last two actions from the queue since we failed to enable Dedicated Sync.
309+
// Those would be `updated_option` with `jetpack_sync_settings_dedicated_sync_enabled` set to 1 and then 0 again.
310+
$queue = $listener->get_sync_queue();
311+
$items = $queue->peek_newest( 2 );
312+
$key = 'jetpack_sync_settings_dedicated_sync_enabled';
313+
314+
if (
315+
isset( $items[0][1][0] )
316+
&& isset( $items[1][1][0] )
317+
&& $items[0][1][0] === $key
318+
&& $items[1][1][0] === $key
319+
) {
320+
$queue->pop_newest( 2 );
321+
}
307322
}
308323
}
309324
}

‎projects/packages/sync/src/sync-queue/class-queue-storage-options.php‎

Lines changed: 24 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -67,35 +67,37 @@ public function insert_item( $item_id, $item ) {
6767
* Fetch items from the queue.
6868
*
6969
* @param int|null $item_count How many items to fetch from the queue.
70-
* The parameter is null-able, if no limit on the amount of items.
70+
* Null for no limit.
71+
* @param string $order Sort direction for the items. Accepts 'ASC' or 'DESC'.
72+
* Any other value will be treated as 'ASC'.
7173
*
72-
* @return array|object|\stdClass[]|null
74+
* @return array|object|null Array of result objects on success, or null on failure.
7375
*/
74-
public function fetch_items( $item_count ) {
76+
public function fetch_items( $item_count, $order = 'ASC' ) {
7577
global $wpdb;
7678

77-
// TODO make it more simple for the $item_count
79+
$order = 'DESC' === $order ? 'DESC' : 'ASC';
80+
81+
$sql_order = "ORDER BY option_name {$order}";
82+
83+
$sql = "SELECT option_name AS id, option_value AS value
84+
FROM $wpdb->options
85+
WHERE option_name LIKE %s
86+
{$sql_order}";
87+
88+
$params = array( "jpsq_{$this->queue_id}-%" );
89+
7890
if ( $item_count ) {
79-
// phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared,WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.DirectDatabaseQuery.DirectQuery
80-
$items = $wpdb->get_results(
81-
$wpdb->prepare(
82-
"SELECT option_name AS id, option_value AS value FROM $wpdb->options WHERE option_name LIKE %s ORDER BY option_name ASC LIMIT %d",
83-
"jpsq_{$this->queue_id}-%",
84-
$item_count
85-
),
86-
OBJECT
87-
);
88-
} else {
89-
// phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared,WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.DirectDatabaseQuery.DirectQuery
90-
$items = $wpdb->get_results(
91-
$wpdb->prepare(
92-
"SELECT option_name AS id, option_value AS value FROM $wpdb->options WHERE option_name LIKE %s ORDER BY option_name ASC",
93-
"jpsq_{$this->queue_id}-%"
94-
),
95-
OBJECT
96-
);
91+
$sql .= ' LIMIT %d';
92+
$params[] = $item_count;
9793
}
9894

95+
// phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching
96+
$items = $wpdb->get_results(
97+
$wpdb->prepare( $sql, $params ), // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared
98+
OBJECT
99+
);
100+
99101
return $items;
100102
}
101103

‎projects/packages/sync/src/sync-queue/class-queue-storage-table.php‎

Lines changed: 24 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -228,55 +228,38 @@ public function insert_item( $item_id, $item ) {
228228
* Fetch items from the queue.
229229
*
230230
* @param int|null $item_count How many items to fetch from the queue.
231-
* The parameter is null-able, if no limit on the amount of items.
231+
* Null for no limit.
232+
* @param string $order Sort direction for the items. Accepts 'ASC' or 'DESC'.
233+
* Any other value will be treated as 'ASC'.
232234
*
233-
* @return object[]|null
235+
* @return array|object|null Array of result objects on success, or null on failure.
234236
*/
235-
public function fetch_items( $item_count ) {
237+
public function fetch_items( $item_count, $order = 'ASC' ) {
236238
global $wpdb;
237239

238-
/**
239-
* Ignoring the linting warning, as there's still no placeholder replacement for DB field name,
240-
* in this case this is `$this->table_name`
241-
*/
242-
// phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared
240+
$order = ( 'DESC' === $order ) ? 'DESC' : 'ASC';
241+
$sql_order = "ORDER BY event_id {$order}";
242+
243+
$sql = "
244+
SELECT
245+
event_id AS id,
246+
event_payload AS value
247+
FROM {$this->table_name}
248+
WHERE queue_id LIKE %s
249+
{$sql_order}
250+
";
251+
252+
$params = array( $this->queue_id );
243253

244-
// TODO make it more simple for the $item_count
245254
if ( $item_count ) {
246-
// phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.PreparedSQL.InterpolatedNotPrepared
247-
$items = $wpdb->get_results(
248-
$wpdb->prepare(
249-
"
250-
SELECT
251-
event_id AS id,
252-
event_payload AS value
253-
FROM {$this->table_name}
254-
WHERE queue_id LIKE %s
255-
ORDER BY event_id ASC
256-
LIMIT %d
257-
",
258-
$this->queue_id,
259-
$item_count
260-
)
261-
);
262-
} else {
263-
// phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.PreparedSQL.InterpolatedNotPrepared
264-
$items = $wpdb->get_results(
265-
$wpdb->prepare(
266-
"
267-
SELECT
268-
event_id AS id,
269-
event_payload AS value
270-
FROM {$this->table_name}
271-
WHERE queue_id LIKE %s
272-
ORDER BY event_id ASC
273-
",
274-
$this->queue_id
275-
)
276-
);
255+
$sql .= ' LIMIT %d';
256+
$params[] = $item_count;
277257
}
278258

279-
// phpcs:enable WordPress.DB.PreparedSQL.InterpolatedNotPrepared
259+
// phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching
260+
$items = $wpdb->get_results(
261+
$wpdb->prepare( $sql, $params ) // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared
262+
);
280263

281264
return $items;
282265
}

‎projects/plugins/jetpack/changelog/update-stop-sending-redundant-dedicated_sync_enabled-actions‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Significance: patch
2+
Type: other
3+
Comment: Update Sync related unit tests
4+
5+

‎projects/plugins/jetpack/tests/php/sync/Jetpack_Sync_Queue_Test.php‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
use Automattic\Jetpack\Sync\Queue;
44
use Automattic\Jetpack\Sync\Queue_Buffer;
5+
use Automattic\Jetpack\Sync\Utils;
56

67
class Jetpack_Sync_Queue_Test extends WP_UnitTestCase {
78
use \Automattic\Jetpack\PHPUnit\WP_UnitTestCase_Fix;
@@ -46,6 +47,15 @@ public function test_peek_items() {
4647
$this->assertEquals( array( 'foo', 'bar' ), $this->queue->peek( 2 ) );
4748
}
4849

50+
public function test_peek_newest_items() {
51+
$this->queue->add( 'foo' );
52+
$this->queue->add( 'bar' );
53+
$this->queue->add( 'baz' );
54+
55+
$this->assertEquals( array( 'baz' ), $this->queue->peek_newest( 1 ) );
56+
$this->assertEquals( array( 'baz', 'bar' ), $this->queue->peek_newest( 2 ) );
57+
}
58+
4959
public function test_items_exist() {
5060
$this->assertFalse( $this->queue->has_any_items() );
5161

@@ -54,6 +64,28 @@ public function test_items_exist() {
5464
$this->assertTrue( $this->queue->has_any_items() );
5565
}
5666

67+
public function test_pop_items() {
68+
$this->queue->add( 'foo' );
69+
$this->queue->add( 'bar' );
70+
$this->queue->add( 'baz' );
71+
72+
$popped = $this->queue->pop( 1 );
73+
74+
$this->assertEquals( array( 'foo' ), Utils::get_item_values( $popped ) );
75+
$this->assertSame( 2, $this->queue->size() );
76+
}
77+
78+
public function test_pop_newest_items() {
79+
$this->queue->add( 'foo' );
80+
$this->queue->add( 'bar' );
81+
$this->queue->add( 'baz' );
82+
83+
$popped = $this->queue->pop_newest( 1 );
84+
85+
$this->assertEquals( array( 'baz' ), Utils::get_item_values( $popped ) );
86+
$this->assertSame( 2, $this->queue->size() );
87+
}
88+
5789
public function test_queue_lag() {
5890
/**
5991
* @var $queue Automattic\Jetpack\Sync\Queue|\PHPUnit\Framework\MockObject\MockObject

‎projects/plugins/jetpack/tests/php/sync/Jetpack_Sync_Settings_Test.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,19 @@ public function test_enabling_dedicated_sync_setting_with_failed_sync_spawn_test
125125

126126
$this->assertTrue( $this->dedicated_sync_test_request_spawned );
127127
$this->assertFalse( Settings::is_dedicated_sync_enabled() );
128+
129+
// Ensure 'updated_option' events related to 'jetpack_sync_settings_dedicated_sync_enabled' have been removed from
130+
// the queue.
131+
$this->sender->do_sync();
132+
$event = $this->server_event_storage->get_most_recent_event(
133+
'updated_option',
134+
get_current_blog_id(),
135+
function ( $event ) {
136+
return isset( $event->args[0] ) && 'jetpack_sync_settings_dedicated_sync_enabled' === $event->args[0];
137+
}
138+
);
139+
140+
$this->assertFalse( $event );
128141
}
129142

130143
public function test_enabling_dedicated_sync_setting_with_successful_sync_spawn_test_request() {
@@ -135,6 +148,7 @@ public function test_enabling_dedicated_sync_setting_with_successful_sync_spawn_
135148
$this->assertTrue( $this->dedicated_sync_test_request_spawned );
136149
$this->assertTrue( Settings::is_dedicated_sync_enabled() );
137150
}
151+
138152
/**
139153
* Intercept HTTP request to run Sync and mock the response.
140154
* Should be hooked on the `pre_http_request` filter.

0 commit comments

Comments
 (0)