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

Deep Merge for group parameter attributes #2432

Merged
merged 4 commits into from
Apr 26, 2024

Conversation

numbata
Copy link
Contributor

@numbata numbata commented Apr 25, 2024

This PR changes the way attributes are merged for parameters within the Grape DSL by adopting a `deep_merge' strategy. This enhancement ensures that nested attributes specified at both the group and parameter level are fully merged, increasing flexibility and configuration precision in API definitions.

Motivation

In the current implementation, specifying nested attributes at the parameter level completely overrides those set at the group level. This behavior can lead to unintended loss of configuration detail, especially in complex API setups where nuanced customizations are required across different parameters.

Example of Current Issue

When using Grape's parameter grouping with the with method, any specific attributes defined at the parameter level will completely override the more generic attributes defined at the group level. This can be problematic when trying to apply general settings to a group of parameters while also specifying additional settings for individual parameters.

with(documentation: { in: 'body' }) do
  optional :vault, documentation: { default: 33 }
  requires :pip_id
end

In the above code:

  • pip_id should inherit the documentation: { in: 'body' } from the group.
  • vault should combine the group's documentation: { in: 'body' } with its specific documentation: { default: 33 } .

However, under the current implementation:

  • vault ends up with only documentation: { default: 33 }, completely losing the in: 'body' setting from the group.

Using deep_merge, the attributes for vault would correctly combine both the group-level and parameter-specific values.

Proposed change

The use of deep_merge allows for a more granular combination of attributes, ensuring that individual parameter settings can extend group settings without being discarded. This method applies not only to documentation settings, but also extends to all other attribute types, providing a more robust and versatile configuration approach.

Potential Impacts and Considerations

This update may affect existing projects that depend on the overriding nature of the current merge strategy. Because this is a potentially disruptive change, it is recommended that this update be evaluated in the context of a major release to allow users to adapt to the new behavior without disruption.

Testing

Additional specifications have been included to validate the new merge behavior across various attribute types to ensure that both existing and new functionality works as expected without regression.

This change is intended to make Grape's parameter configuration more intuitive and flexible, to support a broader range of API design requirements, and to promote consistency in how nested attributes are handled across parameters.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Given that this didn't break any existing specs, the old behavior should be considered undefined, and we don't need a major version increment. However, we do need to document this in README and add a paragraph to UPGRADING.md, please?

I would also add more tests for any nested scenarios or other interesting combinations where this behavior has changed.

Finally, is there some end-user-visible side effect of this? How does this affect your scenario, and can we please add some tests that express those (and include examples in README).

@numbata
Copy link
Contributor Author

numbata commented Apr 26, 2024

I would also add more tests for any nested scenarios or other interesting combinations where this behavior has changed.

More specs with some corner cases added

we do need to document this in README and add a paragraph to UPGRADING.md

Done. Hope it works for you and others grape'ies.

Finally, is there some end-user-visible side effect of this? How does this affect your scenario

In my scenario, it was an issue I found while working on our API endpoint documentation. Since the grape-swagger doesn't support OpenAPI v3, we need to add an x-nullable option to the documentation for the params that accepts nil as a value in addition to other primitive types. At the same time I have a with(documentation: { in: "body" }) wrapper around the params definition to make sure that in OpenAPI schema they will have a right type: body.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I'm merging this.

Can we nest with? Might be worth adding a spec. Feel free to PR separately.

@dblock dblock merged commit 8e2d2fb into ruby-grape:master Apr 26, 2024
45 checks passed
@numbata
Copy link
Contributor Author

numbata commented Apr 26, 2024

Can we nest with? Might be worth adding a spec. Feel free to PR separately.

Unfortunately, no. But I have a fix for that: #2434

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

Successfully merging this pull request may close these issues.

None yet

2 participants