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

【BUG】removeAdditional will remove the valid properties #264

Closed
wilsoncook opened this issue Aug 5, 2016 · 18 comments
Closed

【BUG】removeAdditional will remove the valid properties #264

wilsoncook opened this issue Aug 5, 2016 · 18 comments

Comments

@wilsoncook
Copy link

wilsoncook commented Aug 5, 2016

The json schema:

{"type":"object","required":["idCounter"],"properties":{"LAYJSON_VERSION":{"type":["number"],"default":""},"idCounter":{"type":["number"]},"hotareas":{"type":"array","default":[],"items":{"type":"object","required":["DATAREF_ID"],"properties":{"title":{"type":["string"],"default":"热区"},"type":{"type":"string","default":"hotarea","enum":["hotarea"]},"attr":{"type":"object","default":{},"properties":{"isNewWindow":{"type":["boolean"],"default":false},"href":{"type":["string"],"default":""},"itemPic":{"type":["string"],"default":""},"itemId":{"type":["number","string"],"default":""},"itemTitle":{"type":["string"],"default":""}}},"style":{"type":"object","default":{},"properties":{"left":{"type":["number"],"default":0},"top":{"type":["number"],"default":0},"width":{"type":["number"],"default":300},"height":{"type":["number"],"default":150}}},"DATAREF_ID":{"type":["number"]},"info":{"type":"object","default":{}}}}},"components":{"type":"array","default":[],"items":{"type":"object","required":["DATAREF_ID"],"properties":{"_id":{"type":["number","null"]},"_setting":{"oneOf":[{"type":"object","properties":{"TagId":{"type":["number"],"default":null}}},{"type":"null"}]},"title":{"type":["string"],"default":"新建模块"},"description":{"type":["string","null"],"default":null},"type":{"type":"string","default":"component","enum":["component"]},"style":{"type":"object","default":{},"properties":{"z-index":{"type":"integer","default":0},"height":{"type":["number"],"default":200},"background-color":{"type":["string"],"default":"transparent"},"background-size":{"type":["string"],"default":"100% 100%"},"background-repeat":{"type":"string","default":"no-repeat","enum":["no-repeat","repeat"]}}},"show":{"type":"boolean","default":true},"attr":{"type":"object","default":{},"properties":{"bgUrl":{"type":["string","null"],"default":""}}},"widgets":{"type":"array","default":[],"items":{"oneOf":[{"type":"object","required":["DATAREF_ID"],"properties":{"type":{"type":"string","default":"widget","enum":["widget"]},"title":{"type":["string"],"default":"图片"},"widgetType":{"type":"string","default":"pic","enum":["pic"]},"attr":{"type":"object","default":{},"properties":{"bgUrl":{"type":["string","null"],"default":""}}},"style":{"type":"object","default":{},"properties":{"background-size":{"type":"string","default":"100% 100%"},"background-repeat":{"type":"string","default":"no-repeat","enum":["no-repeat","repeat"]},"z-index":{"type":"integer","default":1},"left":{"type":["number"],"default":0},"top":{"type":["number"],"default":0},"width":{"type":["number"],"default":200},"height":{"type":["number"],"default":100},"background-color":{"type":"string","default":"transparent"},"border-style":{"type":"string"},"border-width":{"type":["number"]},"border-color":{"type":"string"},"border-radius":{"type":["number"]},"transform":{"type":"string","default":"none"}}},"single":{"type":["number"],"default":0},"DATAREF_ID":{"type":["number"]},"info":{"type":"object","default":{}}}},{"type":"object","required":["DATAREF_ID"],"properties":{"type":{"type":"string","default":"widget","enum":["widget"]},"title":{"type":"string","default":"文字"},"widgetType":{"type":"string","default":"text","enum":["text"]},"attr":{"type":"object","default":{},"properties":{"text":{"type":"string","default":"双击即可修改文字"}}},"style":{"type":"object","default":{},"properties":{"color":{"type":"string","default":"#000"},"font-family":{"type":"string","default":"微软雅黑"},"font-size":{"type":["number","string"],"default":"24px"},"text-align":{"type":"string","default":"left"},"font-weight":{"type":"string","default":"normal"},"font-style":{"type":"string","default":"normal"},"line-height":{"type":["number","string"]},"text-decoration":{"type":"string","default":"none"},"z-index":{"type":"integer","default":1},"left":{"type":["number"],"default":0},"top":{"type":["number"],"default":0},"width":{"type":["number"],"default":200},"height":{"type":["number"],"default":100},"background-color":{"type":"string","default":"transparent"},"border-style":{"type":"string"},"border-width":{"type":["number"]},"border-color":{"type":"string"},"border-radius":{"type":["number"]},"transform":{"type":"string","default":"none"}}},"single":{"type":["number"],"default":0},"DATAREF_ID":{"type":["number"]},"info":{"type":"object","default":{}}}},{"type":"object","required":["DATAREF_ID"],"properties":{"type":{"type":"string","default":"widget","enum":["widget"]},"title":{"type":"string","default":"艺术字"},"widgetType":{"type":"string","default":"artword","enum":["artword"]},"attr":{"type":"object","default":{},"properties":{"text":{"type":"string","default":"请在右侧更改文字"},"retUrl":{"type":["string","null"],"default":""}}},"textStyle":{"type":"object","default":{},"properties":{"color":{"type":"string","default":"#000"},"font-family":{"type":"string","default":"微软雅黑"},"font-size":{"type":["number","string"],"default":"24px"},"text-decoration":{"type":"string","default":"none"}}},"style":{"type":"object","default":{},"properties":{"z-index":{"type":"integer","default":1},"left":{"type":["number"],"default":0},"top":{"type":["number"],"default":0},"width":{"type":["number"],"default":200},"height":{"type":["number"],"default":100},"background-color":{"type":"string","default":"transparent"},"border-style":{"type":"string"},"border-width":{"type":["number"]},"border-color":{"type":"string"},"border-radius":{"type":["number"]},"transform":{"type":"string","default":"none"}}},"single":{"type":["number"],"default":0},"DATAREF_ID":{"type":["number"]},"info":{"type":"object","default":{}}}},{"type":"object","required":["DATAREF_ID"],"properties":{"type":{"type":"string","default":"widget","enum":["widget"]},"title":{"type":"string","default":"色块"},"widgetType":{"type":"string","default":"rect","enum":["rect"]},"attr":{"type":"object","default":{}},"style":{"type":"object","default":{},"properties":{"background-color":{"type":"string","default":"green"},"z-index":{"type":"integer","default":1},"left":{"type":["number"],"default":0},"top":{"type":["number"],"default":0},"width":{"type":["number"],"default":200},"height":{"type":["number"],"default":100},"border-style":{"type":"string"},"border-width":{"type":["number"]},"border-color":{"type":"string"},"border-radius":{"type":["number"]},"transform":{"type":"string","default":"none"}}},"single":{"type":["number"],"default":0},"DATAREF_ID":{"type":["number"]},"info":{"type":"object","default":{}}}},{"type":"object","required":["DATAREF_ID"],"properties":{"type":{"type":"string","default":"widget","enum":["widget"]},"title":{"type":"string","default":"视频"},"widgetType":{"type":"string","default":"text","enum":["video"]},"attr":{"type":"object","default":{},"properties":{"flashUrl":{"type":["string","null"],"default":""}}},"style":{"type":"object","default":{},"properties":{"z-index":{"type":"integer","default":1},"left":{"type":["number"],"default":0},"top":{"type":["number"],"default":0},"width":{"type":["number"],"default":200},"height":{"type":["number"],"default":100},"background-color":{"type":"string","default":"transparent"},"border-style":{"type":"string"},"border-width":{"type":["number"]},"border-color":{"type":"string"},"border-radius":{"type":["number"]},"transform":{"type":"string","default":"none"}}},"single":{"type":["number"],"default":0},"DATAREF_ID":{"type":["number"]},"info":{"type":"object","default":{}}}}]}},"single":{"type":["number"],"default":0},"DATAREF_ID":{"type":["number"]},"info":{"type":"object","default":{}}}}}}}

The json value for testing:

var json = {
    "hotareas": [],
    "idCounter": '86',
    "components": [{
        "type": "component",
        "info": {},
        "single": true,
        "attr": {
            "bgUrl": ""
        },
        "style": {
            "z-index": 0,
            "height": 812,
            "background-color": "#ffffff",
            "background-size": "100% 100%",
            "background-repeat": "no-repeat"
        },
        "widgets": [{
            "type": "widget",
            "title": "艺术字",
            "widgetType": "artword",
            "info": {},
            "single": 0,
            "attr": {
                "text": "请在右侧更改文字→",
                "retUrl": "http://img.zx250.com/2016/08/05/[email protected]"
            },
            "textStyle": {
                "color": "#1f9116",
                "font-family": "华康少女体",
                "font-size": 34,
                "text-decoration": "none"
            },
            "style": {
                "z-index": 1,
                "left": 209,
                "top": 145,
                "width": 314,
                "height": 34,
                "background-color": "",
                "border-style": "solid",
                "border-width": 0,
                "border-color": "#000",
                "border-radius": 0,
                "transform": "none"
            },
            "DATAREF_ID": 165
        }],
        "DATAREF_ID": 1,
        "show": true,
        "title": "大图",
        "description": null,
        "*name*": "BaseDataRef"
    }],
    "LAYJSON_VERSION": 1
};

Then validate with option removeAdditional: 'all', the result json below:

{"hotareas":[],"idCounter":86,"components":[{"type":"component","info":{},"single":1,"attr":{"bgUrl":""},"style":{"z-index":0,"height":812,"background-color":"#ffffff","background-size":"100% 100%","background-repeat":"no-repeat"},"widgets":[{"type":"widget","title":"艺术字","widgetType":"artword","info":{},"single":0,"attr":{"text":"请在右侧更改文字→","retUrl":"http://img.zx250.com/2016/08/05/[email protected]"},"style":{"z-index":1,"left":209,"top":145,"width":314,"height":34,"background-color":"","border-style":"solid","border-width":0,"border-color":"#000","border-radius":0,"transform":"none"},"DATAREF_ID":165}],"DATAREF_ID":1,"show":true,"title":"大图","description":null}],"LAYJSON_VERSION":1}

Let us just focus on the datapath: components[0].widgets[0], when using the schema to validate the testing value, the property textStyle will be unproper removed.
the right logic should be matching the scheme of widget - artword (which has defined the property - textStyle).
I guess Ajv was just trying to match the one with less properties, when matched, with option removeAdditional: 'all', it will remove the extra properties (in this situation, the valid textStyle was removed).

This became a big problem, made losing more users' data in production environment.
May provide a solution?

@epoberezkin
Copy link
Member

That is not a bug, it is correct behaviour.

Please see #129 and also #140.

The problem here is that "additional properties" are meant in the sense of additionalProperties keyword - local to the schema that contains properties keyword. As each sub-schema in oneOf is evaluated, any property not mentioned in "properties" in each sub-schema is removed.

The solution is not to use removeAdditional: 'all' option and instead use removeAdditional: true, explicitly use the keyword additionalProperties: false only in subschemas that contain all allowed properties (rather than some part).

To remove not allowed properties properties in widget you can whitelist all properties on the top level (that may still pass through properties from invalid branch of oneOf).

The alternative solution is to use switch keyword (from v5 proposals) and branch based on widgetType property (in this case wrong branches won't be validated and only properties additional to the correct branch will be removed:

{
  "switch": [
    {
      "if": { "properties": { "widgetType": { "constant": "pic" } } },
      "then": { /*schema for "pic" widget, should include "additionalProperties": false */ }
    },
    {
      "if": { "properties": { "widgetType": { "constant": "text" } } },
      "then": { /*schema for "text" widget, should include "additionalProperties": false */ }
    },
    // ...
  ]
}

Make sure to use removeAdditional: true ("all" would still be removing too much when validating subschemas in "if"), v5 option and correct $schema keyword (if you use it).

@wilsoncook
Copy link
Author

wilsoncook commented Aug 5, 2016

@epoberezkin in my example, it seems that Ajv matched the wrong schema, if with removeAdditional: "all", and delete other widgets schema to below:

"widgets": {"type":"array","default":[],"items":{"oneOf":[{"type":"object","required":["DATAREF_ID"],"properties":{"type":{"type":"string","default":"widget","enum":["widget"]},"title":{"type":"string","default":"艺术字"},"widgetType":{"type":"string","default":"artword","enum":["artword"]},"attr":{"type":"object","default":{},"properties":{"text":{"type":"string","default":"请在右侧更改文字"},"retUrl":{"type":["string","null"],"default":""}}},"textStyle":{"type":"object","default":{},"properties":{"color":{"type":"string","default":"#000"},"font-family":{"type":"string","default":"微软雅黑"},"font-size":{"type":["number","string"],"default":"24px"},"text-decoration":{"type":"string","default":"none"}}},"style":{"type":"object","default":{},"properties":{"z-index":{"type":"integer","default":1},"left":{"type":["number"],"default":0},"top":{"type":["number"],"default":0},"width":{"type":["number"],"default":200},"height":{"type":["number"],"default":100},"background-color":{"type":"string","default":"transparent"},"border-style":{"type":"string"},"border-width":{"type":["number"]},"border-color":{"type":"string"},"border-radius":{"type":["number"]},"transform":{"type":"string","default":"none"}}},"single":{"type":["number"],"default":0},"DATAREF_ID":{"type":["number"]},"info":{"type":"object","default":{}}}}]}}

it will not remove the textStyle property.
AND if i put an textStyle property to every widget schema, it is also working (will not remove textStyle).

@epoberezkin
Copy link
Member

Ajv is not matching sub-schemas, it simply removes additional properties as each sub-schema is validated, whether this subschema is valid or not.

"Additional property" is understood in the same meaning as the standard uses for "additionalProperties" keyword - local to the current subschema (not used in "properties" and "patrernProperties" keyword in the current schema object only), not global to the whole schema as in "used in some other place of the schema".

If you add a property in all sub-schemas it won't be additional in any of them, so it won't be removed.

I have suggested several approaches that would work for you, there is nothing that has to be changed in Ajv. I will probably add some example to clarify it.

@epoberezkin
Copy link
Member

it simply removes additional properties as each sub-schema is validated, whether this subschema is valid or not.

I mean with removeAdditional option.

Also there is example in readme already that shows exactly your situation and shows how to construct the schema correctly - see Filtering Data section.

@wilsoncook
Copy link
Author

wilsoncook commented Aug 5, 2016

I also tested that it seems removeAdditional: "all" act as removeAdditional: true + additionalProperties: false, (can i consider that removeAdditional: "all" equals to removeAdditional: true + all schemas with additionalProperties: false ?), so as u said

it simply removes additional properties as each sub-schema is validated, whether this subschema is valid or not.

it means this behavior is the oneOf's standard behavior in json schema draft, may i right?
if that is, then oneOf will not properly for my needs, i will prefer the switch in v5 of your suggessions.
Thank u! :-)

@epoberezkin
Copy link
Member

can i consider that removeAdditional: "all" equals to removeAdditional: true + all schemas with additionalProperties: false

Yes

it means this behavior is the oneOf's standard behavior in json schema draft, may i right?

Removing additional properties is ajv feature, it's not the part of the standard. But it relies on the definition of "additional property" in the standard. "oneOf" must validate all subschemas so all additional properties will be removed. "switch" only validates "then" subschemas that pass "if" test, so you have better control which properties will be considered additional. But have a look at the example in readme, this approach will also work in your case.

@wilsoncook
Copy link
Author

@epoberezkin Got it, Thanks for your patient!

@wilsoncook
Copy link
Author

wilsoncook commented Aug 8, 2016

@epoberezkin i have tried switch, but a weird problem comes.
see schema below:

{"type":"object","required":["idCounter"],"properties":{"LAYJSON_VERSION":{"type":["number"],"default":""},"idCounter":{"type":["number"]},"components":{"type":"array","default":[],"items":{"type":"object","required":["DATAREF_ID"],"properties":{"_id":{"type":["number","null"]},"_setting":{"oneOf":[{"type":"object","properties":{"TagId":{"type":["number"],"default":null}}},{"type":"null"}]},"title":{"type":["string"],"default":"新建模块"},"description":{"type":["string","null"],"default":null},"type":{"type":"string","default":"component","enum":["component"]},"style":{"type":"object","default":{},"properties":{"z-index":{"type":"integer","default":0},"height":{"type":["number","string"],"default":200},"background-color":{"type":["string"],"default":"transparent"},"background-size":{"type":["string"],"default":"100% 100%"},"background-repeat":{"type":"string","default":"no-repeat","enum":["no-repeat","repeat"]}}},"show":{"type":"boolean","default":true},"attr":{"type":"object","default":{},"properties":{"bgUrl":{"type":["string","null"],"default":""}}},"widgets":{"type":"array","default":[],"items":{"switch":[{"if":{"properties":{"widgetType":{"constant":"artword"}}},"then":{"type":"object","required":["DATAREF_ID"],"properties":{"type":{"type":"string","default":"widget","constant":"widget"},"title":{"type":"string","default":"艺术字"},"widgetType":{"type":"string","default":"artword","constant":"artword"},"attr":{"type":"object","default":{},"properties":{"text":{"type":"string","default":"请在右侧更改文字"},"retUrl":{"type":["string","null"],"default":""}}},"textStyle":{"type":"object","default":{},"properties":{"color":{"type":"string","default":"#000"},"font-family":{"type":"string","default":"微软雅黑"},"font-size":{"type":["number","string"],"default":"24px"},"text-decoration":{"type":"string","default":"none"}}},"style":{"type":"object","default":{},"properties":{"z-index":{"type":"integer","default":1},"left":{"type":["number","string"],"default":0},"top":{"type":["number","string"],"default":0},"width":{"type":["number","string"],"default":200},"height":{"type":["number","string"],"default":100},"background-color":{"type":"string","default":"transparent"},"border-style":{"type":"string"},"border-width":{"type":["number","string"]},"border-color":{"type":"string"},"border-radius":{"type":["number","string"]},"transform":{"type":"string","default":"none"}}},"single":{"type":["number"],"default":0},"DATAREF_ID":{"type":["number"]},"info":{"type":"object","default":{}}}}}]}},"single":{"type":["number"],"default":0},"DATAREF_ID":{"type":["number"]},"info":{"type":"object","default":{}}}}}}}

the options includes:

var ajv = new Ajv({
    v5: true,
    coerceTypes: true,
    useDefaults: true,
    removeAdditional: 'all'
});

the json for validating:

var json = ;

then validated failed with:

[{"keyword":"required","dataPath":".components[0].widgets[0]","schemaPath":"#/properties/components/items/properties/widgets/items/switch/0/then/required","params":{"missingProperty":"DATAREF_ID"},"message":"should have required property 'DATAREF_ID'"}]

BUT the DATAREF_ID is actually at there.
Any suggestions? Thank u!

@epoberezkin
Copy link
Member

Can you make a smaller sample please?

@wilsoncook
Copy link
Author

@epoberezkin I made a smaller testing scripts at: https://jsfiddle.net/ppoo24/fckyt5rm/ , i have formatted and simplified the json and schema, the error happened as usual

@epoberezkin
Copy link
Member

epoberezkin commented Aug 8, 2016

All properties get removed when if subschema is validated because you use removeAdditional: "all". You still need to use option removeAdditional: true and add the keyword additionalProperties: false to schemas where you want additional properties removed.

@epoberezkin
Copy link
Member

The properties you see in the final json are defaults (from default keywords) and DATAREF_ID doesn't have default.

@wilsoncook
Copy link
Author

@epoberezkin So when executing if, it is the time to validating already? i thought that if is just a judgement without other actions. now realized the removeAdditional: "all" is not a handy function but a dangerous evil.
can i consider that, if removeAdditional: "all", every schema Ajv meet, it will remove extra properties which not defined in the schema, whatever the schema is in switch or if or other special types ?

@epoberezkin
Copy link
Member

So when executing if, it is the time to validating already? i thought that if is just a judgement without other actions.

Ajv does nothing but validation. There is a schema inside if, so it gets validated. Potentially there is a place to extend the functionality of removeAdditional: 'all' option to exclude some subschemas from removing additional properties, but I think explicitly adding additionalProperties: false and using removeAdditional: true (or 'failing') is a cleaner solution.

now realized the removeAdditional: "all" is not a handy function but a dangerous evil.

For any moderately complex schema 'all' setting for this option is indeed a bit too thorough.

can i consider that, if removeAdditional: "all", every schema Ajv meet, it will remove extra properties which not defined in the schema, whatever the schema is in switch or if or other special types ?

removeAdditional: 'all' will remove additional properties when any schema that have some of the following keywords is validated: properties, patternProperties, additionalProperties, patternGroups (v5). Without these keywords nothing should be removed.

@epoberezkin
Copy link
Member

Maybe required and patternRequired (v5) on their own would also trigger it, need to check.

@wilsoncook
Copy link
Author

@epoberezkin appreciate your answer! i'm using additionalProperties: false now, :)

@epoberezkin epoberezkin added the OK label Aug 11, 2016
@ngryman
Copy link

ngryman commented Aug 20, 2016

@ppoo24 Could you shorten your code sample please? It makes the issue a pain to read 😅. Thanks!

@wilsoncook
Copy link
Author

@ngryman sorry about that

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

No branches or pull requests

3 participants