New validations implemented #199

hbelmiro · 2022-05-09T19:56:31Z

This is a work in progress...

Validations Implemented

Switch state must contains dataCondition or eventCondition, not both
Switch state - Default condition - next state or end required
Switch state - Default condition - next state must be the name of an existing state
Switch state - Event conditions - timeout required
States - Event timeout - timeout must be a valid duration

Final proposal

Validations are grouped by workflow definitions (state, even, function...).

Initial proposal (not approved)

I'm implementing new workflow validations and wanted to check what do you guys think of my approach?
Currently, we’re performing all the validations in the io.serverlessworkflow.validation.WorkflowValidatorImpl#validate method. That method already is quite long, and my new validations would make it even longer. That would increase the code complexity and it would become harder to test.
So I decided to break the new validations into new classes.

I created this io.serverlessworkflow.validation.WorkflowValidation interface:

interface WorkflowValidation {
  List<ValidationError> validate(Workflow workflow);
}

Each validation will implement that interface and be registered as a service on resources/META-INF/services/io.serverlessworkflow.validation.WorkflowValidation, so the original WorkflowValidatorImpl#validate method can load those services and perform the validations.

The current validations are still in the original method. I only implemented a couple of new validations in this new structure. If you like it, we can move all the validations to their respective classes in the future.

Advantages of this approach:

The validation code is simpler and easier to read, since it only cares about what's being validated.
The code is easier to test, since we don't need a full valid workflow to test a class. We can just set the workflow elements that matter for that test and we don't need to care about the other ones.
Changes in one validation won't affect other validation tests.
For instance: If we have a new required attribute in the spec, we just have to create a new validation and test it. We don't need to change all the other tests to include this new required attribute, because they don't care about this new attribute.

Disadvantage of this approach:

We'll might lose a bit of performance, since we'll visit the same structure more than once. But, I don't believe this will be significant.

Please let me know what do you think and if I can continue implementing the validations this way. Feel free to suggest improvements in this solution as well.

tsurdilo · 2022-05-09T20:20:37Z

Does this division play nicely with service loader? https://github.com/serverlessworkflow/sdk-java/blob/main/spi/src/test/resources/META-INF/services/io.serverlessworkflow.api.interfaces.WorkflowValidator

I am honestly not a big fan of splitting up each validation into own class, think long term it will just create more maintenance.
If we have to split this up would prefer a more "natural" division based on workflow def, for example "states", "timeouts", "function def", "events def" ... but still keep a top-level validation impl that can group these together.
@manick02 wdyt?

Also, if you need extra validation ontop of what SDK provides another option is to define your own validator and set it via service loader.

manick02 · 2022-05-10T05:02:52Z

I agree with @tsurdilo
We can have one interface at the top level (Workflow) and the top level implementation of it decides how to validate sub parts of workflow(states, timeouts) etc. The proposed level of subpart validations are very granular to my liking.

hbelmiro · 2022-05-12T12:20:45Z

This is ready to review, @tsurdilo.

validation/src/main/java/io/serverlessworkflow/validation/StatesValidator.java

tsurdilo · 2022-05-12T13:52:52Z

@hbelmiro thanks for the updates. will review as soon as possible.

ricardozanini

So are we proceeding with the validations of the other sections such as functions, events, and so on in other PRs?

tsurdilo · 2022-05-12T13:59:17Z

@hbelmiro so this is just validating switch state at this time?

from @ricardozanini question, if you want to do it in separate prs let's maybe create a branch for this? think it would maybe help until everything is done so we could move to main branch?

So far I think this looks ok, ty. Just really wanna make sure it plays well with service loader in the end.

hbelmiro · 2022-05-12T17:31:28Z

@tsurdilo @ricardozanini
Actually I was thinking of implement on demand. But since you prefer to merge everything together, I think we can do in one single PR.
I'll revert it to draft and continue with the implementation.
Thanks for now, guys.

hbelmiro · 2022-05-12T17:53:34Z

Just really wanna make sure it plays well with service loader in the end.

@tsurdilo currently only the WorkflowValidatorImpl class is a service. The section validations are in a static method which is called by WorkflowValidatorImpl.
We can turn the section validations into services (rather than use static methods) if you prefer. If we decide to do that, we have two alternatives:

Create specific interfaces for each section. This gives the user flexibility to specify implementations for each section.
All section validations implementing WorkflowValidatorImpl. Simpler, but not so flexible. Also, the section implementations would have to implement WorkflowValidatorImpl's methods that don't make much sense for section implementations, like:

WorkflowValidator setWorkflow(Workflow workflow);
WorkflowValidator setSchemaValidationEnabled(boolean schemaValidationEnabled);

tsurdilo · 2022-05-12T19:17:38Z

@hbelmiro thanks for the info! what do you think would be better?
my best guess right now and could be wrong is to just leave the top-level WorkflowValidatorImpl and allow users to set their own if they want via config. if there is a strong need for being able to overwrite only certain groups of validations then your approach would be better imo, so I'm torn :)

In my mind having users extend our WorkflowValidatorImpl and be able to override a particular validation method would work too. wdyt?
Either way I think most of users will just use the validation that comes out of box in sdk, so maybe we are thinking too much about this.

hbelmiro · 2022-05-13T14:30:53Z

@tsurdilo I think we can go on the way we are (users can set their own WorkflowValidator only). In the future we can make it more flexible if needed.

fjtirado

Some things that need to be sorted out specifically commented in the code.
Although I do not mention that specifically, in general, I think we are using Optional too much.
I understood the reason, to not pass the list of error to all the validation methods, and do not have this kind of idiom
if (validateXXX) {errors.add(xxx_ERROR)}
Having
validateXXX().ifPresent(errors:add);
you have the error condidition and the error message in the validateXXX(), but since at the end the error message is a constant, I found easier to read the first one.

validation/src/main/java/io/serverlessworkflow/validation/CommonStateValidator.java

fjtirado · 2022-05-24T08:49:59Z

validation/src/main/java/io/serverlessworkflow/validation/CommonStateValidator.java

+        return error;
+    }
+
+    protected abstract List<ValidationError> runSpecificValidations(Workflow workflow, T state);


Rather than declaring this method as abstract (you can still declare the class as abstract event if you do not have any abstrace method), you can return new ArrayList by default and save a lot of code in childred classes that are just returning new ArrayList

fjtirado · 2022-05-24T08:52:41Z

validation/src/main/java/io/serverlessworkflow/validation/StateValidator.java

+
+interface StateValidator {
+
+    List<ValidationError> validate(Workflow workflow, State state);


Unless there is need to random access, I will declare this as collection rather than list (you might eventually implement equals in ValidationError and use a set without changing the interface)

validation/src/main/java/io/serverlessworkflow/validation/StatesValidator.java

fjtirado · 2022-05-24T08:59:52Z

validation/src/main/java/io/serverlessworkflow/validation/StatesValidator.java

+                if (stateValidator != null) {
+                    errors.addAll(stateValidator.validate(workflow, state));
+                } else {
+                    throw new IllegalStateException("No validator found for state type: " + state.getType());


I think you are not providing the complete set of validator. hence this will eventually throw IlleglStateException even for valid states.
I would rather do nothing if there is not validator for that state and print a log.

Also by doing that, you wont need dummy implementations of validator that just return an empty list of errors (they can be added lated when the validator is implemented)

fjtirado · 2022-05-24T09:01:55Z

validation/src/main/java/io/serverlessworkflow/validation/SwitchStateValidator.java

+        List<ValidationError> errors = new ArrayList<>();
+
+        if (!switchState.getDataConditions().isEmpty() && !switchState.getEventConditions().isEmpty()) {
+            ValidationError error = new ValidationError();


You already have a protected final method in the parent class to create the validation error

fjtirado · 2022-05-24T09:05:17Z

validation/src/main/java/io/serverlessworkflow/validation/SwitchStateValidator.java

+                defaultCondition.getTransition() != null
+                        ? defaultCondition.getTransition().getNextState()
+                        : null;
+        if (nextState != null && !nextState.isEmpty()) {


I think it will be nice to have an utility static method that check if a string is not null and is not empty, so you save a lot of code and dummy erros (in this case you forget the trim, in other case you miss the !)

hbelmiro · 2022-05-24T12:04:03Z

Thanks for the review @fjtirado.
I'm still working on it. I just pushed yesterday as a backup. So, there are several parts that won't stay as they are and some are not even working. Sorry for wasting your time.
However, some reviews are already useful, like the typo, the EnumMap one, and others. So, it wasn't a total waste of time :)

Regarding the use of Optional, I can remove it. In general, I like using Optional (always trying to not overuse it) to let things obvious and more intuitive.

tsurdilo · 2022-05-27T21:18:11Z

@hbelmiro

could we please not mention KOGITO in commit names?
this sdk is not at all related to this project.

I would like to request to update the commits if possible to just include what the changes are. Thanks!!

hbelmiro · 2022-05-27T21:28:19Z

could we please not mention KOGITO in commit names?
this sdk is not at all related to this project.

I would like to request to update the commits if possible to just include what the changes are. Thanks!!

Sure. I'll fix them.

Signed-off-by: Helber Belmiro <helber.belmiro@gmail.com>

fjtirado · 2022-05-30T09:40:56Z

validation/src/main/java/io/serverlessworkflow/validation/state/StatesValidator.java

+
+        if (stateValidator != null) {
+          finalValidationResult =
+              finalValidationResult && stateValidator.isValidState(workflow, state);


so, at the moment a validator fail, we cancel the whole validation process?
I guess this is intentional, but just double checking.
Cancelling the validation process when the first state that has a validation error is found will make the validationErrors list not complete.

No. The idea is to return false, which means "doesn't have valid states", but continue with the validation.
Currently the return value is being ignored, but that's just because it's not fully implemented yet.

Ok, then, you need to change this code because stateValidator.isValidState(workflow, state) is not going to be invoked if previous state returned false.
you should write finalValidationResult&=stateValidator.isValidState(workflow, state);

manick02 · 2022-05-30T14:12:19Z

validation/src/main/java/io/serverlessworkflow/validation/state/StatesValidator.java

+  public StatesValidator(Collection<ValidationError> validationErrors) {
+    this.validationErrors = validationErrors;
+
+    stateValidators.put(Type.EVENT, new EventStateValidator(validationErrors));


Do we have to create the instance of validator for event type every we want to validate states?

Actually, it will create only one instance of StatesValidator per WorkflowValidatorImpl instance.
See: https://github.com/hbelmiro/sdk-java/blob/cdf134a2d324310e7fa37f6e63823019a104d4d4/validation/src/main/java/io/serverlessworkflow/validation/WorkflowValidatorImpl.java#L54
That instance of StatesValidator has several instances of StateValidator (one per type) - I know I need to think about better names :)
In the end, only one instance of each class will be created.

hbelmiro · 2022-06-29T14:04:07Z

Before proceeding with this, I opened a new PR to use Bean Validations. With that PR we'll significantly reduce the amount of manually implemented validations.

hbelmiro · 2024-02-15T20:59:50Z

@ricardozanini closing as stale.

hbelmiro force-pushed the KOGITO-4277 branch from 5cd1cb5 to 1052364 Compare May 12, 2022 12:07

hbelmiro marked this pull request as ready for review May 12, 2022 12:19

hbelmiro requested a review from tsurdilo as a code owner May 12, 2022 12:19

fjtirado reviewed May 12, 2022

View reviewed changes

validation/src/main/java/io/serverlessworkflow/validation/StatesValidator.java Outdated Show resolved Hide resolved

ricardozanini reviewed May 12, 2022

View reviewed changes

hbelmiro marked this pull request as draft May 12, 2022 17:33

fjtirado reviewed May 24, 2022

View reviewed changes

Implemented new validations

05b73da

Signed-off-by: Helber Belmiro <helber.belmiro@gmail.com>

hbelmiro force-pushed the KOGITO-4277 branch from 1bb742e to 05b73da Compare May 27, 2022 21:31

Moved Action validation to ActionValidator

cdf134a

Signed-off-by: Helber Belmiro <helber.belmiro@gmail.com>

fjtirado reviewed May 30, 2022

View reviewed changes

manick02 reviewed May 30, 2022

View reviewed changes

hbelmiro closed this Feb 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New validations implemented #199

New validations implemented #199

hbelmiro commented May 9, 2022 •

edited

tsurdilo commented May 9, 2022

manick02 commented May 10, 2022

hbelmiro commented May 12, 2022

tsurdilo commented May 12, 2022

ricardozanini left a comment

tsurdilo commented May 12, 2022 •

edited

hbelmiro commented May 12, 2022

hbelmiro commented May 12, 2022

tsurdilo commented May 12, 2022 •

edited

hbelmiro commented May 13, 2022

fjtirado left a comment

fjtirado May 24, 2022

fjtirado May 24, 2022

fjtirado May 24, 2022

fjtirado May 24, 2022 •

edited

fjtirado May 24, 2022

fjtirado May 24, 2022

hbelmiro commented May 24, 2022

tsurdilo commented May 27, 2022 •

edited

hbelmiro commented May 27, 2022

fjtirado May 30, 2022

hbelmiro May 30, 2022

fjtirado May 31, 2022

manick02 May 30, 2022

hbelmiro May 30, 2022

hbelmiro commented Jun 29, 2022

hbelmiro commented Feb 15, 2024


		interface StateValidator {

		List<ValidationError> validate(Workflow workflow, State state);

New validations implemented #199

New validations implemented #199

Conversation

hbelmiro commented May 9, 2022 • edited

Validations Implemented

Final proposal

Initial proposal (not approved)

tsurdilo commented May 9, 2022

manick02 commented May 10, 2022

hbelmiro commented May 12, 2022

tsurdilo commented May 12, 2022

ricardozanini left a comment

Choose a reason for hiding this comment

tsurdilo commented May 12, 2022 • edited

hbelmiro commented May 12, 2022

hbelmiro commented May 12, 2022

tsurdilo commented May 12, 2022 • edited

hbelmiro commented May 13, 2022

fjtirado left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjtirado May 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbelmiro commented May 24, 2022

tsurdilo commented May 27, 2022 • edited

hbelmiro commented May 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbelmiro commented Jun 29, 2022

hbelmiro commented Feb 15, 2024

hbelmiro commented May 9, 2022 •

edited

tsurdilo commented May 12, 2022 •

edited

tsurdilo commented May 12, 2022 •

edited

fjtirado May 24, 2022 •

edited

tsurdilo commented May 27, 2022 •

edited