Detect malformed REST responses and display a specific error/help page link

tofumatt commented 2 months ago

Feature Description

Based on #8442, we should detect specific malformed WP REST API JSON responses and display a bespoke error message for them with links to a help document that addresses those errors.

The help page should reference plugin(s) (like Transliterator plugin in #8442) that are known to cause these errors.

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

Acceptance criteria

Any malformed JSON response from the WP REST API (at least for Site Kit endpoints) should be detected by our API client/fetch store and raise a specific error.
- The ability to retry the request should be maintained.
- The message should warn the user that an unexpected response was received from WordPress.
- There should be a link to learn more on a Site Kit documentation page (TBD) that explains what might cause this issue and fixes.

Implementation Brief

[ ] Update assets/js/googlesitekit/data/create-fetch-store.js, adding an optional prop, validateResponse, which defaults to an empty function that simply passes through all props unchanged.
- [ ] Update the RECEIVE reducer case to call validateResponse before calling reducerCallback.
- [ ] Update assets/js/modules/analytics-4/datastore/report.js, create and new REPORT_SCHEMA object like so:
```
rowCount: {
type: "string"
},
...
rows: {
type: Array.isArray,
items: {
    type: "object",
}
}
...
}
```


 * [ ] Pass a new function to the `validateResponse`, `createFetchStore`:
    * [ ]  In this function, recursively loop over the `REPORT_SCHEMA` object and any items children of the first dimension objects. For each type definition, if `type` is a string, use `typeof` to check to match that key in the returned report data, if `type` is a function run this function on that key in the returned report data (this is to allow for Array vs object types which is the crux of the Transliterator issue).
    * [ ] If any items of the report do not conform to the defined schema, dispatch `receiveError` with a malformed REST response error.
    * [ ] Confirm this is handled and displayed correctly in each key analytics report widget.

### Test Coverage

* Create new tests for the new `validateResponse` prop in `assets/js/googlesitekit/data/create-fetch-store.test.js`

## QA Brief

* <!-- One or more bullet points for how to test that the feature works as expected. -->

## Changelog entry

* <!-- One sentence summarizing the PR, to be used in the changelog. -->

binnieshah commented 1 month ago

@jamesozzie @tofumatt please could you confirm next steps on this one?

jamesozzie commented 1 month ago

@binnieshah I have a documentation task for this which I'll update today and send to @adamdunnage for review.

@tofumatt Do you know the error code for this, so I can add to the mapping sheet?

benbowler commented 1 week ago

Hey @tofumatt I've written the IB here, the key challenge here is that we don't have a schema definition consistently for each REST API. The closest we have for some APIs in on the PHP side:

https://github.com/google/site-kit-wp/blob/41f0bbaec9736ba334cccc7b469ce1a5e2cdfc71/includes/Core/Consent_Mode/REST_Consent_Mode_Controller.php#L124-L148

To roll this out across all of our APIs will require defining the schema across all of these APIs in a reliable way. In the IB I propose an approach for this and suggest starting with the getReport request as it is the most used API as well as the one that it effected by the issue caused by the Transliterator plugin. As we apply this to different stores we could extract the logic that compares the schema to the response into a reusable helper to use in other stores.

eugene-manuilov commented 5 days ago

@benbowler, i think the main idea here is not to verify responses to match certain schemas but to detect when an API request returns a response that is not a JSON object. In other words, if the request fails and returns non-JSON response, we should craft a special error and dispatch it in the catch section: https://github.com/google/site-kit-wp/blob/45ca3ddd422f59f2ae0250c038783887c913d8c7/assets/js/googlesitekit/api/index.js#L183-L209

benbowler commented 5 days ago

@eugene-manuilov well, in the issue that caused this ticket to be created (#8442) the JSON returned from the API was still valid JSON, but the expected JSON format for the report had been modified by the Transliterator plugin. Specifically, arrays had been restructured to numerically indexed objects in the response.

eugene-manuilov commented 4 days ago

@benbowler, ah.. thanks for clarifying that, i didn't notice that.

Update assets/js/googlesitekit/data/create-fetch-store.js, adding an optional prop, validateResponse, which defaults to an empty function that simply passes through all props unchanged.

Let's probably just add a new property schema to createFetchStore arguments instead of validateResponse. If a schema is passed the fetch store should automatically verify it.

google / site-kit-wp