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

Improve support for validation of optional data structures #35

Open
thibaudcolas opened this issue Aug 22, 2018 · 7 comments
Open

Improve support for validation of optional data structures #35

thibaudcolas opened this issue Aug 22, 2018 · 7 comments
Assignees

Comments

@thibaudcolas
Copy link

thibaudcolas commented Aug 22, 2018

I'm trying and struggling to get the middleware working when validating data structures using optional objects. Here is an example of a JSON payload I'm working with:

{
  "payer" : {
    "id": 123,
    "company": {
      "id": 456
    },
  }
}

The tricky part here is with payer.company: I would like this to be optional – i.e. validate its attributes if it's there, but otherwise a payer with just an id is valid too. I haven't found a good way to do this with the way the middleware defines validators. The correct respect/Validation way would be to use keySet and key:

$validators = [
  'payer' => v::keySet(
    v::key('id', self::idValidator('must be a valid id'))
    v::key('company', v::keySet(
      v::key('id', self::idValidator('must be a valid id'))
    ), false)
  ),
];

This produces the following error list when payer.company is an empty object:

[
  'payer' => [
    'Must have keys { "id" }',
  ]
]

The error message is what I would expect, but since the middleware operates with a validator defined on payer, the messages' key set to "payer" does not point to what is causing the error. Generally, anything defined as a keySet causes this loss in accuracy of the error keys, since Slim-Validation has no awareness of how validators are defined within a keySet.

This might not be very problematic for a simple structure like this, but of course my real-world use case has a much bigger payload and more levels of nested objects. Is there anything I could do differently to make this work?

@DavidePastore
Copy link
Owner

Hi @thibaudcolas. First of all thanks for using Slim-Validation! It seems that your case is not handled in the way we expect by Respect/Validation itself if you only use the keySet rule. I tried this code to verify what I'm saying:

use Respect\Validation\Exceptions\NestedValidationException;
use Respect\Validation\Validator as v;

$request = [
    'payer' => [
        'id' => 123,
        'company' => []
    ]
];

try {
    v::keySet(
        v::key('payer', v::keySet(
            v::key('id', v::numeric()),
            v::key('company', v::keySet(
                v::key('id', v::numeric())
            ), false)
        ))
    )->assert($request);

    echo "Everything is ok!";
} catch (NestedValidationException $exception) {
    print_r($exception->getMessages());
}

but the printed error is this one:

Array
(
    [0] => Must have keys { "id" }
)

that doesn't explicit tell you what is the position of the id (under payer.company key).

In Slim-Validation you can use the optional rule to obtain your expected result:

$validators = [
   'payer' => [
      'id' => self::idValidator('must be a valid id'),
      'company' => v::optional(v::keySet(
         v::key('id', self::idValidator('must be a valid id'))
      ))
   ],
];

In the case your input is:

{
  "payer" : {
    "id": 123,
    "company": {}
  }
}

you'll get the given error:

[
   'payer.company' => [
      'Must have keys { "id" }',
   ]
]

I hope I have answered your question.

@thibaudcolas
Copy link
Author

thibaudcolas commented Aug 24, 2018

Thank you for taking the time to look into this 🙂. I ended up doing exactly what you suggests, it works for that example but breaks down as soon as there is more nesting:

{
  "payer" : {
    "id": 123,
    "company": {
      "contact": {
        "id": 123
      }
    }
  }
}

Here, if I want to validate that payer.company.contact or payer.company have an id, I'll have the same issue.

I converted my validator into something like:

$validators = [
    'payer' => [
        'id' => v::optional(self::idValidator()),
        'consumer' => v::optional(self::silentAllOf(
            v::key('id', self::idValidator()),
            v::key('nationality', self::countryCodeValidator())
        )),
    ],
];

That works the same as keySet, except it doesn't throw errors if extra keys are added to payer.consumer. Ultimately this is an issue with respect/Validation though, not the middleware.


Investigating further, trying out both the middleware and the validation library on its own, I believe this is simply a fundamental limitation of respect/Validation: It just doesn't seem to be good at reporting useful error messages. I tried to summarise what I ran into over here: Respect/Validation#847 (comment).

Coming back to the middleware, what was problematic for me is that grouping errors by key ('payer.company' => ['some error']) led me to think I would be able to map each error onto the specific field that's causing this in my JSON. It wasn't clear to me that this would not work with inputs that have any kind of dynamic data structure (arrays of arbitrary length, and optional objects) – as soon as you have to use respect/Validation's "array/object" iterators it's not possible for the key to point to the exact field.


Trying to find a way to solve this, I think it might be possible to change the middleware to:

  • Validate requests by iterating through the input's structure, rather than the validators' structure. Then you always know what path you're on when an error is raised.
  • Have an API to define the validators that makes it possible to handle dynamic data structures without using the corresponding helpers from respect/Validation.

Iterating through the input's structure is easy enough, the hard part is having an API in the middleware that makes it possible to set validators for any path in the input.

As a thought experiment, trying to keep backwards compatibility with the current API, defining validators could look something like this:

(explanation below)

$validators = [
  'payer' => [
    'company?' => [
      '__self__' => v::keySet(v::key('id'), v::key('contact', false)),
      'contact?' => [
        'id' => v::intType(),
        'name' => v::stringType(),
      ],
      'emails' => [
        v::email(),
      ],
      'phoneNumbers' => [
        '__self__' => v::arrayType(),
        [
          'countryCode' => v::stringType(),
          'number' => v::phone(),
        ],
      ],
    ],
  ],
];
  • ? at the end of a key treats the value as optional, like v::optional would.
  • In an associative array, the __self__ key can be used to add validation to the array/object itself – here, the validation rules defined at $validators['payer']['company']['__self__'] will apply to the JSON path payer.company – while other validation rules below are applied to keys within payer.company.
  • In an indexed array, index 0 is the rule to apply to all items of the array.
    • We can add __self__ here too to validate the array itself instead of its content (e.g. validate length).
    • If array items are objects, we can pass an array of validators as index 0 and they'll be applied to all items.

I'm sure there are cases I'm forgetting about, but an API like this would make it possible to define (and resolve) validators for paths like payer.company.phoneNumbers[3].countryCode with optional object nesting, and support for arrays.

I might try and implement this some time, but time is scarce and my PHP skills are quite poor so we'll see 😄.

@DavidePastore DavidePastore reopened this Aug 25, 2018
@DavidePastore
Copy link
Owner

Wow! 😮 Your comment adds a lot of useful things. I think the solution you are proposing is really interesting and I'd be curious to see a pull request in this regard. Right now I do not think there are any problems but only a lot of tests in various scenarios could help us to understand it (e.g. what happens if you don't want that your input contains more keys at payer level?).

@thibaudcolas
Copy link
Author

:) I’ll try to make a proof of concept if I find time. It might not be too much work if I manage to overcome my PHP struggles.

@lode
Copy link

lode commented Mar 27, 2019

If it is originally json you want to validate, you could also use https://github.com/justinrainbow/json-schema.

You can for example feed it a schema to validate against like this:

{
	"type": "object",
	"properties": {
		"insert": {"type": "string"},
		"attributes": {"type": "object"}
	},
	"required": ["insert"],
	"additionalProperties": false
}

I made a custom rule to work with this:

class JsonSchema extends AbstractRule {
	/** @var \JsonSchema\Validator **/
	public $validator;
	
	/** @var string **/
	protected $schema;
	
	/**
	 * @param string $schemaPath
	 */
	public function __construct($schemaPath) {
		$this->validator = new \JsonSchema\Validator();
		$this->schema    = [
			'$ref' => 'file://'.$schemaPath,
		];
	}
	
	public function validate($input) {
		if (is_string($input)) {
			$input = json_decode($input);
		}
		
		$this->validator->validate($input, $this->schema);
		
		return $this->validator->isValid();
	}
}

@DavidePastore
Copy link
Owner

@lode This is really interesting! What do you think if you publish this rule as a separate package? I would be more than happy to depend on it if it simplify things. 😄

@lode
Copy link

lode commented Apr 7, 2019

The rule is just the code shown above. The actual validation is done by https://github.com/justinrainbow/json-schema.

Further the specifics of this class depend a lot on how you want to use it. E.g. in my context I'm passing a $schemaName and the constructor knows where to find the path, I've added extra methods to get insights in the error for passing it to a JSON:API output, and even the is_string($input) is something that depends on your context.

I think it is hard to make a generic package, maybe it works better in a Slim context. But I don't know Slim well enough, feel free to use the example to make something useful!

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

No branches or pull requests

3 participants