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

declared_params regression in 1.5.0 with multiple allowed types #2115

Closed
stanhu opened this issue Oct 9, 2020 · 1 comment · Fixed by #2116
Closed

declared_params regression in 1.5.0 with multiple allowed types #2115

stanhu opened this issue Oct 9, 2020 · 1 comment · Fixed by #2116
Labels

Comments

@stanhu
Copy link
Contributor

stanhu commented Oct 9, 2020

As shown in https://github.com/ruby-grape/grape#multiple-allowed-types, if you use an optional type with multiple allowed types:

params do
  optional :status_code, types: [Integer, String]
end
get '/' do
  declared_params
end

You end up with status_code: [] instead of status_code: nil.

I think this is a regression due to https://github.com/ruby-grape/grape/pull/2103/files. This test reproduces the problem:

diff --git a/spec/grape/endpoint/declared_spec.rb b/spec/grape/endpoint/declared_spec.rb
index cf33293..9ddfe8f 100644
--- a/spec/grape/endpoint/declared_spec.rb
+++ b/spec/grape/endpoint/declared_spec.rb
@@ -16,6 +16,7 @@ describe Grape::Endpoint do
         requires :first
         optional :second
         optional :third, default: 'third-default'
+        optional :multiple_types, types: [Integer, String]
         optional :nested, type: Hash do
           optional :fourth
           optional :fifth
@@ -96,6 +97,16 @@ describe Grape::Endpoint do
       expect(JSON.parse(last_response.body)['nested']['fourth']).to be_nil
     end
 
+    it 'should show nil for multiple allowed types if include_missing is true' do
+      subject.get '/declared' do
+        declared(params, include_missing: true)
+      end
+
+      get '/declared?first=present'
+      expect(last_response.status).to eq(200)
+      expect(JSON.parse(last_response.body)['multiple_types']).to be_nil
+    end
+
     it 'does not work in a before filter' do
       subject.before do
         declared(params)
@@ -113,7 +124,7 @@ describe Grape::Endpoint do
       end
       get '/declared?first=present'
       expect(last_response.status).to eq(200)
-      expect(JSON.parse(last_response.body).keys.size).to eq(10)
+      expect(JSON.parse(last_response.body).keys.size).to eq(11)
     end
 
     it 'has a optional param with default value all the time' do
@dblock dblock added the bug? label Oct 9, 2020
@stanhu
Copy link
Contributor Author

stanhu commented Oct 9, 2020

I suppose there is ambiguity if we have an Array in the possible allowed types:

 optional :status_code, types: [Integer, String, Array[Integer, String]]

But if there's no Array there should be no ambiguity.

Do we hack the string parser to look for commas in type (which is "[Integer, String]") and parse this?

stanhu added a commit to stanhu/grape that referenced this issue Oct 9, 2020
Prior to Grape v1.5.0 and ruby-grape#2103, the following
would return `nil`:

```
params do
  optional :status_code, types: [Integer, String]
end
get '/' do
  declared_params
end
```

However, now it turns an empty `Array`.

We restore the previous behavior by not returning an empty `Array` if multiple
types are used.

Closes ruby-grape#2115
stanhu added a commit to stanhu/grape that referenced this issue Oct 9, 2020
Prior to Grape v1.5.0 and ruby-grape#2103, the following
would return `nil`:

```
params do
  optional :status_code, types: [Integer, String]
end
get '/' do
  declared_params
end
```

However, now it turns an empty `Array`.

We restore the previous behavior by not returning an empty `Array` if multiple
types are used.

Closes ruby-grape#2115
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants