bug in load_response.tcl?

Cristian wrote:
> Hi,
> I see that the actual implementation of
load_response.tcl produces a 
> increasingly nested list when parsing multiple values
for a given key. 
> I've corrected this by changing it to use concat
instead of list (diff 
> attached to be run in rivet/rivet-tcl/). I also saw
that no test files 
> were present for this command, so I wrote them too
(attached 
> load_response.rvt and load_response.test to be copied
in tests/).

the load_response problem was discussed last February.
IIRC David explained that changing this function in
that way would have some side effects in case
list valued parameters are passed to the server (see the
list archives).

Still I need something different and
I wrote a 'load_response' method in the root class
of my pages that overrides in the rivet standard proc. This
method is the library function modified in the very same
way you did.

For tcltest I can't help: my ignorance about this package is
vast.

 -- Massimo

> When running the tests, it was quite annoying that the
results from 
> previous tests was shown when a regexp didn't match. So
I included set 
> match "" all over the place, so now it is
more clear that the regexp 
> didn't match. I attached the diff also ( tests.diff to
be applied in 
> tests/).
> When time allows, I will be checking the other commands
and their 
> tests. Till now all the I18n tests fail on my utf-8
encoding system.
>
> Could someone check this and commit if it applies?
>
> Thanks.
>
> Cristian.
>


------------------------------------------------------------
---------
To unsubscribe, e-mail: rivet-dev-unsubscribetcl.apache.org
For additional commands, e-mail: rivet-dev-helptcl.apache.org

Cristian wrote:
>
> Hi massimo,
> I finaly found the archives by googling for rivet-dev
and archive. It 
> is even searchable! Will check there in the future
before posting...
> So if I understand it correctly, wouldn't the following
solve the problem?
>
i suspect the answer depends on what you expect from
load_response.
I can understand from the archive is that the debate was not
only about
load_response "per se", but how everyone expected
to use it to suit their
approach to the problem.

A strong point was that load_response is taken from
NeoWebScript,
a Tcl based web development tool which could be already
familiar to
the a new Rivet user and it wouldn't make sense to confuse
him/her.

Having many procs that make the same job in a different way
would
be a possible solution (indroducing switches like
"-opt1 -opt2" received
little appreciation)

> proc load_response {{arrayName response}} {
>     upvar 1 $arrayName response
>
>     array set response {}
>
>     foreach {var elem} [var all] {
>         if {[info exists response($var)]} {
>             set response($var) [lappend response($var)
[list $elem]]
>         } else {
>             set response($var) [list $elem]
>         }
>     }
> }
>

In my personal case your code would work, but as I said,
this is
only the way in which I used load_response.

I made a test feeding your code and the code in Rivet with a
series
of list valued elements and the output is radically
different.

Cristian's:

for {set i 0} {$i<10} {incr i} { if {[info exists lista]}
{ set lista 
[lappend lista [list elemento $i]] } else { set lista [list
elemento $i] }}

the first element is treated as a 2 elements list whereas
the others are 
nested lists appended as sibling elements

Rivet's:

for {set i 0} {$i<10} {incr i} { if {[info exists lista]}
{ set lista 
[list $lista [list elemento $i]] } else { set lista [list
elemento $i] }}

the output in 'lista' is a series of deeply nested
elements...

> file load_response_test.rvt:
> ---
> load_response
> if { [ array exists response ] } {
>     foreach  [ array names response ]  {
>         puts "response($ck) =
$response($ck)n"
>     }
> }
> ---
>
> which for a 
> http://localhost:8081/load_respo
nse_test.rvt?foo=bar+baz&fee=bat&foo=bak 
> <http://localhost:8081/load_r
esponse_test.rvt?foo=bar+baz&fee=bat&foo=bak>

>
> produces:
> response(fee) = bat
> response(foo) = {bar baz} bak
>
> Or will the load_response proc be deleted from the
sources?
>
> Cristian
I think the answer to the last question is negative, this
would break 
the code and
it wouldn't be a sensible approach to the problem.


 -- Massimo


------------------------------------------------------------
---------
To unsubscribe, e-mail: rivet-dev-unsubscribetcl.apache.org
For additional commands, e-mail: rivet-dev-helptcl.apache.org

On Thu, 04 Oct 2007 13:02:45 +0200
Massimo Manghi <massimo.manghiunipr.it> wrote:

> Cristian wrote:
> >
> > So if I understand it correctly,
> > wouldn't the following solve the problem?
> >
> i suspect the answer depends on what you expect from
> load_response. 
> 
> A strong point was that load_response is taken from
> NeoWebScript, a Tcl based web development tool which
could be
> already familiar to the a new Rivet user and it
wouldn't make
> sense to confuse him/her.

If the point of having load_response is to do the same job
that
load_response in nws did, then it should behave as
load_response
did.

The questions then become: 
a) how did load_response deal with multiple submissions of
the
same cgi var, and

b) how, when examing the response array, can you identify
between response(var) representing a verbatim value, or if
it
represents a var which was submitted multiple times?

The answers are that in nws, what load_response did was
create
response(__var) when it encountered more than one value for
var,
and ensured sure that resposne(var) was a list, with each
element represented one of the submitted cgi values.

Also to note is that when a var had no value, response(var)
was
an empty string, not an empty list.

The following code within load_response would achieve this:

foreach {var elem} [var all] {
	if {[info exists response(__$var)]} {
		# we have seen var multiple times already, add
to the list lappend response($var) $elem
	} elseif {[info exists response($var)]} {
		# second occurence of var,  convert
response(var) list: set response($var) [list
$response($var)
$elem] set response(__$var) ""
	} else {
		# first time seeing this var
		set response($var) $elem
	}
}

So for:
> > http://localhost:8081/load_respo
nse_test.rvt?foo=bar+baz&fee=bat&foo=bak 

We get:

response(fee) = bat
response(foo) = {bar baz} bak
response(__foo) = 

And for
load_response_test.rvt?foo=bar+baz&fee=bat&foo=bak&a
mp;faa=&fii=bar+baz&foo=bee+bii

response(fii) = bar baz
response(faa) = 
response(fee) = bat
response(foo) = {bar baz} bak {bee bii}
response(__foo) = 

Thoughts?

-fr.

-- 
Feargal Reilly, Chief Techie, FBI.
PGP Key: 0xBD252C01 (expires: 2006-11-30)
Web: http://www.fbi.ie/ |
Tel: +353.14988588 | Fax: +353.14988489
Communications House, 11 Sallymount Avenue, Ranelagh, Dublin
6.


-- 
Feargal Reilly.
PGP Key: 0x679DB84D (expires: 2007-11-30)
Web: http://www.helgrim.com/ |
ICQ: 109837009 | YIM: ectoraige
Visit http://ie.bsd.net/ -
BSDs presence in Ireland

------------------------------------------------------------
---------
To unsubscribe, e-mail: rivet-dev-unsubscribetcl.apache.org
For additional commands, e-mail: rivet-dev-helptcl.apache.org

Apologies, my mail client mangled the code, it should read:

foreach {var elem} [var all] {
  if {[info exists response(__$var)]} {
    # we have seen var multiple times already, add to the
list:
    lappend response($var) $elem
  } elseif {[info exists response($var)]} {
    # second occurence of var, convert response(var) to a
list:
    set response($var) [list $response($var) $elem]
    set response(__$var) ""
  } else {
    # first time seeing this var
    set response($var) $elem
  }
}

-fr.

-- 
Feargal Reilly, Chief Techie, FBI.
PGP Key: 0xBD252C01 (expires: 2006-11-30)
Web: http://www.fbi.ie/ |
Tel: +353.14988588 | Fax: +353.14988489
Communications House, 11 Sallymount Avenue, Ranelagh, Dublin
6.


-- 
Feargal Reilly.
PGP Key: 0x679DB84D (expires: 2007-11-30)
Web: http://www.helgrim.com/ |
ICQ: 109837009 | YIM: ectoraige
Visit http://ie.bsd.net/ -
BSDs presence in Ireland

------------------------------------------------------------
---------
To unsubscribe, e-mail: rivet-dev-unsubscribetcl.apache.org
For additional commands, e-mail: rivet-dev-helptcl.apache.org

I quoted some of the arguments brought up on the list almost
4 years ago
because I just wanted to recall the history of the problem.
I personally understand many of your arguments (switches
and
nested lists included), therefore I won't answer every
single point
of your message.

Let me recall that I have a private copy of load_response in
my classes
and it works the way i thought rivet's load_response had to
work.
I didn't change the name because I also wanted it to
override rivet's proc.
As I said, I had brought up the problem on the list when I
was just a user,
only David answered.

I endorse your proposal of having a new procedure with a
cleaner
and more rigorous behavior.  In principle I also like
Feargal's proposal
of  having load_response emulate more precisely NWS'
load_response
but I don't know what impact would have on existing code.

  -- Massimo
 
P.S. For this list you don't need to resort to nable: the
apache
project keeps on the web rivet-dev's archives starting from
2001.
P.P.S I will be unable to answer any message for the next 2
days

Cristian wrote:
>
> On 10/4/07, *Massimo Manghi* < massimo.manghiunipr.it

> <mailto:massimo.manghiunipr.it>> wrote:
>
>
>     i suspect the answer depends on what you expect
from load_response.
>
>
> Reading the docs, I should expect the form values to be
available in 
> an array, but see below...
>
>     I can understand from the archive is that the
debate was not only
>     about
>     load_response "per se", but how everyone
expected to use it to
>     suit their
>     approach to the problem.
>
>
> Archives in nable didn't go back to 2004... now I found
the old 
> discussions and apparently the main issue in discussion
was if 
> load_response had to reset the response array each time
it was called. 
> This is not the issue we are discussing today.
> Citing David from a mail dating from 15 jan 2004,
>
> In addition, I also fixed things so that the element
looks like this:
> response(x) = {this is comment 1} {this is comment 3}
>
> with two values, whereas previously it was:
>
>
> response(x) = this is comment 1 {this is comment 3}
>   
> we can see that the intention is to produce a proper
multi valued var 
> except that the script was tested with only 2 values.
If one more 
> value of x was added to this example I guess they would
have fixed 
> this a long time ago. Nobody said anything because of
this change in 
> the code. What I am proposing is to extend the sought
functionality to 
> "more than 2" valued vars.
>
>     A strong point was that load_response is taken from
NeoWebScript,
>     a Tcl based web development tool which could be
already familiar to
>     the a new Rivet user and it wouldn't make sense to
confuse him/her.
>
>
> I can understand this situation, but if they
"abandoned" NWS for rivet 
> they should be prepared to adapt them selfs to
something new... I 
> guess you can tell that I never used NWS and am new to
rivet as well 
> Seriously though, I think rivet should propose good
code even if it 
> involves deviating from how things were implemented in
another 
> software (where some ideas were taken from). The
crucial thing is to 
> document it well!
>
>     Having many procs that make the same job in a
different way would
>     be a possible solution (indroducing switches like
"-opt1 -opt2"
>     received
>     little appreciation)
>
>
> Ughh! No please. I'd rather prefer one proc that does
what it is 
> supposed to do correctly.
>
>     Cristian's:
>
>     for {set i 0} {$i<10} {incr i} { if {[info
exists lista]} { set lista
>     [lappend lista [list elemento $i]] } else { set
lista [list
>     elemento $i] }}
>
>     the first element is treated as a 2 elements list
whereas the
>     others are
>     nested lists appended as sibling elements
>
>
> The difference between this implementation and the one
on rivet is 
> that [var all] returns already a list , so to compare
outputs with 
> these versions, you should do a foreach {k v}
{{elemento 0} {elemento 
> 1} ...} <- the result of [var all], which would
produce
> {elemento 0} {elemento 1} {elemento 2} ...
>
>     Rivet's:
>
>     for {set i 0} {$i<10} {incr i} { if {[info
exists lista]} { set lista
>     [list $lista [list elemento $i]] } else { set lista
[list elemento
>     $i] }}
>
>     the output in 'lista' is a series of deeply nested
elements...
>
>
> this outputs:
> {{{{{{{{{elemento 0} {elemento 1}} {elemento 2}}
{elemento 3}} 
> {elemento 4}} {elemento 5}} {elemento 6}} {elemento 7}}
{elemento 8}} 
> {elemento 9}
>
> I would like to see the code to parse this... I mean,
who would want 
> something like this to be returned!! (I bet someone
will speak up now 
> and say how fun it was to write the function that
returned the first 
> element  )
> When reading the docs it states that an array is set
with the form 
> variables passed to the page. It is sufficiently
criptic to allow for 
> wild interpretations as how the multivalues would be
returned. But as 
> wild as my imagination would go I couldn't imagine that
this kind of 
> nested list would be returned. that is why the subject
of this mail 
> mentions a possible bug, it was the first thing that
came to my mind 
> when I looked at the code.
> You might say that that is my (and possibly your)
opinion... but it is 
> simply impractical to do anything with a result like
that. So why have 
> this kind of code in rivet?
>
>
>     > Or will the load_response proc be deleted from
the sources?
>     >
>     > Cristian
>     I think the answer to the last question is
negative, this would break
>     the code and
>     it wouldn't be a sensible approach to the problem.

>
>
> Ooops! That last sentence was part of a rant I thought
I had 
> completely deleted and is completely out of context.
The rant went on 
> the fact that the only place where load_response is
used in the rivet 
> sources is in the Diodisplay package and that nothing
would be broken 
> by the change as the code never contemplates the
possibility of multi 
> valued variables. In fact it assumes single values are
passed in 
> several places. Furthermore the fact that nobody except
you mentioned 
> the necessity for multivalued variables (since 2004?)
would indicate 
> that not many people use this proc with multivalued
variables so the 
> risk of breaking things would be small.... etc. Of
course all these 
> inferences are done by reading the code, no real test
done yet.
>
> But then again, we could just write a new proc
load_form_vars or 
> something like that and document in a clear manner the
output of 
> load_response if used with multiple values per
variable.
>
> So what do you, or the others, think?
>
> Cheers,
> Cristian


------------------------------------------------------------
---------
To unsubscribe, e-mail: rivet-dev-unsubscribetcl.apache.org
For additional commands, e-mail: rivet-dev-helptcl.apache.org

On Thu, 04 Oct 2007 13:02:45 +0200
Massimo Manghi <massimo.manghiunipr.it> wrote:

> Cristian wrote:
> >
> > So if I understand it correctly,
> > wouldn't the following solve the problem?
> >
> i suspect the answer depends on what you expect from
> load_response. 
> 
> A strong point was that load_response is taken from
> NeoWebScript, a Tcl based web development tool which
could be
> already familiar to the a new Rivet user and it
wouldn't make
> sense to confuse him/her.

If the point of having load_response is to do the same job
that load_response in nws did, then it should behave as
load_response did.

The questions then become: 
a) how did load_response deal with multiple submissions of
the same cgi var, and

b) how, when examing the response array, can you identify
between response(var) representing a verbatim value, or if
it represents a var which was submitted multiple times?

The answers are that in nws, what load_response did was
create response(__var)
when it encountered more than one value for var, and ensured
sure that
resposne(var) was a list, with each element represented one
of the submitted cgi
values.

Also to note is that when a var had no value, response(var)
was an empty string, not an empty list.

The following code within load_response would achieve this:

foreach {var elem} [var all] {
	if {[info exists response(__$var)]} {
		# we have seen var multiple times already, add to the
list
		lappend response($var) $elem
	} elseif {[info exists response($var)]} {
		# second occurence of var,  convert response(var) list:
		set response($var) [list $response($var) $elem]
		set response(__$var) ""
	} else {
		# first time seeing this var
		set response($var) $elem
	}
}

So for:
> > http://localhost:8081/load_respo
nse_test.rvt?foo=bar+baz&fee=bat&foo=bak 

We get:

response(fee) = bat
response(foo) = {bar baz} bak
response(__foo) = 

And for
load_response_test.rvt?foo=bar+baz&fee=bat&foo=bak&a
mp;faa=&fii=bar+baz&foo=bee+bii

response(fii) = bar baz
response(faa) = 
response(fee) = bat
response(foo) = {bar baz} bak {bee bii}
response(__foo) = 

Thoughts?

-fr.

-- 
Feargal Reilly, Chief Techie, FBI.
PGP Key: 0xBD252C01 (expires: 2006-11-30)
Web: http://www.fbi.ie/ |
Tel: +353.14988588 | Fax: +353.14988489
Communications House, 11 Sallymount Avenue, Ranelagh, Dublin
6.

------------------------------------------------------------
---------
To unsubscribe, e-mail: rivet-dev-unsubscribetcl.apache.org
For additional commands, e-mail: rivet-dev-helptcl.apache.org

I personally use the [var] command, as that guarantees that
you get
things in the format that you think they're in, so I'm not
particularly concerned about load_response myself...

-- 
David N. Welton
http://www.welton.it/dav
idw/

------------------------------------------------------------
---------
To unsubscribe, e-mail: rivet-dev-unsubscribetcl.apache.org
For additional commands, e-mail: rivet-dev-helptcl.apache.org

David Welton wrote:
> I personally use the [var] command, as that guarantees
that you get
> things in the format that you think they're in, so I'm
not
> particularly concerned about load_response myself...
>
>   
ok, since nobody came up opposing or arguing Feargal's
proposal
for load_response I think we should consider to load it
into
rivet. For what I can see the procedure could patch the
current
'load_response' without relevant side effects (I've been
testing it
as a replacement for my private copy, but my load_response
was
already different).

 -- Massimo

------------------------------------------------------------
---------
To unsubscribe, e-mail: rivet-dev-unsubscribetcl.apache.org
For additional commands, e-mail: rivet-dev-helptcl.apache.org

> ok, since nobody came up opposing or arguing Feargal's
proposal
> for load_response I think we should consider to load it
into
> rivet. For what I can see the procedure could patch the
current
> 'load_response' without relevant side effects (I've
been testing it
> as a replacement for my private copy, but my
load_response was
> already different).

Someone did mention some incompatibilities in 'DIO'
(Richard
Dawkins?! but
other than that, since I don't use it, I'm pretty
happy to follow the list consensus.  I think the [var]
commands
provide the best interface, since you can specify exactly
what you
want to receive.  Speaking of which, you guys should also
test the
code in question from the point of view of someone being
malicious -
sending multiple values where one is expected.

-- 
David N. Welton
http://www.welton.it/dav
idw/

------------------------------------------------------------
---------
To unsubscribe, e-mail: rivet-dev-unsubscribetcl.apache.org
For additional commands, e-mail: rivet-dev-helptcl.apache.org