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

Validate params only if another param is present #1047

Merged
merged 1 commit into from
Jun 29, 2015

Conversation

rnubel
Copy link
Contributor

@rnubel rnubel commented Jun 26, 2015

Implements #958 (sort of). This allows for the use case where you have some parameters to an endpoint which are only relevant or meaningful if some other parameters are given. It can be used like so:

params do
  optional :shelf_id, type: Integer
  given :shelf_id do
    requires :bin_id, type: Integer
  end
end

Nested use cases work, too:

params do
  requires :product_info, type: Hash do
    requires :name, type: String
    optional :shelf_id, type: Integer
    given :shelf_id do
      requires :bin_id, type: Integer
    end
  end

One concern: This type of relationship won't show up properly in grape-swagger, or anything which looks at Route#route_params to inspect parameter definitions; even though the elements in the given block are actually optional unless shelf_id is given, Route#route_params will indicate that bin_id is required. I could probably get that to instead show up as optional, but I don't know if that's ideal.

@@ -57,7 +71,7 @@ def required?
protected

# Adds a parameter declaration to our list of validations.
# @param attrs [Array] (see Grape::DSL::Parameters#required)
# @param attrs [Array] (see Grape::DSL::Parameters#requires)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for fixing the comment here.

@dblock
Copy link
Member

dblock commented Jun 26, 2015

I think this is good enough. Swagger doesn't concern be as much, these parameters are closer to required than optional IMO. It needs CHANGELOG and README updates and I will merge.

Usage:

    # ...
    params do
      optional :shelf_id, type: Integer
      given :shelf_id do
        requires :bin_id, type: Integer
      end
    end

This implements ruby-grape#958. In order to achieve the DSL-style implementation,
I introduced the concept of a "lateral scope" as opposed to the
"nested scope" which `requires :foo do` opens up. A lateral scope is
subordinate to its parent, but not nested under an element.
@rnubel
Copy link
Contributor Author

rnubel commented Jun 26, 2015

Updated. I also added a test and fixed a bug where declared(params) would explode if used with given.

dblock added a commit that referenced this pull request Jun 29, 2015
Validate params only if another param is present
@dblock dblock merged commit a590543 into ruby-grape:master Jun 29, 2015
@dblock
Copy link
Member

dblock commented Jun 29, 2015

Merged, thank you.

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