Forum OpenACS Development: db_multirow and db_foreach -unclobber

If you are going to take the switch approach, I think the new functionality should be optional rather than break backwards compatibility needlessly.  As for the code block, I'd rather not overcomplicate the API when it's currently able to get the job done quite nicely.

The old ACS used to have a function that processed a select, and prepended a string to the column names so as to avoid clobbering local vars. One variant allowed you to set the value of the prepended string.

Although we can speculate as to what the correct behavior should have been for db_multirow, it seems to be a few years too late to change the default.

If instead of -unclobber, you used something like -prepend, you would never have to unclobber the vars, also allowing local vars to be available to the tcl code block for the db_multirow. The default to -prepend would be the empty string, allowing the current behavior. db_1row would also benefit from a -prepend switch.

db_multirow is also useful when you know only one row will be returned, and you want to use array/multiple syntax for handling the result.

In fact it should be clear to anyone who has been working with AOLserver's ns_db API: when you are processing rows, the caller's environment is visable. The new db_* API is just a nice wrapper around that API. The proc is just there to allow re-use of code, not to protect the tcl page from unsound programming practices. This is what uplevel is supposed to be used for. If you don't want to clobber the vars, they really should not have the same name on your tcl page in the first place. Using one var name for two distinct purposes is bad programming practice.

I haven't looked at the code for db_multirow, but if it clobbers vars you would not expect it to clobber, like a loop var, or something, that should be documented, and possibly changed, as this would be a bug waiting to surface.

1) db_multirow doesn't clobber variables other than the ones that are named the same as your column.

2) I agree that using the same local variable for multiple purposes is bad practice, but the problem is not with the page design but with the system design.

I think that we've overloaded the local variables namespace. We stick the query variables from ad_page_contract there, we put the columns from our database procs there, and finally, we use the local variables to pass properties to the template.

I actually feel like all 3 of these need to be broken away from the local variables namespace somehow, since they're all part of a 'public API' of sorts, and thus you should have freedom in your choice of naming.

3) I also agree that db_foreach and db_multirow are modeled after foreach, and thus I can see the value of keeping the default behavior what it is now.

So if we could find a way to store query variables in a 'query::' namespace and properties in a 'property::' namespace, then maybe that would solve the problem better.

/Lars

Lars, Andrew:

I was thinking the namespace idea was something helpful as well, but when trying to write a reply that indicated so, I seemed to stall out as to why it would be a good idea.

The truth is, namespaces will not solve the problem, which is one that must be solved on a per page basis, taking into consideration the different variables being used.

Example: you write a query that has several column names that are the same. Obviously they have distinct meanings in the context of the query, so you use 'as' to rename the column. This is the exact problem we have, fortunately it is solved at the sql level in a self documenting format. The fact that higher up on the page, you had a similarly named variable doesn't remove the fact that the variables have distinct meanings, and should have distinct names. Putting the variable into a fixed namespace doesn't shed much light on the meaning of a variable, but renaming it would.

In ad_page_contract, it would be great to have an aliasing mechanism similar to those used in (pl/)sql, but the whole point of db_* procs is to operate in the same namespace, and hopefully the same level. Creating separate namespaces would only foster confusion. Why should variables be named based upon where they came from (query::, property::)? They should be named based on their meaning and use on that page.

ad_page_contract turns vars with a dot in the name into arrays, so you can't put them into another array, I don't think.

I have written perhaps hundreds of complex pages with multiple db_multirow, db_1row, etc. I have yet to find a problem. What is the problem? Can we get one actual example of a problem first. We currently have an entire toolkit that somehow avoids the problem. When you rewrite the toolkit to use property:: and query::, what is a tcl page going to look like once converted?

I'm just guessing that you might end up with something like property::object_id, query::object_id and object_id on the same page. Is this correct? Of course the whole thing falls apart as soon as you have two queries with object_id in them. Please tell me how I am wrong, because I usually misunderstand the most basic concepts.

Then I want that page to be able to show me only rows with a certain user_id, project_id, and/or variable_id.

I also need to list the available project_id's, user_id's, and variable_id's, to let the user choose which ones to show.

Now, of course, when I iterate over the possible user_id's, unless I rename either the public page variable in ad_page_contract to something like 'selected_user_id', or rename the column on the fly using 'as row_user_id', which means that'll also be the name of that column in the multirow which we create, which I also find bad.

Bottom line: As things stand today, I have to find another name for my variable in one of two places (the page query or the template property), both of which are part of the page's "public API".

Hope that clarified things a bit.

/Lars

After all these years the fact that "user_id" is used confusingly just now has become a problem that needs to be resolved by adding namespaces requiring us to touch every page in the toolkit?

At the most extreme that's what's being talked about.

A couple of quick observations ...

1. Tcl is not a block-scoped language, talking about block-local vars ala Java doesn't really make sense.  It's proc-scoped.  Uplevel's not particularly cool but the way it is used in the db_* procs is consistent with the db API and easy to understand.  Given that Tcl doesn't support reference parameters I think the design's a reasonable compromise.

2. I like consistency.  Making some db_* procs work one way, others work another way is just confusing for newcomers and error-prone for everyone else.

3. "user_id", "package_id" and a few other ids are overloaded in the datamodel and ad_conn structure.  The examples mentioned above pinpoint *this* as being the real problem.  If people got in the habit of saying

set current_user_id [ad_conn user_id]

instread of

set user_id [ad_conn user_id]

then from reading this thread, at least, I get the impression that 90% of the supposed problem would fade away.

When I look at all the *important* problems we still need to solve in the toolkit I have to wonder why we would consider this particular minor issue even worth the time spent discussing it???

1. I already have the code sitting in my local checkout. It took about 20 minutes to write. I just posted to ask if it would be useful to other people.

2. The problem can be solved by renaming variables, yes. But it's not quite as simple as you make it. Or, rather, the thing that annoys me is that you have to rename *either* your page variables (which are part of your page's public API), your table columns (which are part of your application's API), or the column names in the multirow (also part of your page's API).

I admit that it's not a terribly big problem, it was just something that bit me and annoyed me a couple too many times, so I figured I'd take 20 minutes to implement a simple solution.

/Lars

Index: 00-database-procs.tcl
===================================================================
RCS file: /cvsroot/openacs-4/packages/acs-tcl/tcl/00-database-procs.tcl,v
retrieving revision 1.19.2.5
diff -u -r1.19.2.5 00-database-procs.tcl
--- 00-database-procs.tcl	23 May 2003 13:11:29 -0000	1.19.2.5
+++ 00-database-procs.tcl	3 Jun 2003 08:05:35 -0000
@@ -442,6 +442,7 @@
 ad_proc -public db_multirow {
     -local:boolean
     -append:boolean
+    -unclobber:boolean
     {-extend {}}
     var_name
     statement_name
@@ -584,6 +585,24 @@
 Columns in this query: [join [lsort -ascii $local_columns] ", "]" "" "ACS_MULTIROW_APPEND_COLUMNS_MISMATCH"
                     }
                 }
+
+                # Save values of columns which we might clobber
+                if { $unclobber_p && ![empty_string_p $code_block] } {
+                    foreach col $columns {
+                        upvar 1 $col column_value __saved_$col column_save
+
+                        if { [info exists column_value] } {
+                            if { [array exists column_value] } {
+                                array set column_save [array get column_value]
+                            } else {
+                                set column_save $column_value
+                            }
+
+                            # Clear the variable
+                            unset column_value
+                        }
+                    }
+                }
             }
 
 	    if { [empty_string_p $code_block] } {
@@ -650,6 +669,30 @@
             incr local_counter
 	    set array_val(rownum) $counter
 	}
+    }
+
+    # Restore values of columns which we've saved
+    if { $unclobber_p && ![empty_string_p $code_block] && $local_counter > 0 } {
+        foreach col $columns {
+            upvar 1 $col column_value __saved_$col column_save
+
+            # Unset it first, so the road's paved to restoring
+            if { [info exists column_value] } {
+                unset column_value
+            }
+
+            # Restore it
+            if { [info exists column_save] } {
+                if { [array exists column_save] } {
+                    array set column_value [array get column_save]
+                } else {
+                    set column_value $column_save
+                }
+                
+                # And then remove the saved col
+                unset column_save
+            }
+        }
     }
 
     # If the if_no_rows_code is defined, go ahead and run it.

An interesting point is that db_multirow, without a foreach block, doesn't clobber the local vars. This means the original intent of db_multirow was to not create these vars in the local context. I believe that the foreach code block was added recently to make it possible to do per row processing. Given that the new db_multirow combines features of the old nonclobbering db_multirow and the clobbering db_foreach, I can see the reason behind your original proposal as it applies to db_multirow.

For db_multirow, the question of what should be the default (-clobber/--unclobber) is still important. If you look at ad_page_contract properties, the multirow is usually only specified as the array name, not the individual column names. If you add a foreach block to the db_multirow for the purpose of, for example, adding a calculated column to each row, your intent is not to have to deal with all these variables that now exist, and possibly have overwritten, your local variables. I'm almost to the point of thinking that -unclobber should be the default, so that things match up with the original db_multirow behavior.

A similar intent could be applied to db_foreach. Many times I have used db_foreach for a purpose not related to setting local variables, and never for setting the variables that are reset for each iteration of the loop.

Lars, I'm sorry that I misintrepreted your reasons for the changes.