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

[WIP] Add support of nested logical combinators (fix #8) #11

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

FluorescentHallucinogen
Copy link
Contributor

@FluorescentHallucinogen FluorescentHallucinogen commented Feb 8, 2019

This pull request adds support of nested logical combinators OR, AND and NOT to create an arbitrary logical combination of validators:

type Mutation {
  createUser (
    name: String! @constraint(where: {minLength: 5, maxLength: 40})
    bio: String @constraint(
      where: {
        OR: [{
          AND: [{
            startsWith: "foo"
          }, {
            endsWith: "bar"
          }]
        }, {
          contains: "baz"
        }]
      }
    )
  ): User
}

The syntax is the same as used in Prisma: https://www.prisma.io/docs/prisma-graphql-api/reference/queries-qwe1/#combining-multiple-filters.

@FluorescentHallucinogen
Copy link
Contributor Author

@vsimko Need your help. I'm stuck on mapObjIndexed and implementing OR, AND, NOT function bodies.

Could you please help implement them? 🙏 And I'll do the rest of the dirty work: updating tests, docs, etc.? 😉

@vsimko
Copy link
Owner

vsimko commented Feb 8, 2019

(at least the build is passing tests now)
As you can see, I changed the API a little bit to be flatter and backwards compatible with the current stable version. The original "flat" arguments are supported as well as the new nested API:

type Query {
  createUser (
    id: ID!
    name: String! @constraint(minLength: 5, maxLength: 40)
    bio: String @constraint(
      where: {
        OR: [{
          AND: [{
            startsWith: "foo"
          }, {
            endsWith: "bar"
          }]
        }, {
          contains: "baz"
        }]
      }
    )
  ): User
}

It would be nice to:

  • avoid the "where" clause
  • make AND implicit e.g. {min:1, max:2}, [{contains:"x"}, {contains:"y"}]

The current implementation needs to be completely changed.
Now it iterates linearly through the arguments and collects errors (thus the use of mapObjIndexed).
For the nested API, the implementation should be recursive by traversing the tree.

@FluorescentHallucinogen
Copy link
Contributor Author

  • make AND implicit e.g. {min:1, max:2}, [{contains:"x"}, {contains:"y"}]

The AND can be omitted since validators are combined via AND by default. I.e.

@constraint(
  where: {
    OR: [{
      AND: [{
        startsWith: "foo"
      }, {
        endsWith: "bar"
      }]
    }, {
      contains: "baz"
    }]
  }
)

is equivalent to

@constraint(
  where: {
    OR: [{
      startsWith: "foo",
      endsWith: "bar"
    }, {
      contains: "baz"
    }]
  }
)

@vsimko
Copy link
Owner

vsimko commented Feb 8, 2019

These lines need to be re-implemented using recursion:
https://github.com/FluorescentHallucinogen/node-graphql-constraint-lambda/blob/c4a9f09ccf7a643f9de7bb14d75be4090f01fe11/src/index.js#L105-L107

          mapObjIndexed((cVal, cName) =>
            validationCallback({ argName, cName, cVal, data: valueToValidate })
          )

The error messages collected from nested logical operators need to be flattened.

What should happen with errors:

  • "NOT": should throw away all errors from its children, and generate a single error message e.g. "subtree violation" if there were no errors.
  • "AND": should pass unchanged errors up to the root.
  • "OR": needs to know if all its children generated at least one error, only then it should pass all of them up to the root. Otherwise it should discard all errors from its children.

@FluorescentHallucinogen
Copy link
Contributor Author

  • avoid the "where" clause

Not sure if this is possible (for nested logical combinators) due to the limitations of GraphQL schema syntax. Need to research.

@FluorescentHallucinogen
Copy link
Contributor Author

@vsimko

The current implementation needs to be completely changed.
These lines need to be re-implemented using recursion:

Could you please do that? I'm not sure I can do it in a reasonable time.

@FluorescentHallucinogen
Copy link
Contributor Author

@vsimko

It would be nice to:

  • avoid the "where" clause
  • make AND implicit

Done. I've figured out. 😉 The current syntax is:

@constraint(
  OR: [{
    startsWith: "foo",
    endsWith: "bar"
  }, {
    contains: "baz"
  }]
)

@thelinuxlich
Copy link

Hey let's merge this :)

@vsimko
Copy link
Owner

vsimko commented Nov 10, 2021

I would like to merge it, but a test is failing.

@FluorescentHallucinogen
Copy link
Contributor Author

@thelinuxlich This #11 (comment) part is still not done. So this PR isn't ready for merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants