Forum OpenACS Improvement Proposals (TIPs): Tip #79 (Implemented) Callbacks

Collapse
Posted by Andrew Grumet on
The objective of this TIP is to give core the ability to invoke code in non-core packages that may or may not be installed, but without cluttering core with an ever-growing list of "if" statements that check if package X is installed before calling its code.

The larger goal is to promote reuse of standard pages and functions by removing the need to create per-package versions of these. So, for the example case of removing a user, we're proposing this architecture

-> Core proc for removing user
   -> Invoke callbacks based on what's installed
      -> Package A logic for removing user
      -> Package B logic for removing user
      -> Package C logic for removing user
      ...
   -> Core logic for removing user
as opposed to this architecture
-> Package A proc for removing user
   -> Package A logic for removing user
   -> Call core proc for removing user

-> Package B proc for removing user
   -> Package B logic for removing user
   -> Call core proc for removing user
The specific proposal is to extend ad_proc as follows
  1. Add a -callback flag to designate the function as a callback system participant (entry point or implementation).
  2. Add an -implementation flag to designate the function as a callback implementation.
and to add a page to api-doc that lists the available callback entry points.

Once implemented, callback entry points would be declared like this:

ad_proc -callback module::op { 
 {-option {} }
 arg1:required 
} {
 docs 
} -
Note the proc has no body. This proc exists solely to document the inputs and outputs, and so we can build a page documenting the callback entry points. Callback implementations would be declared like this:
ad_proc -callback module::op -implementation implname  { ... }
Note the implementation has the same name as the callback, but also has an -implementation flag and a body. To ensure that callbacks aren't accidentally overwritten, say if a developer forgets the implementation flag, ad_proc will error out if the proc has a -callback flag and a body, but no -implementation flag.

More: https://openacs.org/forums/message-view?message%5fid=273782

Collapse
2: Re: Tip #79 Callbacks (response to 1)
Posted by Andrew Grumet on
Another important goal is to provide a simple, flexible alternative to service contracts. We're not proposing at this point to replace existing uses of service contracts with callbacks. The fit may not be good, and in any case we'll have to try this out first before we'll understand it well enough to have those discussions.
Collapse
3: Re: Tip #79 Callbacks (response to 1)
Posted by Malte Sussdorff on
Would you mind giving an example how this would look like in the core package and the custom package? I assume the "ad_proc -implementation {}" bit will be defined in the package, but how do you access this procedure from core, if core has no idea that there exists a function like this within a mounted package.

Furthermore what is the benefit of splitting the documentation away from the actual procedure? Can't we have it in one place?

Collapse
4: Re: Tip #79 Callbacks (response to 1)
Posted by Jeff Davis on
Core uses tcl introspection to find which callbacks exist and invokes them. eg
foreach proc [info procs ::callback::module::op::impl::*] { 
  $proc $args
}
The documentation is split from the procedure since the documentation is for the "callback" not the implementation.

eg: for the apm callbacks right now if you want to know what the after mount callback does you dig through the code to find where its called. under the new scheme you could just use the api-browser to look up apm::after-mount and the declaration would look like:

ad_proc -callback apm::after-mount {
  -package_id:required
  -node_id:required
} { 
   This callback is invoked by the apm when a package is 
   mounted, note that the package may already be mounted
   elsewhere or may have been mounted before and unmounted 
   so try to ensure that these cases are properly handled in 
   your callback implementation.

   Also the implementation name must match the package key 
   in order for the apm to invoke the correct procedure.

   ...
} -
and a specific implementation might also have it's own docs:
ad_proc -callback apm::after-mount -implementation file-storage {} {
  the after mount callback for file storage creates a 
  root folder and sets default permissions.
} {
   ...
}
Note that I think we could supress the parameters for the implementation version and use the arg parse constructed for the callback in each one which would make it far easier to add optional parameters to the callbacks.

Also note how much better our lives would be with respect to the apm callbacks if we did this!

Collapse
5: Re: Tip #79 Callbacks (response to 1)
Posted by Stan Kaufman on
Having spent a considerable bit of time trying to understand what/when/how/why callbacks happen during install and uninstall -- and finding myself at best "confused on a higher plane" than when I started -- I think this is a brilliant development!
Collapse
6: Re: Tip #79 Callbacks (response to 1)
Posted by Jade Rubick on
I approve this proposal. Andrew, are you thinking about implementing this in 5.2 or 5.3? I ask because technically 5.2 is frozen, but we are talking about relaxing that for some of Jeff's TIPs.

The implementation of this is also so straitforward, and it clears up a long-standing problem we've been having, so I'd be willing to allow it for 5.2.

Collapse
7: Re: Tip #79 Callbacks (response to 1)
Posted by Jeff Davis on
We have committed the callback code to openacs. the ad_proc documentation includes the -callback and -impl flags and the documentation for the "callback" function is

callback [ -catch ] [ -impl impl ] callback [ args... ]
Defined in packages/acs-bootstrap-installer/tcl/00-proc-procs.tcl

Invoke the registered callback implementations for the given callback. The callbacks terminate on error unless -catch is provided. The value returned by the callback function is determined by the return codes from the callback implementations.

The return codes returned from the implmentation are treated as follows:

return -code ok or "return"
With a plain return, a non-empty return value will be lappended to the list of returns from the callback function
return -code error or "error"
errors will simply propigate (and no value returned) unless -catch is specified in which case the callback processing will continue but no value will be appended to the return list for the implementation which returned an error.
return -code return
Takes the return value if the implementation returning -code return and returns a one element list with that return value. Note that this means if you have code which returns return -code return {x y}, you will get {{x y}} as the return value from the callback. This is done in order to unambiguously distinguish a pair of callbacks returning x and y respectively from this single callback.
return -code break
return the current list of returned values including this implementations return value if non-empty
return -code continue
Continue processing, ignore the return value from this implementation

Switches:
-catch (boolean) (optional)
if catch specified errors in the callback will be caught, tracebacks logged as errors to the server log, but other callbacks called and the list of returns still returned. If not given an error simply is passed further on.
-impl (defaults to "*") (optional)
invoke a specific implemenation rather than all implementations of the given callback
Parameters:
callback - the callback name without leading or trailing ::
Returns:
list of the returns from each callback that does a normal (non-empty) return

Lee Denison wrote reasonably comprehensive tests for the API and we will be committing some examples shortly. Head installs after these changes and everything seems to work just fine.

Collapse
8: Re: Tip #79 Callbacks (response to 1)
Posted by Andrew Grumet on
Excellent! We've got the original .LRN use case to apply this to.

Also, api-doc should be updated with support for finding these, if it hasn't already.

Though in my immediate future I see bug bashing...

Collapse
9: Re: Tip #79 Callbacks (response to 1)
Posted by Jeff Davis on
the contracts are easy to find in api-doc since they are all of form:
::callback::*::contract, and the implmentations are similiar,
also we add an @see automatically back to the contract for
any given implementation.
Collapse
Posted by Enrique Catalan on
Hi, Nice development.

I used it but I got an error, I'm not sure if it is a bug but I had problem with *.xql files, apparently the implementations could not read the queries of the .xql file.

I did the "watch file" on the xql files and didn't work then I restarted the webserver and neither, so I copied the query from the .xql and pasted it into the tcl and it worked!.

I defined my callback as follow:

ad_proc -callback MergePackageUser {
    -from_user_id:required
    -to_user_id:required
} {
    Merge two accounts
} {}

One of the implementations is:

namespace eval forum::merge {
    ad_proc -callback MergePackageUser -impl forums {
        -from_user_id:required
        -to_user_id:required
    } {
        Merge the *forums* of two users.
        The from_user_id is the user_id of the user
        that will be deleted and all the *forums*
        of this user will be mapped to the to_user_id.

    } {
        ns_log Notice "Merging forums"

        db_transaction {
            db_dml upd_poster { *SQL* }
            db_dml upd_user_id { *SQL* }
            set result 1
        } on_error {
            ns_log Error "I couldn't merge forums. The error was $errmsg"
            set result 0
        }

        return $result
    }
}

The xql file seems :

    <fullquery name="forum::merge::MergePackageUser.upd_poster">
        <querytext>
        update forums_messages
        set last_poster = :to_user_id
        where last_poster = :from_user_id
        </querytext>
    </fullquery>

    <fullquery name="forum::merge::MergePackageUser.upd_user_id">
        <querytext>
        update forums_messages
        set user_id = :to_user_id
        where user_id = :from_user_id
        </querytext>
    </fullquery>

If I run the same proc but without the callbacks flags it works fine, the problem is only when I add that flags but if I copy the queries to the .tcl file, all work fine.

Did I make a mistake in my code?

Thanks in advance.

Enrique.

Collapse
Posted by Jeff Davis on
callbacks are placed in a special namespace so there is no need for namespace eval (the parent namespace is ignored). so for MergePackageUser your implementation would be the function ::callback::MergePackageUser::impl::forums and you could reference queries that way.

You can look in the API browser to see the callback definition and the resulting functions.

Collapse
Posted by Enrique Catalan on
Thanks Jeff :).

it worked as "callback::MergePackageUser::impl::forums".