Invalidation Listener for FunctionBasedValidator

[gtnarg]

To enable validation logic to fire when dependencies of the source ObservableValue are invalidated for a FunctionBasedValidator, it would be useful to be able to add an InvalidationListener as below:

    private FunctionBasedValidator(ObservableValue<T> source) {
        source.addListener((observable, oldValue, newValue) -> {
            validate(newValue);
        });
        source.addListener( (InvalidationListener) (evt -> validate( source.getValue() )));
    }

I have tried extending the class but have no access to the validate method.

I tried making my own ValidationClass re-using the existing logic, but the ValidationStatus class hides #addMessage and #clearMessage.

Thanks,
Grant

[manuel-mauky]

Hi,
can you show a simple example where this is needed? In which situations is the current implementation not sufficient?

[gtnarg]

I'm attempting to do cross field validation. The primary observable isn't changing but another dependent one is.

Depending on the value of the second observable (i.e. field), the validation error changes from an WARNING to ERROR. Because the primary observable isn't changing, the validation never refires...

Let me know if you need more information.

[manuel-mauky]

Ok I understand your use case.
In the code you've provided, the validate is invoked two times for a single change isn't it?
Wouldn't it be better to only have one InvalidationListener? Like this:

private FunctionBasedValidator(ObservableValue<T> source) {
    source.addListener( (InvalidationListener) (evt -> validate( source.getValue() )));
}

I'm not sure if there are any negative implications with this approach but would this solve your use case?

[gtnarg]

Yes that would solve my issue. Be aware, that you would need to make the source an instance variable of the validator otherwise the invalidation listener will get garbage collected.

In my usecase I was actually passing in a Binding that had dependencies. If you don't add the observable to the dependencies then the listener wouldn't get called twice. (i.e. Bindings.createObjectBinding( obs, dep1, dep2 ) )

new FunctionBasedValidator( Bindings.createObjectBinding( obs, dep1, dep2), myFunction )

[gtnarg]

I can report that one invalidation listener works well for my usecase.

I did run into an issue with CompositeValidator that "can" be an issue.

The following will create a validation message that gets stuck (i.e. the validator cannot remove it)

CompositeValidator validator = new CompositeValidator();

StringProperty property = new SimpleStringProperty("NO");

ObjectBinding binding = Bindings.createObjectBinding( ()-> property.getValue(), property, validator.getValidationStatus().validProperty() );

Validator validation = new FunctionBasedValidator<String>( binding, 
        value -> {
            if( !"YES".equals( value ) ){
                return ValidationMessage.error("Validation Failed");
            } else {
                return null;
            }
        } );

validator.addValidators(validation);
validator.getValidationStatus().getMessages().forEach( msg -> System.out.println( msg ) );


System.out.println( "======================================");

property.setValue("YES");
// should NOT print out any messages
validator.getValidationStatus().getMessages().forEach( msg -> System.out.println( msg ) );

I ran into this scenario and it took a long time to figure out why it was occurring.

The following changes to CompositeValidator prevent it:

validator.getValidationStatus().getMessages().addListener(changeListener);

ObservableList<ValidationMessage> messages = validator.getValidationStatus().getMessages();
// ... we first add all existing messages to our own validator messages.
status.addMessage(validator, messages);

Addition of existing messages are done AFTER the changelistener is added.

To prevent recursive validation I changed FunctionBasedValidator#validate(T newValue) to

    boolean validating = false;

    private void validate(T newValue) {
        if( validating ) return;
        try {
            validating = true;
            validationStatus.clearMessages();
            Optional<ValidationMessage> message = validateFunction.apply(newValue);
            message.ifPresent(validationStatus::addMessage);
        } finally {
            validating = false;
        }
    }

[gtnarg]

Any feedback on my findings with CompositeValidator?

[manuel-mauky]

Sorry for the delay. I have to look at this evening

[manuel-mauky]

I've played around a little with the validator but I still have problems to reproduce your original request on why the invalidation listener is needed. For example I've created this test case which uses a Binding with two dependencies as argument to the FunctionBasedValidator and it works just well:

import static org.assertj.core.api.Assertions.assertThat;
...

@Test
public void test() {
	StringProperty propertyA = new SimpleStringProperty("");
	StringProperty propertyB = new SimpleStringProperty("");

	StringBinding concat = Bindings.createStringBinding(() -> {
		String a = propertyA.getValue();
		String b = propertyB.getValue();

		return a + b;
	}, propertyA, propertyB);

	Validator validator = new FunctionBasedValidator<>(concat, value -> {
		if(!Objects.equals(value, "NO")) {
			return ValidationMessage.error("Failed");
		} else {
			return null;
		}
	});

	ValidationStatus validationStatus = validator.getValidationStatus();

	assertThat(validationStatus.getMessages()).hasSize(1);

	propertyA.setValue("N");
	assertThat(validationStatus.getMessages()).hasSize(1);

	propertyB.setValue("O");
	assertThat(validationStatus.getMessages()).isEmpty();

	propertyA.setValue("Y");
	assertThat(validationStatus.getMessages()).hasSize(1);
}

Can you point out where my misunderstanding is? Or can you give me a failing unit test that shows the problem?

On your findings with CompositeValidator: I can reproduce the behaviour but, to be honest, I don't really understand them. ;-)

I haven't thought that switching to InvalidationListener would make such a big difference in the behaviour. Can you try to describe what's happening and why it doesn't work?

[gtnarg]

I will address the first part of your question about why an invalidation listener is preferred. I have modified your test case below:

@Test
public void test1() {

	StringProperty propertyA = new SimpleStringProperty("");
	StringProperty propertyB = new SimpleStringProperty("");
	StringProperty propertyC = new SimpleStringProperty("");

	StringBinding concat = Bindings.createStringBinding(() -> {
		String a = propertyA.getValue();
		String b = propertyB.getValue();

		return a + b;
	}, propertyA, propertyB, propertyC );

	Validator validator = new FunctionBasedValidator<>(concat, value -> {
		if(!Objects.equals(value, "NO")) {
			if( "Y".equals( propertyC.getValue() )){
				return ValidationMessage.warning("Warning - Failed");
			} else {
				return ValidationMessage.error("Error - Failed");
			}
		} else {
			return null;
		}
	});

	ValidationStatus validationStatus = validator.getValidationStatus();

	assertEquals( 1, validationStatus.getMessages().size() );

	propertyA.setValue("N");
	assertEquals( 1, validationStatus.getMessages().size() );
	assertEquals( Severity.ERROR, validationStatus.getHighestMessage().get().getSeverity());
	
	propertyC.setValue("Y");
	assertEquals( Severity.WARNING, validationStatus.getHighestMessage().get().getSeverity());

}

I suppose the concat binding could return a Map of all observable values (or a holder object) so that the value used by the validation will always change if any of the dependencies change?

@Test
public void test2() {

	StringProperty propertyA = new SimpleStringProperty("");
	StringProperty propertyB = new SimpleStringProperty("");
	StringProperty propertyC = new SimpleStringProperty("");

	ObjectBinding<Map<String,String>> concat = Bindings.createObjectBinding(() -> {
		String a = propertyA.getValue();
		String b = propertyB.getValue();
		String c = propertyC.getValue();
		
		Map<String,String> map = new HashMap<String,String>();
		map.put("a", a );
		map.put("b", b );
		map.put("c", c );

		return map;
	}, propertyA, propertyB, propertyC );

	Validator validator = new FunctionBasedValidator<Map<String,String>>(concat, value -> {
		
		if(!Objects.equals( value.get("a") + value.get("b"), "NO")) {
			if( "Y".equals( value.get("c") )){
				return ValidationMessage.warning("Warning - Failed");
			} else {
				return ValidationMessage.error("Error - Failed");
			}
		} else {
			return null;
		}
	});

	ValidationStatus validationStatus = validator.getValidationStatus();

	assertEquals( 1, validationStatus.getMessages().size() );

	propertyA.setValue("N");
	assertEquals( 1, validationStatus.getMessages().size() );
	assertEquals( Severity.ERROR, validationStatus.getHighestMessage().get().getSeverity());
	
	propertyC.setValue("Y");
	assertEquals( Severity.WARNING, validationStatus.getHighestMessage().get().getSeverity());

}

The second option "feels" a bit like I'm working around the framework (but I'm open to feedback on that).

If you still see the benefit of an invalidation listener, I can explain the issue with CompositeValidator further.

[manuel-mauky]

For me this kind of looks like a hack with the FunctionBasedValidator. A function should only depend on it's input values (in this case the lambda parameter value) but you are additionally using propertyC.getValue() inside of the function.
And your intuition is correct: We would need the additional parameter as function argument like in your second example. Only problem is that Java isn't a good functional language and this style of programming looks ugly in Java.

My conclusion is that your use case is perfectly valid but the FunctionBasedValidator isn't the right tool for it. We should keep the FunctionBasedValidator as it is and instead think about how we can implement your use case either by using on (or a combination of) the other validator types or introducing a new validator type for this kind of use cases.

For example we could extend the ObservableRuleBasedValidator. At the moment it takes ObservableValue<Boolean> as arguments and when one of the observable is false it shows a validation error. We could introduce the possibility to add ObservableValue<ValidationMessage> like this:

StringProperty propertyA = new SimpleStringProperty("");
StringProperty propertyB = new SimpleStringProperty("");
StringProperty propertyC = new SimpleStringProperty("");

StringBinding concat = Bindings.createStringBinding(() -> {
	String a = propertyA.getValue();
	String b = propertyB.getValue();

	return a + b;
}, propertyA, propertyB);

ObservableRuleBasedValidator validator = new ObservableRuleBasedValidator();

ObjectBinding<ValidationMessage> rule = Bindings.createObjectBinding(() -> {
	if (!Objects.equals(concat.get(), "NO")) {
		if ("Y".equals(propertyC.getValue())) {
			return ValidationMessage.warning("Warning - Failed");
		} else {
			return ValidationMessage.error("Error - Failed");
		}
	} else {
		return null;
	}
});

validator.addRule(rule);

Notice that I've removed the propertyC from the concat binding as it's not directly used. It is however a dependency of the rule. This way you also have a cleaner separation between these aspects.

What do you think of this API?

Regarding the CompositeValidator: I think the source of this problem is that you have the validation status as dependency of the validator. This way it's like a cyclic dependency.
This sounds like a bad choice but I'm not sure if there are use cases where this can't be avoided.
On the other hand: The CompositeValidator shouldn't depend on the actual implementation of the child validators so from this perspective it looks like a bug and should be fixed.

[gtnarg]

I think you hit the nail on the head! The only reason I used FunctionBasedValidator is so that I could create my own validation message. The suggested API looks good :)

You analyzed the problem with the CompositeValidator correctly although it only occurs with an invalidation listener with a cyclic dependency but the fix is simple as described above (add existing messages are done AFTER the changelistener is added).

[manuel-mauky]

I've extended the ObservableRulesBasedValidator as described above. See PR #452.
Can you try it out and let me know what you think. If this works as expected I will merge the PR.

I still need to update the documentation in the wiki and need to fix the problem with the cyclic validation.

[gtnarg]

I needed to modify the following to make it work correctly:

    public void addRule(ObservableValue<ValidationMessage> rule) {
        complexRules.add(rule);

        rule.addListener((observable, oldValue, newValue) -> {
            //if(newValue == null) {
            //   validationStatus.removeMessage(oldValue);
            //} else {
            //    validationStatus.addMessage(newValue);
            //}
        	if( oldValue != null ){
    			validationStatus.removeMessage( oldValue );
    		}
    		if( newValue != null){
    			validationStatus.addMessage( newValue );
    		}
        });

        if(rule.getValue() != null) {
            validationStatus.addMessage(rule.getValue());
        }
    }

In the situations where the status of the message changed, the old message was not getting removed.

Also, it would be nice to have a helper method (or an overloaded constructor) so that I can define the rule in one line:

// would require addRule() to return the validator	
ObservableRuleBasedValidator.instance().addRule( myRule );
	
OR
	
new ObservableRuleBasedValidator( myRule );

Thanks,
Grant

[manuel-mauky]

Hi Grant,

I've changed the implementation according to your comment. And I've added a constructor for convenience. You can directly add one or multiple rules.

Thanks for the good ideas and for testing.

[gtnarg]

Did the problem with cyclic validation get fixed under this? I put quite a bit of effort into tracking it down when it occurred for me. I would hate for it to get lost :(

[manuel-mauky]

Hi Grant,

sorry, I've missed this. I've created a new issue #457 and have created a test case that can reproduce this issue. As none of our existing validators are leading to this issue I've created a new hypothetical validator with an InvalidationListener and created a binding that has a circular dependency.

However, the proposed fix for CompositeValidator (adding the listener first) don't seem to be enough (or I'm overseeing something) as the test case is still red.