Forum OpenACS Development: What do we do with db_multirow and template::multirow?

I've added a new ticket over at https://openacs.org/sdm/one-baf.tcl? baf_id=1368 about the way that db_multirow currently works, which is dangerous.

I know there's a beta going on, but let's talk a bit about what we want this API to look like going forward, anyway.

It's clear that we need to be able to set a multirow variable just as easily whether it's from a DB-query or data pulled from somewhere else. Currently, the difference between "db_multirow" and "template::multirow" is too big.

Also, right now I use this trick a lot:

db_multirow foo qry { select file_id, '' as file_url from files } {
    set file_url "...$file_id..."
}
The trick? I'll select an empty string and give it some name, in order to get db_multirow to create that column in the multirow. Then I can use the code block to assign it a value.

This works brilliantly, but there's really no need to include that in the query. We could have a flag to db_multirow that tells it what columns to add, in addition to those the query.

The difference between setting a multirow from a query and setting it in some other way, is that with the query, you can set it in one go, whereas when you set it some other way, you have to first declare (template::multirow create), then add each row one at a time (template::multirow append) with the values for the columns you declared at creation time.

It might also be useful to have a proc that loops over an already populated multirow, where you can add new columns, and/or alter the values of columns in the multirow? A "multirow_foreach"?

Coments on these thoughts?

Forgot to mention that I'll be happy to provide a patch, as soon as we (Don? :)) decide what we want db_multirow to do.
"We", not "me" :)

In the course of dotLRN work OF ran into the need to add rows to an existing multirow.  Rather than switch to the template multirow API Arjun added an "-append" flag which does the Obvious Thing.  It's pretty useful in some circumstances.

So I'm all for extending db_multirow and other db_* procs in useful ways.

Do you want to make a specific, concise proposal for extending the proc so others can chime in?  Do others have extensions to the db API they'd like to propose, while we're at it?

Something like a "multirow_foreach" (or perhaps template::multirow foreach?  Seems to fit well there) might be useful.  Stuff one does now in the .adp template using "if" tags while looping through a multirow might be easier to pull into the .tcl script if such a construct existed, and I'm all in favor of being able to do that as IMO templates should contain as little logic as possible.

We might as well do a little public planning here and look into extending the API for the next release (which means it can be in the tree shortly after beta if you or someone else has time to do it).

Since you mention the template db API, there's a deeper global issue which I label "improved integration of the template stuff with the OpenACS core".  For example, the template code, which is always installed, stomps over request processor extension handlers which I think makes those obsolete (? haven't looked that deeply into this).  In other words there are rough edges in the integration.

And I'd like to get rid of the template db API altogether, the toolkit doesn't really need two unrelated db APIs but that's another story ...

Ah, so -append is like a UNION? You call db_multirow twice, each time with a different query, the second time with -append?

Or can you also call db_mulirow -append with no query, but directly specifying the values?

The first case, in essence like doing a UNION (though in Arjun's case the queries were separated by logic that I don't think would've mapped to a single query-with-UNION).

To add items without doing a query you'd still need to use template::multirow append.

I don't mind that particularly, after all the db_ prefix indicates that db_multirow hits the database and we should preserve that.

What *does* bother me somewhat, again falling in the "rough edges around ATS integration with the ACS" bucket, is the fact that the parameter parsing and general namespace/pseudo-object dispatch style of coding in the template engine results in an API which is of a different style than everything else in the toolkit.

"better" or "worse" not being the issue so much as simply "different". I worship the gods "consistency" and "uniformity".

It is "worse" in one respect in that there's damned little error checking in the template parameter parsing code.  Mistyping a name in a dash-style parameter results in silent failure.

Don,

We're totally in line, then.

I like to have separate db_foo and foo to make it clear that one's hitting the DB, the other ain't.

And I'm annoyed that the templating system doesn't follow the conventions from the rest of the system (isn't it funny when geography gets written into code like this?).

So the roadmap should be to eventually get a templating API that follows the conventions of the rest of the API, and have db_multirow separate from a "manual" multirow proc.

You should see the templating system that Jon and I did. I wonder where that is now. :)

I just saw your ticket, and ugh ... what can I say?  That's a rather lame assumption made by the regsub (that no variable in the code block will contain the multirow identifier as its first N chars.  I don't like this one bit ...

Yes, separate db_multirow and multirow procs makes sense and yes, I think we're on the same wavelength because it's not the existence of template::multirow that's annoying, just its different API style.

Is this really "geography in the code" or just "Karl in the code", that's always been something I've wondered about. :)

I didn't know you and Jon wrote a templating system...

I also agree that I pretty much like the way the templating system works, just not the way it is implemented.

On another tangent since we're having a nice, public design discussion ... was there any thought at aD to having ad_page_contract get smart about building and verifying forms?  It seems that integration with the template formbuilder would be nice and while perhaps a bit tricky to define (ad_page_contract wants to be simple from the user point of view) not impossible.

Using the "self-submit" style which the form builder is meant to encourage involves a bunch of rote stuff: lots of "element" calls each time to build or validate the form as necessary, an optional validation block, a "request" block (which often does little or nothing but build context bars and the like) and a "valid submission" block.

In other words proper use of the form builder implies that you'll build pages with a stereotyped structure and that stereotyped structure could be partially supported by ad_page_contract.  In particular taking form element name, type and default argument args and issuing template "form" commands automatically (or otherwise duplicating the behavior).

I haven't thought a lot about this but it seems that building forms could be made a *lot* easier.  Right now you have to drop the convenient and extremely readable ad_page_contract-style of page param declarations if you want the nice, in-form error messages built by the form builder (rather than ad_page_contract's separate "you screwed up, hit 'back' and try not to screw up again" error page).

Wow this is great! I was talking to Carl last week about using db_multirow twice with the same array name. Do you need the append flag, or can it just do the right thing? What is the current effect of calling db_multirow twice, maybe with the second call producing less rows than the first? The idea of a UNION type relationship is great, unfortunately in my case last week this would have been impractical because I needed sorted rows. Maybe an option to sort...okay I'll stop now.

You do need the "-append" flag currently and I think that's probably best because it makes it clear that you're doing it intentionally and not simply screwing up.  Currently if you do two in a row without the "-append" you lose the contents of the first, silently.  Is that good or bad?  Well ... it's consistent with the other db_* API routines, at least.

Another spot that needs better integration between the templating system and ad_page_contract is variable definition (query block of ad_page_contract) and validation.

I have found it extremely annoying that I can't use ad_page_contract to receive and validate variables that are passed from either template include calls or master property calls. In either of these cases I have to do the validation "old-style," that is:

if {![exists_and_not_null foo]} { set foo bar }

Instead of being able to say in the query block of ad_page_contract:

{foo "bar"}

Can we come up with a plan to fix this as well?

You're right, that, too is annoying.

It's also annoying that the "properties" section of ad_page_contract -- the one where you "tell the template" what data sources are available -- has no consequence whatsoever. It's not enforced, it has no meaning, hence nobody bothers with keeping it alive.

It should either have some meaning, or it should go.

I asked basically that same question in this thread (https://openacs.org/bboard/q-and-a-fetch-msg.tcl?msg_id=00041U&topic_id=OpenACS%204%2e0%20Testing&topic=14) Shouldn't the -properties flag create the documentation instead of using that funky commenting format?

It might be nice if the -properties section could be used in some type of validation mode. Once things are set, it seems relatively useless. Hmm, maybe what would be useful is if it would cause the display of something more helpful than the useless stack trace you get on an error. The stack trace error is usually the second error that comes in the log file, and I usually have to look at the log file instead of the goop in the browser. I'm not too sure this is what the properties section could help out on.