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

An error occurred while verifying the array #525

Closed
zh-five opened this issue Jul 20, 2018 · 7 comments
Closed

An error occurred while verifying the array #525

zh-five opened this issue Jul 20, 2018 · 7 comments

Comments

@zh-five
Copy link

zh-five commented Jul 20, 2018

        $data = [1,3,4];
        $schema = [
            "type"  => "array",
            "items" => [
                "type"      => "string",
                "minLength" => 5,
            ],
            'minItmes' => 1,
        ];

        // Validate
        $validator = new JsonSchema\Validator();
        $validator->check($data, (object)$schema );

        if ($validator->isValid()) {
            echo "The supplied JSON validates against the schema.\n";
        } else {
            echo "JSON does not validate. Violations:\n";
            foreach ($validator->getErrors() as $error) {
                echo sprintf("[%s] %s\n", $error['property'], $error['message']);
            }
        }

//output
//The supplied JSON validates against the schema.

test by PHP5.6

@shmax
Copy link
Collaborator

shmax commented Jul 27, 2018

@zh-five which version of the library are you using? I just tried your sample with the latest version and the schema failed to validate as expected.

@shmax
Copy link
Collaborator

shmax commented Jul 27, 2018

Scratch that, the schema does fail validation, but there's some kind of problem with isValid and getErrors. More soon...

@shmax
Copy link
Collaborator

shmax commented Jul 28, 2018

@erayd This will probably sound familiar, but the issue here is that he's passing in an associative array for the schema (the (object) does a shallow cast), and the items gets interpreted as an array of item configurations instead of a single item configuration with keys. The code tries to loop over the array in good faith and gets confused. I can fix this fairly easily, but none of the tests are set up for it; they all expect a JSON string, which gets deeply converted to an object.

Not sure how to proceed. We could:

  1. manually force all schemas to deep objects under the hood
  2. add a new tier of tests that operate on an associative schema
  3. fix the one spot where it breaks and not worry about it (the tests still pass, after all)

Any thoughts?

@erayd
Copy link
Contributor

erayd commented Jul 28, 2018

@shmax I think we call this user error, and don't fix it.

  1. There is no case in which an associative array is a valid schema;
  2. Deep casting assoc arrays to objects is expensive
  3. Casting requires us to guess whether a numerically-indexed array is associative or not.

That last point is the important one for me I think - guessing feels inappropriate in this case. There's nothing preventing the user from doing their own deep casting, and the user knows much more about their schema than we do - so they can make the correct decisions rather than blindly guessing and hoping it's right, which is what we would have to do.

@shmax
Copy link
Collaborator

shmax commented Jul 28, 2018

It's user error, but you can't really blame him for it; all of the samples in the main README support what he's doing. The only hint suggesting the more recommended usage I've noticed is a vaguely-worded comment on validate (not check). As a long-time user of this library I probably should have known better right away myself, but I had to work with the debugger for an hour to figure out what was going on.

Deep casting may be slow, but we seem to be encouraging it as a best practice in our own documentation, and furthermore, it turns out we're already resigned to taking a stab at it in at least one place (it doesn't save us here, because is_array doesn't do a deep check), anyway:
https://github.com/justinrainbow/json-schema/blob/master/src/JsonSchema/Constraints/SchemaConstraint.php#L45

I fired up my own codebase and dug around a bit. I have no idea what other people do, but I encode my schemas as php files. As you know, PHP doesn't really have what you would call an object literal, so I have to use associative arrays. To convert them to objects, I do one of these:

$schema = json_decode(json_encode($arr),false)

I added a timer on one of my heavier pages and totaled up the time spent on deep casting about 50 calls. The total was just under a tenth of a second. Not terrible, especially with the debugger on on a local machine, but given a preference I would rather not have to cast at all and get that time back.

Even if I were to store JSON files (which I can't really do, because my schema files are littered with const references and other bits of domain knowledge from my PHP codebase), I would still have to run them through json_decode, which is one half of what we're trying to avoid.

There is no case in which an associative array is a valid schema

Can you expand on that a bit? I'm wondering if we should just make a reasonable effort to support associative schemas. That would remove the need to cast from my end, and issues like the one we have here could be tested and dealt with.

@DannyvdSluijs
Copy link
Collaborator

@erayd / @shmax maybe can pick this one up to make a decision and complete the issue?

I get the points both of you are making. But maybe want to add one consideration which is availability/time. Since this project is being not very active, opening up the deep casting option can lead to more bugs/questions etc. Which in my opinion will require even more time.

Picking the option to not support shallow casted objects nor do deep casting on the users behalf is the safest option and would only require once in a while answering to an issue report that we don't support such scenarios

@shmax
Copy link
Collaborator

shmax commented Feb 9, 2024

I think the implicit decision made by @erayd all those years ago was to consider it user error, which is fine with me. Feel free to close.

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

No branches or pull requests

4 participants