Deep Merge for group parameter attributes

numbata commented 2 months ago

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.

numbata commented 2 months ago

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.

numbata commented 2 months ago

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

Unfortunately, no. But I have a fix for that: https://github.com/ruby-grape/grape/pull/2434

ruby-grape / grape