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

feat(extend): allow adding of custom validators globally (#1077) #1079

Closed
wants to merge 3 commits into from

Conversation

ysfaran
Copy link

@ysfaran ysfaran commented Aug 11, 2021

Description

Everything is already described in #1077.
This PR is my suggested implementation for the validators part. I didn't implement it for sanitizers so far.

To-do list

  • I have added tests for what I changed.
  • This pull request is ready to merge. (of course I want to hear opinions first 😃 this is also my frist contribution so I'm not sure how the process is here)

Copy link
Member

@fedeci fedeci left a comment

Choose a reason for hiding this comment

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

Thanks @ysfaran, this is an interesting solution, but is too similar to the current customValidator validator. For full extensibility we have to inject validators and sanitizers directly in the chain.

@ysfaran
Copy link
Author

ysfaran commented Aug 13, 2021

@fedeci could you please explain what you mean with "full extensibility"? What are the plans here?

I agree though that it's quite similar to customValidator and it probably would make sense to reuse some code here.

Copy link
Member

@fedeci fedeci left a comment

Choose a reason for hiding this comment

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

Right now we do not inject the extended ValidatorsImpl in the middlewares like check(), body()... so they would not be available in the chain

// e.g.
body('name').isFoo() // not available

Pluggability of validators is something that we need to think more accurately also considering typescript requirements and security (we can continue discuss about it in the linked issue).
For the moment I like the idea of allowing the user to set options in .custom() and .customSanitizer(), but I would prefer to keep the meta parameter as the second one and move options at the last position.

@ysfaran
Copy link
Author

ysfaran commented Aug 14, 2021

I updated the PR and extended CustomValidator so it can work with custom options aswell.

[...] so they would not be available in the chain

With the change I did they should be available actually. I created a fully working typescript example in this test project: https://github.com/ysfaran/express-validator-extend-test

With npm link I was able to test everything locally, so let's have a look at the example.

In extensions.ts I created a custom validator isToken(value, meta, options) that optionally receives a length property as part of options. (it's worth noting that options could also be a simple number here). I am able to add this validator globally, because of extend which is part of this PR:

// extensions.ts
import { extend } from "express-validator";

type TokenValidatorOptions = {
  length?: number;
};

extend.validators({
  isToken(value: string, meta, { length = 42 }: TokenValidatorOptions = {}) {
    if (typeof value !== "string") {
      throw Error("value must be a string");
    }

    if (!value.startsWith("0x")) {
      throw Error("value  must start with '0x'");
    }

    if (value.length !== length) {
      throw Error(`value must be ${length} characters long`);
    }

    return true;
  },
});

extension.ts needs to be loaded in a top-level file so it's executed before a custom validator is actually used, which is why it's imported at the top of index.ts:

// index.ts
import "./extensions";

import express from "express";
import { body, validationResult } from "express-validator";

const app = express();
app.use(express.json());

const port = 8080;

app.post("/", body("token").isToken({ length: 3 }), (req, res) => {
  const errors = validationResult(req);

  if (errors.isEmpty()) {
    res.send("body has indeed a valid token!");
  } else {
    res.status(400).send(errors.array());
  }
});

app.listen(port, () => {
  console.log(`server started at http://localhost:${port}`);
});

As you can see I use the newly added isToken validator here. The express server can be started with npm start.

I checked the functionality by executing following curl commands:

$ curl -H "Content-Type: application/json" -d "{\"token\":\"0x1\"}" http://localhost:8080
body has indeed a valid token!
$ curl -H "Content-Type: application/json" -d "{\"token\":\"0x\"}" http://localhost:8080
[{"value":"0x","msg":"value must be 3 characters long","param":"token","location":"body"}]

So body("token").isToken({ length: 3 }) actually works here.

[...] also considering typescript requirements [...]

To make this actually work in typescript you have to tell the compiler that isToken really existis in the chain:

// express-validator.d.ts
import "express-validator";

declare module "express-validator" {
 interface ValidationChain {
   isToken: (options?: { length?: number }) => ValidationChain;
 }
}

This approach is similiar to expect.extend from jest.

Please let me know if I missed something here 😃

@gustavohenke
Copy link
Member

Worked on something myself that superseded this.
See #1209

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.

3 participants