Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Workflow::Exception::Validation #137

Closed
ehuelsmann opened this issue Jul 14, 2021 · 7 comments · Fixed by #193
Closed

Remove Workflow::Exception::Validation #137

ehuelsmann opened this issue Jul 14, 2021 · 7 comments · Fixed by #193
Assignees
Labels
blocking Issue blocks release of the milestone it's in deprecation
Milestone

Comments

@ehuelsmann
Copy link
Member

Description

In 2.0, we will be parting with the idea that a condition evaluating to "False" is an exceptional situation. As a result, we removed the requirement for conditions to raise an exception on failure, replacing it with the requirement to return true/false values.

As it turns out, the 1.x code base uses the same pattern for validators as we used to use for 1.x: Workflow->validate() documents an exception being thrown on validation failure. However, it doesn't raise this exception itself, apparently expecting validators to raise it. Since @oliwel would like us to consolidate conditions and validators (or at least for validators to be able to act as conditions), we should remove the idea of "failures are exceptions" in validators as well as in conditions.

@ehuelsmann ehuelsmann added deprecation blocking Issue blocks release of the milestone it's in labels Jul 14, 2021
@ehuelsmann ehuelsmann added this to the 2.0 milestone Jul 14, 2021
@oliwel
Copy link
Collaborator

oliwel commented Jul 15, 2021

What I think that should work: use a combined class that has three sections:

  1. A "validate" method, arguments are (as of today) the input values defined by the config, return true/false
  2. A "condition" method, no arguments but accessor to the current context
  3. A list of "named params", with in-class default values, that are read from the configuration

This allows a user to put the logic based on the parameters into a class method and call it as needed from the validate/condition. Classes that implement both hook methods can be used as either condition or validator function with a single code base.

The class should also have a defined method to add "error details" that can be either logged or returned to the caller for display (at least in validators).

@ehuelsmann
Copy link
Member Author

Thinking from the structure we have in place now: Condition has a class method which should be called in order to evaluate conditions. Should we create similar infrastructure in validations, where the class method sets up the additional arguments from the workflow context, as the action currently does (from the list of "named parameters" and the available workflow context)?

When we have these two class methods, we could probably combine them in an "Assertion" parent class, leaving two empty classes (Condition and Validation), which inherit from the same parent.

Once the two are merged into one parent class, we could eliminate the specific Validation and Conditon classes, although some children probably will be used as conditions only (and others as validations only).

A question that pops up is: should LazyOR and LazyAND and GreedyOR grow "validate()" capabilities too? (That doesn't really make sense, though, does it, since the parameters need to be mapped into the validate() argument list?)

@ehuelsmann
Copy link
Member Author

Another approach to preventing code-duplication would be to have a condition which says "the action satisfies all validations" or "the action satisfies validation X".

@ehuelsmann
Copy link
Member Author

ehuelsmann commented Aug 4, 2021

Taking a step back to look at the function validators perform (versus conditions), I've come up with the following analysis. Excuse me for putting in total obviousness (I hope it provides new insights too);

Comparing Validators with Conditions

The comparison below helps me understand the commonalities and differences between conditions and validators as a tool to understand why they are different implementations today and how they are different in their intended role within Workflow (different requirements) -- building a base to test oliwel's proposal on.

Architectural comparison

Commonality

Both Conditions and Validators make an assertion about the state of the workflow in relation to an action, based on the information available in the context. The outcome of this assertion is in both cases success or failed.

Differences

The two main differences between Conditions and Validators are (a) the timing and (b) effect of their execution.

Timing

  1. Conditions are performed while determining the set of available actions
  2. Validations are performed prior the execution of an available action

Effect of the execution

  1. Conditions prevent actions for which a condition fails (an unavailable action) from being offered for execution
  2. Validations prevent the action to be executed from receiving acceptable state

Functional comparison

From an architectural perspective, both perform the same transformation (f(workflow_state) => "success"/"failed"). They differ in the way they need to provide insight in the reason for the outcome:

  1. Conditions need to provide insight to administrators on their success/failure
  2. Validators need to provide insight to end-users on their success/failure

By providing conditions access to a logging mechanism, they can provide the required insight to administrators.

A similar mechanism does not work for validators, since the logging mechanism is normally disjoint from the application using Workflow, whereas the workflow consumer needs access to the failure condition(s) in order to present them to the user.

Implementation comparison

Historic background

The documentation - which is for me the only source to extract a sense of history around the library - gives the impression that Conditions were originally designed with the purpose described above, but the purpose for Validators was less generic.

Validators were added more specifically to assert "valid input" on fields. Since multiple fields can have the same requirement(s) to be asserted "valid", a method was devised to prevent duplication of definitions and code. By providing the ability to do an in-line mapping of "values in the workflow context" (fields?), validators need to be coded once (and defined once, with an in-line mapping on use).

Although the purpose of Validators or their original addition to the library was focused on valid input values - the fact that they receive a list of input values testifies to that - their use is not necessarily restricted to that purpose by design as they also receive the entire workflow state.

API

Conditions have an evaluate method. This method is (in Workflow 2.0) called by State (or a nesting Condition) from the class (wrapper) method evaluate_condition, which takes as its arguments a condition name and a workflow. The wrapper is responsible for result caching and resolution of the condition name to a condition instance. Due to this construct (i.e. caching), conditions cannot be declared in-line and can only be evaluated by name.

Validators have a validate method. This method is called by Action without wrapper; there is no caching of the result (because their use-case does not include repeated evaluation whereas the use-case for Conditions does).

Examples

There are several examples providing suggestions on implementation of conditions and validators. For Conditions, there are examples/ implementations included for nesting. Validators don't come with similar examples.

Summary

Coming from their own backgrounds (imagined use-cases), Conditions and Validators have differing implementations due to what might have been requirements at the time of their design

Requirement Conditions Validators
Nesting yes no
- Caching yes no
- Explicit naming yes no
User-feedback Admin End-User
Workflow specified parametersNote 1 yes yes
Loose couplingNote 2 no yes

Note 1: Workflow specified parameters allow setting of fixed values - available to the assertion - as part of the workflow definition. The <arg> tag is used for this purpose.
Note 2: Loose coupling is used to map values from the workflow context into the argument list of the assertion. The <param> tag is used for this purpose.

Conclusion

The function performed by validators and conditions is the same, with the exception of the target audience of validation failures. Although the current conditions implementation does not take explicit parameters, adding this feature can be added in a fully backward compatible way; another strategy could be to pass the names of the fields to be validated in the <param> list (which removes the need for supporting a run-time map entirely). Similarly, although Validators aren't in need of a cache, having one (as a consequence of using a single harmonized implementation between conditions and validators) won't hurt.

Given the previous paragraph, it seems like a single assertion infrastructure design can be created, where assertions can be used either as conditions or as validators. This design primarily needs to address reporting of assertion failure and success based on the role the assertion is being used for.

A second concern the design would need to address, is the combination of caching and in-line declaration of validator configuration in the workflow. The other option being that this currently supported type of configuration is removed and all validator configuration needs to be specified in a (named) validator, just as is currently the case with conditions.

@ehuelsmann
Copy link
Member Author

@jonasbn I hope we can discuss the analysis above (maybe on Slack sometime) so I come up with a PR to shoot at.

@jonasbn
Copy link
Collaborator

jonasbn commented Sep 12, 2021

Hi @ehuelsmann I would really love too, but currently I am stuck in a work tar pit, with 60+ hour work weeks, so I am not able to do anything constructive or time consuming at this point, due to time constraints. My hope is work will settle down over the next few weeks and I can get back to a normal work-life balance and to doing some more open source again

@ehuelsmann
Copy link
Member Author

Hi @ehuelsmann I would really love too, but currently I am stuck in a work tar pit, with 60+ hour work weeks, so I am not able to do anything constructive or time consuming at this point, due to time constraints. My hope is work will settle down over the next few weeks and I can get back to a normal work-life balance and to doing some more open source again

Thanks for the update! Good luck at work and stay healthy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking Issue blocks release of the milestone it's in deprecation
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants