brutusin / json-forms

JSON Schema to HTML form generator, supporting dynamic subschemas (on the fly resolution). Extensible and customizable library with zero dependencies. Bootstrap add-ons provided
http://brutusin.org/json-forms
Apache License 2.0
607 stars 168 forks source link

Required Fields (draft 3 vs draft 4) #56

Closed vincentmorneau closed 7 years ago

vincentmorneau commented 7 years ago

First, thank you so much for this project. It is so much simpler than the alternatives like json-schema-form and others that all use angular and react (why?).

I noticed one thing about required fields, you have them listed as a boolean property within the field.

image

JSON Schema draft 4 has the required property listed as an array, after the objects themselves. Example:

image

Are you following JSON Schema draft 3? Any plan to move to the current draft 4?

idelvall commented 7 years ago

Hi, I was aware when I implemented it. I chose the v3 way because its locality allows a cleaner recursive implementation, whereas v4 way needs some contextual information. I guess the v4 way was introduced with reusability in mind, letting the referencing schema set if the referenced subschema is or not required...

So a plan to fully support v4? No (in fact v3 is probabily not fully supported). But add support for this specific case? Absolutely doable.

It should not be much difficult to implement, give me some days and I come back to this issue when it's ready.

My idea is to support both ways, and in case of conflicting rules give preference to the v4 (contextual) one.

What do you think?

vincentmorneau commented 7 years ago

That would be more than perfect. It would covers most simple v4 cases.

I'm super impressed btw. I tried implementing a json schema to html form myself before finding your project. Not an easy task to say to least. Turned out to be way bigger than I expected!

graciano commented 7 years ago

So, I checked the spec for json-schema and found that they are currently at v5. I don't know about @idelvall's ideas about it, but if the project would be updated (and, consequently, a new major semver release) why specifically version 4 and not the most recent 5?

vincentmorneau commented 7 years ago

Didn't know about v5, but 100% agree with @graciano .

idelvall commented 7 years ago

v5 defines required as a keyword to be used only for object schemas https://tools.ietf.org/html/draft-wright-json-schema-validation-00#section-5.15

Nothing new then.

Thanks @graciano

idelvall commented 7 years ago

Done, example: http://brutusin.org/json-forms/#13

What do you think?

vincentmorneau commented 7 years ago

Thanks @idelvall . Very appreciated.

fabifabi commented 7 years ago

That's great BUT i can't fidn it in the github of the project? i watched,and nothing.....still the old syntax. And a bug about it : I can have an OPTIONAL field (imagine GPS field) who need to be complete (so,it is required long/lat).with the actual required, i MUST do it, means, don't let it empty,even the GPS object is not required.......(great project, i wanna help!) (i want do an article for the french magazine "programmez!" about your project,really nice one.)

fabifabi commented 7 years ago

......and i am silly,was corrected 1 days ago :D i just downlaod one week ago, sorry, forget me, i will hide in a big hole forever !

fabifabi commented 7 years ago
var ex = {
    "title": "Product set",
    "type": "array",
    "minItems": 1,
    "items": {
        "title": "Product",
        "type": "object",
        "properties": {
            "id": {
                "description": "The unique identifier for a product",
                "type": "number",

            },
            "name": {
                "type": "string",
            },
            "price": {
                "type": "number",
                "minimum": 0,
                "exclusiveMinimum": true,
            },
            "tags": {
                "type": "array",
                "items": {
                    "type": "string"
                },
                "minItems": 1,
                "uniqueItems": true
            },
            "dimensions": {
                "type": "object",
                "properties": {
                    "length": {"type": "number", "required": true},
                    "width": {"type": "number", "required": true},
                    "height": {"type": "number", "required": true}
                },
                "required": ["length", "width", "height"]
            },
            "warehouseLocation": {
                "description": "Coordinates of the warehouse with the product",
                "type": "object",
                "properties": {
                    "latitude": {"type": "number"},
                    "longitude": {"type": "number"}
                }
            }
        },
        "required": ["id", "name", "price"]
    }
};

var schema = ex;
var BrutusinForms = brutusin["json-forms"];
var bf = BrutusinForms.create(schema);
var container = document.getElementById('container');
bf.render(container, {});

as u can see,the "dimension" object is NOT required. but if people begin to write in it,he must do the three fields, it is my mistake ? look like a recursion problem (check if father object need this fields or not).Thanks in advance.

idelvall commented 7 years ago

Hi @fabifabi and welcome! yes I can see the problem. I'am working on it

Thanks!

idelvall commented 7 years ago

fixed, I updated too the example http://brutusin.org/json-forms/#13 with this case.

vincentmorneau commented 7 years ago

Would dependencies be complicated to implement too?

https://spacetelescope.github.io/understanding-json-schema/reference/object.html#property-dependencies

Then I believe all cases of required fields would be covered.

idelvall commented 7 years ago

Please @vincentmorneau open a new issue for this new topic Thanks!

vincentmorneau commented 7 years ago

@idelvall I'm sorry to come back on this one, but it doesn't seem to work with nested objects.

Example:

{
    "$schema": "http://json-schema.org/draft-04/schema#",
    "type": "object",
    "title": "Root",
    "properties": {
        "appURL": {
            "type": "string",
            "title": "Application URL"
        },
        "jsConcat": {
            "type": "object",
            "title": "JavaScript Concatenation",
            "properties": {
                "enabled": {
                    "type": "boolean",
                    "title": "Enabled"
                },
                "finalName": {
                    "type": "string",
                    "title": "Concatenated Name",
                }
            },
            "required": [
                "enabled"
            ]
        }
    },
    "required": [
        "appURL"
    ]
}

appURL is required. Works well. jsConcat.enabled is required. Does not work.

Any idea?

Thank you again for your great work.

idelvall commented 7 years ago

Thanks Vincent, you are right, fixing it!

idelvall commented 7 years ago

It should work now @vincentmorneau. Thanks!

vincentmorneau commented 7 years ago

Tested and it works! Thanks @idelvall

fabifabi commented 7 years ago

In my mind, it worked well : enabled is required ONLY if jsConcat is not empty ! For do what u want to do, i will write: require:["appURL","jsConcat"]

2017-02-10 22:43 GMT+01:00 Vincent Morneau notifications@github.com:

@idelvall https://github.com/idelvall I'm sorry to come back on this one, but it doesn't seem to work with nested objects.

Example:

{ "$schema": "http://json-schema.org/draft-04/schema#", "type": "object", "title": "Root", "properties": { "appURL": { "type": "string", "title": "Application URL" }, "jsConcat": { "type": "object", "title": "JavaScript Concatenation", "properties": { "enabled": { "type": "boolean", "title": "Enabled" }, "finalName": { "type": "string", "title": "Concatenated Name", } }, "required": [ "enabled" ] } }, "required": [ "appURL" ] }

appURL is required. Works well. jsConcat.enabled is required. Does not work.

Any idea?

Thank you again for your great work.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/brutusin/json-forms/issues/56#issuecomment-279074270, or mute the thread https://github.com/notifications/unsubscribe-auth/AYbVLf7ojBHdLom-OAC3ImauCF5r4FHOks5rbNoCgaJpZM4L3Wd2 .

idelvall commented 7 years ago

@fabifabi there was a bug for nested objects but now it behaves as expected

vincentmorneau commented 7 years ago

Minor issue: it doesn't work within an array. Here's an example:

{
    "$schema": "http://json-schema.org/draft-03/schema#",
    "type": "object",
    "properties": {
        "sorts": {
            "type": "array",
            "title": "Sorting",
            "items": {
                "type": "object",
                "properties": {
                    "fieldName": {
                        "type": "string",
                        "title": "Field name",
                        "required": true
                    }
                },
                "required": [
                    "fieldName"
                ]
            }
        }
    }
}

Overall it's been very useful.

idelvall commented 7 years ago

Hi vincent, this should work:

{
    "$schema": "http://json-schema.org/draft-03/schema#",
    "type": "object",
    "properties": {
        "sorts": {
            "type": "array",
            "title": "Sorting",
            "items": {
                "type": "object",
                "properties": {
                    "fieldName": {
                        "type": "string",
                        "title": "Field name",
                        "required": true
                    }
                },
                "required": true                
            }
        }
    }
}

The key point here is making an array item required as a whole (notice the new required="true"), in order to make its inner required properties validated when some other property is informed (in this case there is only one of them)

The problem here is that version v4 defines required only for inner objects, and no for top objects, hence v3 syntax must be used.

A possible fix for v4 (going beyond the spec) could be make the items inherit the required state of the array (meaning not accepting nulls as items)

vincentmorneau commented 7 years ago

I see. I'll use v3 for this one case then. Thank you.