[Nitro] Questions about the big bundle.

Jonathan Buch john at oxyliquit.de
Tue Oct 10 10:42:58 EDT 2006


> Some questions regarding the latest big patch bundle.
> - what is the sqlite3 memory store?

Apparently sqlite has the ability to use the memory as database when the
filename is ':memory:', which is pretty cool I think.

> - can someone please describe the problem with index params, what
>   exactly does this patch fix and how.

Please provide the full patch descriptions as it is hard for me to remember
exactly the patch when it was done some time ago ^^;

I believe you mean this code piece in dispatcher.rb:

     unless found
       action = 'index'
       if klass.respond_to_action_or_template?(action)
         a = klass.instance_method(:index).arity
         if a < 0 || a >= parts.size
           idx = -1
           found = true

as opposed to your

     unless found
       action = 'index'
       if klass.respond_to_action_or_template?(action)
         idx = -1
         found = true

This is merely a patch to the 'fail early' strategy.  This patch can  
safely be

> - there is a patch named 'post params', what does it fix?

Not sure which patch you mean here, do you mean this one?

Fri Sep 29 15:46:06 CEST 2006  Jonathan Buch <john at oxyliquit.de>
   * Remove query munching from dispatcher, this is done in cgi.rb already
   This prevented mixing POST and GET parameters together and only GET  
   were used.
   Possible problems with SCGI adapter.

This is pretty much self explanatory:
When doing a `POST /action/param/param?more=params` with some attached
form data, the form data was thrown away and only the GET parameters
where used.

Anyway, to explain the other param changes just in case:

Your code:

       param_count = @controller.instance_method(action.to_sym).arity

       if param_count != 0
         code << %{
           params = []
           unless context.query_string.blank?
             all_params = context.query_string.split(/[&;]/)
             all_params.each_with_index do |param, i|
               break unless i < #{param_count};
               if param.index '='
                 params << CGI.unescape(param.split(/=/).last || '')
                 params << CGI.unescape(param)

           # Fill the array with nils for the missing params.
           (#{param_count} - params.size).times { params << nil }

           action_return_value = #{action}(*params)
         code << %{
           action_return_value = #{action}

This has a few problems:

* It replaces not available parameters with `nil`.  This might seem
   convenient at first, as no error gets thrown when the user enters
   a wrong URL, but it still is wrong.
   To create a variable parameter we write `def action(param1 = nil)`
   and not `def action(param1)`.  Fail early, why should I catch this
   error in my own action when it can be cought before?
* It mixes GET parameters into the nice-url parameters.
   Might seem convenient as well at first, still wrong.  GET parameters
   can be in any order, they should be treated like a hash, not a
   list of parameters which are shoved into a action.
   Accessing GET (and POST) parameters via `request.params[]` is fast
   and is less error prone.
* The arity of the method is used for determining the number of params.
   This is also wrong, since -1 and -2 are also possible as arity.
   Those two are linked to how Ruby works from the C side. (Copied from
   Ruby book.)
      argc   Function prototype
     0..17   VALUE func(VALUE self, VALUE arg...)
             The C function will be called with this many actual arguments.
     -1      VALUE func(int argc, VALUE *argv, VALUE self)
             The C function will be given a variable number of arguments  
             as a C array.
     -2      VALUE func(VALUE self, VALUE args)
             The C function will be given a variable number of arguments  
             as a Ruby array.
   Your function assumes that the arity will be greather or equal zero. It  
   work, when your function has more or the same amount of parameters than
   given on command-line.  This problem is even worse when combined with
   aforementioned GET param mixin.

All three points here create problems with certain kinds of usages, but  
with others and or workarounds.

       code << %{
         params = context.headers['ACTION_PARAMS'].to_s.split(';')
           action_return_value = #{action}(*params)
         rescue ArgumentError => e
           raise NoActionError, "No action for  
\#{@action_name}(\#{params.join(', ')})."

This is my code.  It assumes that the error handling is done one step  
when the code is actually eval'ed.  It doesn't care what parameters it  
to the actual function, as the ruby function itself knows what it wants  
It catches ArgumentErrors and returns to the same behaviour as it would  
the action didn't exist.

It might be that my theory is inherently flawed as you told me on IRC that
there are problems with it.  Please tell me what problems arose when you  
it, and if it's more the behaviour (not filling in missing params) or the
strategy (not failing at once, catching `ArgumentError` too late?).

If you test it, please tell me with what kinds of function it fails, read  
tc_controller_params.rb, make more tests to that testcase which prove my
method wrong etc.  As I wrote that testcase, the testcase might be flawed  
well, please look into it to check the behaviour and tell me if anything is
wrong with it.

Now to other small glitches in your repo:

* dispatcher.rb

   180:  key, ignore_query = path.split('?', 2)
   181:  key = key.split('/')

   In your dispatcher the first line is missing.  The query was first used
   for query munching (which is done in cgi.rb already).  Now the query  
   be thrown away like this:
   180:  key, * = path.split('?', 2)
   But you don't have this line anymore, is this now done somewhere before
   that?  When not doing this while having a URL like '/action/?asf=asd/sad'
   it would fail pretty hard, and even without the '/'.

* publishable.rb

   `def self.action_methods`, this method has to be revamped again.  Manveru
   had accidentially merged in a wrong version of this method. Patch  

* og.rb

   147: m.manage_classes
   This should read:
   147: m.manage_classes(options[:classes])

   And I propose to move the stuff in the .start function out of the
   begin/rescue block.  A database dependent application should fail if the
   Og store can't be set up instead of silently failing.
   This change was done in one of my `rescue Object` fixes which where never
   merged.  Read my post again back then for more info why `rescue Object`  
   a bad idea.

Sorry for the long rant and I hope things are more clear now than they were

So long,


Feel the love
-------------- next part --------------
A non-text attachment was scrubbed...
Name: publ.patch.tar.bz2
Type: application/bzip2
Size: 10047 bytes
Desc: not available
Url : http://rubyforge.org/pipermail/nitro-general/attachments/20061010/05a57e85/attachment.bin 

More information about the Nitro-general mailing list