Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/3644 remove rest batch infrastructure #3755

Merged
merged 14 commits into from
Jul 29, 2021
Merged

Feature/3644 remove rest batch infrastructure #3755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2c2bce1
Remove data rest route.
Jul 23, 2021
58c6b66
Remove get_batch_data.
Jul 23, 2021
86b079f
Merge branch 'develop' into feature/3644-remove-rest-batch-infrastruc…
Jul 26, 2021
7a31faf
Batch request.
Jul 26, 2021
7dfdff0
Create request outside of closure.
Jul 26, 2021
cbe1888
Remove test for removed functionality.
Jul 26, 2021
84f0f54
Remove removed endpoint.
Jul 26, 2021
8cdc7b8
Merge branch 'develop' into feature/3644-remove-rest-batch-infrastruc…
Jul 27, 2021
4b8db8a
Add parser for webdatastreams-batch.
Jul 27, 2021
bdf7984
Add array wrap.
Jul 27, 2021
ca11a0e
Rename function.
Jul 27, 2021
75fa9af
Merge branch 'develop' into feature/3644-remove-rest-batch-infrastruc…
Jul 29, 2021
7886d76
Process all responses and multiple data streams.
Jul 29, 2021
4713411
Remove error_to_response.
Jul 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 0 additions & 148 deletions includes/Core/Modules/Module.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,154 +232,6 @@ final public function set_data( $datapoint, $data ) {
);
}

/**
* Gets data for multiple datapoints in one go.
*
* When needing to fetch multiple pieces of data at once, this method provides a more performant approach than
* {@see Module::get_data()} by combining multiple external requests into a single one.
*
* @since 1.0.0
*
* @param \stdClass[]|Data_Request[] $datasets List of datapoints with data attached.
* @return array List of responses. Each item is either the response data, or a WP_Error on failure.
*/
final public function get_batch_data( array $datasets ) {
// Ensure all services are initialized.
try {
$this->get_service( 'default' );
} catch ( Exception $e ) {
// Internal error.
if ( ! is_array( $this->google_services ) ) {
return array();
}
}

$restore_defer = $this->with_client_defer( true );

$datapoint_definitions = $this->get_datapoint_definitions();
$service_batches = array();

$data_requests = array();
$results = array();
foreach ( $datasets as $dataset ) {
if ( ! $dataset instanceof Data_Request ) {
$dataset = new Data_Request(
'GET',
'modules',
$dataset->identifier,
$dataset->datapoint,
(array) $dataset->data,
$dataset->key
);
}

/* @var Data_Request $dataset Request object. */
if ( $this->slug !== $dataset->identifier ) {
continue;
}

$definition_key = "{$dataset->method}:{$dataset->datapoint}";
if ( ! isset( $datapoint_definitions[ $definition_key ] ) ) {
continue;
}

$key = $dataset->key ?: wp_rand();
$data_requests[ $key ] = $dataset;
$datapoint = $dataset->datapoint;

try {
$this->validate_data_request( $dataset );
$request = $this->create_data_request( $dataset );
} catch ( Exception $e ) {
$request = $this->exception_to_error( $e, $datapoint );
}

if ( is_wp_error( $request ) ) {
$results[ $key ] = $request;
continue;
}

if ( $request instanceof Closure ) {
try {
$response = $request();
$results[ $key ] = $response;

if ( ! is_wp_error( $response ) ) {
$results[ $key ] = $this->parse_data_response( $dataset, $response );
}
} catch ( Exception $e ) {
$results[ $key ] = $this->exception_to_error( $e, $datapoint );
}
continue;
}

$datapoint_service = $datapoint_definitions[ $definition_key ]['service'];
if ( empty( $datapoint_service ) ) {
continue;
}

if ( ! isset( $service_batches[ $datapoint_service ] ) ) {
$service_batches[ $datapoint_service ] = $this->google_services[ $datapoint_service ]->createBatch();
}

$service_batches[ $datapoint_service ]->add( $request, $key );
$results[ $key ] = $definition_key;
}

foreach ( $service_batches as $service_identifier => $batch ) {
try {
$batch_results = $batch->execute();
} catch ( Exception $e ) {
// Set every result of this batch to the exception.
foreach ( $results as $key => $definition_key ) {
if ( is_wp_error( $definition_key ) ) {
continue;
}

$datapoint_service = ! empty( $datapoint_definitions[ $definition_key ] )
? $datapoint_definitions[ $definition_key ]['service']
: null;

if ( is_string( $definition_key ) && $service_identifier === $datapoint_service ) {
$results[ $key ] = $this->exception_to_error( $e, explode( ':', $definition_key, 2 )[1] );
}
}
continue;
}

foreach ( $batch_results as $key => $result ) {
if ( 0 === strpos( $key, 'response-' ) ) {
$key = substr( $key, 9 );
}
if ( ! isset( $results[ $key ] ) || ! is_string( $results[ $key ] ) ) {
continue;
}

if ( ! $result instanceof Exception ) {
$results[ $key ] = $result;
$results[ $key ] = $this->parse_data_response( $data_requests[ $key ], $result );
} else {
$definition_key = $results[ $key ];
$results[ $key ] = $this->exception_to_error( $result, explode( ':', $definition_key, 2 )[1] );
}
}
}

$restore_defer();

// Cache the results for storybook.
if (
! empty( $results )
&& null !== $this->context->input()->filter( INPUT_GET, 'datacache' )
&& current_user_can( 'manage_options' )
) {
$cache = new Cache();
$cache->cache_batch_results( $datasets, $results );
}

return $results;
}

/**
* Returns the list of datapoints the class provides data for.
*
Expand Down
97 changes: 0 additions & 97 deletions includes/Core/REST_API/REST_Routes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,69 +167,6 @@ private function get_routes() {
};

$routes = array(
// TODO: This route is super-complex to use and needs to be simplified.
eugene-manuilov marked this conversation as resolved.
Show resolved Hide resolved
new REST_Route(
'data',
array(
array(
'methods' => WP_REST_Server::CREATABLE,
'callback' => function( WP_REST_Request $request ) {
if ( ! $request['request'] ) {
return new WP_Error( 'no_data_requested', __( 'Missing request data.', 'google-site-kit' ), array( 'status' => 400 ) );
}

// Datasets are expected to be objects but the REST API parses the JSON into an array.
$datasets = array_map(
function ( $dataset_array ) {
return (object) $dataset_array;
},
$request['request']
);

$modules = $this->modules->get_active_modules();

$responses = array();
foreach ( $modules as $module ) {
$filtered_datasets = array_filter(
$datasets,
function( $dataset ) use ( $module ) {
return 'modules' === $dataset->type && $module->slug === $dataset->identifier; // phpcs:ignore WordPress.NamingConventions.ValidVariableName
}
);
if ( empty( $filtered_datasets ) ) {
continue;
}
$additional_responses = $module->get_batch_data( $filtered_datasets );
if ( is_array( $additional_responses ) ) {
$responses = array_merge( $responses, $additional_responses );
}
}
$responses = array_map(
function ( $response ) {
if ( is_wp_error( $response ) ) {
return $this->error_to_response( $response );
}
return $response;
},
$responses
);

return new WP_REST_Response( $responses );
},
'permission_callback' => $can_view_insights,
'args' => array(
'request' => array(
'type' => 'array',
'description' => __( 'List of request objects.', 'google-site-kit' ),
'required' => true,
'items' => array(
'type' => 'object',
),
),
),
),
)
),
// TODO: Remove this and replace usage with calls to wp/v1/posts.
new REST_Route(
'core/search/data/post-search',
Expand Down Expand Up @@ -363,38 +300,4 @@ function ( $response ) {
*/
return apply_filters( 'googlesitekit_rest_routes', $routes );
}

/**
* Converts a WP_Error to its response representation.
*
* Adapted from \WP_REST_Server::error_to_response
*
* @since 1.2.0
*
* @param WP_Error $error Error to transform.
*
* @return array
*/
protected function error_to_response( WP_Error $error ) {
$errors = array();

foreach ( (array) $error->errors as $code => $messages ) {
foreach ( (array) $messages as $message ) {
$errors[] = array(
'code' => $code,
'message' => $message,
'data' => $error->get_error_data( $code ),
);
}
}

$data = $errors[0];
if ( count( $errors ) > 1 ) {
// Remove the primary error.
array_shift( $errors );
$data['additional_errors'] = $errors;
}

return $data;
}
}
45 changes: 31 additions & 14 deletions includes/Modules/Analytics_4.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,21 +385,14 @@ protected function create_data_request( Data_Request $data ) {
);
}

return function() use ( $data ) {
$requests = array();

foreach ( $data['propertyIDs'] as $property_id ) {
$requests[] = new Data_Request(
'GET',
'modules',
self::MODULE_SLUG,
'webdatastreams',
array( 'propertyID' => $property_id ),
$property_id
);
}
$analyticsadmin = $this->get_service( 'analyticsadmin' );
$batch_request = $analyticsadmin->createBatch();
foreach ( $data['propertyIDs'] as $property_id ) {
$batch_request->add( $analyticsadmin->properties_webDataStreams->listPropertiesWebDataStreams( self::normalize_property_id( $property_id ) ) ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase
}

return $this->get_batch_data( $requests );
return function() use ( $batch_request ) {
return $batch_request->execute();
};
}

Expand Down Expand Up @@ -445,6 +438,8 @@ function( $property ) {
return self::filter_property_with_ids( $response );
case 'GET:webdatastreams':
return array_map( array( self::class, 'filter_webdatastream_with_ids' ), $response->getWebDataStreams() );
case 'GET:webdatastreams-batch':
return self::parse_webdatastreams_batch( $response );
}

return parent::parse_data_response( $data, $response );
Expand Down Expand Up @@ -622,6 +617,28 @@ public static function filter_webdatastream_with_ids( $webdatastream ) {
return $obj;
}

/**
* Parses a response, adding the _id and _propertyID params and converting to an array keyed by the propertyID and web datastream IDs.
*
* @since n.e.x.t
*
* @param GoogleAnalyticsAdminV1alphaListWebDataStreamsResponse[] $response Array of GoogleAnalyticsAdminV1alphaListWebDataStreamsResponse objects.
* @return \stdClass[] Array of models containing _id and _propertyID attributes, keyed by the propertyID.
*/
public static function parse_webdatastreams_batch( $response ) {
$mapped = array();
foreach ( $response as $single_response ) {
$webdatastreams = $single_response->getWebDataStreams();
foreach ( $webdatastreams as $webdatastream ) {
$value = self::filter_webdatastream_with_ids( $webdatastream );
$key = $value->_propertyID; // phpcs:ignore WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase
$mapped[ $key ] = isset( $mapped[ $key ] ) ? $mapped[ $key ] : array();
$mapped[ $key ][] = $value;
}
}
return $mapped;
}

/**
* Normalizes account ID and returns it.
*
Expand Down
41 changes: 0 additions & 41 deletions tests/phpunit/integration/Core/Modules/ModuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,47 +135,6 @@ public function test_set_data() {
$this->assertEquals( 2, $method->getNumberOfRequiredParameters() );
}

public function test_get_batch_data() {
// get_batch_data is a wrapper for the protected execute_data_request method.
$method = new \ReflectionMethod( self::MODULE_CLASS_NAME, 'get_batch_data' );
// Make assertions that affect backwards compatibility
$this->assertTrue( $method->isPublic() );
// Number of parameters can increase while preserving B/C, but not decrease
$this->assertEquals( 1, $method->getNumberOfParameters() );
// Number of required parameters can decrease while preserving B/C, but not increase
$this->assertEquals( 1, $method->getNumberOfRequiredParameters() );

$module = new FakeModule( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ) );
$batch_responses = $module->get_batch_data(
array(
new Data_Request(
'GET',
'modules',
$module->slug,
'test-request',
array( 'foo' => 'bar' ),
'request-1'
),
new Data_Request(
'GET',
'modules',
$module->slug,
'test-request',
array( 'bar' => 'baz' ),
'request-2'
),
)
);
$response = $batch_responses['request-1'];
$this->assertEquals( 'GET', $response->method );
$this->assertEquals( 'test-request', $response->datapoint );
$this->assertEquals( array( 'foo' => 'bar' ), (array) $response->data );
$response = $batch_responses['request-2'];
$this->assertEquals( 'GET', $response->method );
$this->assertEquals( 'test-request', $response->datapoint );
$this->assertEquals( array( 'bar' => 'baz' ), (array) $response->data );
}

public function test_get_datapoints() {
$module = new FakeModule( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ) );

Expand Down
1 change: 0 additions & 1 deletion tests/phpunit/integration/Core/REST_API/REST_RoutesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ public function test_register() {
'/' . REST_Routes::REST_ROOT . '/core/modules/data/info',
'/' . REST_Routes::REST_ROOT . '/core/modules/data/activation',
'/' . REST_Routes::REST_ROOT . '/modules/(?P<slug>[a-z0-9\\-]+)/data/(?P<datapoint>[a-z\\-]+)',
'/' . REST_Routes::REST_ROOT . '/data',
'/' . REST_Routes::REST_ROOT . '/modules/(?P<slug>[a-z0-9\\-]+)/data/notifications',
'/' . REST_Routes::REST_ROOT . '/modules/(?P<slug>[a-z0-9\\-]+)/data/settings',
'/' . REST_Routes::REST_ROOT . '/core/search/data/post-search',
Expand Down