Unexpected InvalidJsonPointerException when no errors limit set and required property is missing

[llpo]

I wouldn't expect an exception Opis...\InvalidJsonPointerException, when a missing path/property is required. For me, required properties should have a higher "priority" and references should be checked only after requirements of a higher "order" are meet.

If I take your example and change the $data variable (remove "password") and set no errors limit (-1) I get such exception:

use Opis\JsonSchema\{
    Validator,
    ValidationResult,
    ValidationError,
    FilterContainer,
    IFilter
};

$filters = new FilterContainer();

$filters->add("string", "sameAs", new class implements IFilter {
    /**
     * @inheritDoc
     */
    public function validate($data, array $args): bool {
        return isset($args['value']) && $data === $args['value'];
    }
});

$validator = new Validator();
$validator->setFilters($filters);

$schema = <<<'JSON'
{
    "type": "object",
    "properties": {
        "password": {
            "type": "string"
        },
        "passwordRepeat": {
            "type": "string",
            "$filters": {
                "$func": "sameAs",
                "$vars": {
                    "value": {
                        "$ref": "1/password"
                    }
                }
            }
        }
    },
    "required": ["password", "passwordRepeat"]
}
JSON;

$schema = json_decode($schema, false);

$data = (object) [
    // "password" => "secret",
    "passwordRepeat" => "secret"
];

/** @var ValidationResult $result */
$result = $validator->dataValidation($data, $schema, -1 /* no limit of errors /*);
if ($result->isValid()) {
    echo "Valid", PHP_EOL;
} else {
    echo "Invalid", PHP_EOL;
}

I would expect a validation error (ValidationError) instead. In my opinion, an exception would be appropriate only when "password" is not required (and missing of course).

[sorinsarca]

For me, required properties should have a higher "priority" and references should be checked only after requirements of a higher "order" are meet.

This is happening right now, but when using max_errors with a value greater than 1, the previous error is ignored and the validation keeps going on like nothing happened.

I would expect a validation error (ValidationError) instead. In my opinion, an exception would be appropriate only when "password" is not required (and missing of course).

This might solve your problem, but it can also introduce unexpected behavior. According to json-pointer specs, when resolving a json pointer and the value is nonexistent I have only two options: either throw an error/exception and stop, or use a fallback value and continue.

However, you can look at this issue from two different angles

Perspective A

The problem is that the json pointer references a nonexisting value, so this can be solved in two ways:

1. Remove the reference

This is the quickest solution and requires no change in the opis/json-schema lib. However, you must create a new filter and move it one level up

{
   "type": "object",
   "properties": {
       "password": {"type": "string"},
       "repeatPassword": {"type": "string"}
   },
   "required": ["password", "repeatPassword"],
   "$filters": {
       "$func": "propsAreEqual",
       "$vars": {"left": "password", "right": "repeatPassword"}
   }
}

And the filter in php

// propsAreEqual
function ($value, array $args): bool {
   if (!property_exists($value, $args['left'])) {
       return false;
   }
   if (!property_exists($value, $args['right'])) {
       return false;
   }
   return $value->{$args['left']} === $value->{$args['right']};
}

2. Fallback to a default value

This requires a small change in the opis/json-schema lib in order to provide a fallback value.

Example of schema (if the changes are made)

{
    "type": "object",
    "properties": {
        "password": {"type": "string"},
        "passwordRepeat": {
            "type": "string",
            "$filters": {
                "$func": "sameAs",
                "$vars": {
                    "value": {
                        "$ref": "1/password",
                        "default": null
                    }
                }
            }
        }
    },
    "required": ["password", "passwordRepeat"]
}

Now, when the 1/password is nonexistent and the default keyword is present the value provided by the default will be used instead of throwing an InvalidJsonPointerException, in our case 1/password will be considered null and the filter will handle the validation further.

Perspective B

The problem is that max_errors (errors limit) parameter is recommended only in debug mode, in production is recommended to stop at first error.

Well, this might not sound so good if you are trying to validate a form and display the appropriate error messages for every form field.
I have no idea where and how you want to use the validation, but for a form I'll recommend to use client
side validation and the server to respond only with the first error. Having unlimited errors will only slow down the validation on server side, and the end user can receive too many errors at once. In my opinion,
fixing errors one by one is the best approach for end user (if is a human).

In the next major release the max_errors might be removed so it is not recommended to rely on it.

[llpo]

Thank you for your detailed response, I evaluated and tested your suggestion (perspective A) before submitting this issue, but it didn't feel elegant, and I wanted to make you aware of it, because the behaviour I expect seems more consistent to me, and maybe for other people as well.

Well, this might not sound so good if you are trying to validate a form and display the appropriate error messages for every form field.

Yes I want that, I intent do validate a client request (browser) and I want to return in the same response as much errors as possible .

I have no idea where and how you want to use the validation, but for a form I'll recommend to use client side validation and the server to respond only with the first error.

This option isn't good for me, error validation belong to the backend in my case, mainly to keep consistency and to prevent logic duplication.

Having unlimited errors will only slow down the validation on server side, and the end user can receive too many errors at once. In my opinion, fixing errors one by one is the best approach for end user (if is a human).

Well I see it a bit different. If the user receive too many errors depends of the interface/layout/design/... and can be very subjective. We don't need to discuss about that. In my case I want to give feedback immediately and try to avoid the user gets new errors on each (request) try, what can be quite annoying.

In the next major release the max_errors might be removed so it is not recommended to rely on it.

Ok, I understand why you plan to remove max_errors, if you remove it there is no need to fix/change anything. Thank you very much.

[sorinsarca]

I evaluated and tested your suggestion (perspective A) before submitting this issue, but it didn't feel elegant, and I wanted to make you aware of it, because the behaviour I expect seems more consistent to me, and maybe for other people as well.

Well, to me looks like it can lead to undefined behaviour because it is not explicit (like A.2.) but it relies on other keywords (such as "required"). On a complex schema it is hard to figure what is really going on.

Ok, I understand why you plan to remove max_errors, if you remove it there is no need to fix/change anything.

I've already proposed a change (A.2. Fallback to a default value) to fix this issue, so there is no reason to say that. Plus, a PR is always welcome if you have better idea/solution.

On the other hand, there can be an A.3. solution: since passwordRepeat depends on password you can use dependencies keyword.

{
    "type": "object",
    "properties": {
        "password": {"type": "string"}
    },
    "required": ["password", "passwordRepeat"],
    "dependencies": {
      "password": {
         "properties": {
           "passwordRepeat": {
                "type": "string",
                "$filters": {
                    "$func": "sameAs",
                    "$vars": {
                        "value": {
                            "$ref": "1/password"
                        }
                    }
                }
            }
         }
      }
    }
}

[msarca]

I wouldn't expect an exception Opis...\InvalidJsonPointerException, when a missing path/property is required. For me, required properties should have a higher "priority" and references should be checked only after requirements of a higher "order" are meet.

We do have a hierarchical error system, but at a deeper analysis it seems that max_errors is preventing it from doing its job properly. The issue is not with max_errors itself, but with the way we apply max_errors. This means that max_errors will not be removed after all.

[KodyWiremane]

I would say A.1 has an advantage over A.2 in the sense that A.1 allows comparing values of any type, while "default": null may interfere with nullable values. Also it looks good in the sense that with max_errors === 1 the error will point to a missing required property (the root cause), while with the nested filter the error will be slightly ambiguous.

Can it be so that a developer cannot ensure existence of a property, but still refers to it with a JSON pointer? Maybe just fail the "$filter" in such a case? Surely if a filter cannot retrieve its variables, it fails. Maybe add some explanatory argument into keywordArgs, like "cause" => "filter failed" | "cause" => "invalid pointer $pointer".

[sorinsarca]

In v2.0 you can use the data-exists filter to see if your json-pointer points to something. Just wrap it with an if-then-else

{
  "if": {
    "$filters": {
      "$func": "data-exists",
      "$vars": {
        "ref": "/path/to/data"
      }
    }
  },
  "then": {
    "$comment": "/path/to/data exists"
  },
  "else": {
    "$comment": "cannot resolve /path/to/data"
  }
}