Forum OpenACS Development: title attribute in both acs_objects and cr_revisions

In the CMS package, adding a new item fails (on HEAD, maybe oacs-5-2 but I'm only looking at HEAD) because the code that dynamically generates the insert query yields two "title"s -- one from acs_objects and one from cr_revisions:
ERROR:  Attribute 'title' specified more than once

SQL: insert into cr_revisionsi ( item_id, revision_id, creation_user, creation_ip, title, description, mime_type, title )
values ( '699', '722', null, null, 'Test', 'Test', 'text/plain', 'Test' )
    while executing
"ns_pg_bind dml nsdb0 -bind d13 {insert into cr_revisionsi ( item_id, revision_id, creation_user, creation_ip, title, description, mime_type, title )
v..."
    ("eval" body line 1)
    invoked from within
"eval [list ns_pg_bind $type $db -bind $bind $sql]"
    ("postgresql" arm line 2)
    invoked from within
"switch $driverkey {
                    oracle {
                        return [eval [list ns_ora $type $db -bind $bind $sql] $args]
                ..."

The list of columns to insert into cr_revisionsi comes from this query (in content::get_attributes called by content::attribute_insert_statement called by content::add_revision):

    select
      attribute_name,datatype,default_value,ancestor
    from
      acs_attributes,
      (
        select 
          o2.object_type as ancestor, tree_level(o2.tree_sortkey) as type_order
        from
          (
            SELECT *
            FROM acs_object_types
            WHERE object_type = 'content_revision'
          ) o1, acs_object_types o2
        where
          o2.tree_sortkey <= o1.tree_sortkey
        AND
          o1.tree_sortkey between o2.tree_sortkey and tree_right(o2.tree_sortkey)

      ) types
    where
      object_type = ancestor
    order by type_order desc, sort_order

Here's what the query produces when tested in psql:

 attribute_name | datatype | default_value |     ancestor     | type_order | sort_order 
----------------+----------+---------------+------------------+------------+------------
 nls_language   | text     |               | content_revision |          2 |          1
 title          | text     |               | content_revision |          2 |          1
 description    | text     |               | content_revision |          2 |          2
 publish_date   | date     |               | content_revision |          2 |          3
 mime_type      | text     |               | content_revision |          2 |          4
 context_id     | integer  |               | acs_object       |          1 |          1
 creation_date  | date     |               | acs_object       |          1 |          1
 creation_ip    | string   |               | acs_object       |          1 |          1
 creation_user  | integer  |               | acs_object       |          1 |          1
 last_modified  | date     |               | acs_object       |          1 |          1
 modifying_ip   | string   |               | acs_object       |          1 |          1
 object_type    | string   |               | acs_object       |          1 |          1
 package_id     | integer  |               | acs_object       |          1 |          1
 title          | string   |               | acs_object       |          1 |          1
(14 rows)

Yup, there is "title" -- twice.

So is this a bug in the CR? In the basic object model? Or is it in CMS? The CMS code attempts to remain ignorant of the columns it is managing, and it appears to assume that there will be no conflicts in the column names -- obviously not a safe assumption. Writing something to toss one of the "title" columns from this query seems like a massive kludge that doesn't deal with the real issue.

Is anyone using the CMS? Is this worth fixing in the CMS? Is this a larger issue that needs to be fixed?

(FWIW, I was poking at the CMS simply to see how it might -- or might not -- deal with symlinks -- see this thread. Until I can actually add some stuff to CMS, I can't really explore this.)

Collapse
Posted by Dave Bauer on
Stan,

It needs to be smart about the attributes of acs_objects now. The column for acs_objects is called "object_title" there is also "object_package_id"

It should not be a bid deal making to "know" about the attributes of just the acs_objects table. Ideally title would be removed from cr_revisions, but that is not done yet. Then the view would "just work" transparently.

Collapse
Posted by Stan Kaufman on
Dave, are you referring to acs_attributes? In HEAD, the title column of acs_objects is still named "title", not "object_title". So I'm guessing you do. However, HEAD doesn't install those attributes that way now:

oacsh=# select object_type, attribute_name from acs_attributes ;
     object_type     |   attribute_name   
---------------------+--------------------
 acs_object          | object_type
 acs_object          | creation_date
 acs_object          | creation_ip
 acs_object          | last_modified
 acs_object          | modifying_ip
 acs_object          | creation_user
 acs_object          | context_id
 acs_object          | package_id
 acs_object          | title
...snip...
 content_revision    | title
 content_revision    | description
 content_revision    | publish_date
 content_revision    | mime_type
 content_revision    | nls_language

If the attributes for acs_objects were correct in acs_attributes, all would be happy, no? Is this something to fix in kernel installation (not sure where this would be)?

Collapse
Posted by Stan Kaufman on
Altering content::attribute_insert_statement thus:

Index: cms/tcl/form-procs.tcl
===================================================================
RCS file: /cvsroot/openacs-4/packages/cms/tcl/form-procs.tcl,v
retrieving revision 1.27
diff -u -r1.27 form-procs.tcl
--- cms/tcl/form-procs.tcl      17 May 2005 21:25:08 -0000      1.27
+++ cms/tcl/form-procs.tcl      6 Oct 2005 16:30:00 -0000
@@ -807,7 +807,7 @@
                 }
             }
             
-            if { ! [string equal $value {} ] } {
+            if { ! [string equal $value {} ] && [lsearch -exact $columns $attribute_name] == -1 } {
                 ns_set put $bind_vars $attribute_name $value
 
                 lappend columns $attribute_name

eliminates the error. This is quite a kludge, though, as it simply takes the first attribute it gets -- and in this case uses the version of title from cr_revisions instead of acs_objects. Since they're the same, this doesn't matter. But if they did differ (would they ever?) then this would be wrong.

Is this an acceptable fix? I'll commit it if so.

Collapse
Posted by Dave Bauer on
Stan,

The cr_revisioni view is where the acs_objects.title column i s aliased to object_title.

You need to special case acs_objects.title when it builds the list or columns to insert.

Here is how its done in the CR. We just skip all the attributes of content_revision (and by inheritance acs_object) since we _know_ what they are:

if { [exists_and_not_null attributes] } {
	set type_attributes [package_object_attribute_list $content_type]
	set valid_attributes [list]
	# add in extended attributes for this type, ingore
	# content_revision as those are already captured as named
	# parameters to this procedure

	foreach type_attribute $type_attributes {
	    if {![string equal "cr_revisions" [lindex $type_attribute 1]]} {
		lappend valid_attributes [lindex $type_attribute 2]
	    }
	}
	foreach attribute_pair $attributes {
            foreach {attribute_name attribute_value} $attribute_pair {break}
	    if {[lsearch $valid_attributes $attribute_name] > -1}  {

                # first add the column name to the list
		append attribute_names  ", ${attribute_name}"
		# create local variable to use for binding
		set $attribute_name $attribute_value
		append attribute_values ", :${attribute_name}"
	    }
	}
    }

So you could so the same thing but just skip acs_objects (and add those in manually).

Collapse
Posted by Stan Kaufman on
Dave, since it's acs_objects.title we want and cr_revisions.title that we don't, why not simply special case *skip* the latter:

Index: cms/tcl/form-procs.tcl
===================================================================
RCS file: /cvsroot/openacs-4/packages/cms/tcl/form-procs.tcl,v
retrieving revision 1.27
diff -u -r1.27 form-procs.tcl
--- cms/tcl/form-procs.tcl      17 May 2005 21:25:08 -0000      1.27
+++ cms/tcl/form-procs.tcl      6 Oct 2005 17:41:23 -0000
@@ -807,7 +807,7 @@
                 }
             }
             
-            if { ! [string equal $value {} ] } {
+            if { ! [string equal $value {} ] && ![expr { [string equal $ancestor "content_revision"] && [string equal $attribute_name "title"] }] } {
                 ns_set put $bind_vars $attribute_name $value
 
                 lappend columns $attribute_name

Doesn't that accomplish the same thing without manually adding in all the acs_objects attributes?

Collapse
Posted by Dave Bauer on
Stan, you need BOTH. You need to set acs_objects.title and cr_revisions.title otherwise your code won't work.
Dave, I finally realized why you said both title and object_title need to be handled once I looked again at the schema of cr_revisionsi. Duh.
Collapse
Posted by Stan Kaufman on
Hi Dave. Actually it works just fine (the earlier version did too). The insert statement gets correctly constructed. From error.log:

insert into cr_revisionsi ( item_id, revision_id, creation_user, creation_ip, description, mime_type, title )
values ( '749', '750', null, null, 'Test', 'text/plain', 'Another Test' )

What isn't OK about that? The file gets uploaded, shows up in the folder UI, etc etc.

Collapse
Posted by Stan Kaufman on
Maybe what isn't clear is which proc we're talking about. It's content::attribute_insert_statement, and all it does is return an sql string for inserting into cr_revisioni:

ad_proc -private content::attribute_insert_statement { 
    content_type table_name bind_vars form_name {prefix {}} {new_p 1}
} {

    Prepare the insert statement into the attribute input view for a new
    revision (see the content repository documentation for details about
    the view).

    @param content_type The content type of the item for which a new
                        revision is being prepared.

    @param table_name The storage table of the content type.

    @param bind_vars The name of an ns_set in which to store the
                     attribute values for the revision.  (Typically
                     duplicates the contents of [ns_getform])
    
    @param form_name The name of the ATS form object used to process the
                     submission.

} {
    # get creation_user and creation_ip
    ns_set put $bind_vars creation_user null
    ns_set put $bind_vars creation_ip null


    # initialize the column and value list 
    set columns [list item_id revision_id creation_user creation_ip]
    set values [list :item_id :revision_id null null]
    set default_columns [list] 
    set default_values [list]
    set missing_columns [list]

    # query for attribute names and datatypes
    foreach attribute [get_attributes $content_type attribute_name datatype default_value ancestor] { 

        foreach {attribute_name datatype default_value ancestor} $attribute { break }

        # get the form value
        if { [template::element exists $form_name $prefix$attribute_name] } {

            set value [template::element get_value $form_name $prefix$attribute_name]

            # Convert dates to linear "YYYY MM DD HH24 MI SS" format
            if { [string equal $datatype date] } {
                set value [template::util::date get_property linear_date $value]
                foreach i {1 2} { 
                    if {[string equal [lindex $value $i] "00"]} { 
                        set value [lreplace $value $i $i 01]
                    }
                }
            }
            
            if { ! [string equal $value {} ] && ![expr { [string equal $ancestor "content_revision"] && [string equal $attribute_name "title"] }] } {
                ns_set put $bind_vars $attribute_name $value

                lappend columns $attribute_name
                lappend values [get_sql_value $attribute_name $datatype]
            }
        } elseif { ![string equal $ancestor "acs_object"] 
                   && ( ![string equal $ancestor "cr_revision"] 
                        || [lsearch -exact {revision_id item_id publish_date} $attribute_name] == -1) } { 
            # We preserve attributes not in the form and not "special" like acs_object and some of cr_revision.
            lappend missing_columns $attribute_name
            if {$new_p && ![string equal $default_value {}]} { 
                ns_set put $bind_vars $attribute_name $default_value
                
                lappend default_columns $attribute_name
                lappend default_values [get_sql_value $attribute_name $datatype]
            }
        } 
    }
    
    if {$new_p} { 
        set insert_statement "insert into ${table_name}i ( [join [concat $columns $default_columns] ", "] )\nvalues ( [join [concat $values $default_values] ", "] )"
    } else { 
        set insert_statement "insert into ${table_name}i ( [join [concat $columns $missing_columns] ", "] )\nselect [join [concat $values $missing_columns] ", "]\nfrom ${table_name}i\nwhere revision_id = content_item.get_latest_revision(:item_id)"
    }
    return $insert_statement
}

So it doesn't need to alias anything -- just return the correct args, eh?

Collapse
Posted by Dave Bauer on
Stan,

Sure this code works fine. Hopefully we'll get the attributes all straightended out and won't be chaning acs_objects again!

Thanks for looking into this.

Collapse
Posted by Stan Kaufman on
Thanks for checking this, Dave. I'll commit to HEAD and oacs-5-2.
Collapse
Posted by Stan Kaufman on
Oops, this doesn't set package_id in acs_objects when a new item is added to the CMS. Gotta fix that.
Stan,

Sorry - I missed your original post. I've done a lot of work on the package over the last year, primarily on usability and toolkit integration (it was a standalone module). There's still a way to go but I do use it on a production site.

I haven't done much in the last few months but the work I've done since we branched oacs-5-2 has been there. If you plan to more work on it (and I hope you do!), please work off of that branch. It's a bit odd considering I'm running the code in production against a 5.1.5 install (explaining why you've hit those 5.2 errors and have not), but that is how things have worked out.

Collapse
Posted by Stan Kaufman on
Hey Michael, I'd tumbled to the fact that the version on oacs-5-2 is beyond HEAD. I think I'll merge the oacs-5-2 code up to HEAD and poke at things there where it's safer.

What is the state of handling symlinks in CMS at this point? Can you symlink a folder (containing other files and folders) in one CMS instance to another CMS instance -- eg a "Main Repository" instance and a "Review Committee" instance that would presumably be in a different subsite? How do you use symlinks in your production setting?

Hi Stan, CMS is not a core package, so I'd highly encourage you to work off the soon to be current branch which is 5.2, as this will be current for quite some time. Saves your the headache of tagging it 5.2 compat all the time.
Collapse
Posted by Stan Kaufman on
Hey Malte. I guess I'm still confused about the guidelines of what gets developed where -- on HEAD or on a branch. I had thought that new functionality -- and particularly new data models -- should be developed on HEAD first, while bug fixes go on the current released branch -- oacs-5-2 now. However, Michael did his substantial work on CMS on oacs-5-2, as did you with FS (i18n etc) -- and not on HEAD. So at this point, HEAD is behind the oacs-5-2 branch for those two packages. It appears that stuff needs to be merged up from oacs-5-2 to HEAD instead of (as I would have thought) down from HEAD to oacs-5-2, or the next branch release.

I'm working to move stuff I developed on the 3.2.5 platform to 5.x, but I'm not in a particular hurry, and I want to target where OpenACS is going to be in the future, not where it is right now -- so I don't have to play the upgrade script game before I even deploy. (The periodic "move CR features into acs_objects" discussions suggest that there are some basic core changes coming down the line -- though maybe that time is far off, I don't know.) This is why I want to work with a HEAD system, not a branch. (That and the fact that no one is supposed to use HEAD for production, so breaking things temporarily isn't such a critical concern.)

What I thought I'd do for CMS (and for FS for that matter) would be to merge the oacs-5-2 stuff into HEAD just to get the new/fixed code up there. This has been done periodically (isn't it usually Jeff who handles this?) at a new branch release (so all the bug fixes get merged to HEAD), but shouldn't it happen whenever someone makes some substantive improvements to a package on a branch?

Put another way, I want to be working with etch or even with sid -- and not sarge -- since by the time I need it, the release I use will be "stable" and not "testing" or "unstable". Or am I mistaken entirely here about the parallel between OpenACS and Debian releases?

I seem to recall that you use a complex mixed system(s) at your company, so these are issues that you obviously grapple with -- and have figured out? How do you mix different versions of core and non-core packages from cvs checkouts?

Stan,

I worked on HEAD while doing the majority of the work. I switched to oacs-5-2 when we branched because, at that point, I was working mostly on very small UI fixes. In any case, I really haven't touched the data model.

Moving forward, I've have some plans to do more work on CMS in the near future and I'd like to be working on the same branch as anyone else contributing. From what I've read of the CVS docs, oacs-5-2 is the proper place to do this work.

Wrt your question about symlinks, the index page of the sitemap module calls an sql function that resolves symlinks before presenting any information. I don't use symlinks, though, so I haven't really exercised any of that code. You can't symlink to content outside of an instance though. At least not with the current UI. Sounds to me like you need some workflow type functionality. This (simple author->edit->approve workflow) is, in fact, what I plan to start adding on soon via integration with the workflow module.