From d89580cf709a0f0b67caf6fe1ba7bc30a4a4de3e Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Thu, 18 Jul 2024 14:23:44 -0700 Subject: [PATCH 1/7] Do not call `EventQueue` functions with empty arrays --- includes/EventManager.php | 14 +- tests/phpunit/includes/EventManagerTest.php | 218 +++++++++++++++++++- 2 files changed, 216 insertions(+), 16 deletions(-) diff --git a/includes/EventManager.php b/includes/EventManager.php index 0de4661..12ee25b 100644 --- a/includes/EventManager.php +++ b/includes/EventManager.php @@ -215,14 +215,14 @@ protected function send_request_events( array $events ): void { continue; } - $queue = EventQueue::getInstance()->queue(); - if ( is_wp_error( $response ) ) { EventQueue::getInstance()->queue()->push( $events ); continue; } - EventQueue::getInstance()->queue()->push( $response['failedEvents'] ); + if ( ! empty( $response['failedEvents'] ) ) { + EventQueue::getInstance()->queue()->push( $response['failedEvents'] ); + } } } @@ -265,10 +265,14 @@ public function send_saved_events_batch(): void { } // Remove from the queue. - $queue->remove( array_keys( $response['succeededEvents'] ) ); + if ( ! empty( $response['succeededEvents'] ) ) { + $queue->remove( array_keys( $response['succeededEvents'] ) ); + } // Release the 'reserve' we placed on the entry, so it will be tried again later. - $queue->release( array_keys( $response['failedEvents'] ) ); + if ( ! empty( $response['failedEvents'] ) ) { + $queue->release( array_keys( $response['failedEvents'] ) ); + } } } } diff --git a/tests/phpunit/includes/EventManagerTest.php b/tests/phpunit/includes/EventManagerTest.php index 2e773fa..b02e290 100644 --- a/tests/phpunit/includes/EventManagerTest.php +++ b/tests/phpunit/includes/EventManagerTest.php @@ -197,12 +197,15 @@ function () use ( $batch_queue_mock ) { $hiive_connection_subscriber->expects( 'notify' ) ->once() ->andReturn( - array( 'succeededEvents' => array( 15 => array() ), 'failedEvents' => array() ) + array( + 'succeededEvents' => array( 15 => array() ), + 'failedEvents' => array( 16 => array() ), + ) ); WP_Mock::userFunction( 'is_wp_error' ) - ->once() - ->andReturnFalse(); + ->once() + ->andReturnFalse(); $batch_queue_mock->expects( 'remove' ) ->once() @@ -210,7 +213,141 @@ function () use ( $batch_queue_mock ) { $batch_queue_mock->expects( 'release' ) ->once() - ->with( array() ); + ->with( array( 16 ) ); + + $sut->send_saved_events_batch(); + + $this->assertConditionsMet(); + } + + /** + * @covers ::send_saved_events_batch + * @covers ::send + */ + public function test_send_saved_events_happy_path_no_failed_events(): void { + + $batch_queue_mock = Mockery::mock( BatchQueue::class ); + + \Patchwork\redefine( + array( EventQueue::class, '__construct' ), + function () {} + ); + \Patchwork\redefine( + array( EventQueue::class, 'queue' ), + function () use ( $batch_queue_mock ) { + return $batch_queue_mock; + } + ); + + $sut = Mockery::mock( EventManager::class )->makePartial(); + + $event = Mockery::mock( Event::class ); + + $batch_queue_mock->expects( 'pull' ) + ->once() + ->with( 100 ) + ->andReturn( + array( + 15 => $event, + ) + ); + + $batch_queue_mock->expects( 'reserve' ) + ->once() + ->with( array( 15 ) ); + + $hiive_connection_subscriber = Mockery::mock( HiiveConnection::class ); + + $sut->expects( 'get_subscribers' ) + ->once() + ->andReturn( array( $hiive_connection_subscriber ) ); + + $hiive_connection_subscriber->expects( 'notify' ) + ->once() + ->andReturn( + array( + 'succeededEvents' => array( 15 => array() ), + 'failedEvents' => array(), + ) + ); + + WP_Mock::userFunction( 'is_wp_error' ) + ->once() + ->andReturnFalse(); + + $batch_queue_mock->expects( 'remove' ) + ->once() + ->with( array( 15 ) ); + + $batch_queue_mock->expects( 'release' ) + ->never(); + + $sut->send_saved_events_batch(); + + $this->assertConditionsMet(); + } + + /** + * @covers ::send_saved_events_batch + * @covers ::send + */ + public function test_send_saved_events_happy_path_no_successful_events(): void { + + $batch_queue_mock = Mockery::mock( BatchQueue::class ); + + \Patchwork\redefine( + array( EventQueue::class, '__construct' ), + function () {} + ); + \Patchwork\redefine( + array( EventQueue::class, 'queue' ), + function () use ( $batch_queue_mock ) { + return $batch_queue_mock; + } + ); + + $sut = Mockery::mock( EventManager::class )->makePartial(); + + $event = Mockery::mock( Event::class ); + + $batch_queue_mock->expects( 'pull' ) + ->once() + ->with( 100 ) + ->andReturn( + array( + 15 => $event, + ) + ); + + $batch_queue_mock->expects( 'reserve' ) + ->once() + ->with( array( 15 ) ); + + $hiive_connection_subscriber = Mockery::mock( HiiveConnection::class ); + + $sut->expects( 'get_subscribers' ) + ->once() + ->andReturn( array( $hiive_connection_subscriber ) ); + + $hiive_connection_subscriber->expects( 'notify' ) + ->once() + ->andReturn( + array( + 'succeededEvents' => array(), + 'failedEvents' => array( 16 => array() ), + ) + ); + + WP_Mock::userFunction( 'is_wp_error' ) + ->once() + ->andReturnFalse(); + + $batch_queue_mock->expects( 'remove' ) + ->never(); + + $batch_queue_mock->expects( 'release' ) + ->once() + ->with( array( 16 ) ); $sut->send_saved_events_batch(); @@ -323,7 +460,10 @@ function () use ( $batch_queue_mock ) { $hiive_connection_subscriber->expects( 'notify' ) ->once() ->andReturn( - array( 'succeededEvents' => array(), 'failedEvents' => array( 19 => array() ) ) + array( + 'succeededEvents' => array(), + 'failedEvents' => array( 19 => array() ), + ) ); WP_Mock::userFunction( 'is_wp_error' ) @@ -344,9 +484,59 @@ function () use ( $batch_queue_mock ) { /** * @covers ::shutdown - * @covers ::send + * @covers ::send_request_events + */ + public function test_shutdown_happy_path_no_failed_events(): void { + + $sut = new EventManager(); + + $event = Mockery::mock( Event::class )->makePartial(); + $event->key = 'test'; + + $sut->push( $event ); + + $batch_queue_mock = Mockery::mock( BatchQueue::class ); + + \Patchwork\redefine( + array( EventQueue::class, '__construct' ), + function () {} + ); + \Patchwork\redefine( + array( EventQueue::class, 'queue' ), + function () use ( $batch_queue_mock ) { + return $batch_queue_mock; + } + ); + + $hiive_connection_subscriber = Mockery::mock( HiiveConnection::class ); + + $sut->add_subscriber( $hiive_connection_subscriber ); + + $hiive_connection_subscriber->expects( 'notify' ) + ->once() + ->andReturn( + array( + 'succeededEvents' => array(), + 'failedEvents' => array(), + ) + ); + + WP_Mock::userFunction( 'is_wp_error' ) + ->once() + ->andReturnFalse(); + + $batch_queue_mock->expects( 'push' )->never(); + + $sut->shutdown(); + + $this->assertConditionsMet(); + } + + /** + * @covers ::shutdown + * @covers ::send_request_events */ - public function test_shutdown_happy_path(): void { + public function test_shutdown_happy_path_with_failed_events(): void { $sut = new EventManager(); @@ -375,7 +565,10 @@ function () use ( $batch_queue_mock ) { $hiive_connection_subscriber->expects( 'notify' ) ->once() ->andReturn( - array( 'succeededEvents' => array(), 'failedEvents' => array( 2 => array( 'key' => 'test' ) ) ) + array( + 'succeededEvents' => array(), + 'failedEvents' => array( 2 => array( 'key' => 'test' ) ), + ) ); WP_Mock::userFunction( 'is_wp_error' ) @@ -383,7 +576,7 @@ function () use ( $batch_queue_mock ) { ->andReturnFalse(); $batch_queue_mock->expects( 'push' )->once() - ->with(array( 2 => array( 'key' => 'test' ) )); + ->with( array( 2 => array( 'key' => 'test' ) ) ); $sut->shutdown(); @@ -469,7 +662,10 @@ function () use ( $batch_queue_mock ) { $hiive_connection_subscriber->expects( 'notify' ) ->once() ->andReturn( - array( 'succeededEvents' => array(), 'failedEvents' => array( 18 => array( 'key' => 'event ') ) ) + array( + 'succeededEvents' => array(), + 'failedEvents' => array( 18 => array( 'key' => 'event ' ) ), + ) ); WP_Mock::userFunction( 'is_wp_error' ) @@ -477,7 +673,7 @@ function () use ( $batch_queue_mock ) { ->andReturnFalse(); $batch_queue_mock->expects( 'push' )->once() - ->with(array( 18 => array( 'key' => 'event ') )); + ->with( array( 18 => array( 'key' => 'event ' ) ) ); $sut->shutdown(); From a377eed7eb65a55d0ae3e3b3f8759a50bf658866 Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Thu, 18 Jul 2024 15:06:57 -0700 Subject: [PATCH 2/7] Fix recursive call on successful reconnect --- includes/HiiveConnection.php | 2 +- .../phpunit/includes/HiiveConnectionTest.php | 28 ++++++++++++++++--- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/includes/HiiveConnection.php b/includes/HiiveConnection.php index eea3feb..01613e5 100644 --- a/includes/HiiveConnection.php +++ b/includes/HiiveConnection.php @@ -344,7 +344,7 @@ public function hiive_request( string $path, ?array $payload = array(), ?array $ $body = json_decode( $request_response['body'], true ); if ( 'Invalid token for url' === $body['message'] ) { if ( $this->reconnect() ) { - $this->hiive_request( $path, $args ); + $this->hiive_request( $path, $payload, $args ); } else { return new WP_Error( 'hiive_connection', __( 'This site is not connected to the hiive.' ) ); } diff --git a/tests/phpunit/includes/HiiveConnectionTest.php b/tests/phpunit/includes/HiiveConnectionTest.php index e5f5ca5..cff26a5 100644 --- a/tests/phpunit/includes/HiiveConnectionTest.php +++ b/tests/phpunit/includes/HiiveConnectionTest.php @@ -25,7 +25,11 @@ function ( $input ) { ); WP_Mock::userFunction( 'wp_parse_args' )->andReturnUsing( function () { - return array_merge( func_get_args() ); + $merged = array(); + foreach ( func_get_args() as $arr ) { + $merged = array_merge( $merged, $arr ); + } + return $merged; } ); } @@ -182,7 +186,13 @@ function ( $request_response ) { public function test_notify_bad_token(): void { $sut = Mockery::mock( HiiveConnection::class )->makePartial(); - $event = Mockery::mock( Event::class ); + $event = Mockery::mock( Event::class ); + $event->category = 'admin'; + $event->key = 'plugin_search'; + $event->data = array( + 'type' => 'term', + 'query' => 'seo', + ); WP_Mock::userFunction( 'get_option' ) ->with( 'nfd_data_token' ) @@ -197,8 +207,18 @@ public function test_notify_bad_token(): void { $sut->expects( 'get_core_data' )->times( 2 )->andReturn( array( 'brand' => 'etc' ) ); WP_Mock::userFunction( 'wp_remote_request' ) - ->with( '/sites/v2/events', \WP_Mock\Functions::type( 'array' ) ) - ->twice()->andReturnValues( + ->withArgs( + function ( $url, $args ) { + assert( '/sites/v2/events' === $url ); + + $body = json_decode( $args['body'], true ); + assert( 'seo' === $body['events'][0]['data']['query'] ); + + return true; + } + ) + ->twice() + ->andReturnValues( array( array( 'response' => array( From ec74a66bd8880bb4922ca924189f7f2891d5b3f1 Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Thu, 18 Jul 2024 15:09:45 -0700 Subject: [PATCH 3/7] Fix spelling of headers `application/json` --- includes/HiiveConnection.php | 4 ++-- tests/phpunit/includes/HiiveConnectionTest.php | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/includes/HiiveConnection.php b/includes/HiiveConnection.php index 01613e5..39dcedd 100644 --- a/includes/HiiveConnection.php +++ b/includes/HiiveConnection.php @@ -319,8 +319,8 @@ public function hiive_request( string $path, ?array $payload = array(), ?array $ $defaults = array( 'method' => 'POST', 'headers' => array( - 'Content-Type' => 'applicaton/json', - 'Accept' => 'applicaton/json', + 'Content-Type' => 'application/json', + 'Accept' => 'application/json', 'Authorization' => 'Bearer ' . self::get_auth_token(), ), 'timeout' => wp_is_serving_rest_request() ? 15 : 60, // If we're responding to the frontend, we need to be quick. diff --git a/tests/phpunit/includes/HiiveConnectionTest.php b/tests/phpunit/includes/HiiveConnectionTest.php index cff26a5..614dfb1 100644 --- a/tests/phpunit/includes/HiiveConnectionTest.php +++ b/tests/phpunit/includes/HiiveConnectionTest.php @@ -214,6 +214,9 @@ function ( $url, $args ) { $body = json_decode( $args['body'], true ); assert( 'seo' === $body['events'][0]['data']['query'] ); + assert( 'application/json' === $args['headers']['Content-Type'] ); + assert( 'application/json' === $args['headers']['Accept'] ); + return true; } ) From 6e0b3c762b5e85db69758387f963ec722359568e Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Thu, 18 Jul 2024 16:47:17 -0700 Subject: [PATCH 4/7] Create EventManagerWPUnitTest.php --- .../includes/EventManagerWPUnitTest.php | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 tests/wpunit/includes/EventManagerWPUnitTest.php diff --git a/tests/wpunit/includes/EventManagerWPUnitTest.php b/tests/wpunit/includes/EventManagerWPUnitTest.php new file mode 100644 index 0000000..f69b615 --- /dev/null +++ b/tests/wpunit/includes/EventManagerWPUnitTest.php @@ -0,0 +1,49 @@ +category = 'admin'; + $event->key = 'plugin_search'; + $event->data = array( + 'type' => 'term', + 'query' => 'seo', + ); + + $sut->push( $event ); + + $hiive_connection = Mockery::mock( HiiveConnection::class ); + $hiive_connection->expects('notify' ) + ->once() + ->andReturn( + array( + 'succeededEvents' => array(), + 'failedEvents' => array(), + ) + ); + + $sut->add_subscriber( $hiive_connection ); + + $sut->shutdown(); + } +} From 2c24feeefa682191a32a23d51c822632712144e2 Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Wed, 24 Jul 2024 11:57:01 -0700 Subject: [PATCH 5/7] Handle 500 errors from Hiive which are not structured --- includes/HiiveConnection.php | 11 ++- .../includes/HiiveConnectionWPUnitTest.php | 94 ++++++++++++++++++- 2 files changed, 101 insertions(+), 4 deletions(-) diff --git a/includes/HiiveConnection.php b/includes/HiiveConnection.php index 39dcedd..f6c6b7a 100644 --- a/includes/HiiveConnection.php +++ b/includes/HiiveConnection.php @@ -292,11 +292,18 @@ public function notify( $events ) { return new WP_Error( wp_remote_retrieve_response_code( $hiive_response ), wp_remote_retrieve_response_message( $hiive_response ) ); } - return json_decode( wp_remote_retrieve_body( $hiive_response ), true ); + $response_body = json_decode( wp_remote_retrieve_body( $hiive_response ), true ); + + // If the response from Hiive is not shaped as expected, e.g. a more serious 500 error, return as an error, not as the expected array. + if ( ! is_array( $response_body ) || ! array_key_exists( 'succeededEvents', $response_body ) || ! array_key_exists( 'failedEvents', $response_body ) ) { + return new WP_Error( 'hiive_response', 'Response body does not contain succeededEvents and failedEvents keys.' ); + } + + return $response_body; } /** - * Send a HTTP request to Hiive and return the body of the request. + * Send an HTTP request to Hiive and return the body of the request. * * Handles throttling and reconnection, clients should handle queueing if necessary. * diff --git a/tests/wpunit/includes/HiiveConnectionWPUnitTest.php b/tests/wpunit/includes/HiiveConnectionWPUnitTest.php index 5565064..e47eb63 100644 --- a/tests/wpunit/includes/HiiveConnectionWPUnitTest.php +++ b/tests/wpunit/includes/HiiveConnectionWPUnitTest.php @@ -130,7 +130,7 @@ function () { 'pre_http_request', function () { return array( - 'body' => json_encode( + 'body' => json_encode( array( 'data' => array( array( @@ -143,7 +143,7 @@ function () { ), ) ), - 'response' => array( + 'response' => array( 'code' => 200, 'message' => 'OK', ), @@ -164,4 +164,94 @@ function () { $this->assertEquals( 'notification123', $result[0]['id'] ); } + + /** + * Hiive may return a 500 error which contains data `succeededEvents` and `failedEvents` which can be acted upon. + * If those keys are absent, treat it as a more serious 500 error. + */ + public function test_500_error_without_data(): void { + + $sut = Mockery::mock( HiiveConnection::class )->makePartial(); + $sut->expects('get_core_data')->andReturn(array()); + update_option( 'nfd_data_token', 'appear-connected' ); + + /** + * @see WP_Http::request() + */ + add_filter( + 'pre_http_request', + function () { + return array( + 'body' => 'Internal Server Error', + 'response' => array( + 'code' => 500, + 'message' => 'OK', + ), + ); + } + ); + + $event = new Event( + 'admin', + 'plugin_search', + array( + 'type' => 'term', + 'query' => 'seo', + ), + ); + + $result = $sut->notify( array( $event ) ); + + $this->assertWPError( $result ); + } + + /** + * Hiive may return a 500 error which contains data `succeededEvents` and `failedEvents` which can be acted upon. + * If those keys are absent, treat it as a more serious 500 error. + */ + public function test_500_error_with_data(): void { + + $sut = Mockery::mock( HiiveConnection::class )->makePartial(); + $sut->expects('get_core_data')->andReturn(array()); + update_option( 'nfd_data_token', 'appear-connected' ); + + $event = new Event( + 'admin', + 'plugin_search', + array( + 'type' => 'term', + 'query' => 'seo', + ), + ); + + /** + * @see WP_Http::request() + */ + add_filter( + 'pre_http_request', + function () use ( $event ) { + return array( + 'body' => json_encode( + array( + 'succeededEvents' => array( 1 => $event ), + 'failedEvents' => array( 2 => $event ), + ) + ), + 'response' => array( + 'code' => 500, + 'message' => 'OK', + ), + ); + } + ); + + $result = $sut->notify( + array( + 1 => $event, + 2 => $event, + ) + ); + + $this->assertIsArray( $result ); + } } From ad71e78383b04c7d2ed1b0bc7b59e81d008195cf Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Wed, 24 Jul 2024 11:57:17 -0700 Subject: [PATCH 6/7] lint/comment/type --- includes/EventQueue/Queryable.php | 2 +- includes/EventQueue/Queues/BatchQueue.php | 2 +- includes/Helpers/Plugin.php | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/includes/EventQueue/Queryable.php b/includes/EventQueue/Queryable.php index 89bd16d..573cb3e 100644 --- a/includes/EventQueue/Queryable.php +++ b/includes/EventQueue/Queryable.php @@ -27,7 +27,7 @@ protected function table() { * Returns number of affected (inserted) rows. * * @param string $table - * @param array[] $rows + * @param non-empty-array $rows * * @return bool|int */ diff --git a/includes/EventQueue/Queues/BatchQueue.php b/includes/EventQueue/Queues/BatchQueue.php index 20ef896..2e2b45b 100644 --- a/includes/EventQueue/Queues/BatchQueue.php +++ b/includes/EventQueue/Queues/BatchQueue.php @@ -29,7 +29,7 @@ public function __construct( Container $container ) { /** * Push events onto the queue * - * @param Event[] $events + * @param non-empty-array $events * * @return bool */ diff --git a/includes/Helpers/Plugin.php b/includes/Helpers/Plugin.php index dae20c8..335b81c 100644 --- a/includes/Helpers/Plugin.php +++ b/includes/Helpers/Plugin.php @@ -50,9 +50,9 @@ public static function collect_installed() { /** * Grab relevant data from plugin data - and only what we want * - * @param array $slug The slug for the plugin - * @param array $data The plugin meta data from the header - * @param array $mu Whether the plugin is installed as an mu + * @param string $slug The slug for the plugin. + * @param array $data The plugin meta-data from its header. + * @param array $mu Whether the plugin is installed as a must-use plugin. * * @return array Hiive relevant plugin details */ From ccc985bdb704e9e8eb2d8a5b30dccd6853c6f0dd Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Wed, 24 Jul 2024 11:57:23 -0700 Subject: [PATCH 7/7] Update EventManagerTest.php --- tests/phpunit/includes/EventManagerTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/phpunit/includes/EventManagerTest.php b/tests/phpunit/includes/EventManagerTest.php index b02e290..cd74a8f 100644 --- a/tests/phpunit/includes/EventManagerTest.php +++ b/tests/phpunit/includes/EventManagerTest.php @@ -474,8 +474,7 @@ function () use ( $batch_queue_mock ) { ->once() ->with( array( 19 ) ); - $batch_queue_mock->expects( 'remove' )->once() - ->with( array() ); + $batch_queue_mock->expects( 'remove' )->never(); $sut->send_saved_events_batch();