Implement smarter merge #55
Conversation
| if key == 'parameters' | ||
| base[key] |= value | ||
| base[key].uniq! { |param| param.slice('name', 'in') } | ||
| if !base[key].key?("$ref") |
There was a problem hiding this comment.
This is a special treatment to avoid merging anything to Reference Object.
The reference object can have only $ref field.
# invalid
schema:
$ref: "#/components/schemas/Pet"
description: "override"
# valid
schema:
allOf:
- $ref: "#/components/schemas/Pet"
- description: "override"
There was a problem hiding this comment.
Is this for the case that $ref is manually added to an OpenAPI schema file? If you manually maintain the schema of the response, why don't you skip updating OpenAPI from such test cases? Do you have a test case that calls multiple endpoints and want to update OpenAPI for only one of these?
| elsif base[key].is_a?(Array) && value.is_a?(Array) | ||
| # parameters need to be merged as if `name` and `in` were the Hash keys. | ||
| if key == 'parameters' | ||
| base[key] |= value | ||
| base[key].uniq! { |param| param.slice('name', 'in') } | ||
| else | ||
| base[key] = value |
There was a problem hiding this comment.
I wondered if just overwriting the value of base is fine, but given that what we have as an array in the current test in this repository are just servers, tags, parameters, and example, it's probably okay to rewrite it, ignoring the possibility of the need to partially update arrays inside it. We may need to recursively do something if we find other array fields that are not compatible with this idea, but I don't come up with such a thing at this moment.
| end | ||
| else | ||
| # no-op | ||
| base[key] = value |
There was a problem hiding this comment.
Similar to https://github.com/k0kubun/rspec-openapi/pull/55/files#r877575816, as long as it updates only fields that rspec-openapi generates, it's probably fine, I guess.
I'm okay with merging this patch, but note that the behavior of your implementation is not what you quoted. This PR simply doesn't implement "everything under the path/method should be erased". For example, when an endpoint currently returns Achieving it while still leaving legit manual changes isn't a simple task. It shouldn't be a simple recursive method, but it should be aware of which level of the schema it's currently at. That's why I called it "intelligent". But at this point, I'm not comfortable leaving #48 open since nobody would understand my intention of the current description. I don't afford to put so much effort on explaining it with concrete examples either. So, let's merge this and close #48. At least it brings us closer to what #48 says. Thanks. |
Closes #48
This attempts to implement the behavior described in