avian2 / jsonmerge

Merge a series of JSON documents.
MIT License
214 stars 25 forks source link

Support 'allOf' and 'anyOf' with overwrite strategies #29

Closed hmpcabral closed 6 years ago

hmpcabral commented 6 years ago

If a schema containing an 'allOf'/'anyOf' keyword has an 'overwrite' strategy, or if all the subschemas of the keyword have 'overwrite' strategies themselves (explicitly or by default), the keywords don't need to be descended and the instances may be safely merged.

This came up in some complex schemas we had at work and I thought it might be useful to a wider audience. I'm not entirely sure I went with the best approach; if you feel this should be refactored in some way just point me in the right direction!

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-2.0%) to 95.133% when pulling df2e2d277e8359206572178dd1b93b0a7dfca239 on hmpcabral:anyOf_allOf_overwrite into e0a56926b962dcf426e609acc39a60e8fdf8c327 on avian2:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.4%) to 96.675% when pulling 6403838ea14179c633d92b7ecc72732d574503ef on hmpcabral:anyOf_allOf_overwrite into e0a56926b962dcf426e609acc39a60e8fdf8c327 on avian2:master.

avian2 commented 6 years ago

Hi. Thanks for the nice pull request. I see how it can be useful to allow an "overwrite" strategy on the top level of an anyOf/allOf schema. I'm not sure about the other case - where all subschemas below anyOf/allOf have "overwrite". For instance, I don't see where this would make sense:

schema = {
    'mergeStrategy': 'objectMerge',
    'anyOf': [
        {
            'mergeStrategy': 'overwrite'
        },
        {
            'type': 'array'
        },
        {
            'type': 'string'
        },
    ]
}

I think it's also a bit misleading to allow for this, since all those "overwrite" strategies don't actually get their methods called, just the top-level one.

hmpcabral commented 6 years ago

Ah, I see your point. For some context, this is the sort of use case I had in mind for the subschema strategies:

schema = {
    "ipaddr": {
        "anyOf": [
            {"type": "string", "pattern": "^!?(?:[0-9]{1,3}\\.){3}[0-9]{1,3}(?:\\/[0-9]{1,2})?$"},
            {"type": "string", "format": "hostname"}
        ]
    }
}

I just realised, however, I could simply add "mergeStrategy": "overwrite" directly to the key-level schema for the same result, with safer side-effects as you mention.

I will remove the subschema descent and also add a top-level check for 'overwrite' to the 'oneOf' descender, if that makes sense to you.

avian2 commented 6 years ago

Merged. Thanks!

avian2 commented 6 years ago

After some thought, I removed the special case for overwrite strategy. I think it's safe to allow any strategy to be defined at the same level as oneOf, anyOf, etc. since in that case jsonmerge will not descend into these keywords at all. Plus I think JSON schema allows for other keywords to be present at the same level as anyOf etc. anyway. Your use case should still work (the tests you added pass).

https://github.com/avian2/jsonmerge#support-for-keywords-that-apply-subschemas

hmpcabral commented 6 years ago

That sounds good, didn't think of that! Thanks for the feedback :)