Forum OpenACS Development: WARNING: site_node::get_children returns incorrect result set

site_node::get_children returns a result set (list) with one too many elements. It returns the passed node itself. Which isn't really a child of itself.

I found this while coding a recursive tcl proc that processes an entire subtree of the site map. Needless to say, aolserver crashed when it encountered my recursive proc with no terminating condition.

ad_proc -private site_node_delete_recursive {
    node_id
} {
    Recursively delete site node given in node_id by calling self with
    all available child site nodes.

    @param node_id current site node
    @return 1 always. Error causes exception. Programming error
    causes a crash!
} {
    set child_node_ids [site_node::get_children \
			    -element node_id \
			    -node_id $node_id]

    for {set i 0} {$i<[llength $child_node_ids]} {incr i} {
	set child_node_id [lindex $child_node_ids $i]


	# --Recurse--
	# Bug in get_children causes self to be returned!
	if {! $child_node_id == $node_id} {
	    site_node_delete_recursive $child_node_id
	}

	# Delete
	site_node_delete $child_node_id
    }

    # Delete top node
    site_node_delete $node_id

    return 1
}

The argument could be made by some that returning the passed node is not a bug. It is to me, but... Anyhow, I've logged bugs, posted patches, and made announcements and there doesn't seem to be much (if any) interest. At least nobody's said anything yet...

Yes, I'm deleting subsites. However, it's not limited to that scope. You can feed it any site node and it will clean up beneath that point.

I don't have (unrealistic) high hopes that removing nodes already populated with user data will be a snap. Not from what I've seen so far of packages cleaning up without RI violations.

If you disregard the complexities of cleaning up data and assume the package writer has done it right, the code to delete a subsite is fairly simple and straightforward.

I placed a file in the openacs storage area for you at https://openacs.org/storage/download/site-tree-delete.tcl?version_id=120452.

You'll see some hacks in there. There's a particularly ugly one that compensates for a kernel bug. Yes, I posted a patch, but it's no less ugly.

# Shows bug in site_node::get_children - reurns passed node
aa_register_case -cats {
    script
} -on_error {
    site_node::get_children returns root node!
} test_for_root_node_inclusion {
    @author Randy O'Meara
} {
    # Start with a known site-map entry
    set node_id [site_node::get_node_id -url "/"]

    set child_node_ids [site_node::get_children \
			    -all \
			    -element node_id \
			    -node_id $node_id]

    # lsearch returns '-1' if not found
    aa_equals "site_node::get_children" [lsearch -exact $child_node_ids $node_id] -1
}