pboettch / json-schema-validator

JSON schema validator for JSON for Modern C++
Other
466 stars 134 forks source link

Default values of all "oneOf" sub-schema are inserted in patch #229

Closed didier-brizet closed 10 months ago

didier-brizet commented 1 year ago

Only the default value of one of "oneOf" sub-schema must be inserted in patch returned by json_validator::validate.

For instance, the validation of {"name": "bar"} against the following JSON schema:

{
    "$schema": "http://json-schema.org/draft-07/schema#",
    "type": "object",
    "oneOf": [
        {
            "type": "object",
            "properties": {
                "name": {
                    "enum": "foo"
                },
                "code": {
                    "const": 1,
                    "default": 1
                }
            }
        },
        {
            "type": "object",
            "properties": {
                "name": {
                    "enum": "bar"
                },
                "code": {
                    "const": 2,
                    "default": 2
                }
            }
        }
    ]
}

must returns the patch [{"op":"add","path":"/code","value":2}] but the following patch is returned: [{"op":"add","path":"/code","value":1},{"op":"add","path":"/code","value":2}]

didier-brizet commented 1 year ago

I propose the following patch:

issue_229.txt

UX3D-becher commented 1 year ago

@pboettch We have the same issue with a complex schema, is there anything that still needs to be done in the PR before it can be merged?

pboettch commented 1 year ago

It's a huge change. Hard to review. Especially if you're not convinced of the handling of default values inside the validator.

I'd rather prefer to have an outside-of-the-validator solution of default-value-handling. But this is off-topic for this PR.

UX3D-becher commented 1 year ago

Hm I'm not sure if this applies here, the patch is intended to be a way to make the invalid json valid again, so the patch

[{"op":"add","path":"/code","value":2}]

would really be the only valid solution, or am I missing something here?

With a patch like the one that is currently returned

[
  {"op": "add", "path": "/code", "value": 1}, 
  {"op": "add", "path": "/code", "value": 2}
]

there are two main problems from my perspective:

sjoubert commented 10 months ago

Any updates on this issue? Especially, is there something blocking #231 from being merged?

pboettch commented 10 months ago

I won't accept big changes anymore on patch creation concerning default values. I'm not convinced that this library is the right place. Or at least it should not be integrated in the code as it is today.