Potential Rack-Contrib contributions

Potential Rack-Contrib contributions

[Geoff Buesing]

I've pulled together some useful generic middleware into
http://github.com/gbuesing/rackables:

* Rackables::Branch - Conditionally re-routes the Rack stack at
runtime to an alternate endpoint
* Rackables::CacheControl - Sets response Cache-Control header
* Rackables::DefaultCharset - Sets charset directive in Content-Type
header
* Rackables::Get - Allows creation of simple endpoints and path
routing with a syntax similar to Sinatra's get method
* Rackables::HideExceptions - Rescues exceptions with a static
exception page
* Rackables::TrailingSlashRedirect - 301 Redirects requests paths with
a trailing slash

If any of these would be a good fit for Rack-Contrib, let me know, I'd
be happy to pull together patches.

Re: Potential Rack-Contrib contributions

[Ryan Tomayko]

On Fri, Jan 15, 2010 at 8:23 AM, Geoff Buesing <gbuesing@gmail.com> wrote:
> I've pulled together some useful generic middleware into
> http://github.com/gbuesing/rackables:
>
> * Rackables::Branch - Conditionally re-routes the Rack stack at
> runtime to an alternate endpoint
> * Rackables::CacheControl - Sets response Cache-Control header
> * Rackables::DefaultCharset - Sets charset directive in Content-Type
> header
> * Rackables::Get - Allows creation of simple endpoints and path
> routing with a syntax similar to Sinatra's get method
> * Rackables::HideExceptions - Rescues exceptions with a static
> exception page
> * Rackables::TrailingSlashRedirect - 301 Redirects requests paths with
> a trailing slash
>
> If any of these would be a good fit for Rack-Contrib, let me know, I'd
> be happy to pull together patches.

You have some great stuff in there.

So far, the rack-contrib policy has been to accept anything that's
generally useful, has tests, and is more or less documented. All of
the above appear to pass those tests.

The only thing I would have a problem with is Rackables::Get -- the
name is just too general. How about SimpleGetEndpoint or something
similarly verbose. Classname brevity isn't a feature when it comes to
rack-contrib.

If you want to roll these into one rack-contrib patch per middleware,
I'd be very happy to review and merge them.

Thanks,
Ryan

Re: Potential Rack-Contrib contributions

[Iñaki Baz Castillo]

El Viernes, 15 de Enero de 2010, Geoff Buesing escribió:
> * Rackables::DefaultCharset - Sets charset directive in Content-Type
> header

Hi, I'm inspecting DefaultCharset middleware:


  class DefaultCharset
    HAS_CHARSET = /charset=/
 
    def initialize(app, value = 'utf-8')
      @app = app
      @value = value
    end
 
    def call(env)
      response = @app.call(env)
      headers = response[1]
      content_type = headers['Content-Type']
      if content_type && content_type !~ HAS_CHARSET
        headers['Content-Type'] = "#{content_type}; charset=#{@value}"
      end
      response
    end
  end


I see two issues here:

1) HAS_CHARSET = /charset=/
Note that HTTP grammar is case insensitive (in most of the cases) so IMGO it 
shoud be:

  HAS_CHARSET = /charset=/i

2) content_type = headers['Content-Type']
Perhaps I'm wrong, but what about if the response headers created by other 
middleware or Rack app uses "content-type" or "CONTENT-type"? again RFC 2616 
BNF grammar is case insensitive by default.

However in case 2 perhaps I'm wrong as I don't know if Rack require some exact 
format for headers name into generated responses.


Regards.



-- 
Iñaki Baz Castillo <ibc@aliax.net>

Re: Potential Rack-Contrib contributions

[Iñaki Baz Castillo]

El Sábado, 16 de Enero de 2010, Iñaki Baz Castillo escribió:

> 1) HAS_CHARSET = /charset=/
> Note that HTTP grammar is case insensitive (in most of the cases) so IMGO
>  it shoud be:
> 
>   HAS_CHARSET = /charset=/i


Humm, definitively it's not enough. HTTP BNF grammar is really more complex 
that the above hyper simplified assumption.

The folllowing headers are all valid "Content-Type" headers, and all of them 
contain a valid "charset" param (replace <LF> with a real \r symbol and <TAB> 
with \t):


Content-Type: text/plain ; CharSet<LF>
  = utf-8


content-TYPE:text/plain ; CharSet<LF>
  = utf-8


Content-Type: text/plain <LF>
 ; CharSet <LF>
  = utf-8


Content-Type:<TAB><TAB>text/plain<TAB>; <LF>
 charset <LF>
<TAB><TAB>  <TAB> = <TAB><LF>
  utf-8


Well for sure we can assume that other Rack middlewwares and also our Rack app 
would generate a simple inline header value. Then I would rewrite the regex to 
this one (the ";" is important):

  HAS_CHARSET = /;[\s\t]*charset[\s\t]*=[\s\t]*/i


Regards.


-- 
Iñaki Baz Castillo <ibc@aliax.net>

Re: Potential Rack-Contrib contributions

[Michael Fellinger]

> Well for sure we can assume that other Rack middlewwares and also our Rack app
> would generate a simple inline header value. Then I would rewrite the regex to
> this one (the ";" is important):
>
>  HAS_CHARSET = /;[\s\t]*charset[\s\t]*=[\s\t]*/i

"\t" =~ /\s/
# 0
means
/;\s*charset\s*=\s*/i
will be enough.

Regarding the case of Content-Type in the header Hash, as long as it's
an instance of HeaderHash, the case will not matter, we normalize it
behind the scene.

-- 
Michael Fellinger
CTO, The Rubyists, LLC

Re: Potential Rack-Contrib contributions

[Iñaki Baz Castillo]

El Sábado, 16 de Enero de 2010, Michael Fellinger escribió:
> > Well for sure we can assume that other Rack middlewwares and also our
> > Rack app would generate a simple inline header value. Then I would
> > rewrite the regex to this one (the ";" is important):
> >
> >  HAS_CHARSET = /;[\s\t]*charset[\s\t]*=[\s\t]*/i
> 
> "\t" =~ /\s/
> # 0
> means
> /;\s*charset\s*=\s*/i
> will be enough.

Right, my fault, normally I use [ \t]* instead of [\s\t]* which makes no sense 
as \s includes space and tabulator.


> Regarding the case of Content-Type in the header Hash, as long as it's
> an instance of HeaderHash, the case will not matter, we normalize it
> behind the scene.

ok, that's why I said "in case 2 perhaps I'm wrong" :)

Thanks a lot.


-- 
Iñaki Baz Castillo <ibc@aliax.net>

Re: Potential Rack-Contrib contributions

[Geoff Buesing]

@Ryan - thanks, I'll pull together patches. And I agree that
Rackables::Get is to generic of a name; SimpleGetEndpoint is much
better. Running with that idea, I'm thinking of going a bit more
generic: Rack::SimpleEndpoint, with a verb requirement as an optional
argument.

@Iñaki and @Michael - thanks for the case-sensitivity heads up, and
the regex. I'll make sure to put in tests for these cases.

Geoff

On Jan 16, 8:47 am, Iñaki Baz Castillo <i...@aliax.net> wrote:
> El Sábado, 16 de Enero de 2010, Michael Fellinger escribió:
>
> > > Well for sure we can assume that other Rack middlewwares and also our
> > > Rack app would generate a simple inline header value. Then I would
> > > rewrite the regex to this one (the ";" is important):
>
> > >  HAS_CHARSET = /;[\s\t]*charset[\s\t]*=[\s\t]*/i
>
> > "\t" =~ /\s/
> > # 0
> > means
> > /;\s*charset\s*=\s*/i
> > will be enough.
>
> Right, my fault, normally I use [ \t]* instead of [\s\t]* which makes no sense
> as \s includes space and tabulator.
>
> > Regarding the case of Content-Type in the header Hash, as long as it's
> > an instance of HeaderHash, the case will not matter, we normalize it
> > behind the scene.
>
> ok, that's why I said "in case 2 perhaps I'm wrong" :)
>
> Thanks a lot.
>
> --
> Iñaki Baz Castillo <i...@aliax.net>

Re: Potential Rack-Contrib contributions

[Geoff Buesing]

@Ryan - patches added as Lighthouse tickets:

http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/3-new-middleware-rackbranch
http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/4-new-middleware-rackcachecontrol
http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/5-new-middleware-rackdefaultcharset
http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/6-new-middleware-rackhideexceptions
http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/7-new-middleware-racksimpleendpoint
http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/8-rackenv-reader-and-rackenvdevelopment-etc-inquiry-methods

Thanks!

Geoff

On Jan 16, 11:47 am, Geoff Buesing <gbues...@gmail.com> wrote:
> @Ryan - thanks, I'll pull together patches. And I agree that
> Rackables::Get is to generic of a name; SimpleGetEndpoint is much
> better. Running with that idea, I'm thinking of going a bit more
> generic: Rack::SimpleEndpoint, with a verb requirement as an optional
> argument.
>
> @Iñaki and @Michael - thanks for the case-sensitivity heads up, and
> the regex. I'll make sure to put in tests for these cases.
>
> Geoff
>
> On Jan 16, 8:47 am, Iñaki Baz Castillo <i...@aliax.net> wrote:
>
>
>
> > El Sábado, 16 de Enero de 2010, Michael Fellinger escribió:
>
> > > > Well for sure we can assume that other Rack middlewwares and also our
> > > > Rack app would generate a simple inline header value. Then I would
> > > > rewrite the regex to this one (the ";" is important):
>
> > > >  HAS_CHARSET = /;[\s\t]*charset[\s\t]*=[\s\t]*/i
>
> > > "\t" =~ /\s/
> > > # 0
> > > means
> > > /;\s*charset\s*=\s*/i
> > > will be enough.
>
> > Right, my fault, normally I use [ \t]* instead of [\s\t]* which makes no sense
> > as \s includes space and tabulator.
>
> > > Regarding the case of Content-Type in the header Hash, as long as it's
> > > an instance of HeaderHash, the case will not matter, we normalize it
> > > behind the scene.
>
> > ok, that's why I said "in case 2 perhaps I'm wrong" :)
>
> > Thanks a lot.
>
> > --
> > Iñaki Baz Castillo <i...@aliax.net>

Re: Potential Rack-Contrib contributions

[Geoff Buesing]

... and one more:

http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/9-new-middleware-racktrailingslashredirect


On Jan 17, 12:32 pm, Geoff Buesing <gbues...@gmail.com> wrote:
> @Ryan - patches added as Lighthouse tickets:
>
> http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/3-n...http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/4-n...http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/5-n...http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/6-n...http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/7-n...http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/8-r...
>
> Thanks!
>
> Geoff
>
> On Jan 16, 11:47 am, Geoff Buesing <gbues...@gmail.com> wrote:
>
>
>
> > @Ryan - thanks, I'll pull together patches. And I agree that
> > Rackables::Get is to generic of a name; SimpleGetEndpoint is much
> > better. Running with that idea, I'm thinking of going a bit more
> > generic: Rack::SimpleEndpoint, with a verb requirement as an optional
> > argument.
>
> > @Iñaki and @Michael - thanks for the case-sensitivity heads up, and
> > the regex. I'll make sure to put in tests for these cases.
>
> > Geoff
>
> > On Jan 16, 8:47 am, Iñaki Baz Castillo <i...@aliax.net> wrote:
>
> > > El Sábado, 16 de Enero de 2010, Michael Fellinger escribió:
>
> > > > > Well for sure we can assume that other Rack middlewwares and also our
> > > > > Rack app would generate a simple inline header value. Then I would
> > > > > rewrite the regex to this one (the ";" is important):
>
> > > > >  HAS_CHARSET = /;[\s\t]*charset[\s\t]*=[\s\t]*/i
>
> > > > "\t" =~ /\s/
> > > > # 0
> > > > means
> > > > /;\s*charset\s*=\s*/i
> > > > will be enough.
>
> > > Right, my fault, normally I use [ \t]* instead of [\s\t]* which makes no sense
> > > as \s includes space and tabulator.
>
> > > > Regarding the case of Content-Type in the header Hash, as long as it's
> > > > an instance of HeaderHash, the case will not matter, we normalize it
> > > > behind the scene.
>
> > > ok, that's why I said "in case 2 perhaps I'm wrong" :)
>
> > > Thanks a lot.
>
> > > --
> > > Iñaki Baz Castillo <i...@aliax.net>