#3059: switch RJSF validator to cfworker/jsonschema #7081

twschiller · 2023-12-09T00:47:48Z

What does this PR do?

Closes Support RJSF Schema validation in strict CSP #3059
@fregante @grahamlangford I started this draft PR just to see the t-shirt size of how much work it will be to ensure there aren't regressions when switching from AJV6 to cfworker/jsonschema
Includes the RJSF validator test suite for AJV6 for us to identify discrepancies
Drops AJV6 dependency

Remaining Work

Finish parity with AJV6 (see failing validator tests)
- Fix handling of rootSchema in isValid -- I don't understand the intended behavior yet
- cfworker/jsonschema reports some extra error paths. Need to determine if that's OK in the context of RJSF or if they need to get filtered out
- Decide if we want to rewrite messages for required properties
  - After: #/required Instance does not have required property \"pass2\".
  - Before: .pass2 is a required property
- Decide if we want to rewrite message for type errors
  - After: Instance type \"number\" is invalid. Expected \"string\"
Figure out why if you mark a dropdown field as required when E2E via the Page Editor, the form can be submitted without selecting something from the dropdown (that could be a Form Builder issue, e.g., it's not marking the properly as required properly)

Reviewer Tips

GitHub might not show the validator test file because it's long. I copied it from the RJSF test suite for AJV6 so we can benchmark cfworker/jsonschema

Discussion

Testing in E2E is sort of tricky because the HTML widgets enforce format before submission (e.g., for email)
I decided to use AJV6 as the baseline even though it's deprecated in order to reduce the behavioral changes. We might want to use AJV8 instead
RJSF Validator type uses any a lot. So we end up needing to use it. (I think that's OK, as the validator has full test coverage)

Demo

Note: Under normal circumstances, the html5 validation will display errors instead of RJSF. I disabled the html5 validation to fallback to the schema validation

Future Work

Upstream to RJSF or put as a separate NPM package

Checklist

Add tests
New files added to src/tsconfig.strictNullChecks.json (if possible)
Designate a primary reviewer

src/validators/formValidator.ts

src/validators/formValidator.test.ts

…feature/3059-rjsf-cfworker

src/validators/formValidator.test.ts

codecov · 2023-12-15T21:58:01Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (933427f) 71.14% compared to head (579084f) 71.18%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
src/validators/formValidator.ts	87.32%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7081      +/-   ##
==========================================
+ Coverage   71.14%   71.18%   +0.03%     
==========================================
  Files        1213     1214       +1     
  Lines       37613    37687      +74     
  Branches     7075     7088      +13     
==========================================
+ Hits        26760    26826      +66     
- Misses      10853    10861       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

twschiller · 2023-12-20T00:41:39Z

src/bricks/renderers/CustomFormComponent.tsx

+              // Deep clone the schema because otherwise the schema is not extensible
+              // This breaks validation when @cfworker/json-schema dereferences the schema
+              // See https://github.com/cfworker/cfworker/blob/263260ea661b6f8388116db7b8daa859e0d28b25/packages/json-schema/src/dereference.ts#L115
+              schema={cloneDeep(schema)}


👍 Does the cloned schema need to be memoized?

I started off doing that, but I haven't had any issues with performance or mounting/unmounting. I know with formik and react-hook-form, the component won't remount/reset unless the key changes. I think we have the same situation here.

twschiller · 2023-12-20T17:45:59Z

BLoe

Would like to see some cleanup of the comments in the isValid function, do we need to make further changes in this PR? Are the comments needed for the future? Just seems a bit uncertain what's going on there, when reading the code/comments from an outside perspective.

src/bricks/transformers/ephemeralForm/EphemeralForm.tsx

src/components/formBuilder/preview/FormPreview.tsx

BLoe · 2023-12-20T18:33:21Z

src/validators/formValidator.ts

+    // XXX: technically we may want to read from $schema to determine which draft version to validate against
+    // Draft 7 is the default for RJSF: https://rjsf-team.github.io/react-jsonschema-form/docs/usage/validation/#custom-meta-schema-validation
+
+    if (!this.isValidSchema(schema)) {
+      return false;
+    }
+
+    // FIXME: figure out how to add the root schema to the validator
+    const validator = new Validator(
+      withIdRefPrefix(schema) as Schema,
+      // The default used by RJSF: https://rjsf-team.github.io/react-jsonschema-form/docs/usage/validation/#custom-meta-schema-validation
+      "7",
+      true,
+    );
+    // This breaks due to duplicate ids: validator.addSchema(rootSchema as Schema);


Is there anything that needs to be done here with this series of XXX / FIXME / "This breaks" comments? Do we need to leave all of these in here?

This one here is the main reason I didn't approve the PR yet, fyi

Good catch - this is a feature that I'm not sure if we need or not, or even what the feature does

At one point I confirmed that this is called, but I'm still unclear what it's doing. Every test case I could come up with is passing. I'll remove the comment.

Co-authored-by: Ben Loe <below413@gmail.com>

github-actions · 2023-12-21T15:28:16Z

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

[WIP]: switch form validator to cfworker

c91a3b3

twschiller changed the title ~~#3059: [WIP] switch form validator to cfworker~~ #3059: [WIP] switch form validator to cfworker/jsonschema Dec 9, 2023

twschiller assigned fregante Dec 9, 2023

twschiller added the mv3 label Dec 9, 2023

twschiller commented Dec 9, 2023

View reviewed changes

src/validators/formValidator.ts Outdated Show resolved Hide resolved

twschiller commented Dec 9, 2023

View reviewed changes

src/validators/formValidator.ts Outdated Show resolved Hide resolved

Default to draft 7

250fe42

twschiller commented Dec 9, 2023

View reviewed changes

src/validators/formValidator.ts Outdated Show resolved Hide resolved

vercel bot deployed to Preview December 9, 2023 01:05 View deployment

twschiller commented Dec 9, 2023

View reviewed changes

src/validators/formValidator.test.ts Outdated Show resolved Hide resolved

twschiller changed the title ~~#3059: [WIP] switch form validator to cfworker/jsonschema~~ #3059: [WIP] switch RJSF validator to cfworker/jsonschema Dec 9, 2023

fregante mentioned this pull request Dec 10, 2023

Support RJSF Schema validation in strict CSP #3059

Closed

grahamlangford assigned grahamlangford and unassigned fregante Dec 11, 2023

grahamlangford added 2 commits December 15, 2023 09:18

merge main

011a7cb

update package-lock

233aff2

vercel bot deployed to Preview December 15, 2023 15:23 View deployment

wip

13633cb

vercel bot deployed to Preview December 15, 2023 18:54 View deployment

Merge branch 'main' of github.com:pixiebrix/pixiebrix-extension into …

696aa7b

…feature/3059-rjsf-cfworker

vercel bot deployed to Preview December 15, 2023 19:17 View deployment

wip

9377e4e

vercel bot deployed to Preview December 15, 2023 21:19 View deployment

wip

408cd36

vercel bot deployed to Preview December 15, 2023 21:52 View deployment

grahamlangford reviewed Dec 15, 2023

View reviewed changes

src/validators/formValidator.test.ts Outdated Show resolved Hide resolved

code cleanup

a771c4c

vercel bot deployed to Preview December 15, 2023 22:01 View deployment

more code cleanup

018c23c

vercel bot deployed to Preview December 19, 2023 15:26 View deployment

fixes schema is not extensible issue

e3d40e7

vercel bot deployed to Preview December 19, 2023 21:26 View deployment

grahamlangford changed the title ~~#3059: [WIP] switch RJSF validator to cfworker/jsonschema~~ #3059: switch RJSF validator to cfworker/jsonschema Dec 19, 2023

twschiller commented Dec 20, 2023

View reviewed changes

Merge branch 'main' into feature/3059-rjsf-cfworker

7268b27

vercel bot deployed to Preview December 20, 2023 13:33 View deployment

cloneDeep schema for other RJSF forms

84f0541

vercel bot deployed to Preview December 20, 2023 13:45 View deployment

grahamlangford marked this pull request as ready for review December 20, 2023 14:34

grahamlangford requested review from fregante and BLoe December 20, 2023 14:34

BLoe reviewed Dec 20, 2023

View reviewed changes

grahamlangford and others added 2 commits December 20, 2023 15:31

Update src/bricks/transformers/ephemeralForm/EphemeralForm.tsx

e07e8da

Co-authored-by: Ben Loe <below413@gmail.com>

Update src/components/formBuilder/preview/FormPreview.tsx

b870ba2

Co-authored-by: Ben Loe <below413@gmail.com>

vercel bot deployed to Preview December 20, 2023 21:34 View deployment

remove unnecessary comments

5d4637b

vercel bot deployed to Preview December 20, 2023 21:39 View deployment

removes unused method args

19a835e

vercel bot deployed to Preview December 20, 2023 21:41 View deployment

removes unnecessary comments

579084f

vercel bot deployed to Preview December 21, 2023 15:18 View deployment

BLoe approved these changes Dec 21, 2023

View reviewed changes

grahamlangford merged commit 180a749 into main Dec 21, 2023
14 checks passed

grahamlangford deleted the feature/3059-rjsf-cfworker branch December 21, 2023 15:50

fregante mentioned this pull request Jan 21, 2024

CSP error from Page Editor analysis rule in MV3 #7265

Closed

fregante mentioned this pull request Aug 2, 2024

Could ajv be *optionally* swapped with @cfworker/json-schema? rjsf-team/react-jsonschema-form#3269

Open

1 task

benjdlambert mentioned this pull request Sep 2, 2024

🚀 Feature: Replacing the non-precompiled AJV in the frontend backstage/backstage#22063

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#3059: switch RJSF validator to cfworker/jsonschema #7081

#3059: switch RJSF validator to cfworker/jsonschema #7081

twschiller commented Dec 9, 2023 •

edited by grahamlangford

Loading

codecov bot commented Dec 15, 2023 •

edited

Loading

twschiller Dec 20, 2023

grahamlangford Dec 20, 2023

twschiller commented Dec 20, 2023

BLoe left a comment

BLoe Dec 20, 2023

BLoe Dec 20, 2023

twschiller Dec 20, 2023

grahamlangford Dec 20, 2023

grahamlangford Dec 20, 2023

github-actions bot commented Dec 21, 2023

#3059: switch RJSF validator to cfworker/jsonschema #7081

#3059: switch RJSF validator to cfworker/jsonschema #7081

Conversation

twschiller commented Dec 9, 2023 • edited by grahamlangford Loading

What does this PR do?

Remaining Work

Reviewer Tips

Discussion

Demo

Future Work

Checklist

codecov bot commented Dec 15, 2023 • edited Loading

Codecov Report

twschiller Dec 20, 2023

Choose a reason for hiding this comment

grahamlangford Dec 20, 2023

Choose a reason for hiding this comment

twschiller commented Dec 20, 2023

BLoe left a comment

Choose a reason for hiding this comment

BLoe Dec 20, 2023

Choose a reason for hiding this comment

BLoe Dec 20, 2023

Choose a reason for hiding this comment

twschiller Dec 20, 2023

Choose a reason for hiding this comment

grahamlangford Dec 20, 2023

Choose a reason for hiding this comment

grahamlangford Dec 20, 2023

Choose a reason for hiding this comment

github-actions bot commented Dec 21, 2023

twschiller commented Dec 9, 2023 •

edited by grahamlangford

Loading

codecov bot commented Dec 15, 2023 •

edited

Loading