filipstefansson / nexus-validate

🔑 Add argument validation to your GraphQL Nexus API.
35 stars 8 forks source link

Pass nexus context to yup validate function #13

Closed ysfaran closed 2 years ago

ysfaran commented 3 years ago

Problem

If you want to add a custom validation method to a yup schema you can use yup.addMethod:

yup.addMethod(yup.string, "currency", function () {
  return this.test(function(value){
    const currencies = ["€", "$"];
    const isCurrency = currencies.includes(value!);

    if (!isCurrency) {
      return this.createError({
        message: `${value} is not a valid currency, use on of [${currencies}]`,
      });
    }

    return true;
  });
});

Usage (you would need to add also global typings but it's not necessary to show this here):

validate(({ string }) => ({ currencyField: string().currency() }) 

The problem that I have is that my currencies are fetched once at startup from another backend. This means it's not static data. So I pass it to the graphl context, with apollo it would look like this:

const currencies = await getCurrenciesFromAPI(someConfig);

const server = new ApolloServer({
    context: () => ({ currencies }),
    // ...
 });

To break it down: currently you have no access to the context in "yup land".

Solution

The easiest way to solve this would be to pass the contex to yup.validate and change this line from:

https://github.com/filipstefansson/nexus-validate/blob/314be02cfdab6c55c7dc1373e68c10d935e18607/src/resolver.ts#L64

to:

await schema.validate(args , { context: ctx };

Then you could implement the same currency method this way:

yup.addMethod(yup.string, "currency", function () {
  return this.test(async (value, testContext) => {
    const graphQLContext = testContext.options.context
    const isCurrency = graphQLContext.currencies.includes(value!);

    if (!isCurrency) {
      return this.createError({
        message: `${value} is not a valid currency, use on of [${currencies}]`,
      });
    }

    return true;
  });
});

I presented a relatively simple example but you could also use this to make database validation if your context contains a database client and much more.

Alternatives

You could alternatively also pass the context to the custom method everytime you use it:

validate(({ string }, args, ctx) => ({ currencyField: string().currency(ctx) }) 

But IMHO the solution above looks way cleaner and make this library more extensible. What do you think ? 🙂

filipstefansson commented 2 years ago

@ysfaran I think your first solution looks clean. Feel free to create a PR with the updates!

ysfaran commented 2 years ago

@filipstefansson I was finally able to do this little PR #15 😄

filipstefansson commented 2 years ago

@filipstefansson I was finally able to do this little PR #15 😄

Awesome, let me take a look :)

filipstefansson commented 2 years ago

@ysfaran I had a look at the PR this morning I think what you implemented is already possible to do.

// grab context from the third argument
validate: ({ number }, _args, context) => ({
  id: number().test(
    'is-allowed-id',
    'given id is not allowed',
    // no need to grab context from here
    (value: number) => {
      return context.user.id === value;
    }
  ),
}),

You can access the context in the third argument on the validate method.

Does this solve your problem or am I missing something?

ysfaran commented 2 years ago

@filipstefansson thanks for looking into it, but unfortunately not. What you are suggesting is what I meant with "Alternatives". The problem here is that if you want to reuse is-allowed-id you would have to copy it over and over again.

Let's say you have 10 mutations/queries and all want to validate the userid (as in your example). You would have to copy it 10 times, which gets quite error prone once you want to adapt something.

A better idea in this case would be to use yup.addMethod:

yup.addMethod(yup.string, "allowedUserId", function (userId) {
  return this.test(function(value){
    const allowed = userId === value;;

    if (!allowed) {
      return this.createError({
        message: `${value} is not a valid userId, only ${userId} is allowed`,
      });
    }

    return true;
  });
});

Then you could use it anywhere in your code like this (similar to what I said in "Alternatives"):

validate: ({ number }, _args, context) => ({
    id: number().allowedUserId(context.user.id)
 }),
),

What I want to achieve with this PR is, that you don't need to pass context.user.id every time again because it's always dependent on the context value. With my PR you could adapt the yup.addMethod

- yup.addMethod(yup.string, "allowedUserId", function (userId) {
+ yup.addMethod(yup.string, "allowedUserId", function () {
  return this.test(function(value){
+   const graphQLContext = testContext.options.context
+   const userId = graphQLContext.currencies.includes(value!);
    const allowed = userId === value;;

    if (!allowed) {
      return this.createError({
        message: `${value} is not a valid userId, only ${userId} is allowed`,
      });
    }

    return true;
  });
});

And then:

validate: ({ number }, _args, context) => ({
-  id: number().allowedUserId(context.user.id)
+  id: number().allowedUserId()
  ),
}),

Things are getting worse if you rely on more than just one context value, e.g database clients and so on, which involve more complex logic.

From user side you have no way to inject the GraphQL context during validation because nexus-validate triggers the schema validation:

https://github.com/filipstefansson/nexus-validate/blob/314be02cfdab6c55c7dc1373e68c10d935e18607/src/resolver.ts#L64

So my suggestion was to inject the context (it has no other use case anyway) as shown in my PR.

I hope this makes things clearer 🙂

filipstefansson commented 2 years ago

It does make it clearer, thank you! I'll get the PR merged asap.

peacechen commented 2 years ago

Since this project has been abandoned, I've published an updated version: https://github.com/peacechen/nexus-validate

That's based off of JoosepAlviste's fork which incorporates the context in the validate callback.

I hope interested parties will join as maintainers in the new version.

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 1.3.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: