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.

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

Expose Ajv in order to support alternate schemas #794

Closed
wants to merge 5 commits into from

Conversation

johnrom
Copy link

@johnrom johnrom commented Dec 18, 2017

Reasons for making this change

react-jsonschema-form does not expose its Ajv implementation so that users can configure Ajv with different schemas and whatnot. a la #783

My use case: implementing ASP.Net React Forms via https://github.com/RSuter/NJsonSchema

This is probably not the way to do it, but I figured I'd offer my implementation. This implementation allows a dev to do this:

import Form from "react-jsonschema-form";
import { getValidator } from "react-jsonschema-form";
  
const validator = getValidator();
validator.addMetaSchema(require('ajv/lib/refs/json-schema-draft-04.json'));

I might rather something like the following, but I'm also using TypeScript and it cried when I attempted to implement this so it tells me it might not be the way to go.

import Form from "react-jsonschema-form";

const validator = Form.getValidator();
validator.addMetaSchema(require('ajv/lib/refs/json-schema-draft-04.json'));

I might even go so far as to say the following is the real way, but I really have no idea which is the React way as I'm just learning it.

            return <Form
                method="POST"
                action={this.props.lead.action}
                schema={this.props.lead.schema}
                uiSchema={this.props.lead.uiSchema}
                formData={this.props.lead.formData}
                fields={fields}
                validateAgainst={require('ajv/lib/refs/json-schema-draft-04.json')}
            />;

Either way, I'd like feedback before implementing anything further than I have.

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

@edi9999
Copy link
Collaborator

edi9999 commented Jan 10, 2018

Imo this code is clean and can be merged.

@glasserc
Copy link
Contributor

Sorry for the delay in reviewing -- the holidays got the better of me. I agree that the use case is important and I agree equally that this approach probably isn't the way to go. Sharing a single global variable as the singleton validator is already probably not great, but if we start modifying it, I think it would be worse.

I'm not a React expert either, but I think passing it as a prop, as you do in your final example, would be better. We could treat the existing ajv variable as a default to use if none is passed.

@edi9999
Copy link
Collaborator

edi9999 commented Jan 11, 2018

Indeed, I also think that avoiding to have a global variable that anyone can change, it would probably be better to set the ajv variable from the props , something like :

return <Form
    method="POST"
    action={this.props.lead.action}
    schema={this.props.lead.schema}
    uiSchema={this.props.lead.uiSchema}
    formData={this.props.lead.formData}
    fields={fields}
    ajv={myCustomAJV}
/>;

@johnrom
Copy link
Author

johnrom commented Jan 17, 2018

I think that the need to change validation schema is independent of the fact that the project uses a specific schema validator currently. I would prefer to use my final example above, but I would name the attribute validationSchema to fit with the rest of the schema attribute names. Then devs can simply provide a schema to validate against.

DamianOsipiuk pushed a commit to Juniper/contrail-ui-third-party-cache that referenced this pull request Jan 30, 2018
- extranous properties in ui:order not throwing errors
rjsf-team/react-jsonschema-form#814
- exported validator instance for customization
rjsf-team/react-jsonschema-form#794
@doncesarts
Copy link
Contributor

In which release will be included this feature ?

@narthollis
Copy link

narthollis commented Feb 21, 2018

Having another prop on Form that takes an AJV instance is the best in my opinion.
This is due to the fact that AJV can be extended with additional format validators, keywords and other items as well as schemas.

https://github.com/epoberezkin/ajv#addschemaarrayobjectobject-schema--string-key---ajv
https://github.com/epoberezkin/ajv#addmetaschemaarrayobjectobject-schema--string-key---ajv
https://github.com/epoberezkin/ajv#api-addformat
https://github.com/epoberezkin/ajv#addkeywordstring-keyword-object-definition---ajv

As it stands I am likely going to have to downgrade the version of react-jsonschema-form as I am not in a position currently to upgrade all of our draft-4 schemas to draft-6

Alliteratively

This could be bundled with providing an external validators - in which case providing an example validators using AJV would be ideal

@johnrom
Copy link
Author

johnrom commented Feb 21, 2018

I do plan on finishing this pull request once I reach a point in a project where I need this functionality, probably in a week or two. However it seems we haven't come to an agreement on what method to use.

Personally, I do not want to create a custom AJV instance especially because an update to this project may update AJV and my custom configuration might not even be valid / may create a conflict. Instead, I would like to provide a schema that this module would validate against, because even if down the line another JSON validator takes its place, this module will still understand that I want to use a specific schema to validate against.

Per @narthollis note adding other customizable features, I think it'd be best to decide if those are implementation-independent and if we should support adding those customizable features as properties as well, leaving the current AJV implementation untouched -- as part of a different PR.

return <Form
    method="POST"
    action={action}
    schema={schema}
    uiSchema={uiSchema}
    // this could be any valid JSON schema
    validationSchema={require('ajv/lib/refs/json-schema-draft-04.json')}
    formData={formData}
    fields={fields}
/>;

@johnrom
Copy link
Author

johnrom commented Mar 1, 2018

My initial thought of passing a custom meta-schema is not going to work, because now additional work needs to be done in order to set up v4 meta-schema:
https://github.com/epoberezkin/ajv/releases/tag/v6.0.0

I now think the three available options are:

  1. to pass a custom AJV instance to the Form. I personally don't like this idea because users will have to duplicate the options and Formats that are used internally to make it work the same, and maintain them over time

    const ajv = new AJV({...allInternalOptionsAndCustomOptions})
        .addMetaSchema(require('path/to/schema.json'))
        .addFilters({...allTheInternalFormatsAndCustomFormats});
    
  2. to pass configuration options and an onConfigureValidator callback. The user needs to know a bit about AJV, but they don't need to duplicate and maintain this package's custom options.

    function onConfigureValidator(validator) {
        validator.addMetaSchema(require('path/to/schema.json'));
    }
    
    function render() {
        const validatorOpts = { idSchema: 'auto' }; // or whatev
    
        return (
            <Form validatorOpts={validatorOpts} onConfigureValidator={this.onConfigureValidator} />
        );
    }
    
  3. to specify a jsonSchema version and handle the technicals internally. then inside the validation file the package would decide how to configure AJV based on the metaSchema parameter. I think this'd work best if for some reason the validator changed.

    function render() {
        <Form metaSchema="draft-04" />
    }
    

@narthollis
Copy link

narthollis commented Mar 1, 2018

The more I interact with and think about this issue, the more I am inclined to say that the property should be a custom validator, with defined shape eg

interface CustomValidator {
    validate(formData: any, schema: JSONSchema4 | JsonSchema6 | JsonSchema7): ValidationError[]
}

interface ValidationError {
    name: string;      // Ajv.ErrorObject.keyword
    property: string;  // Ajv.ErrorObject.dataPath
    message: string;   // Ajv.ErrorObject.message
    params?: Ajv.ErrorParameters;
    stack: string;     // Ajv.ErrorObject.dataPath + Ajv.ErrorObject.message
}

If React JSON Schema Form continues to use AJV then the internal AJV Error to Validation Error methods should be exposed to allow easier creation of a custom validator.

My resonsing for using passing an Object with a validation method on it is to make it easier for sharing AJV instances (or instances of other validators)

Usage could be something like:

import metaSchema4 from 'ajv/lib/refs/json-schema-draft-04.json';
import { transformAjvErrors } from 'src/validate.js'; // this would need to be exported in some better way

class CustomValidator {
    constructor() {
        this.ajv = new Ajv({
            meta: false,
            extendRefs: true,
            unknownFormats: 'ignore',
        });
       ajv.addMetaSchema(metaSchema4);
       ajv._opts.defaultMeta = metaSchema.id;
    }

    public validate(formData: any, schema: JSONSchema4 | JsonSchema6 | JsonSchema7) {
        this.ajv.validate(schema, formData);
        return transformAjvErrors(this.ajv.errors);
    }
}
export const MyCustomValidatorSingleton = new CustomValidator();

### Break
import * as React from 'react';
import RJSF from 'react-jsonschema-form';
import { MyCustomValidatorSingleton } from 'customValidator';

interface Props {
    schema: JSONSchema4;
}

export default MyFormComponent extends React.PureComponent<Props> {
    public render(): React.ReactNode { 
        return <RJSF
            schema={this.props.schema}
            customValidator={MyCustomValidatorSingleton}
        />;
    }
}

@johnrom
Copy link
Author

johnrom commented Mar 14, 2018

@narthollis that is certainly useful for the purpose you are describing. But I think in your case it might be easier to just allow the user to completely override the validateFormData function. The current API only appears to add additional errors instead of a complete override. The code to accomplish this could be really simple.

export default class Form extends Component {

  static defaultProps = {
    onValidate: validateFormData, // validateFormData is the regular function from validate.js
  };

  validate(formData, schema) {
    const { validate, transformErrors, onValidate } = this.props;
    return onValidate(
      // args
    );
  }
}

Form.propTypes = {
    onValidate: PropTypes.func
}

So the only change to your code would be the form call:

return <RJSF
    schema={this.props.schema}
    onValidate={MyCustomValidatorSingleton.validate}
/>;

I would say, however, that for someone who only wants to change the JSON Schema, this is overkill. I would rather something like adding callbacks to the AJV initialization process itself, allowing me to change the args used to create the object, and also allowing a dev to perform actions immediately after initialization. I'm going to do a proof of concept of that within this pull request as that is what I think my solution is.

@epicfaace
Copy link
Member

epicfaace commented Jan 20, 2019

Another way to do this would be to specify the $schema property as in #783.

@johnrom Are you still working on this? You outlined several options; any thoughts on the best way to solve this?

@johnrom
Copy link
Author

johnrom commented Jan 22, 2019

Unfortunately, I ended up removing the shared schema due to complexity so someone else will have to maintain a PR if there is interest in this.

Edit: I'll leave this open while a path forward is determined.

@epicfaace
Copy link
Member

@johnrom if the reason we want to expose ajv is just to support alternate schemas, perhaps the approach outlined in #1130 will do the trick? Or are there additional benefits to exposing the ajv instance itself?

@johnrom
Copy link
Author

johnrom commented Jan 22, 2019

Unfortunately, I stopped using this repo almost a year ago, so I'm not the correct person to ask. I initially just wanted to use a v4 schema which hadn't been supported any longer, in order to integrate with another piece of technology from a backend server. I assumed that if I ran into that issue, that future schema versions would cause the issue again, so I recommended extensibility when implementing a fix. However I decided to manage the data models and presentation layer separately as things became more complex, so I won't be able to contribute any more thorough feedback to this PR.

@epicfaace
Copy link
Member

Okay, thanks for letting me know. Closing this as #1130 seems to do the job.

@epicfaace epicfaace closed this Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants