-
Notifications
You must be signed in to change notification settings - Fork 63
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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