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

Jump to bottom

JSON schema and validation #182

Merged
merged 21 commits into from
Nov 15, 2023
Merged

JSON schema and validation #182

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6e2084b
JSON schema and validation
humitos Nov 8, 2023
cb9dcad
JSON Schema validator for analytics addon
humitos Nov 9, 2023
3c967a7
JSON validator for EthicalAd addon
humitos Nov 9, 2023
1b70b14
JSON schema for Notifications addon
humitos Nov 9, 2023
27990c8
lint and build
humitos Nov 9, 2023
21d9d36
Cleanup
humitos Nov 9, 2023
8aedcad
Define all keywords are required
humitos Nov 9, 2023
2f49788
Required keys for EthicalAds
humitos Nov 9, 2023
6d33c2e
Update `$id` URIs to faked ones
humitos Nov 10, 2023
14a3b87
Use data-validation on DocDiff addon
humitos Nov 10, 2023
6b16309
Use JSON validation for flyout addon
humitos Nov 13, 2023
27f44ec
Use JSON validation for Hotkeys Addon
humitos Nov 13, 2023
f49acde
JSON validation for Search Addon
humitos Nov 13, 2023
e02a144
npm run build
humitos Nov 13, 2023
1304369
Merge branch 'main' of github.com:readthedocs/readthedocs-client into…
humitos Nov 13, 2023
cee5add
Dependencies
humitos Nov 13, 2023
e5217aa
Merge branch 'main' of github.com:readthedocs/readthedocs-client into…
humitos Nov 14, 2023
3ceb594
Split the pattern into `isEnabled` and `isConfigValid`
humitos Nov 14, 2023
edcfd08
Update validation data
humitos Nov 14, 2023
e135e8b
Update tests and data validation
humitos Nov 14, 2023
7584f53
All tests passing using the new data validation
humitos Nov 14, 2023
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
Prev Previous commit
Next Next commit
JSON Schema validator for analytics addon
  • Loading branch information
@humitos
humitos committed Nov 9, 2023
commit cb9dcadd6a84f1f14dbdf50cb1ee9fe0fccfc49a
6 changes: 5 additions & 1 deletion src/analytics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { ajv } from "./data-validation";
import { AddonBase } from "./utils";
import { CLIENT_VERSION } from "./utils";

Expand Down Expand Up @@ -86,6 +87,9 @@ export class AnalyticsAddon extends AddonBase {
}

static isEnabled(config) {
return config.addons && config.addons.analytics.enabled === true;
const validate = ajv.getSchema(
"https://readthedocs.org/schemas/addons.analytics.json",
humitos marked this conversation as resolved.
Show resolved Hide resolved
);
return validate(config) && config.addons.analytics.enabled === true;
}
}
52 changes: 48 additions & 4 deletions src/data-validation.js
View file
Open in desktop
humitos marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,59 @@ const addons = {
additionalProperties: true,
};

// Validator for Analytics Addon
const addons_analytics = {
$id: "https://readthedocs.org/schemas/addons.analytics.json",
type: "object",
properties: {
code: { type: "string" },
enabled: { type: "boolean" },
addons: {
type: "object",
properties: {
analytics: {
type: "object",
properties: {
code: { type: ["string", "null"] },
enabled: { type: "boolean" },
},
},
},
},
projects: {
type: "object",
properties: {
current: {
type: "object",
properties: {
slug: { type: "string" },
// Optional
language: {
type: "object",
properties: {
code: { type: "string" },
},
},
programming_language: {
type: "object",
properties: {
code: { type: "string" },
},
},
},
},
},
},
versions: {
type: "object",
properties: {
current: {
type: "object",
properties: {
slug: { type: "string" },
},
},
},
},
},
required: ["enabled"],
additionalProperties: false,
};

const addons_ethicalads = {
Expand Down
8 changes: 0 additions & 8 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import * as docdiff from "./docdiff";
import * as flyout from "./flyout";
import * as ethicalads from "./ethicalads";
import * as hotkeys from "./hotkeys";
import { ajv } from "./data-validation";
import { domReady, isReadTheDocsEmbedPresent } from "./utils";

export function setup() {
Expand Down Expand Up @@ -39,13 +38,6 @@ export function setup() {
return getReadTheDocsConfig(sendUrlParam);
})
.then((config) => {
const validate = ajv.getSchema(
"https://readthedocs.org/schemas/response.json",
);
if (!validate(config)) {
throw new Error("JSON response does not validate.");
}

let promises = [];

// IS_PRODUCTION comes from Webpack and is undeclared otherwise
Expand Down