Update validation for analytics shared requests #5727

nfmohit · 2022-08-23T20:44:32Z

Summary

Addresses issue:

Limit shared requests for Analytics reports to metrics and dimensions used by the Site Kit dashboard #5711

Relevant technical choices

This is a follow-up PR addressing this code review.

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 4.7 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

aaemnnosttv

Thanks @nfmohit - this looks great, just a few minor points to address 👍

includes/Core/Modules/Module.php

includes/Core/REST_API/Exception/Invalid_Dimensions_Exception.php

aaemnnosttv

One more thing 😄

includes/Modules/Analytics.php

nfmohit · 2022-08-23T22:16:26Z

Thank you for the kind reviews @aaemnnosttv! I've addressed them.

aaemnnosttv

Thanks @nfmohit ! I think we were a bit ambitious with the addition of the exceptions here and after giving it another look, I think it was a bit simpler before and while I still think they are good to use, we can aim for a bit of a middle ground here.

Apologies for the iterations – I've gone ahead and made the final adjustments (mostly undoing some of the additions here) to wrap this up for the release, but see my comments below.

aaemnnosttv · 2022-08-24T18:07:31Z

includes/Core/Validation/Exception/Invalid_Dimensions_Exception.php

+	 * @param array     $dimensions   Optional. Dimensions that are invalid.
+	 */
+	public function __construct( $message = '', $code = 0, $previous = null, $dimensions = array() ) {
+		if ( ! isset( $message ) || empty( $message ) ) {


Using isset here is redundant as empty uses it internally.

Suggested change

if ( ! isset( $message ) || empty( $message ) ) {

if ( empty( $message ) ) {

aaemnnosttv · 2022-08-24T18:51:16Z

includes/Core/Validation/Exception/Invalid_Dimensions_Exception.php

+			$message = sprintf(
+				/* translators: %s is replaced with a comma separated list of the invalid dimensions. */
+				_n(
+					'Unsupported dimension requested: %s',
+					'Unsupported dimensions requested: %s',
+					count( $dimensions ),
+					'google-site-kit'
+				),
+				join(
+					/* translators: used between list items, there is a space after the comma. */
+					__( ', ', 'google-site-kit' ),
+					$dimensions
+				)
+			);


Looking at this again, none of our other exceptions handle the message internally. I think it makes sense to leave this with the consumer for now.

aaemnnosttv · 2022-08-24T18:55:19Z

includes/Core/Validation/Exception/Invalid_Dimensions_Exception.php

+	public function to_wp_error() {
+		return new WP_Error(
+			static::WP_ERROR_CODE,
+			$this->getMessage(),
+			array(
+				'status' => 400, // Bad request.
+			)
+		);
+	}


I realize this was based on my initial thoughts but after seeing it in use here, I don't think it's right for this use case.

I think the WP Error is best returned from the module class, which can then be module-specific. It also seems a bit improper to dictate the status code here – especially as a 400 which indicates that the request is not valid as given which is not really accurate in this case as the validation is a sharing specific detail. The request wouldn't be any different and not throw if the module wasn't shared.

aaemnnosttv · 2022-08-24T18:56:40Z

includes/Modules/Analytics.php

-								array( 'status' => 400 )
-							);
-						}
+						$this->validate_report_dimensions( $dimensions );


I think this was mostly correct before. The only change to really make is adding a try/catch around the validation and returning the appropriate WP_Error.

aaemnnosttv · 2022-08-24T18:58:33Z

includes/Modules/Analytics.php

-							return new WP_Error(
-								'invalid_dimensions',
-								$invalid_dimensions_error_message,
-								array( 'status' => 400 )


As mentioned before, 400 isn't really the proper status here. We can keep this as the default 500 as this is more of an "internal server" error than something wrong with the request itself, even though it is directly related to the requested data. The only reason it's an error is due to the internal server state related to sharing.

aaemnnosttv · 2022-08-24T19:00:01Z

includes/Modules/Analytics.php

-				),
-				implode( ', ', $invalid_metrics )
-			);
+			throw new Invalid_Metrics_Exception( '', 0, null, $invalid_metrics );


As mentioned before, let's give the message to the exception rather than have it generate its own. This makes the instantiation much simpler as well as we don't need to provide it with the metrics (although maybe this would make sense to do in the future), it's not needed yet.

nfmohit · 2022-08-25T08:25:47Z

Thanks @nfmohit ! I think we were a bit ambitious with the addition of the exceptions here and after giving it another look, I think it was a bit simpler before and while I still think they are good to use, we can aim for a bit of a middle ground here.

Apologies for the iterations – I've gone ahead and made the final adjustments (mostly undoing some of the additions here) to wrap this up for the release, but see my comments below.

I've looked at all the mentioned suggestions, the implemented changes, and I agree with all of them. Thank you very much for showing me the right track here and implementing the appropriate changes, @aaemnnosttv!

nfmohit added 3 commits August 24, 2022 02:34

Add exception classes for invalid metrics and dimensions.

7339288

Update validation methods to use new exception classes.

ed28b0e

Change shared credentials usage property only for the current request.

e1d9a2b

nfmohit changed the base branch from develop to main August 23, 2022 20:45

nfmohit mentioned this pull request Aug 23, 2022

Limit analytics shared requests #5721

Merged

18 tasks

aaemnnosttv requested changes Aug 23, 2022

View reviewed changes

includes/Core/Modules/Module.php Outdated Show resolved Hide resolved

includes/Core/REST_API/Exception/Invalid_Dimensions_Exception.php Outdated Show resolved Hide resolved

includes/Core/REST_API/Exception/Invalid_Dimensions_Exception.php Outdated Show resolved Hide resolved

nfmohit added 2 commits August 24, 2022 03:35

Update comment to be more useful.

f0d5cd0

Change namespaces for the new exception classes.

97474ec

aaemnnosttv reviewed Aug 23, 2022

View reviewed changes

includes/Modules/Analytics.php Outdated Show resolved Hide resolved

includes/Modules/Analytics.php Outdated Show resolved Hide resolved

Update params to expect an array of the appropriate instance.

d935f65

nfmohit requested a review from aaemnnosttv August 23, 2022 22:16

Refactor validation exceptions.

89771bf

aaemnnosttv reviewed Aug 24, 2022

View reviewed changes

aaemnnosttv added 2 commits August 24, 2022 15:08

Fix dimensions referenced in exception.

a89ee09

Fix docblocks for exceptions.

dbd3d9e

aaemnnosttv approved these changes Aug 24, 2022

View reviewed changes

aaemnnosttv merged commit 511042e into main Aug 24, 2022

aaemnnosttv deleted the enhancement/#5711-update-validation branch August 24, 2022 19:33

aaemnnosttv mentioned this pull request Aug 24, 2022

Limit shared requests for AdSense reports to metrics and dimensions used by the Site Kit dashboard. #5726

Merged

18 tasks

felixarntz mentioned this pull request Aug 25, 2022

Limit shared requests for Analytics reports to metrics and dimensions used by the Site Kit dashboard #5711

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update validation for analytics shared requests #5727

Update validation for analytics shared requests #5727

nfmohit commented Aug 23, 2022

aaemnnosttv left a comment

aaemnnosttv left a comment

nfmohit commented Aug 23, 2022

aaemnnosttv left a comment

aaemnnosttv Aug 24, 2022

aaemnnosttv Aug 24, 2022

aaemnnosttv Aug 24, 2022

aaemnnosttv Aug 24, 2022

aaemnnosttv Aug 24, 2022

aaemnnosttv Aug 24, 2022

nfmohit commented Aug 25, 2022

	if ( ! isset( $message ) \|\| empty( $message ) ) {
	if ( empty( $message ) ) {

Update validation for analytics shared requests #5727

Update validation for analytics shared requests #5727

Conversation

nfmohit commented Aug 23, 2022

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

aaemnnosttv left a comment

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

nfmohit commented Aug 23, 2022

aaemnnosttv left a comment

Choose a reason for hiding this comment

aaemnnosttv Aug 24, 2022

Choose a reason for hiding this comment

aaemnnosttv Aug 24, 2022

Choose a reason for hiding this comment

aaemnnosttv Aug 24, 2022

Choose a reason for hiding this comment

aaemnnosttv Aug 24, 2022

Choose a reason for hiding this comment

aaemnnosttv Aug 24, 2022

Choose a reason for hiding this comment

aaemnnosttv Aug 24, 2022

Choose a reason for hiding this comment

nfmohit commented Aug 25, 2022