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.

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

Allow URL metric schema to be extended #1492

Merged
merged 17 commits into from
Sep 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
79 changes: 79 additions & 0 deletions plugins/optimization-detective/class-od-strict-url-metric.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
<?php
/**
* Optimization Detective: OD_Strict_URL_Metric class
*
* @package optimization-detective
* @since n.e.x.t
*/

// Exit if accessed directly.
if ( ! defined( 'ABSPATH' ) ) {
exit;
}

/**
* Representation of the measurements taken from a single client's visit to a specific URL without additionalProperties allowed.
*
* This is used exclusively in the REST API endpoint for capturing new URL metrics to prevent invalid additional data from being
* submitted in the request. For URL metrics which have been stored the looser OD_URL_Metric class is used instead.
*
* @since n.e.x.t
* @access private
*/
final class OD_Strict_URL_Metric extends OD_URL_Metric {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit confusing to have "strict" URL metric extend the "extendable" URL metric - would make more sense to be the other way around. This is also highlighted by how the strict class will still run the filters, but without that serving any purpose.

Taking this a bit further, strictly speaking neither of the two makes sense because a strict URL metric is not an extendable URL metric and vice-versa. So it would make more sense to use e.g. a decorator pattern, or make the two classes separate implementations of a OD_URL_Metric interface. Of course that would be a decent amount of refactoring to do - simple, but still some code changes so we could consider that separately.

So at this point I mostly think:

  • Should the extension tree be the other way around? If not, why not?
  • Maybe there's a more clear name we can use than just "strict"? Maybe include "REST" so that it's clear that this class is only used for REST API requests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand. The filters are still used. It's just that any objects have additionalProperties set to false in the strict version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, in the strict version, it sets additionalProperties to false which refers to the properties which aren't recognized as part of the schema. The filters are still applying, however, so if a plugin adds new properties to the schema these "additional" properties will be recognized. It's just that additional unrecognized properties will be rejected in the strict verison.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, I had something wrong in my logic here. Some of my other feedback on the class hierarchy still applies, but that's not a big deal in regards to this PR. I do think we should separately consider using a OD_URL_Metric interface and use that in all (or at least most of) the type hints so that the class implementations can be independent.


/**
* Gets JSON schema for URL Metric without additionalProperties.
*
* @since n.e.x.t
*
* @return array<string, mixed> Schema.
*/
public static function get_json_schema(): array {
return self::set_additional_properties_to_false( parent::get_json_schema() );
}

/**
* Recursively processes the schema to ensure that all objects have additionalProperties set to false.
*
* This is a forked version of `rest_default_additional_properties_to_false()` which isn't being used itself because
* it does not override `additionalProperties` to be false, but rather only sets it when it is empty.
*
* @since n.e.x.t
* @see rest_default_additional_properties_to_false()
*
* @param mixed $schema Schema.
* @return mixed Processed schema.
*/
private static function set_additional_properties_to_false( $schema ) {
if ( ! isset( $schema['type'] ) ) {
return $schema;
}

$type = (array) $schema['type'];

if ( in_array( 'object', $type, true ) ) {
if ( isset( $schema['properties'] ) ) {
foreach ( $schema['properties'] as $key => $child_schema ) {
$schema['properties'][ $key ] = self::set_additional_properties_to_false( $child_schema );
}
}

if ( isset( $schema['patternProperties'] ) ) {
foreach ( $schema['patternProperties'] as $key => $child_schema ) {
$schema['patternProperties'][ $key ] = self::set_additional_properties_to_false( $child_schema );
}
}

$schema['additionalProperties'] = false;
}

if ( in_array( 'array', $type, true ) ) {
if ( isset( $schema['items'] ) ) {
$schema['items'] = self::set_additional_properties_to_false( $schema['items'] );
}
}

return $schema;
}
}
163 changes: 144 additions & 19 deletions plugins/optimization-detective/class-od-url-metric.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,42 +47,42 @@
* @since 0.1.0
* @access private
*/
final class OD_URL_Metric implements JsonSerializable {
class OD_URL_Metric implements JsonSerializable {

/**
* Data.
*
* @var Data
*/
private $data;
protected $data;

/**
* Constructor.
*
* @phpstan-param Data|array<string, mixed> $data Valid data or invalid data (in which case an exception is thrown).
*
* @param array<string, mixed> $data URL metric data.
*
* @throws OD_Data_Validation_Exception When the input is invalid.
*
* @param array<string, mixed> $data URL metric data.
*/
public function __construct( array $data ) {
if ( ! isset( $data['uuid'] ) ) {
$data['uuid'] = wp_generate_uuid4();
}
$this->validate_data( $data );
$this->data = $data;
$this->data = $this->prepare_data( $data );
}

/**
* Validate data.
* Prepares data with validation and sanitization.
*
* @phpstan-assert Data $data
* @throws OD_Data_Validation_Exception When the input is invalid.
*
* @param array<string, mixed> $data Data to validate.
* @throws OD_Data_Validation_Exception When the input is invalid.
* @return Data Validated and sanitized data.
*/
private function validate_data( array $data ): void {
$valid = rest_validate_object_value_from_schema( $data, self::get_json_schema(), self::class );
private function prepare_data( array $data ): array {
$schema = static::get_json_schema();
$valid = rest_validate_object_value_from_schema( $data, $schema, self::class );
if ( is_wp_error( $valid ) ) {
throw new OD_Data_Validation_Exception( esc_html( $valid->get_error_message() ) );
}
Expand All @@ -105,6 +105,7 @@ private function validate_data( array $data ): void {
)
);
}
return rest_sanitize_value_from_schema( $data, $schema, self::class );
}

/**
Expand All @@ -116,12 +117,12 @@ private function validate_data( array $data ): void {
*/
public static function get_json_schema(): array {
/*
* The intersectionRect and clientBoundingRect are both instances of the DOMRectReadOnly, which
* The intersectionRect and boundingClientRect are both instances of the DOMRectReadOnly, which
* the following schema describes. See <https://developer.mozilla.org/en-US/docs/Web/API/DOMRectReadOnly>.
* Note that 'number' is used specifically instead of 'integer' because the values are all specified as
* floats/doubles.
*/
$properties = array_fill_keys(
$dom_rect_properties = array_fill_keys(
array(
'width',
'height',
Expand All @@ -139,17 +140,17 @@ public static function get_json_schema(): array {
);

// The spec allows these to be negative but this doesn't make sense in the context of intersectionRect and boundingClientRect.
$properties['width']['minimum'] = 0.0;
$properties['height']['minimum'] = 0.0;
$dom_rect_properties['width']['minimum'] = 0.0;
$dom_rect_properties['height']['minimum'] = 0.0;

$dom_rect_schema = array(
'type' => 'object',
'required' => true,
'properties' => $properties,
'properties' => $dom_rect_properties,
'additionalProperties' => false,
);

return array(
$schema = array(
'$schema' => 'http://json-schema.org/draft-04/schema#',
'title' => 'od-url-metric',
'type' => 'object',
Expand Down Expand Up @@ -225,12 +226,136 @@ public static function get_json_schema(): array {
'intersectionRect' => $dom_rect_schema,
'boundingClientRect' => $dom_rect_schema,
),
'additionalProperties' => false,

// Additional properties may be added to the schema for items of elements via the od_url_metric_schema_element_item_additional_properties filter.
// See further explanation below.
'additionalProperties' => true,
),
),
),
'additionalProperties' => false,
// Additional root properties may be added to the schema via the od_url_metric_schema_root_additional_properties filter.
// Therefore, additionalProperties is set to true so that additional properties defined in the extended schema may persist
// in a stored URL Metric even when the extension is deactivated. For REST API requests, the OD_Strict_URL_Metric
// which sets this to false so that newly-submitted URL Metrics only ever include the known properties.
'additionalProperties' => true,
felixarntz marked this conversation as resolved.
Show resolved Hide resolved
);

/**
* Filters additional schema properties which should be allowed at the root of a URL metric.
*
* @since n.e.x.t
*
* @param array<string, array{type: string}> $additional_properties Additional properties.
*/
$additional_properties = (array) apply_filters( 'od_url_metric_schema_root_additional_properties', array() );
if ( count( $additional_properties ) > 0 ) {
$schema['properties'] = self::extend_schema_with_optional_properties( $schema['properties'], $additional_properties, 'od_url_metric_schema_root_additional_properties' );
}

/**
* Filters additional schema properties which should be allowed for an elements item in a URL metric.
*
* @since n.e.x.t
*
* @param array<string, array{type: string}> $additional_properties Additional properties.
*/
$additional_properties = (array) apply_filters( 'od_url_metric_schema_element_item_additional_properties', array() );
if ( count( $additional_properties ) > 0 ) {
$schema['properties']['elements']['items']['properties'] = self::extend_schema_with_optional_properties(
$schema['properties']['elements']['items']['properties'],
$additional_properties,
'od_url_metric_schema_root_additional_properties'
);
}

return $schema;
}

/**
* Extends a schema with additional optional properties.
*
* @since n.e.x.t
*
* @param array<string, mixed> $properties_schema Properties schema to extend.
* @param array<string, mixed> $additional_properties Additional properties.
* @param string $filter_name Filter name used to extend.
*
* @return array<string, mixed> Extended schema.
*/
protected static function extend_schema_with_optional_properties( array $properties_schema, array $additional_properties, string $filter_name ): array {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole method is a bit complex with all the validation in it. Is there some existing WP function we could use for schema validation instead? Or does WP do this validation later on anyway?

I don't see similar validation in register_rest_field for instance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WordPress does have a _doing_it_wrong() check which emits:

Function rest_validate_value_from_schema was called incorrectly. The "type" schema keyword for OD_URL_Metric[timestamp] is required. (This message was added in version 5.5.0.)

But then it goes ahead and tries to access it anyway, resulting in a warning:

image

Maybe the validation is excessive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the validation here makes sense, specifically because some of those requirements we have here don't apply to the WordPress validation (for instance, we want to prohibit required, but that's only relevant here, not in general WordPress validation of REST properties).

$doing_it_wrong = static function ( string $message ) use ( $filter_name ): void {
_doing_it_wrong(
esc_html( "Filter: '{$filter_name}'" ),
esc_html( $message ),
'Optimization Detective n.e.x.t'
westonruter marked this conversation as resolved.
Show resolved Hide resolved
);
};
foreach ( $additional_properties as $property_key => $property_schema ) {
if ( ! is_array( $property_schema ) ) {
continue;
}
if ( isset( $properties_schema[ $property_key ] ) ) {
$doing_it_wrong(
sprintf(
/* translators: property name */
__( 'Disallowed override of existing schema property "%s".', 'optimization-detective' ),
$property_key
)
);
continue;
}
if ( ! isset( $property_schema['type'] ) || ! ( is_string( $property_schema['type'] ) || is_array( $property_schema['type'] ) ) ) {
$doing_it_wrong(
sprintf(
/* translators: 1: property name, 2: 'type' */
__( 'Supplied schema property "%1$s" with missing "%2$s" key.', 'optimization-detective' ),
'type',
$property_key
)
);
continue;
}
if ( isset( $property_schema['required'] ) && false !== $property_schema['required'] ) {
$doing_it_wrong(
sprintf(
/* translators: 1: property name, 2: 'required' */
__( 'Supplied schema property "%1$s" has a truthy value for "%2$s". All extended properties must be optional so that URL Metrics are not all immediately invalidated once an extension is deactivated.', 'optimization-detective' ),
$property_key,
'required'
)
);
}
$property_schema['required'] = false;

if ( isset( $property_schema['default'] ) ) {
$doing_it_wrong(
sprintf(
/* translators: 1: property name, 2: 'default' */
__( 'Supplied schema property "%1$s" has disallowed "%2$s" specified.', 'optimization-detective' ),
$property_key,
'default'
)
);
unset( $property_schema['default'] );
}

$properties_schema[ $property_key ] = $property_schema;
}
return $properties_schema;
}

/**
* Gets property value for an arbitrary key.
*
* This is particularly useful in conjunction with the `od_url_metric_schema_root_additional_properties` filter.
*
* @since n.e.x.t
*
* @param string $key Property.
* @return mixed|null The property value, or null if not set.
*/
public function get( string $key ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could make use of generics to specify the expected return type given the input $key.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not, as then attempting to access an extended property will result in a PHPStan error.

return $this->data[ $key ] ?? null;
}

/**
Expand Down
1 change: 1 addition & 0 deletions plugins/optimization-detective/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ static function ( string $version ): void {
require_once __DIR__ . '/class-od-data-validation-exception.php';
require_once __DIR__ . '/class-od-html-tag-processor.php';
require_once __DIR__ . '/class-od-url-metric.php';
require_once __DIR__ . '/class-od-strict-url-metric.php';
require_once __DIR__ . '/class-od-url-metrics-group.php';
require_once __DIR__ . '/class-od-url-metrics-group-collection.php';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ static function ( $url_metric_data ) use ( $trigger_error ) {
* Stores URL metric by merging it with the other URL metrics which share the same normalized query vars.
*
* @since 0.1.0
* @todo There is duplicate logic here with od_handle_rest_request().
*
* @param string $slug Slug (hash of normalized query vars).
* @param OD_URL_Metric $new_url_metric New URL metric.
Expand Down
Loading