Bug in Rack::JSONP?

Bug in Rack::JSONP?

[Aslak Hellesøy]

Hi all,

I'm using Rack::JSONP from rack-contrib in a Sinatra project. When I
issue a GET request that's not JSON (and that doesn't set a Content-
Type header, I get:

NoMethodError: undefined method `include?' for nil:NilClass
	/Users/aslakhellesoy/.rvm/gems/ruby-1.8.7-p249@cukepatch/gems/rack-
contrib-1.0.1/lib/rack/contrib/jsonp.rb:44:in `is_json?'

Am I the only one running into this? Should I patch with:

(headers['Content-Type'] || []).include?('application/json') in rack-
contrib-1.0.1/lib/rack/contrib/jsonp.rb:44

Or is it better to patch rack to return an empty array instead of nil
in #headers.

I'm on rack 1.2.1 and Sinatra 1.0.

Cheers,
Aslak

Re: Bug in Rack::JSONP?

[Magnus Holm]

From the spec: "There must be a Content-Type, except when the Status
is 1xx, 204 or 304, in which case there must be none given."

I guess we should rather make JSONP a noop when the status is 1xx, 204 or 304.

// Magnus Holm


On Tue, Jun 22, 2010 at 14:08, Aslak Hellesøy <aslak.hellesoy@gmail.com> wrote:
> Hi all,
>
> I'm using Rack::JSONP from rack-contrib in a Sinatra project. When I
> issue a GET request that's not JSON (and that doesn't set a Content-
> Type header, I get:
>
> NoMethodError: undefined method `include?' for nil:NilClass
>        /Users/aslakhellesoy/.rvm/gems/ruby-1.8.7-p249@cukepatch/gems/rack-
> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44:in `is_json?'
>
> Am I the only one running into this? Should I patch with:
>
> (headers['Content-Type'] || []).include?('application/json') in rack-
> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44
>
> Or is it better to patch rack to return an empty array instead of nil
> in #headers.
>
> I'm on rack 1.2.1 and Sinatra 1.0.
>
> Cheers,
> Aslak

Re: Bug in Rack::JSONP?

[Magnus Holm]

What about something like this:

diff --git a/lib/rack/contrib/jsonp.rb b/lib/rack/contrib/jsonp.rb
index 723d63a..7eefe00 100644
--- a/lib/rack/contrib/jsonp.rb
+++ b/lib/rack/contrib/jsonp.rb
@@ -19,6 +19,10 @@ module Rack
     #
     def call(env)
       status, headers, response = @app.call(env)
+
+      if STATUS_WITH_NO_ENTITY_BODY.include?(status)
+        return status, headers, response
+      end

       headers = HeaderHash.new(headers)
       request = Rack::Request.new(env)
diff --git a/test/spec_rack_jsonp.rb b/test/spec_rack_jsonp.rb
index eb6570f..c1abc59 100644
--- a/test/spec_rack_jsonp.rb
+++ b/test/spec_rack_jsonp.rb
@@ -69,5 +69,15 @@ context "Rack::JSONP" do
     body = Rack::JSONP.new(app).call(request).last
     body.should.equal [test_body]
   end
-
+
+  specify "should not change anything if the request doesn't have a body" do
+    app1 = lambda { |env| [100, {}, []] }
+    app2 = lambda { |env| [204, {}, []] }
+    app3 = lambda { |env| [304, {}, []] }
+    request = Rack::MockRequest.env_for("/", :params =>
"callback=foo", 'HTTP_ACCEPT' => 'application/json')
+
+    Rack::JSONP.new(app1).call(request).should.equal app1.call({})
+    Rack::JSONP.new(app2).call(request).should.equal app2.call({})
+    Rack::JSONP.new(app3).call(request).should.equal app3.call({})
+  end
 end
\ No newline at end of file


// Magnus Holm



On Tue, Jun 22, 2010 at 14:22, Magnus Holm <judofyr@gmail.com> wrote:
> From the spec: "There must be a Content-Type, except when the Status
> is 1xx, 204 or 304, in which case there must be none given."
>
> I guess we should rather make JSONP a noop when the status is 1xx, 204 or 304.
>
> // Magnus Holm
>
>
> On Tue, Jun 22, 2010 at 14:08, Aslak Hellesøy <aslak.hellesoy@gmail.com> wrote:
>> Hi all,
>>
>> I'm using Rack::JSONP from rack-contrib in a Sinatra project. When I
>> issue a GET request that's not JSON (and that doesn't set a Content-
>> Type header, I get:
>>
>> NoMethodError: undefined method `include?' for nil:NilClass
>>        /Users/aslakhellesoy/.rvm/gems/ruby-1.8.7-p249@cukepatch/gems/rack-
>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44:in `is_json?'
>>
>> Am I the only one running into this? Should I patch with:
>>
>> (headers['Content-Type'] || []).include?('application/json') in rack-
>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44
>>
>> Or is it better to patch rack to return an empty array instead of nil
>> in #headers.
>>
>> I'm on rack 1.2.1 and Sinatra 1.0.
>>
>> Cheers,
>> Aslak
>

Re: Bug in Rack::JSONP?

[Gaius]

+1, Magnus

On Jun 22, 8:32 am, Magnus Holm <judo...@gmail.com> wrote:
> What about something like this:
>
> diff --git a/lib/rack/contrib/jsonp.rb b/lib/rack/contrib/jsonp.rb
> index 723d63a..7eefe00 100644
> --- a/lib/rack/contrib/jsonp.rb
> +++ b/lib/rack/contrib/jsonp.rb
> @@ -19,6 +19,10 @@ module Rack
>      #
>      def call(env)
>        status, headers, response = @app.call(env)
> +
> +      if STATUS_WITH_NO_ENTITY_BODY.include?(status)
> +        return status, headers, response
> +      end
>
>        headers = HeaderHash.new(headers)
>        request = Rack::Request.new(env)
> diff --git a/test/spec_rack_jsonp.rb b/test/spec_rack_jsonp.rb
> index eb6570f..c1abc59 100644
> --- a/test/spec_rack_jsonp.rb
> +++ b/test/spec_rack_jsonp.rb
> @@ -69,5 +69,15 @@ context "Rack::JSONP" do
>      body = Rack::JSONP.new(app).call(request).last
>      body.should.equal [test_body]
>    end
> -
> +
> +  specify "should not change anything if the request doesn't have a body" do
> +    app1 = lambda { |env| [100, {}, []] }
> +    app2 = lambda { |env| [204, {}, []] }
> +    app3 = lambda { |env| [304, {}, []] }
> +    request = Rack::MockRequest.env_for("/", :params =>
> "callback=foo", 'HTTP_ACCEPT' => 'application/json')
> +
> +    Rack::JSONP.new(app1).call(request).should.equal app1.call({})
> +    Rack::JSONP.new(app2).call(request).should.equal app2.call({})
> +    Rack::JSONP.new(app3).call(request).should.equal app3.call({})
> +  end
>  end
> \ No newline at end of file
>
> // Magnus Holm
>
>
>
> On Tue, Jun 22, 2010 at 14:22, Magnus Holm <judo...@gmail.com> wrote:
> > From the spec: "There must be a Content-Type, except when the Status
> > is 1xx, 204 or 304, in which case there must be none given."
>
> > I guess we should rather make JSONP a noop when the status is 1xx, 204 or 304.
>
> > // Magnus Holm
>
> > On Tue, Jun 22, 2010 at 14:08, Aslak Hellesøy <aslak.helle...@gmail.com> wrote:
> >> Hi all,
>
> >> I'm using Rack::JSONP from rack-contrib in a Sinatra project. When I
> >> issue a GET request that's not JSON (and that doesn't set a Content-
> >> Type header, I get:
>
> >> NoMethodError: undefined method `include?' for nil:NilClass
> >>        /Users/aslakhellesoy/.rvm/gems/ruby-1.8.7-p249@cukepatch/gems/rack-
> >> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44:in `is_json?'
>
> >> Am I the only one running into this? Should I patch with:
>
> >> (headers['Content-Type'] || []).include?('application/json') in rack-
> >> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44
>
> >> Or is it better to patch rack to return an empty array instead of nil
> >> in #headers.
>
> >> I'm on rack 1.2.1 and Sinatra 1.0.
>
> >> Cheers,
> >> Aslak

Re: Bug in Rack::JSONP?

[aslak hellesoy]

On Tue, Jun 22, 2010 at 2:32 PM, Magnus Holm <judofyr@gmail.com> wrote:
> What about something like this:
>

Thanks a ton Magnus - this works great for me.
I see you haven't pushed anything, so I have pushed this to my own
fork in case someone wants to merge it in. I hope so, it's obviously a
bug (and a bugfix).

http://github.com/aslakhellesoy/rack-contrib/commit/952cb23311c69fcad16e5dfade746a3884b22b9e

Aslak

> diff --git a/lib/rack/contrib/jsonp.rb b/lib/rack/contrib/jsonp.rb
> index 723d63a..7eefe00 100644
> --- a/lib/rack/contrib/jsonp.rb
> +++ b/lib/rack/contrib/jsonp.rb
> @@ -19,6 +19,10 @@ module Rack
>     #
>     def call(env)
>       status, headers, response = @app.call(env)
> +
> +      if STATUS_WITH_NO_ENTITY_BODY.include?(status)
> +        return status, headers, response
> +      end
>
>       headers = HeaderHash.new(headers)
>       request = Rack::Request.new(env)
> diff --git a/test/spec_rack_jsonp.rb b/test/spec_rack_jsonp.rb
> index eb6570f..c1abc59 100644
> --- a/test/spec_rack_jsonp.rb
> +++ b/test/spec_rack_jsonp.rb
> @@ -69,5 +69,15 @@ context "Rack::JSONP" do
>     body = Rack::JSONP.new(app).call(request).last
>     body.should.equal [test_body]
>   end
> -
> +
> +  specify "should not change anything if the request doesn't have a body" do
> +    app1 = lambda { |env| [100, {}, []] }
> +    app2 = lambda { |env| [204, {}, []] }
> +    app3 = lambda { |env| [304, {}, []] }
> +    request = Rack::MockRequest.env_for("/", :params =>
> "callback=foo", 'HTTP_ACCEPT' => 'application/json')
> +
> +    Rack::JSONP.new(app1).call(request).should.equal app1.call({})
> +    Rack::JSONP.new(app2).call(request).should.equal app2.call({})
> +    Rack::JSONP.new(app3).call(request).should.equal app3.call({})
> +  end
>  end
> \ No newline at end of file
>
>
> // Magnus Holm
>
>
>
> On Tue, Jun 22, 2010 at 14:22, Magnus Holm <judofyr@gmail.com> wrote:
>> From the spec: "There must be a Content-Type, except when the Status
>> is 1xx, 204 or 304, in which case there must be none given."
>>
>> I guess we should rather make JSONP a noop when the status is 1xx, 204 or 304.
>>
>> // Magnus Holm
>>
>>
>> On Tue, Jun 22, 2010 at 14:08, Aslak Hellesøy <aslak.hellesoy@gmail.com> wrote:
>>> Hi all,
>>>
>>> I'm using Rack::JSONP from rack-contrib in a Sinatra project. When I
>>> issue a GET request that's not JSON (and that doesn't set a Content-
>>> Type header, I get:
>>>
>>> NoMethodError: undefined method `include?' for nil:NilClass
>>>        /Users/aslakhellesoy/.rvm/gems/ruby-1.8.7-p249@cukepatch/gems/rack-
>>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44:in `is_json?'
>>>
>>> Am I the only one running into this? Should I patch with:
>>>
>>> (headers['Content-Type'] || []).include?('application/json') in rack-
>>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44
>>>
>>> Or is it better to patch rack to return an empty array instead of nil
>>> in #headers.
>>>
>>> I'm on rack 1.2.1 and Sinatra 1.0.
>>>
>>> Cheers,
>>> Aslak
>>
>

Re: Bug in Rack::JSONP?

[Matt Todd]

+1

Is STATUS_WITH_NO_ENTITY_BODY defined elsewhere and not included just
by accident? I did a cursory search and didn't see it, but it's
probably part of Rack or rack-contrib that I didn't look at...

Also, it's OK that we wrap a callback around an empty response, right?
The callback just gets called with no param, which is fine as long as
the user takes that into account (or just never lets that happen on
the server side).

It's been a while since I've looked at this code, but it's been
refactored quite nicely since it was first added.

Matt



On Tue, Jun 22, 2010 at 11:30 AM, aslak hellesoy
<aslak.hellesoy@gmail.com> wrote:
> On Tue, Jun 22, 2010 at 2:32 PM, Magnus Holm <judofyr@gmail.com> wrote:
>> What about something like this:
>>
>
> Thanks a ton Magnus - this works great for me.
> I see you haven't pushed anything, so I have pushed this to my own
> fork in case someone wants to merge it in. I hope so, it's obviously a
> bug (and a bugfix).
>
> http://github.com/aslakhellesoy/rack-contrib/commit/952cb23311c69fcad16e5dfade746a3884b22b9e
>
> Aslak
>
>> diff --git a/lib/rack/contrib/jsonp.rb b/lib/rack/contrib/jsonp.rb
>> index 723d63a..7eefe00 100644
>> --- a/lib/rack/contrib/jsonp.rb
>> +++ b/lib/rack/contrib/jsonp.rb
>> @@ -19,6 +19,10 @@ module Rack
>>     #
>>     def call(env)
>>       status, headers, response = @app.call(env)
>> +
>> +      if STATUS_WITH_NO_ENTITY_BODY.include?(status)
>> +        return status, headers, response
>> +      end
>>
>>       headers = HeaderHash.new(headers)
>>       request = Rack::Request.new(env)
>> diff --git a/test/spec_rack_jsonp.rb b/test/spec_rack_jsonp.rb
>> index eb6570f..c1abc59 100644
>> --- a/test/spec_rack_jsonp.rb
>> +++ b/test/spec_rack_jsonp.rb
>> @@ -69,5 +69,15 @@ context "Rack::JSONP" do
>>     body = Rack::JSONP.new(app).call(request).last
>>     body.should.equal [test_body]
>>   end
>> -
>> +
>> +  specify "should not change anything if the request doesn't have a body" do
>> +    app1 = lambda { |env| [100, {}, []] }
>> +    app2 = lambda { |env| [204, {}, []] }
>> +    app3 = lambda { |env| [304, {}, []] }
>> +    request = Rack::MockRequest.env_for("/", :params =>
>> "callback=foo", 'HTTP_ACCEPT' => 'application/json')
>> +
>> +    Rack::JSONP.new(app1).call(request).should.equal app1.call({})
>> +    Rack::JSONP.new(app2).call(request).should.equal app2.call({})
>> +    Rack::JSONP.new(app3).call(request).should.equal app3.call({})
>> +  end
>>  end
>> \ No newline at end of file
>>
>>
>> // Magnus Holm
>>
>>
>>
>> On Tue, Jun 22, 2010 at 14:22, Magnus Holm <judofyr@gmail.com> wrote:
>>> From the spec: "There must be a Content-Type, except when the Status
>>> is 1xx, 204 or 304, in which case there must be none given."
>>>
>>> I guess we should rather make JSONP a noop when the status is 1xx, 204 or 304.
>>>
>>> // Magnus Holm
>>>
>>>
>>> On Tue, Jun 22, 2010 at 14:08, Aslak Hellesøy <aslak.hellesoy@gmail.com> wrote:
>>>> Hi all,
>>>>
>>>> I'm using Rack::JSONP from rack-contrib in a Sinatra project. When I
>>>> issue a GET request that's not JSON (and that doesn't set a Content-
>>>> Type header, I get:
>>>>
>>>> NoMethodError: undefined method `include?' for nil:NilClass
>>>>        /Users/aslakhellesoy/.rvm/gems/ruby-1.8.7-p249@cukepatch/gems/rack-
>>>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44:in `is_json?'
>>>>
>>>> Am I the only one running into this? Should I patch with:
>>>>
>>>> (headers['Content-Type'] || []).include?('application/json') in rack-
>>>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44
>>>>
>>>> Or is it better to patch rack to return an empty array instead of nil
>>>> in #headers.
>>>>
>>>> I'm on rack 1.2.1 and Sinatra 1.0.
>>>>
>>>> Cheers,
>>>> Aslak
>>>
>>
>



-- 
Matt Todd
Highgroove Studios
www.highgroove.com
cell: 404-314-2612
blog: maraby.org

Scout - Web Monitoring and Reporting Software
www.scoutapp.com

Re: Bug in Rack::JSONP?

[Magnus Holm]

I simply don't really see the value of a fork instead of a simple
patchfile. 6 months from now nearly all forks are gone - this piece of
text lives on forever in all clients and archives.

On Tuesday, June 22, 2010, aslak hellesoy <aslak.hellesoy@gmail.com> wrote:
> On Tue, Jun 22, 2010 at 2:32 PM, Magnus Holm <judofyr@gmail.com> wrote:
>> What about something like this:
>>
>
> Thanks a ton Magnus - this works great for me.
> I see you haven't pushed anything, so I have pushed this to my own
> fork in case someone wants to merge it in. I hope so, it's obviously a
> bug (and a bugfix).
>
> http://github.com/aslakhellesoy/rack-contrib/commit/952cb23311c69fcad16e5dfade746a3884b22b9e
>
> Aslak
>
>> diff --git a/lib/rack/contrib/jsonp.rb b/lib/rack/contrib/jsonp.rb
>> index 723d63a..7eefe00 100644
>> --- a/lib/rack/contrib/jsonp.rb
>> +++ b/lib/rack/contrib/jsonp.rb
>> @@ -19,6 +19,10 @@ module Rack
>>     #
>>     def call(env)
>>       status, headers, response = @app.call(env)
>> +
>> +      if STATUS_WITH_NO_ENTITY_BODY.include?(status)
>> +        return status, headers, response
>> +      end
>>
>>       headers = HeaderHash.new(headers)
>>       request = Rack::Request.new(env)
>> diff --git a/test/spec_rack_jsonp.rb b/test/spec_rack_jsonp.rb
>> index eb6570f..c1abc59 100644
>> --- a/test/spec_rack_jsonp.rb
>> +++ b/test/spec_rack_jsonp.rb
>> @@ -69,5 +69,15 @@ context "Rack::JSONP" do
>>     body = Rack::JSONP.new(app).call(request).last
>>     body.should.equal [test_body]
>>   end
>> -
>> +
>> +  specify "should not change anything if the request doesn't have a body" do
>> +    app1 = lambda { |env| [100, {}, []] }
>> +    app2 = lambda { |env| [204, {}, []] }
>> +    app3 = lambda { |env| [304, {}, []] }
>> +    request = Rack::MockRequest.env_for("/", :params =>
>> "callback=foo", 'HTTP_ACCEPT' => 'application/json')
>> +
>> +    Rack::JSONP.new(app1).call(request).should.equal app1.call({})
>> +    Rack::JSONP.new(app2).call(request).should.equal app2.call({})
>> +    Rack::JSONP.new(app3).call(request).should.equal app3.call({})
>> +  end
>>  end
>> \ No newline at end of file
>>
>>
>> // Magnus Holm
>>
>>
>>
>> On Tue, Jun 22, 2010 at 14:22, Magnus Holm <judofyr@gmail.com> wrote:
>>> From the spec: "There must be a Content-Type, except when the Status
>>> is 1xx, 204 or 304, in which case there must be none given."
>>>
>>> I guess we should rather make JSONP a noop when the status is 1xx, 204 or 304.
>>>
>>> // Magnus Holm
>>>
>>>
>>> On Tue, Jun 22, 2010 at 14:08, Aslak Hellesøy <aslak.hellesoy@gmail.com> wrote:
>>>> Hi all,
>>>>
>>>> I'm using Rack::JSONP from rack-contrib in a Sinatra project. When I
>>>> issue a GET request that's not JSON (and that doesn't set a Content-
>>>> Type header, I get:
>>>>
>>>> NoMethodError: undefined method `include?' for nil:NilClass
>>>>        /Users/aslakhellesoy/.rvm/gems/ruby-1.8.7-p249@cukepatch/gems/rack-
>>>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44:in `is_json?'
>>>>
>>>> Am I the only one running into this? Should I patch with:
>>>>
>>>> (headers['Content-Type'] || []).include?('application/json') in rack-
>>>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44
>>>>
>>>> Or is it better to patch rack to return an empty array instead of nil
>>>> in #headers.
>>>>
>>>> I'm on rack 1.2.1 and Sinatra 1.0.
>>>>
>>>> Cheers,
>>>> Aslak
>>>
>>
>

-- 
// Magnus Holm

Re: Bug in Rack::JSONP?

[aslak hellesoy]

On Tue, Jun 22, 2010 at 7:21 PM, Magnus Holm <judofyr@gmail.com> wrote:
> I simply don't really see the value of a fork instead of a simple
> patchfile. 6 months from now nearly all forks are gone - this piece of
> text lives on forever in all clients and archives.
>

I needed to share the code with my team and deploy to a server, hence
a fork with the fix. A patchfile doesn't cut it in this case. I'm not
going to interfere with how the rack-contrib maintainers keep it on
the radar and merge it in.

Aslak

> On Tuesday, June 22, 2010, aslak hellesoy <aslak.hellesoy@gmail.com> wrote:
>> On Tue, Jun 22, 2010 at 2:32 PM, Magnus Holm <judofyr@gmail.com> wrote:
>>> What about something like this:
>>>
>>
>> Thanks a ton Magnus - this works great for me.
>> I see you haven't pushed anything, so I have pushed this to my own
>> fork in case someone wants to merge it in. I hope so, it's obviously a
>> bug (and a bugfix).
>>
>> http://github.com/aslakhellesoy/rack-contrib/commit/952cb23311c69fcad16e5dfade746a3884b22b9e
>>
>> Aslak
>>
>>> diff --git a/lib/rack/contrib/jsonp.rb b/lib/rack/contrib/jsonp.rb
>>> index 723d63a..7eefe00 100644
>>> --- a/lib/rack/contrib/jsonp.rb
>>> +++ b/lib/rack/contrib/jsonp.rb
>>> @@ -19,6 +19,10 @@ module Rack
>>>     #
>>>     def call(env)
>>>       status, headers, response = @app.call(env)
>>> +
>>> +      if STATUS_WITH_NO_ENTITY_BODY.include?(status)
>>> +        return status, headers, response
>>> +      end
>>>
>>>       headers = HeaderHash.new(headers)
>>>       request = Rack::Request.new(env)
>>> diff --git a/test/spec_rack_jsonp.rb b/test/spec_rack_jsonp.rb
>>> index eb6570f..c1abc59 100644
>>> --- a/test/spec_rack_jsonp.rb
>>> +++ b/test/spec_rack_jsonp.rb
>>> @@ -69,5 +69,15 @@ context "Rack::JSONP" do
>>>     body = Rack::JSONP.new(app).call(request).last
>>>     body.should.equal [test_body]
>>>   end
>>> -
>>> +
>>> +  specify "should not change anything if the request doesn't have a body" do
>>> +    app1 = lambda { |env| [100, {}, []] }
>>> +    app2 = lambda { |env| [204, {}, []] }
>>> +    app3 = lambda { |env| [304, {}, []] }
>>> +    request = Rack::MockRequest.env_for("/", :params =>
>>> "callback=foo", 'HTTP_ACCEPT' => 'application/json')
>>> +
>>> +    Rack::JSONP.new(app1).call(request).should.equal app1.call({})
>>> +    Rack::JSONP.new(app2).call(request).should.equal app2.call({})
>>> +    Rack::JSONP.new(app3).call(request).should.equal app3.call({})
>>> +  end
>>>  end
>>> \ No newline at end of file
>>>
>>>
>>> // Magnus Holm
>>>
>>>
>>>
>>> On Tue, Jun 22, 2010 at 14:22, Magnus Holm <judofyr@gmail.com> wrote:
>>>> From the spec: "There must be a Content-Type, except when the Status
>>>> is 1xx, 204 or 304, in which case there must be none given."
>>>>
>>>> I guess we should rather make JSONP a noop when the status is 1xx, 204 or 304.
>>>>
>>>> // Magnus Holm
>>>>
>>>>
>>>> On Tue, Jun 22, 2010 at 14:08, Aslak Hellesøy <aslak.hellesoy@gmail.com> wrote:
>>>>> Hi all,
>>>>>
>>>>> I'm using Rack::JSONP from rack-contrib in a Sinatra project. When I
>>>>> issue a GET request that's not JSON (and that doesn't set a Content-
>>>>> Type header, I get:
>>>>>
>>>>> NoMethodError: undefined method `include?' for nil:NilClass
>>>>>        /Users/aslakhellesoy/.rvm/gems/ruby-1.8.7-p249@cukepatch/gems/rack-
>>>>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44:in `is_json?'
>>>>>
>>>>> Am I the only one running into this? Should I patch with:
>>>>>
>>>>> (headers['Content-Type'] || []).include?('application/json') in rack-
>>>>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44
>>>>>
>>>>> Or is it better to patch rack to return an empty array instead of nil
>>>>> in #headers.
>>>>>
>>>>> I'm on rack 1.2.1 and Sinatra 1.0.
>>>>>
>>>>> Cheers,
>>>>> Aslak
>>>>
>>>
>>
>
> --
> // Magnus Holm
>

Re: Bug in Rack::JSONP?

[Magnus Holm]

I simply don't really see the value of a fork instead of a simple
patchfile. 6 months from now nearly all forks are gone - this piece of
text lives on forever in all clients and archives.

On Tuesday, June 22, 2010, aslak hellesoy <aslak.hellesoy@gmail.com> wrote:
> On Tue, Jun 22, 2010 at 2:32 PM, Magnus Holm <judofyr@gmail.com> wrote:
>> What about something like this:
>>
>
> Thanks a ton Magnus - this works great for me.
> I see you haven't pushed anything, so I have pushed this to my own
> fork in case someone wants to merge it in. I hope so, it's obviously a
> bug (and a bugfix).
>
> http://github.com/aslakhellesoy/rack-contrib/commit/952cb23311c69fcad16e5dfade746a3884b22b9e
>
> Aslak
>
>> diff --git a/lib/rack/contrib/jsonp.rb b/lib/rack/contrib/jsonp.rb
>> index 723d63a..7eefe00 100644
>> --- a/lib/rack/contrib/jsonp.rb
>> +++ b/lib/rack/contrib/jsonp.rb
>> @@ -19,6 +19,10 @@ module Rack
>>     #
>>     def call(env)
>>       status, headers, response = @app.call(env)
>> +
>> +      if STATUS_WITH_NO_ENTITY_BODY.include?(status)
>> +        return status, headers, response
>> +      end
>>
>>       headers = HeaderHash.new(headers)
>>       request = Rack::Request.new(env)
>> diff --git a/test/spec_rack_jsonp.rb b/test/spec_rack_jsonp.rb
>> index eb6570f..c1abc59 100644
>> --- a/test/spec_rack_jsonp.rb
>> +++ b/test/spec_rack_jsonp.rb
>> @@ -69,5 +69,15 @@ context "Rack::JSONP" do
>>     body = Rack::JSONP.new(app).call(request).last
>>     body.should.equal [test_body]
>>   end
>> -
>> +
>> +  specify "should not change anything if the request doesn't have a body" do
>> +    app1 = lambda { |env| [100, {}, []] }
>> +    app2 = lambda { |env| [204, {}, []] }
>> +    app3 = lambda { |env| [304, {}, []] }
>> +    request = Rack::MockRequest.env_for("/", :params =>
>> "callback=foo", 'HTTP_ACCEPT' => 'application/json')
>> +
>> +    Rack::JSONP.new(app1).call(request).should.equal app1.call({})
>> +    Rack::JSONP.new(app2).call(request).should.equal app2.call({})
>> +    Rack::JSONP.new(app3).call(request).should.equal app3.call({})
>> +  end
>>  end
>> \ No newline at end of file
>>
>>
>> // Magnus Holm
>>
>>
>>
>> On Tue, Jun 22, 2010 at 14:22, Magnus Holm <judofyr@gmail.com> wrote:
>>> From the spec: "There must be a Content-Type, except when the Status
>>> is 1xx, 204 or 304, in which case there must be none given."
>>>
>>> I guess we should rather make JSONP a noop when the status is 1xx, 204 or 304.
>>>
>>> // Magnus Holm
>>>
>>>
>>> On Tue, Jun 22, 2010 at 14:08, Aslak Hellesøy <aslak.hellesoy@gmail.com> wrote:
>>>> Hi all,
>>>>
>>>> I'm using Rack::JSONP from rack-contrib in a Sinatra project. When I
>>>> issue a GET request that's not JSON (and that doesn't set a Content-
>>>> Type header, I get:
>>>>
>>>> NoMethodError: undefined method `include?' for nil:NilClass
>>>>        /Users/aslakhellesoy/.rvm/gems/ruby-1.8.7-p249@cukepatch/gems/rack-
>>>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44:in `is_json?'
>>>>
>>>> Am I the only one running into this? Should I patch with:
>>>>
>>>> (headers['Content-Type'] || []).include?('application/json') in rack-
>>>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44
>>>>
>>>> Or is it better to patch rack to return an empty array instead of nil
>>>> in #headers.
>>>>
>>>> I'm on rack 1.2.1 and Sinatra 1.0.
>>>>
>>>> Cheers,
>>>> Aslak
>>>
>>
>

-- 
// Magnus Holm

Re: Bug in Rack::JSONP?

[Magnus Holm]

Sure, it was just an explanation of my personal opinion :-)

// Magnus Holm



On Tue, Jun 22, 2010 at 19:28, aslak hellesoy <aslak.hellesoy@gmail.com> wrote:
> On Tue, Jun 22, 2010 at 7:21 PM, Magnus Holm <judofyr@gmail.com> wrote:
>> I simply don't really see the value of a fork instead of a simple
>> patchfile. 6 months from now nearly all forks are gone - this piece of
>> text lives on forever in all clients and archives.
>>
>
> I needed to share the code with my team and deploy to a server, hence
> a fork with the fix. A patchfile doesn't cut it in this case. I'm not
> going to interfere with how the rack-contrib maintainers keep it on
> the radar and merge it in.
>
> Aslak
>
>> On Tuesday, June 22, 2010, aslak hellesoy <aslak.hellesoy@gmail.com> wrote:
>>> On Tue, Jun 22, 2010 at 2:32 PM, Magnus Holm <judofyr@gmail.com> wrote:
>>>> What about something like this:
>>>>
>>>
>>> Thanks a ton Magnus - this works great for me.
>>> I see you haven't pushed anything, so I have pushed this to my own
>>> fork in case someone wants to merge it in. I hope so, it's obviously a
>>> bug (and a bugfix).
>>>
>>> http://github.com/aslakhellesoy/rack-contrib/commit/952cb23311c69fcad16e5dfade746a3884b22b9e
>>>
>>> Aslak
>>>
>>>> diff --git a/lib/rack/contrib/jsonp.rb b/lib/rack/contrib/jsonp.rb
>>>> index 723d63a..7eefe00 100644
>>>> --- a/lib/rack/contrib/jsonp.rb
>>>> +++ b/lib/rack/contrib/jsonp.rb
>>>> @@ -19,6 +19,10 @@ module Rack
>>>>     #
>>>>     def call(env)
>>>>       status, headers, response = @app.call(env)
>>>> +
>>>> +      if STATUS_WITH_NO_ENTITY_BODY.include?(status)
>>>> +        return status, headers, response
>>>> +      end
>>>>
>>>>       headers = HeaderHash.new(headers)
>>>>       request = Rack::Request.new(env)
>>>> diff --git a/test/spec_rack_jsonp.rb b/test/spec_rack_jsonp.rb
>>>> index eb6570f..c1abc59 100644
>>>> --- a/test/spec_rack_jsonp.rb
>>>> +++ b/test/spec_rack_jsonp.rb
>>>> @@ -69,5 +69,15 @@ context "Rack::JSONP" do
>>>>     body = Rack::JSONP.new(app).call(request).last
>>>>     body.should.equal [test_body]
>>>>   end
>>>> -
>>>> +
>>>> +  specify "should not change anything if the request doesn't have a body" do
>>>> +    app1 = lambda { |env| [100, {}, []] }
>>>> +    app2 = lambda { |env| [204, {}, []] }
>>>> +    app3 = lambda { |env| [304, {}, []] }
>>>> +    request = Rack::MockRequest.env_for("/", :params =>
>>>> "callback=foo", 'HTTP_ACCEPT' => 'application/json')
>>>> +
>>>> +    Rack::JSONP.new(app1).call(request).should.equal app1.call({})
>>>> +    Rack::JSONP.new(app2).call(request).should.equal app2.call({})
>>>> +    Rack::JSONP.new(app3).call(request).should.equal app3.call({})
>>>> +  end
>>>>  end
>>>> \ No newline at end of file
>>>>
>>>>
>>>> // Magnus Holm
>>>>
>>>>
>>>>
>>>> On Tue, Jun 22, 2010 at 14:22, Magnus Holm <judofyr@gmail.com> wrote:
>>>>> From the spec: "There must be a Content-Type, except when the Status
>>>>> is 1xx, 204 or 304, in which case there must be none given."
>>>>>
>>>>> I guess we should rather make JSONP a noop when the status is 1xx, 204 or 304.
>>>>>
>>>>> // Magnus Holm
>>>>>
>>>>>
>>>>> On Tue, Jun 22, 2010 at 14:08, Aslak Hellesøy <aslak.hellesoy@gmail.com> wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> I'm using Rack::JSONP from rack-contrib in a Sinatra project. When I
>>>>>> issue a GET request that's not JSON (and that doesn't set a Content-
>>>>>> Type header, I get:
>>>>>>
>>>>>> NoMethodError: undefined method `include?' for nil:NilClass
>>>>>>        /Users/aslakhellesoy/.rvm/gems/ruby-1.8.7-p249@cukepatch/gems/rack-
>>>>>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44:in `is_json?'
>>>>>>
>>>>>> Am I the only one running into this? Should I patch with:
>>>>>>
>>>>>> (headers['Content-Type'] || []).include?('application/json') in rack-
>>>>>> contrib-1.0.1/lib/rack/contrib/jsonp.rb:44
>>>>>>
>>>>>> Or is it better to patch rack to return an empty array instead of nil
>>>>>> in #headers.
>>>>>>
>>>>>> I'm on rack 1.2.1 and Sinatra 1.0.
>>>>>>
>>>>>> Cheers,
>>>>>> Aslak
>>>>>
>>>>
>>>
>>
>> --
>> // Magnus Holm
>>
>