Bug #23736 for Workflow: Patch for cached conditions

I have received a patch from Alexander Klink (details below), just putting it here for the record. Dear Jonas, here is another patch for Workflow from me. Again from the OpenXPKI background. Until now, we had accumulated some conditions that were checking one thing and the exact opposite (for example whether a forked workflow instance has successfully been completed or not). We did this by distinguishing within the condition by condition name and throwing a workflow_error in one case and none in the other. Unluckily, this means two calls to the condition, and things changed in between (for example, the forked workflow finished). This is why I implemented this feature directly in Workflow. Using this patch, Workflow is now able to cache the result of a condition's evaluate() method in the Workflow::State object of a given state. I've also added a syntax for "inverted conditions", which allows one to specify <condition name="!something">, which returns the opposite of the something condition. As the cached result is used here, too, this is safe for cases like above. Here is a bit on how I implemented it: - in the create_condition_objects() method, I check whether the condition name starts with '!'. If this is so, I don't add a real object, but a hashref containing the name. If it does not, everything is as usual - in evaluate_action(), I set $orig_condition to the name of the "original" condition without the '!'. If an entry in the hashref $self->{'_condition_result_cache'} exists for this name, we use this an answer according to whether we are asked for the normal or the inverse condition (also mentioning that we are using the cache). If no cached result exists, we replace the hash with the name by the real object from the factory and call evaluate on it. If this throws exceptions, we set the result cache hash entry to 0, otherwise it is set to 1. According to whether the original or the inverse condition was asked for, we throw an exception or not. I've also added some documentation in Workflow.pm and Workflow/Condition.pm. Hope this justifies another release, so that we can update our requirements and change some configuration and code. Kind regards, Alex

diff -ru /tmp/Workflow-0.23/lib/Workflow/Condition.pm ./lib/Workflow/Condition.pm --- /tmp/Workflow-0.23/lib/Workflow/Condition.pm 2006-09-12 20:15:32.000000000 +0200 +++ ./lib/Workflow/Condition.pm 2006-11-29 16:51:53.000000000 +0100 @@ -49,10 +49,18 @@ <workflow> <state> ... - <action name="MyAction"> + <action name="SomeAdminAction"> ... <condition name="IsAdminUser" /> </action> + <action name="AnotherAdminAction"> + ... + <condition name="IsAdminUser" /> + </action> + <action name="AUserAction"> + ... + <condition name="!IsAdminUser" /> + </action> </state> ... </workflow> @@ -147,6 +155,24 @@ #=head3 init +=head2 Caching and inverting the result + +If in one state, you ask for the same condition again, Workflow uses +the cached result, so that within one list of available actions, you +will get a consistent view. Note that if we would not use caching, +this might not necessary be the case, as something external might +change between the two evaluate() calls. + +Caching is also used with an inverted condition, which you can specify +in the definition using C<<condition name="!some_condition">>. +This condition returns exactly the opposite of the original one, i.e. +if the original condition fails, this one does not and the other way +round. As caching is used, you can model "yes/no" decisions using this +feature - if you have both C<<condition name="some_condition">> and +C<<condition name="!some_condition">> in your workflow state definition, +exactly one of them will succeed and one will fail - which is particularly +useful if you use "autorun" a lot. + =head1 COPYRIGHT Copyright (c) 2003-2004 Chris Winters. All rights reserved. diff -ru /tmp/Workflow-0.23/lib/Workflow/State.pm ./lib/Workflow/State.pm --- /tmp/Workflow-0.23/lib/Workflow/State.pm 2006-09-12 20:15:32.000000000 +0200 +++ ./lib/Workflow/State.pm 2006-11-29 16:02:06.000000000 +0100 @@ -64,14 +64,101 @@ my @conditions = $self->get_conditions( $action_name ); foreach my $condition ( @conditions ) { - my $condition_name = $condition->name; + my $condition_name; + if (exists $condition->{name}) { # hash only, no object + $condition_name = $condition->{name}; + } + else { + $condition_name = $condition->name; + } + my $orig_condition = $condition_name; + my $opposite = 0; + $log->is_debug && - $log->debug( "Evaluating condition '$condition_name'" ); - eval { $condition->evaluate( $wf ) }; - if ( $@ ) { - # TODO: We may just want to pass the error up without wrapping it... - workflow_error "No access to action '$action_name' in ", - "state '$state' because: $@"; + $log->debug( "Checking condition $condition_name" ); + + if ($condition_name =~ m{ \A ! }xms) { + # this condition starts with a '!' and is thus supposed + # to return the opposite of an original condition, whose + # name is the same except for the '!' + $orig_condition =~ s{ \A ! }{}xms; + $opposite = 1; + $log->is_debug && + $log->debug( "Condition starts with a !: '$condition_name'" ); + } + + if (exists $self->{'_condition_result_cache'}->{$orig_condition}) { + # The condition has already been evaluated and the result + # has been cached + $log->is_debug && + $log->debug( "Condition has been cached: '$orig_condition', cached result: ", $self->{'_condition_result_cache'}->{$orig_condition} ); + if (! $opposite) { + $log->is_debug && + $log->debug( "Opposite is false." ); + if (! $self->{'_condition_result_cache'}->{$orig_condition}) { + $log->is_debug && + $log->debug( "Cached condition result is false." ); + workflow_error "No access to action '$action_name' in ", + "state '$state' because cached ", + "condition 'orig_condition' already ", + "failed before."; + } + } + else { + # we have to return an error if the original cached + # condition did NOT fail + $log->is_debug && + $log->debug( "Opposite is true." ); + if ($self->{'_condition_result_cache'}->{$orig_condition}) { + $log->is_debug && + $log->debug( "Cached condition is true." ); + workflow_error "No access to action '$action_name' in ", + "state '$state' because cached ", + "condition '$orig_condition' did NOT ", + "fail before and we are being asked ", + "for the opposite."; + } + } + } + else { + # we did not evaluate the condition yet, we have to do + # it now + if ($opposite) { + # so far, the condition is just a hash containing a + # name. As the result has not been cached, we have + # to get the real condition with the original + # condition name and evaluate that + $condition = FACTORY->get_condition( $orig_condition ); + } + $log->is_debug && + $log->debug( q{Evaluating condition '}, $condition->name, q{'} ); + eval { $condition->evaluate( $wf ) }; + if ( $@ ) { + # TODO: We may just want to pass the error up + # without wrapping it... + $self->{'_condition_result_cache'}->{$orig_condition} = 0; + if (! $opposite) { + workflow_error "No access to action '$action_name' in ", + "state '$state' because: $@"; + } + else { + $log->is_debug && + $log->debug( "Opposite and condition failed" ); + } + } + else { + $self->{'_condition_result_cache'}->{$orig_condition} = 1; + if ($opposite) { + workflow_error "No access to action '$action_name' in ", + "state '$state' because condition ", + "$orig_condition did NOT fail and we ", + "are checking $condition_name."; + } + else { + $log->is_debug && + $log->debug(" Opposite false and condition OK"); + } + } } $log->is_debug && $log->debug( "Condition '$condition_name' evaluated successfully" ); @@ -229,9 +316,19 @@ }); } else { - $log->is_info && - $log->info( "Fetching condition '$condition_info->{name}'" ); - push @condition_objects, FACTORY->get_condition( $condition_info->{name} ); + if ( $condition_info->{name} =~ m{ \A ! }xms ) { + $log->is_debug && + $log->debug( "Condition starts with !, pushing hash with name only" ); + # push a hashref only, not a real object + # the real object will be gotten from the factory + # if needed in evaluate_action + push @condition_objects, { 'name' => $condition_info->{name} }; + } + else { + $log->is_info && + $log->info( "Fetching condition '$condition_info->{name}'" ); + push @condition_objects, FACTORY->get_condition( $condition_info->{name} ); + } } } return @condition_objects; diff -ru /tmp/Workflow-0.23/lib/Workflow.pm ./lib/Workflow.pm --- /tmp/Workflow-0.23/lib/Workflow.pm 2006-09-12 20:15:32.000000000 +0200 +++ ./lib/Workflow.pm 2006-11-29 16:28:36.000000000 +0100 @@ -328,7 +328,7 @@ <action name="upload file" resulting_state="uploaded" /> </state> <state name="uploaded" autorun="yes"> - <action name="verify file" resulting_state="annotate"> + <action name="verify file" resulting_state="verified file"> <!-- everyone other than 'CWINTERS' must verify --> <condition test="$context->{user} ne 'CWINTERS'" /> </action> @@ -336,10 +336,13 @@ <condition test="$context->{user} eq 'CWINTERS'" /> </action> </state> - <state name="verify file"> + <state name="verified file"> <action name="annotate"> <condition name="can_annotate" /> </action> + <action name="null"> + <condition name="!can_annotate" /> + </action> </state> <state name="annotated"> <action name="null" resulting_state="finished" />