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

Error in extended schemas #7

Closed
asargento opened this issue May 17, 2012 · 24 comments
Closed

Error in extended schemas #7

asargento opened this issue May 17, 2012 · 24 comments
Assignees
Labels

Comments

@asargento
Copy link

I was performing some tests of json-schema-validator and found the following behavior.
I defined the following schema

{
"extends":{"$ref" : "http://json-schema.org/draft-03/schema#"},
"properties":{
"indexed":{"type": "boolean", "default":false}
}
}

and after that I try the validate the next schema.

{
"type" : "object",
"properties" : {
"from" : { "type" : "string", "indexed":111 }
}
}

It should give an error, since Indexed is a boolean property and not a integer. And it don't.
The schema is validated and is considered correct.

@fge
Copy link
Collaborator

fge commented May 20, 2012

(what version?)

That sounds strange, for one reason at least: http://json-schema.org/draft-03/schema# does not exist (404).

If I adapt the schema like this:

{
    "extends": {
        "$ref": "#/core"
    },
    "properties": {
        "indexed": {
            "type": "boolean",
            "default": false
        }
    },
    "core": {
        "$schema" : "http://json-schema.org/draft-03/schema#",
        "id" : "http://json-schema.org/draft-03/schema#",
        "type" : "object",

        "properties" : {
            "type" : {
                "type" : ["string", "array"],
                "items" : {
                    "type" : ["string", { "$ref" : "#" } ]
                },
                "uniqueItems" : true,
                "default" : "any"
            },

            "properties" : {
                "type" : "object",
                "additionalProperties" : { "$ref" : "#" },
                "default" : {}
            },

            "patternProperties" : {
                "type" : "object",
                "additionalProperties" : { "$ref" : "#" },
                "default" : {}
            },

            "additionalProperties" : {
                "type" : [ { "$ref" : "#" }, "boolean" ],
                "default" : {}
            },

            "items" : {
                "type" : [ { "$ref" : "#" }, "array" ],
                "items" : {
                    "$ref" : "#"
                },
                "default" : {}
            },

            "additionalItems" : {
                "type" : [ { "$ref" : "#" }, "boolean" ],
                "default" : {}
            },

            "required" : {
                "type" : "boolean",
                "default" : false
            },

            "dependencies" : {
                "type" : "object",
                "additionalProperties" : {
                    "type" : ["string", "array", { "$ref" : "#" } ],
                    "items" : { "type" : "string" }
                },
                "default" : {}
            },

            "minimum" : {
                "type" : "number"
            },

            "maximum" : {
                "type" : "number"
            },

            "exclusiveMinimum" : {
                "type" : "boolean",
                "default" : false
            },

            "exclusiveMaximum" : {
                "type" : "boolean",
                "default" : false
            },

            "minItems" : {
                "type" : "integer",
                "minimum" : 0,
                "default" : 0
            },

            "maxItems" : {
                "type" : "integer",
                "minimum" : 0
            },

            "uniqueItems" : {
                "type" : "boolean",
                "default" : false
            },

            "pattern" : {
                "type" : "string",
                "format" : "regex"
            },

            "minLength" : {
                "type" : "integer",
                "minimum" : 0,
                "default" : 0
            },

            "maxLength" : {
                "type" : "integer"
            },

            "enum" : {
                "type" : "array",
                "minItems" : 1,
                "uniqueItems" : true
            },

            "default" : {
                "type" : "any"
            },

            "title" : {
                "type" : "string"
            },

            "description" : {
                "type" : "string"
            },

            "format" : {
                "type" : "string"
            },

            "divisibleBy" : {
                "type" : "number",
                "minimum" : 0,
                "exclusiveMinimum" : true,
                "default" : 1
            },

            "disallow" : {
                "type" : ["string", "array"],
                "items" : {
                    "type" : ["string", { "$ref" : "#" } ]
                },
                "uniqueItems" : true
            },

            "extends" : {
                "type" : [ { "$ref" : "#" }, "array" ],
                "items" : {
                    "$ref" : "#"
                },
                "default" : {}
            },

            "id" : {
                "type" : "string",
                "format" : "uri"
            },

            "$ref" : {
                "type" : "string",
                "format" : "uri"
            },

            "$schema" : {
                "type" : "string",
                "format" : "uri"
            }
        },

        "dependencies" : {
            "exclusiveMinimum" : "minimum",
            "exclusiveMaximum" : "maximum"
        },

        "default" : {}
    }
}

and run this simple main program:

    public static void main(final String... args)
        throws IOException
    {
        final JsonNode schemaNode = JsonLoader.fromResource("/t.json");
        final JsonNode data = JsonLoader.fromResource("/t2.json");

        final JsonSchema schema = JsonSchema.fromNode(schemaNode);

        final ValidationReport report = new ValidationReport();

        schema.validate(report, data);

        for (final String msg: report.getMessages())
            System.out.println(msg);

        System.exit(0);
    }

then the output is as expected:

#/properties/from/indexed: instance does not match any allowed primitive type

Care to show your code?

@asargento
Copy link
Author

The version that I am using is 0.5.0 beta 3.
Then, about the code. I have three files, schema-draftv3.json, schema1.json and schema2.json.
The first file has the schema drat v3 (your #/core) and other files have the following contents

schema1.json

{
     "extends":{"$ref" : "http://json-schema.org/draft-03/schema#"},
     "properties":{
    "indexed":{"type": "boolean", "default":false} 
     }
}

schema2.json

{
    "type" : "object",
    "properties" : {
        "from" : { "type" : "string", "required":true, "indexed":111 }
    }
}

My test program is

JsonNode draftv3 = JsonLoader.fromFile(new File("schema-draftv3.json"));
JsonNode schema1 = JsonLoader.fromFile(new File("schema1.json"));
JsonNode schema2 = JsonLoader.fromFile(new File("schema2.json"));

JsonSchema schema = JsonSchema.fromNode(draftv3);
schema.validate(valReport, schema1);
for (String msg : valReport.getMessages())
    System.out.println(msg);

JsonSchema temp1schema = JsonSchema.fromNode(schema1);
temp1schema.validate(valReport, schema2);
for (String msg : valReport.getMessages())
    System.out.println(msg);

This the test that I was performed. Maybe I am making something wrong.
One question? If http://json-schema.org/draft-03/schema was not found, then that error should be reported, isn't it?

@fge
Copy link
Collaborator

fge commented May 21, 2012

About your question: the JsonReference class maitains a map of schemas with URIs if an id is found, which is program-wide. Which means, as schema-draftv3.json has "http://json-schema.org/draft-03/schema#" in "id", the value will be cached, and it will not be looked up over the net.

That's probably a misfeature, I don't know. This part of the implementation is clearly not very nice. But that explains why you don't see an error.

As to the behaviour you see, I'll try and reproduce it.

@asargento
Copy link
Author

Ok, thanks for your help.

@fge
Copy link
Collaborator

fge commented May 21, 2012

OK, I have reproduced the bug...

Fixing it will not be easy :/ The implementation suffers a design problem AFAICS. This is a first impression, unfortunately I don't have the required time to debug the issue.

I'll update tonight.

@fge
Copy link
Collaborator

fge commented May 21, 2012

"I don't have the required time to debug the issue"

I meant, I don't have the time at the moment.

$ref is a bitch :/

@fge
Copy link
Collaborator

fge commented May 21, 2012

OK, I have found the source of the bug, and it is quite serious -- fixing it will require refactoring... JsonSchema as it is cannot handle this case.

I make this a top priority item. Thanks a lot for the reproducer. I'll keep you informed.

fge pushed a commit that referenced this issue May 21, 2012
Quite serious, and I don't even understand the whole picture leading to this
bug, but it is clearly there.
@asargento
Copy link
Author

Ok, if you need any help please fell free to ask.

@fge
Copy link
Collaborator

fge commented May 21, 2012

One question: do you use maven to grab artifacts or do you git clone?

If I need to have a fix tested, this will make a difference...

@asargento
Copy link
Author

I use git clone.

@ghost ghost assigned fge May 21, 2012
fge added a commit that referenced this issue May 24, 2012
There is really no other word for it.

Maven's unit test plugin (I don't know what its name is yet, or even whether
more than one such plugin exists) will not even _run_ a damn test if the _class_
doesn't have Test in its name. Rename the class to work around this f*cking
brain damaged reasoning. I use TestNG, damnit, and TestNG has annotations to
tell whether a class, or method, is a test. Maven, this is the 21st century,
wake up!

I'm sorry, Maven, but you're fired. You are just too stupid for my tastes. It's
just that I don't know enough about gradle yet to switch to it.
fge pushed a commit that referenced this issue Jul 5, 2012
This will allow to concentrate for now on issue #7 problems, but is an
indication that the whole registry/container/node mechanism needs some rework.
fge added a commit that referenced this issue Jul 11, 2012
This method is going to become obsolete.

This means, though, that we don't have a performance test anymore, nor the test
for issue #7.
fge added a commit that referenced this issue Jul 11, 2012
A redesign is in the pipe, which means these tests will need to be modified.
fge added a commit that referenced this issue Jul 11, 2012
With all these changes, the context is now properly transferred, which means
"extends" actually works.
@fge
Copy link
Collaborator

fge commented Jul 11, 2012

OK, the bug is fixed, but the API needs refining before I put it in the wild.

In the meanwhile, you can try and see about the new API in Issue7Test. It is not final yet.

@asargento
Copy link
Author

Fine. I will check it.

@asargento
Copy link
Author

The problem is solved.

@asargento
Copy link
Author

Hi. I was upgrading for latest version (yes, I was using a very older version..., 0.5.x) and when testing this issue again I realise that it is reproducible again. Maybe I am doing something wrong since I only adapt the old test to the new version.

@fge
Copy link
Collaborator

fge commented Dec 27, 2012

OK, I admit I am too lazy to be reading this thread from the start again, so could you please post:

  • the version you are currently using,
  • the schema you use,
  • an instance which validates successfully but you think should not validate successfully, and
  • an instance which does not validate successfully but you think it should?

@asargento
Copy link
Author

You wrote a Test case (Issue7TestCase that can be found in commit 7f3b82a) that summarises this issue.

The test case is:

package org.eel.kitchen;

import com.fasterxml.jackson.databind.JsonNode;
import org.eel.kitchen.jsonschema.ValidationContext;
import org.eel.kitchen.jsonschema.schema.JsonSchema;
import org.eel.kitchen.jsonschema.schema.JsonSchemaFactory;
import org.eel.kitchen.jsonschema.util.JsonLoader;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;

import java.io.IOException;

import static org.testng.Assert.*;

public final class Issue7Test
{
private JsonNode draftv3, schema1, schema2;

@BeforeClass
public void setUp()
    throws IOException
{
    draftv3 = JsonLoader.fromResource("/schema-draftv3.json");
    schema1 = JsonLoader.fromResource("/schema1.json");
    schema2 = JsonLoader.fromResource("/schema2.json");
}

@Test
public void testIssue7()
{
    final JsonSchemaFactory factory = new JsonSchemaFactory();

    ValidationContext context;
    JsonSchema schema;

    context = factory.newContext();
    schema = factory.create(draftv3);

    schema.validate(context, schema1);
    assertTrue(context.isSuccess());

    context = factory.newContext();
    schema = factory.create(schema1);

    schema.validate(context, schema2);
    assertFalse(context.isSuccess());
}

}

The schema1.json file is

{
"extends":{
"$ref" : "http://json-schema.org/draft-03/schema#"
},
"properties":{
"indexed":{
"type": "boolean",
"default":false
}
}
}

The schema2.json is

{
"type" : "object",
"properties" : {
"from" : {
"type" : "string",
"required":true,
"indexed":111
}
}
}

The problem is that on schema2, the "indexed" property has an incorrect value 111, instead of true or false.
The schema is validated correctly, but should give an error.
I was using 0.5.3 beta X and now I am using 1.4.1.

@fge
Copy link
Collaborator

fge commented Dec 27, 2012

The code has changed quite a lot since 0.5.x days, the test case you point to cannot work anymore. Can you provide a more up to date example? For instance, based on one of the examples in the org.eel.kitchen.jsonschema.examples package?

@asargento
Copy link
Author

I adapt this test case for the version 1.4.1. Where is the code. Is based on the Examples and is not a junit case.

import java.io.File;
import java.io.IOException;

import com.fasterxml.jackson.databind.JsonNode;
import org.eel.kitchen.jsonschema.main.JsonSchema;
import org.eel.kitchen.jsonschema.main.JsonSchemaFactory;
import org.eel.kitchen.jsonschema.report.ValidationReport;
import org.eel.kitchen.jsonschema.util.JsonLoader;

public class JsonSchemaTest {

public static void main(String[] args) {
    try {
        JsonNode draftv3 = JsonLoader.fromResource("/draftv3/schema");
        JsonNode schema1 = JsonLoader.fromFile(new File("schema1.json"));
        JsonNode schema2 = JsonLoader.fromFile(new File("schema2.json"));

        ValidationReport report;

        JsonSchemaFactory factory = JsonSchemaFactory.defaultFactory();
        JsonSchema draftv3Schema = factory.fromSchema(draftv3);
        report = draftv3Schema.validate(schema1);
        if (!report.isSuccess())
            System.out.println("Schema1: " + report.asJsonObject());

        JsonSchema schema1Schema = factory.fromSchema(schema1);
        report = schema1Schema.validate(schema2);
        if (!report.isSuccess())
            System.out.println("Schema2: " + report.asJsonObject());
    } catch (IOException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }
}

}

@fge
Copy link
Collaborator

fge commented Dec 27, 2012

OK, how does that fail and when? Can you give the full output?

@asargento
Copy link
Author

The problem is that it should fail, but the schema2 is validated correctly.
Basically the all pictures is more less as follows:

  • Define a schema1 that is an extension from draftv3 and that add a new property, 'indexed', that is declared as boolean.
  • Define an instance of schema1, schema2. The value for property 'indexed' is 111.
  • The validator should give an error, since for property 'indexed' only a boolean value should be allowed.

@fge
Copy link
Collaborator

fge commented Dec 27, 2012

OK, I think I am starting to get the whole picture -- and why it does not work.

Before I give a detailed answer, can you confirm one thing: you are trying to validate a schema, and not an instance?

@asargento
Copy link
Author

Yes, I am trying to validate a schema. Sorry, in the previous comment I incorrectly use the term 'instance' to name the schema2.

@fge
Copy link
Collaborator

fge commented Dec 27, 2012

Well, I thought I had figured out why you got this error. Now, after having debugged through a test case I have written (which essentially matches yours), I see my first thought was wrong.

There is a fundamentally flawed logic somewhere in my implementation and I need to figure it out :/

Since this issue is rather old and not relevant to new code anymore, would you mind re-opening a new issue with sample code? I have only checked HEAD at the moment, I am pretty sure that 1.4.x is affected and I surmise 1.2.x is affected as well. But I need to "start anew" here. This buggers me :/

@asargento
Copy link
Author

I have a clue... I think that the validation context is not being created correctly when you have schema that is an extension of other schema.
And I will open an new issue.

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

No branches or pull requests

2 participants