Add more schemas to validate against #1130

aerfio · 2019-01-16T14:01:56Z

Reasons for making this change

Right now we have no clear way to validate data using anything other then draft-6, which is default for ajv version 5. By updating ajv to newest available version (6.7.0) and making changes listed here: https://github.com/epoberezkin/ajv#using-version-6 we would be able to validate data using other drafts. There are no breaking changes between v5 and v6: https://github.com/epoberezkin/ajv/releases/tag/v6.0.0

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
- If I'm adding a new section, I've updated the Table of Content
I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
- I've run npm run cs-format on my branch to conform my code to prettier coding style
I'm adding a new feature
- I've updated the playground with an example use of the feature

aerfio · 2019-01-16T14:08:26Z

Such a schema fails to be validated:

{
  "$ref": "#/definitions/Dataset",
  "$schema": "http://json-schema.org/draft-04/schema#",
  "definitions": {
    "Dataset": {
      "properties": {
        "datasetId": {
          "description": "A user-specified unique ID for this BigQuery dataset.",
          "maxLength": 1024,
          "minLength": 1,
          "pattern": "^[_a-zA-Z0-9]+$",
          "title": "Dataset ID",
          "type": "string"
        },
        "defaultTableExpirationMs": {
          "description": "yadayada",
          "title": "Default table expiration",
          "type": "string"
        },
        "description": {
          "description": "A user-friendly description of the BigQuery dataset.",
          "title": "Description",
          "type": "string"
        },
        "friendlyName": {
          "description": "A descriptive name for the BigQuery dataset.",
          "title": "Friendly name",
          "type": "string"
        },
        "labels": {
          "additionalProperties": { "type": "string" },
          "description": "To organize your project, add arbitrary labels as key/value pairs to the BigQuery dataset. Use labels to indicate different environments, services, teams, and so on.",
          "title": "Labels",
          "type": "object",
          "x-googleProperty": { "type": "LABELS" }
        },
        "location": {
          "default": "US",
          "description": "The geographic location where the BigQuery dataset should reside.",
          "enum": ["US", "EU"],
          "title": "Location",
          "type": "string"
        }
      },
      "required": ["datasetId"],
      "type": "object"
    }
  },
  "form": [
    {
      "key": "datasetId",
      "validationMessage": "Must contain only letters, numbers, or underscores. Max length 1024."
    },
    "location",
    "friendlyName",
    "description",
    "labels"
  ]
}

And there are no errors in console and they do not get displayed, as written
here:https://github.com/mozilla-services/react-jsonschema-form/blob/4c81b02742270e5225c826025f5e99efdc35ea32/src/validate.js#L161

If I put console.error there then the message is:

aerfio · 2019-01-18T10:23:36Z

@glasserc @edi9999 Any thought on this?

edi9999 · 2019-01-18T10:27:57Z

src/validate.js

 });
+//add more schemas to validate against, draft-7 remains default
+ajv.addMetaSchema(require("ajv/lib/refs/json-schema-draft-04.json"));
+ajv.addMetaSchema(require("ajv/lib/refs/json-schema-draft-06.json"));


Not a fan of these lines, this means that we always load in JS 4,6 and 7 definition, but we only need one normally.

Then maybe hide them under some kind of flag? Something like

<Form enableDraft4 ... />

and in validate.js:

if(enableDraft4){ ajv.addMetaSchema(require("ajv/lib/refs/json-schema-draft-04.json")); }

What do you think?

@aerfio Perhaps is there some way to call addMetaSchema based on the value for $schema in the schema definition itself?

when this is compiled by webpack, even if they are if statements, all the JSON files will be in the bundled file.

I think we should rather have a way of doing :

<Form jsonSchema={require("ajv/lib/refs/json-schema-draft-04.json"} </Form>

aerfio · 2019-01-21T08:31:03Z

@epicfaace Did you mean something like this? Regexp matches both "http://json-schema.org/draft-04/schema#" and "http://json-schema.org/draft-04/schema" and draft-06 respectively, (look at "#" sign at the end), because both of those two addresses work.

@edi9999 what do you think about this?

epicfaace · 2019-01-21T17:13:51Z

@aerfio , I think @edi9999 's proposal is that we do not include draft-04 or draft-06 within react-jsonschema-form, as it would unnecessarily increase our bundle size. Rather, if someone wants to use draft-04, they must install ajv themselves and use the following code:

<Form
     jsonSchema={require("ajv/lib/refs/json-schema-draft-04.json")}
</Form>

Did that make sense? Would you be able to change your PR to work this way instead?

aerfio · 2019-01-22T08:26:15Z

@epicfaace @edi9999 ok, I think now we have the least invasive change, I just added additionalSchema prop, so if user wants to add draft-4, draft-6, or even some custom schema (or array of schemas), he/she has an ability to do so, and it doesn't increase bundle size.

I'll wait with documentation/tests till I hear what do you think about this :P

edi9999 · 2019-01-22T08:48:32Z

Looks good to me !!

epicfaace

Looks good -- is there a reason why it's called additionalSchema instead of jsonSchema? Will passing a value for additionalSchema make ajv validate both for the latest draft and for the schema you passed in?

epicfaace · 2019-01-22T14:25:39Z

src/components/Form.js

@@ -58,9 +58,10 @@ export default class Form extends Component {
    const { definitions } = schema;
    const formData = getDefaultFormState(schema, props.formData, definitions);
    const retrievedSchema = retrieveSchema(schema, definitions, formData);
-
+    const additionalSchema =
+      props.additionalSchema || this.props.additionalSchema;


What's the difference between props.additionalSchema and this.props.additionalSchema?

It's unnecesary fallback, good catch!

edi9999 · 2019-01-23T06:41:16Z

src/validate.js

 });
+//flag indicating whether we've already added custom schemas
+let addedAdditionalSchemas = false;


Why would we be disallowed from adding schemas multiple times ?

I could see a case where you would have schema-06 for one form, and schema-04 for an other form.

@glasserc You can't add the same schema multiple times, ajv throws errors:

If you want to you can add multiple schemas at once, like so additionalSchema ={[ schema1, schema2 ]}

And if you want to validate one form using draft-6 and one using draft-4 you'd just use correct $schema in your schema, like in example provided in my second comment, read this for clarification https://github.com/epoberezkin/ajv#validateschemaobject-schema---boolean .

If schema has $schema property, then the schema with this id (that should be previously added) is used to validate passed schema.

also

By default this method is called automatically when the schema is added, so you rarely need to use it directly.

@epicfaace They're additional jsonSchemas :P variable name can be changed, no problem

Ok makes sense !

aerfio · 2019-01-23T09:19:16Z

@epicfaace According to http://json-schema.org/latest/json-schema-core.html#rfc.section.6.4

This extended meta-schema SHOULD be referenced using the "$schema" keyword, to allow tools to follow the correct behaviour.

aerfio · 2019-01-25T07:23:35Z

@glasserc ?

epicfaace

Looks good, I suggested a few changes to wording.

In addition to those changes, can you do the following:

update the propTypes on the <Form> component
add some tests for passing in different schemas to metaSchema
add a console error - if someone has entered a value $schema but hasn't added the proper metaSchema (see my comment on validate.js)

docs/validation.md

src/validate.js

epicfaace · 2019-01-25T16:34:48Z

src/validate.js

+    ajv.addMetaSchema(metaSchema);
+    addedMetaSchemas = true;
+  }
+
  try {
    ajv.validate(schema, formData);
  } catch (e) {


Can we add a console.error here so if the schema is invalid, the user gets an error?

aerfio · 2019-01-28T15:25:40Z

I'll probably write those tests tomorrow, don't merge for now! :D

test/ArrayField_test.js

epicfaace

Few more changes.

Additionally, @aerfio , I'm wondering about the metaSchema prop -- is there really a need to be able to pass in multiple values for a metaSchema? A schema will only have a single value for the $schema attribute, anyway. Unless you can think of a use case for having multiple values in metaSchema, I think we should just accept a single value for it. (It's also unintuitive for a singular property -- metaSchema -- to accept an array)

(Let me loop you in @edi9999 since you first suggested being able to have multiple metaSchema's)

docs/validation.md

test/validate_test.js

aerfio · 2019-01-29T07:30:39Z

On one hand I agree with you @epicfaace, but on the other it would be taking out features, that ajv already supports.

It would be useful in a case when we don't really know which draft will finally use, (like getting data from some kind of backend) so we can preemptively add e.g. draft-4 and draft-6. And prop name metaSchema (singular) is intentional - usual case would be that user adds one meta schema (while still allowing to use more than one at a time)

aerfio · 2019-01-30T07:34:07Z

@epicfaace What do you think?

epicfaace · 2019-01-30T17:00:06Z

Yeah, I see your logic here. If I set metaSchema={require("ajv/lib/refs/json-schema-draft-04.json")} then this means the form will support both draft-04 and draft-07, right? If so, can you clarify on the documentation that that will be the case?

(In fact, if that's the case, it may even be clearer to rename metaSchema to additionalSchemas as you had mentioned earlier or additionalMetaSchemas -- what do you think?)

docs/validation.md

aerfio · 2019-01-31T09:37:06Z

src/validate.js

-    // swallow errors thrown in ajv due to invalid schemas, these
-    // still get displayed
+  } catch (err) {
+    console.error(err);


only problem with this console.error is that in test in few places we have now

One such error is in my test (intentional), two are in old tests

Oh, my bad -- I didn't realize that as per the comment, even if the errors aren't logged to the console, they still get displayed. I think you can safely remove the console.error and revert back to the original strategy of not doing anything in the catch block.

aerfio · 2019-01-31T09:38:49Z

ok @epicfaace, I've changed the prop name and updated docs

epicfaace

Thanks for the changes! Few more things.

epicfaace · 2019-02-01T03:43:15Z

src/validate.js

-    // swallow errors thrown in ajv due to invalid schemas, these
-    // still get displayed
+  } catch (err) {
+    console.error(err);


Oh, my bad -- I didn't realize that as per the comment, even if the errors aren't logged to the console, they still get displayed. I think you can safely remove the console.error and revert back to the original strategy of not doing anything in the catch block.

epicfaace · 2019-02-01T04:22:10Z

docs/validation.md

+
+render((
+  <Form schema={schema} 
+        additionalMetaSchemas={additionalMetaSchemas}/>


Can we make additionalMetaSchemas always an array, just for consistency?

src/components/fields/SchemaField.js

aerfio · 2019-02-25T16:54:21Z

@epicfaace OK, I have deleted validationErrors and merged it into errors and errorSchema, so that they're handled by existing components:

Only thing that may matter is that error produced by validation errors doesn't have all the fields that normal error have, it only has stack https://github.com/mozilla-services/react-jsonschema-form/pull/1130/files#diff-6ca5d88062570937214ee7ea244b05c5R194, but then again, those incomplete errors only occur in case of failed validation, so normal errors don't even show up, and previously we had no errors whatsoever in such a case, so I don't introduce any breaking changes

Unless we always guarantee that errors have specific fields, but I couldn't find anything about it in docs

epicfaace · 2019-02-25T17:14:54Z

@aerfio I think that what you did works well and makes sense -- nice work!

One thing to keep in mind is that ajv actually throws an error when the $schema given is invalid (rather than adding it to the errors object -- see this fiddle). However, it makes sense on our end to instead add this error to the errors object, given that we're making user-facing forms.

epicfaace · 2019-02-25T17:18:40Z

@aerfio regarding error documentation, yes, we don't have a documentation for that yet. I'm adding that in this PR though, so once it's merged we can update the documentation to mention that errors regarding a $schema problem will only have the stack attribute.

epicfaace · 2019-02-25T17:16:24Z

src/validate.js

-    // swallow errors thrown in ajv due to invalid schemas, these
-    // still get displayed
+  } catch (err) {
+    validationErrors = err;


@aerfio validationErrors is an object, not an array, right? Would it make more sense to rename it to validationError?

epicfaace · 2019-02-25T17:19:52Z

src/validate.js

    if (Array.isArray(parent.__errors)) {
      // We store the list of errors for this node in a property named __errors
      // to avoid name collision with a possible sub schema field named
      // "errors" (see `validate.createErrorHandler`).
      parent.__errors = parent.__errors.concat(message);
    } else {
-      parent.__errors = [message];
+      if (message) {


@aerfio when would message be undefined?

If there is no message field in particular error, like in that special case I've introduced
If I'd make validation error like

{ stack: something, message: something }

then it would look like this:

Which is bad because we have the same info in errorList and in the footer

@epicfaace Error thrown by ajv has only name and stack fields, name is Error, stack is like in pic above

Advantage of using message field is that while form has validation errors, then all fields are red, opposite to #1130 (comment)

aerfio · 2019-02-25T20:55:16Z

@epicfaace The case you are refering to, when ajv throws errors connected to bad schema are already handled, it's even tested behaviour https://github.com/mozilla-services/react-jsonschema-form/pull/1130/files#diff-6654a6c320ad438fbbb53fa54fe3b95dR307

And regarding docs - could you please add it in that PR, if you're already writing it? Choose what version you like, with message field in error or without it, and I'll change this PR accordingly (or leave it as it is)

epicfaace · 2019-02-25T23:21:37Z

@epicfaace The case you are refering to, when ajv throws errors connected to bad schema are already handled, it's even tested behaviour https://github.com/mozilla-services/react-jsonschema-form/pull/1130/files#diff-6654a6c320ad438fbbb53fa54fe3b95dR307

And regarding docs - could you please add it in that PR, if you're already writing it? Choose what version you like, with message field in error or without it, and I'll change this PR accordingly (or leave it as it is)

Will do. One more thing -- since we're adding meta schema errors (i.e., "no schema with key or ref "http://json-schema.org/draft-04/schema#\"") to the errors object, we might also need to add these errors to the errorSchema. Can you update that?

aerfio · 2019-02-26T09:21:34Z

@epicfaace We are already doing it, because we're merging {stack: something} into errors before toErrorSchema call: https://github.com/mozilla-services/react-jsonschema-form/pull/1130/files#diff-6ca5d88062570937214ee7ea244b05c5R205.

But those specific errors do not have message field so they do not get used in errorSchema, so we're back to this discussion about message field.

epicfaace · 2019-02-26T14:36:27Z

@aerfio I think it would be better if errorSchema has every error that is in errors. Maybe one way to do it is like this:
Imagine an invalid schema like this:

{
  "title": "A registration form",
  "description": "A simple form example.",
  "type": "test"
}

The errorSchema constructed looks like this (and then the error does not show up twice):

{
  "type": {
    "__errors": [
      "should be equal to one of the allowed values",
      "should be array",
      "should match some schema in anyOf"
    ]
  }
}

So if we have a problem with the wrong $schema, can we construct an error list like this?

{
  "$schema": {
    "__errors": [
      "no schema with key or ref \"test\""
    ]
  }
}

epicfaace · 2019-02-26T14:37:19Z

Also, here's another problem: it seems like an extra error is added for any validation error, not just a schema validation error.

For example, imagine I have an invalid schema as follows:

{
  "title": "A registration form",
  "description": "A simple form example.",
  "type": "test"
}

Then, this code adds an extra error ("schema is invalid: data.type should be equal to one of the allowed values, data.type should be array, data.type should match some schema in anyOf") which is redundant.

We need to make sure that the try-catch statement is specific enough so that it only adds $schema-specific errors to the errors list.

aerfio · 2019-02-26T19:11:09Z

@epicfaace I have reduced validation errors to only those with error message starting with no schema with key or ref which I think is as specific as it gets.

And I have added

{
  "$schema": {
    "__errors": [
      "no schema with key or ref yada yada"
    ]
  }
}

to errorSchema.

epicfaace · 2019-03-03T14:29:53Z

@aerfio everything looks good, thanks!

aerfio · 2019-03-03T20:59:40Z

@epicfaace thanks! when can we expect next release? 😀

* Add more schemas to validate against * Edit validate.js so that it calls addMetaSchema conditionally * Add props which accepts custom schemas for ajv * remove unnecesary fallback * rename variable name * add documentation for `metaSchema` prop * Update docs/validation.md Co-Authored-By: aerfio <[email protected]> * Update src/validate.js Co-Authored-By: aerfio <[email protected]> * Update src/validate.js Co-Authored-By: aerfio <[email protected]> * Update docs/validation.md Co-Authored-By: aerfio <[email protected]> * Update docs/validation.md Co-Authored-By: aerfio <[email protected]> * Update docs/validation.md Co-Authored-By: aerfio <[email protected]> * return errors from validation in validateFormData * add initial tests for meta schemas * add one proper test case * add more test cases * Update docs/validation.md Co-Authored-By: aerfio <[email protected]> * Add test cases for multiple metaSchemas in validateFormData * Change prop name and update docs * Change additionalMetaSchemas prop to array only and remove console.error * make sure that validateFormData's additionalMetaShemas argument is always an array * Commit "broken" test to see whether they pass * Fix tests and create new Ajv instance when additionalMetaSchemas changes * fix: create new ajv instance when additionalMetaSchemas prop is null, add tests * handle validation errors * fix tests and delete validation errors * Delete unneeded field in return statement * dont stop showing red overlay when validation errors are present * Make errors and test more sensible * rename variable * handle only $schema related errors * fix tests

aerfio mentioned this pull request Jan 16, 2019

'Provision Service Class' form does not support regex validation kyma-project/kyma#1891

Closed

edi9999 reviewed Jan 18, 2019

View reviewed changes

epicfaace added the awaiting changes label Jan 21, 2019

epicfaace reviewed Jan 22, 2019

View reviewed changes

epicfaace mentioned this pull request Jan 22, 2019

Expose Ajv in order to support alternate schemas #794

Closed

9 tasks

edi9999 reviewed Jan 23, 2019

View reviewed changes

aerfio force-pushed the add-more-drafts branch 2 times, most recently from a82988b to f8f65ec Compare January 23, 2019 09:39

edi9999 approved these changes Jan 25, 2019

View reviewed changes

epicfaace requested changes Jan 25, 2019

View reviewed changes

epicfaace reviewed Jan 28, 2019

View reviewed changes

test/ArrayField_test.js Show resolved Hide resolved

aerfio force-pushed the add-more-drafts branch from 639043d to b438f4d Compare January 28, 2019 20:12

epicfaace requested changes Jan 29, 2019

View reviewed changes

docs/validation.md Show resolved Hide resolved

docs/validation.md Outdated Show resolved Hide resolved

test/validate_test.js Show resolved Hide resolved

aerfio commented Jan 31, 2019

View reviewed changes

docs/validation.md Show resolved Hide resolved

aerfio force-pushed the add-more-drafts branch from a4f7b7c to c3b539b Compare January 31, 2019 09:31

aerfio commented Jan 31, 2019

View reviewed changes

epicfaace requested changes Feb 1, 2019

View reviewed changes

aerfio force-pushed the add-more-drafts branch from d60e300 to 2ec6ecc Compare February 25, 2019 13:54

aerfio commented Feb 25, 2019

View reviewed changes

src/components/fields/SchemaField.js Show resolved Hide resolved

Mateusz Puczynski added 3 commits February 25, 2019 15:13

Delete unneeded field in return statement

83509dc

dont stop showing red overlay when validation errors are present

bbd8bdb

Make errors and test more sensible

2c2923e

epicfaace reviewed Feb 25, 2019

View reviewed changes

rename variable

7469b1a

aerfio force-pushed the add-more-drafts branch from 6bbd64b to 7469b1a Compare February 25, 2019 20:40

Mateusz Puczynski added 2 commits February 26, 2019 19:47

handle only $schema related errors

2e5d5b9

fix tests

2baace1

epicfaace added awaiting review and removed awaiting response labels Mar 3, 2019

epicfaace approved these changes Mar 3, 2019

View reviewed changes

epicfaace merged commit 92233e4 into rjsf-team:master Mar 3, 2019

aerfio deleted the add-more-drafts branch March 3, 2019 14:42

sbusch mentioned this pull request May 3, 2019

Specifying a meta schema in $schema forces validation error: no schema with key or ref "http://json-schema.org/draft-06/schema#" #1274

Closed

3 tasks

snyk-bot mentioned this pull request Jan 30, 2020

[Snyk] Upgrade react-jsonschema-form from 1.2.0 to 1.8.1 preset-io/superset#11

Closed

epicfaace mentioned this pull request Sep 27, 2021

Bump/ajv #2542

Closed

7 tasks

Add more schemas to validate against #1130

Add more schemas to validate against #1130

Conversation

aerfio commented Jan 16, 2019 • edited Loading

Reasons for making this change

Checklist

aerfio commented Jan 16, 2019 • edited Loading

aerfio commented Jan 18, 2019

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

aerfio commented Jan 21, 2019 • edited Loading

epicfaace commented Jan 21, 2019 • edited Loading

aerfio commented Jan 22, 2019

edi9999 commented Jan 22, 2019

epicfaace left a comment • edited Loading

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

aerfio Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerfio commented Jan 23, 2019

aerfio commented Jan 25, 2019

epicfaace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerfio commented Jan 28, 2019 • edited Loading

epicfaace left a comment • edited Loading

Choose a reason for hiding this comment

aerfio commented Jan 29, 2019

aerfio commented Jan 30, 2019

epicfaace commented Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerfio commented Jan 31, 2019

epicfaace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerfio commented Feb 25, 2019

epicfaace commented Feb 25, 2019

epicfaace commented Feb 25, 2019

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

aerfio commented Feb 25, 2019 • edited Loading

epicfaace commented Feb 25, 2019

aerfio commented Feb 26, 2019

epicfaace commented Feb 26, 2019

epicfaace commented Feb 26, 2019

aerfio commented Feb 26, 2019

epicfaace commented Mar 3, 2019

aerfio commented Mar 3, 2019

aerfio commented Jan 16, 2019 •

edited

Loading

aerfio commented Jan 16, 2019 •

edited

Loading

aerfio commented Jan 21, 2019 •

edited

Loading

epicfaace commented Jan 21, 2019 •

edited

Loading

epicfaace left a comment •

edited

Loading

aerfio Jan 23, 2019 •

edited

Loading

aerfio commented Jan 28, 2019 •

edited

Loading

epicfaace left a comment •

edited

Loading

epicfaace commented Jan 30, 2019 •

edited

Loading

aerfio commented Feb 25, 2019 •

edited

Loading