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

Pass the top-level inputs object to the predicate functions as a third argument. #102

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

Conversation

boyko
Copy link

@boyko boyko commented Jul 3, 2018

Hello and thanks for this great library!
A while ago I needed to validate a structure like this one:

const obj = {
	person: { id: 'someId'},
    car: {
    	owner: 'someId'
    }
}

where the car.owner needs to match person.id in order to
pass validation. Maybe I got it totally wrong but my first attempt was:

const validationRules = {
	car: {
		owner: [
			[
				(value, inputs) => {
						if (value === inputs.person.id) {
								return true;
						}
						return false;
				},
				'Car not owned by person.'
			]
		]
	}
}

It failed because the predicate function is only passed the car object and not the whole input.

This PR changed the validation function so that it passes the top-level input as a third argument to the predicate functions so that the above validation problem can be tackled like this:

const validationRules = {
	car: {
		owner: [
			[
				(value, input, allInputs) => {
						if (value === allInputs.person.id) {
								return true;
						}
						return false;
				},
				'Car not owned by person.'
			]
		]
	}
}

@busypeoples
Copy link
Owner

Thanks for the great input @boyko
I think this is very similar to #100 from @davidchase
Which would also solve the problem mentioned in above issue more elegantly.
I will have a look at your implementation tomorrow or Friday and either provide feedback or straight merge.

@davidchase
Copy link

@boyko @busypeoples 👋 this looks really great! i havent tested it out in my scenario just yet (busy @ work) but this could be it thanks a bunch 😄

@busypeoples
Copy link
Owner

Didn't have the time to run this. Will try to check it out today or tomorrow and then merge.

@benneq
Copy link

benneq commented Jan 19, 2019

The 2nd argument of the predicate is the "parent" object. I don't think that "allInputs" as 3rd argument is a good idea in general.

I'd go for some kind of "composable validation specs". What I mean: Let's say, that you want to reuse your personEqualsCarOwnerRule in some other context, then you cannot know where inside the allInputs your personEqualsCarOwnerRule is.

Example:

const data1 = {
  person: { id: 'someId' },
  car: { owner: 'someId' }
};

const data2 = {
  foo: {
    person: { id: 'someId' },
    car: { owner: 'someId' }
  }
};

const personEqualsCarOwnerRule = {
  person: [[...]]
  car: {
    owner: [[
      (input, allInputs) => ???, // for data1 it would check "allInputs.person.id"
      // but for data2 it would check "allInputs.foo.person.id"
      // this means: you cannot reuse this rule that easy.
      'Car not owned by person.'
    ]]
  }
}

spected(data1, personEqualsCarOwnerRule);
spected(data2, { foo: personEqualsCarOwnerRule });

Of course you could use something like this with allInputs:

const personEqualsCarOwnerRule = (path) => ({
  person: [[...]]
  car: {
    owner: [[
      (input, allInputs) => ???, // then use "allInputs[path].person.id"
      'Car not owned by person.'
    ]]
  }
})

spected(data2, { foo: personEqualsCarOwnerRule("foo") });

But this doesn't look nice for deeply nested stuff. And it gets really hard when it comes to arrays:

const data3 = {
  bar: [
    {
      foo: {
        person: { id: 'someId' },
        car: { owner: 'someId' }
      }
    },
    ...
  ]
}

Now you've got to access allInputs.bar[0].foo.person.id.


I think a much better solution would be if we were able to access parent, parent.parent, parent.parent.parent, ... within the predicate.

Then you could simply write:

const personEqualsCarOwnerRule = () => ({
  person: [[...]]
  car: {
    owner: [[
      (input, parent, parentParent) => ???, // use "parentParent.person.id"
      'Car not owned by person.'
    ]]
  }
})

And this rule can now be applied to all objects with a { person: { id }, car: { owner } } structure.

Though I'm not sure if passing parent, parentParent, parentParentParent, ... as arguments to the predicate is the way to go.

Another possibility would be something like this:

const personEqualsCarOwnerRule = () => ({
  person: [[...]]
  car: {
    owner: [[
      (input, ancestors) => ???, // where ancestors is an array
      // and each index contains one level deeper parent:
      // ancestors[0] === car object === { owner }
      // ancestors[1] === car's parent object === { person: { id }, car: { owner } }
      // So you would use `ancestors[1].person.id`
      'Car not owned by person.'
    ]]
  }
})

Maybe there are other solution, too, that are even nicer to use.


EDIT: Another solution could be some kind of ref. I don't know if this could be implemented somehow. It's just an idea:

const personEqualsCarOwnerRule = () => {
  const ref = { actualRef: null };
  return {
    person: compose(createRef(ref), [[...]]) // compose will just somehow (magically)
    // run the "createRef" and then return the validations array.
    // "createRef" will simply set the "actualRef" within the "ref" object.
    car: {
      owner: [[
        (input) => ???, // use "ref.actualRef.id"
        'Car not owned by person.'
      ]]
    }
  }
}

@busypeoples
Copy link
Owner

Thanks @benneq and @boyko for all the input! Let's think about the possible ways to tackle this.

@benneq
Copy link

benneq commented Jan 20, 2019

For maximum flexibility, I really like the ref approach. This way, you don't have to rely on the object structure at all. Because you can reference everything you like.

But for refs, we need #104 / #106 , I guess.

Using ref would be pretty similar to using allInputs: You could simply create a ref of the root and call it allInputs. But additionally you are also able to create a ref for a smaller part of the object tree.


The parent, parent.parent, ... stuff is nice, too. Because it's simple. But it will become pretty ugly when you have deep objects trees. And: You lose some flexibility which you could have with refs.

Example (pseudo code):

const fooRule = {
  bar: ...,
  baz: ...
}

const rootRule = {
  a: ...,
  b: ...,
  foo: fooRule,
  nested: {
    xyz: fooRule
  }
}

Now you want to create some rule for fooRule.bar:

  • Using parent you only can access the contents of the fooRule object for sure. Because you cannot know for sure what parent.parent is: It could be rootRule or rootRule.nested.
  • Using ref you could do whatever you want: Make fooRule a function that receives a ref as argument. Like this:
const fooRule = (ref) => {
  return {
    bar: ..., // use ref here
    baz: ...
  }
}

const rootRule = () => {
  const aRef = ...;
  return {
    a: ..., // createRef here
    b: ...,
    foo: fooRule(aRef),
    nested: {
      xyz: fooRule(aRef)
    }
  )
}

Providing allInputs, as I already said above, doesn't feel nice. Yes, it will work, if you have a single static validation object and you are 100% sure about its structure. But it will fail if you want to build small composable rules, and then combine them.


Looking at other validation libs: For example yup is using string refs. 1. I don't like to use strings for matching field by name. 2. it only supports siblings (and their children). So it's basically the same as the current 2nd arg for the predicate of spected.

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.

None yet

4 participants