JSON schema and validation #182

humitos · 2023-11-08T16:39:01Z

Define a JSON schema and use ajv to load/compile it. Then, use this schema to validate the response returned by the API. If the response does not validate the schema we throw an exception and don't load any of the addons.

I plan to modify this to validate each part of the response by the addon itself and disable a particular addon if the validation fails. This will allow having a broken addon without interfering with the rest.

Related #149

Define a JSON schema and use `ajv` to load/compile it. Then, use this schema to validate the response returned by the API. If the response does not validate the schema we throw an exception and don't load any of the addons. I plan to modify this to validate each part of the response by the addon itself and disable a particular addon if the validation fails. This will allow having a broken addon without interfering with the rest. Related #149

zanderle

I love this approach. I've used it in the past and it's a great way to keep backend and frontend in sync. It's also a great way to write some tests to keep checking if the API response conforms to the schema or not.

zanderle · 2023-11-08T17:00:31Z

src/index.js

+ "https://readthedocs.org/schemas/response.json",
+ );
+ if (!validate(config)) {
+ throw new Error("JSON response does not validate.");


Should we just kill all the addons if it doesn't validate? I'm thinking if we ever change the schema: there might be some discrepancies - should we maybe let the addons resolve as much as possible anyway? Or maybe validate per add-on?

Or maybe validate per add-on?

Yes. This is my end goal. I want to be able to enable addons if the data required for that addon is validated, even if there are parts of the data returned by the API that's corrupted.

In fact, I don't think we will end up validating that data returned by the API "as a whole" since we may don't care about all the attributes of it.

Following this approach, what I have in mind for analytics addons, for example, it will validate:

required projects.current.slug (string)

required versions.current.slug (string)

optional projects.current.language.code (string)

optional projects.current.programming_language.code (string)

optional readthedocs.analytics.code (string)`

To be able to enable the analytics addon, we don't care if builds.current.slug is invalid, for example, or any other field outside of the ones mentioned here 👍🏼

I added two commits with this approach:

Analytics: cb9dcad

EthicalAds: 3c967a7

I will continue with this idea because I like it the most so far.

It also changes the pattern used insider `render()` method. The `config` object is assigned to the addon _only if_ it validates via `isEnabled()` static method. I think this pattern is better since we don't have to validate the `config` object on each render of the addon.

This way we ensure that everything we need from the addons is present.

humitos · 2023-11-09T18:45:39Z

I updated this PR and I'm happier with the approach I'm proposing here. I will continue working on this PR to apply this approach to all the addons. I will also take a look at the testing suite to check how the approach works together with that.

agjohnson

This approach is looking great so far and per-addon validation seemingly makes the most sense so far.

src/analytics.js

src/data-validation.js

humitos · 2023-11-10T15:44:09Z

I'm getting this error when running tests and I'm not able to trace it down. @agjohnson I'd appreciate if you have a clue here that can help me to debug this better.

      TypeError: (0 , applicator_1.default) is not a function
        at node_modules/ajv/dist/vocabularies/draft7.js:19:30

I found this issue ajv-validator/ajv#2306

humitos · 2023-11-10T16:17:30Z

I understand this issue is because of UMD and ES modules, and I understand we are doing some type of module's type conversion when running the tests via rollup, but I'm not sure to understand all of this 🙃

humitos · 2023-11-10T16:26:27Z

It seems the key issue because of the Rollup plugin is doing something that may not be 100% accurate. Commenting this lines https://github.com/readthedocs/readthedocs-client/blob/14a3b874d7f067b8fd69bc451ff0d957459cdb94/web-test-runner.config.mjs#L24-L27 I get a different error:

SyntaxError: The requested module './../node_modules/ajv/dist/ajv.js' does not provide an export named 'default'

… humitos/json-schema-validator

humitos · 2023-11-13T13:21:51Z

I finished this PR regarding the JSON schema and validation. I'm happy with the current pattern. It's strict enough to help us find issues with invalid data and keeping the other addons sane when the data is invalid only for one addon.

I'd like to get some help on making the test suite to run properly, without adding more tests for now, but adapting the existent ones to follow the current approach. I will add tests in a following PR after merging this one.

agjohnson · 2023-11-13T18:59:17Z

So, the reason why these type of errors come up is generally because of cyclical exports in the library. For the most part, you can get around unclear exports by using:

import { default as Ajv } from "ajv";

However, in this case, this doesn't quite solve the issue as the exports from the built bundle are a little off already. This is from built bundle:

...
module.exports = exports = Ajv;
Object.defineProperty(exports, "__esModule", { value: true });
exports.default = Ajv;
...

This is the start of the cyclical export that Rollup and the commonjs plugin seem to have trouble with.

This probably doesn't help explain the solution too much yet, and is mostly a review of what I found setting up WTR. I'll experiment a bit more first, but we should be able to resolve this on our side.

agjohnson · 2023-11-14T00:17:18Z

I took another swing at this. See #185 for the fix to WTR config to make our configuration play a bit nicer with both ajv and visual-dom-diff.

Ultimately, I found the issue because most of the require calls in ajv were expecting the return to include the whole module.exports -- that is your error above, the expected return from the require() needed to be:

applicator_1 = {default: getApplicator}

Instead, with Rollup's commonjs plugin, and the preferred mode to requireReturnsDefault, the return was:

applicator_1 = getApplicator

Specifically, the module exports were squashed because the intention would never to use applicator_1.default directly.

I got new test failures, but they look like exactly what I would expect. The new failures are because we are only passing in partial configuration objects and with your change we can no longer do that.

Preferred default export mode is not required anymore, and this plays nicely with other TS libraries. Refs #182

… humitos/json-schema-validator

Re-use these static methods across all the addons by simply defining some variables in the addons class.

humitos · 2023-11-14T09:22:46Z

This PR is ready to review and merge. I'm happy with the pattern and I'm feeling more comfortable with it. There are things we will want to refactor in the near future, but as I said before, I wanted to be pretty explicit at this point since it's pretty easy to make mistakes with the JSON schema.

In the meanwhile, I will open another PR based on this one to write some extra test cases 👍🏼

agjohnson

This all looks great and it all feels easier to work with now 👍

agjohnson · 2023-11-14T17:39:01Z

src/data-validation.js

Just a note here, I didn't inspect the actual schemas here deeply. I think we can adjust if they don't match what we are outputting from the application and don't need to be strict about ensuring they are strictly accurate yet.

Are we planning to add some backend tests to see if the API response conforms to the schema(s)?

@agjohnson

Just a note here, I didn't inspect the actual schemas here deeply. I think we can adjust if they don't match what we are outputting from the application and don't need to be strict about ensuring they are strictly accurate yet.

Yes. Currently, they are "good enough" but not perfect. I've been finding issues while writing the test cases, so I expect to find more as we keep working on the addons. We already have the pattern defined, so updating them shouldn't be too hard

@zanderle

Are we planning to add some backend tests to see if the API response conforms to the schema(s)?

This is something that I'd eventually want but we can't prioritize right now. We will need to find a way to share the JSON schema between the two repositories and execute the tests in both platforms with the same schema. "Sharing the schema" probably means writing the schema in JSON instead of JS that's something I tried to avoid because JSON doesn't support comments among others painful things, like not supporting trailing comma 🙃

Do you have experience doing this?

The way I did it in the past was to have the schema in json and then use description and $comment keywords to provide context where needed.

It's been a few years since I did this, so I can't remember how exactly I shared it between the frontend and backend repos. Let me try to find that information actually.

humitos mentioned this pull request Nov 8, 2023

Follow pattern to handle exceptions (objectPath) #149

Closed

zanderle reviewed Nov 8, 2023

View reviewed changes

humitos added 7 commits November 9, 2023 18:44

JSON Schema validator for analytics addon

cb9dcad

JSON validator for EthicalAd addon

3c967a7

lint and build

27990c8

Cleanup

21d9d36

Define all keywords are required

8aedcad

This way we ensure that everything we need from the addons is present.

Required keys for EthicalAds

2f49788

agjohnson reviewed Nov 9, 2023

View reviewed changes

src/analytics.js Outdated Show resolved Hide resolved

src/data-validation.js Outdated Show resolved Hide resolved

humitos added 2 commits November 10, 2023 11:39

Update $id URIs to faked ones

6d33c2e

Use data-validation on DocDiff addon

14a3b87

humitos mentioned this pull request Nov 10, 2023

Type Error: applicator is not a function ajv-validator/ajv#2306

Closed

humitos added 6 commits November 13, 2023 13:42

Use JSON validation for flyout addon

6b16309

Use JSON validation for Hotkeys Addon

27f44ec

JSON validation for Search Addon

f49acde

npm run build

e02a144

Merge branch 'main' of github.com:readthedocs/readthedocs-client into…

1304369

… humitos/json-schema-validator

Dependencies

cee5add

humitos marked this pull request as ready for review November 13, 2023 13:22

humitos requested a review from a team as a code owner November 13, 2023 13:22

humitos requested a review from agjohnson November 13, 2023 13:22

agjohnson mentioned this pull request Nov 14, 2023

Alter the configuration around externals and default exports #185

Merged

humitos pushed a commit that referenced this pull request Nov 14, 2023

Alter the configuration around externals and default exports (#185)

7f1cbc3

Preferred default export mode is not required anymore, and this plays nicely with other TS libraries. Refs #182

humitos added 5 commits November 14, 2023 08:52

Merge branch 'main' of github.com:readthedocs/readthedocs-client into…

e5217aa

… humitos/json-schema-validator

Split the pattern into isEnabled and isConfigValid

3ceb594

Re-use these static methods across all the addons by simply defining some variables in the addons class.

Update validation data

edcfd08

Update tests and data validation

e135e8b

All tests passing using the new data validation

7584f53

agjohnson approved these changes Nov 14, 2023

View reviewed changes

humitos merged commit 8be915c into main Nov 15, 2023
4 checks passed

humitos deleted the humitos/json-schema-validator branch November 15, 2023 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON schema and validation #182

JSON schema and validation #182

humitos commented Nov 8, 2023

zanderle left a comment

zanderle Nov 8, 2023

humitos Nov 9, 2023

humitos Nov 9, 2023

humitos Nov 9, 2023

humitos commented Nov 9, 2023

agjohnson left a comment

humitos commented Nov 10, 2023 •

edited

Loading

humitos commented Nov 10, 2023

humitos commented Nov 10, 2023

humitos commented Nov 13, 2023

agjohnson commented Nov 13, 2023

agjohnson commented Nov 14, 2023

humitos commented Nov 14, 2023

agjohnson left a comment

agjohnson Nov 14, 2023

zanderle Nov 14, 2023

humitos Nov 15, 2023

zanderle Nov 15, 2023

JSON schema and validation #182

JSON schema and validation #182

Conversation

humitos commented Nov 8, 2023

zanderle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Nov 9, 2023

agjohnson left a comment

Choose a reason for hiding this comment

humitos commented Nov 10, 2023 • edited Loading

humitos commented Nov 10, 2023

humitos commented Nov 10, 2023

humitos commented Nov 13, 2023

agjohnson commented Nov 13, 2023

agjohnson commented Nov 14, 2023

humitos commented Nov 14, 2023

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Nov 10, 2023 •

edited

Loading