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

Request: Support $data on AJV configuration #920

Closed
ZakRabe opened this issue May 2, 2021 · 2 comments · Fixed by #923
Closed

Request: Support $data on AJV configuration #920

ZakRabe opened this issue May 2, 2021 · 2 comments · Fixed by #923

Comments

@ZakRabe
Copy link
Contributor

ZakRabe commented May 2, 2021

Today I was trying to use the ValidateMultipartFormDataBody hook to validate user registration data. But I ran into an issue with the AJV instance

If I wanted to reference the value in the password, for the password confirmation, I would use a schema that looks like this

password: {
  type: "string",
  maxLength: 255,
  minLength: 7,
  pattern: "^(?=.*?[A-Z])(?=.*?[a-z])(?=.*?[0-9])(?=.*?[#?!@$%^&*-]).{7,}$",
},
confirmPassword: {
  const: {
    $data: "1/password",
  },
  type: "string",
},

However AJV doesn't support this feature unless the $data: true option is passed at initialization. See here in the AJV documentation for the attribute.

I saw in #633 that not all the options for the AJV instance are supported in the configuration, and you'd be adding support as they are requested.

I also confirmed for myself that the option isn't being passed.

export function getAjvInstance(): Ajv.Ajv {
if (!_instanceWrapper.instance) {
_instanceWrapper.instance = new Ajv({
allErrors: Config.get('settings.ajv.allErrors', 'boolean'),
coerceTypes: Config.get('settings.ajv.coerceTypes', 'boolean', true),
nullable: Config.get('settings.ajv.nullable', 'boolean'),
removeAdditional: Config.get('settings.ajv.removeAdditional', 'boolean|string', true) as boolean|'all'|'failing',
useDefaults: Config.get('settings.ajv.useDefaults', 'boolean|string', true) as boolean|'empty'|'shared',
});
}
return _instanceWrapper.instance;
}

Obviously its pretty simple to do this sort of check in the actual controller code, or another hook, so this issue isn't blocking.
However I think this usage of the JSON Schema to validate password confirmations is a pretty common use case, and warrants supporting in FoalTS.

If you'd like me to submit a PR, I'd be willing to, as it's a pretty trivial change. I'm just not sure how to best test a change like this to pass the PR approval process.

Thanks.

@LoicPoullain LoicPoullain added this to Backlog in Issue tracking via automation May 3, 2021
@LoicPoullain
Copy link
Member

Sounds a good idea. 👍 Feel free to open a PR @ZakRabe .

About the tests, the should accept custom configuration from the Config. will probably need an update. We will also probably need a test similar to should throw a ConfigTypeError when the value of "settings.ajv.allErrors" has an invalid type. but with the $data setting.

@ZakRabe ZakRabe mentioned this issue May 5, 2021
8 tasks
@LoicPoullain LoicPoullain moved this from Backlog to Work In Progress in Issue tracking May 5, 2021
@LoicPoullain LoicPoullain mentioned this issue May 5, 2021
4 tasks
LoicPoullain added a commit that referenced this issue May 5, 2021
@LoicPoullain
Copy link
Member

Feature added in v2.4.0

Issue tracking automation moved this from Work In Progress to Done / Closed This Release May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Issue tracking
  
Done / Closed This Release
Development

Successfully merging a pull request may close this issue.

2 participants