From 9dfd33fbf7805bb5d6fb85a398a67439edd4b761 Mon Sep 17 00:00:00 2001 From: Manuel Mauky Date: Wed, 15 Jun 2016 16:49:13 +0200 Subject: [PATCH] Fix for #398: CompositeValidator now correctly handles identical ValidationMessages of different validators. --- .../validation/CompositeValidationResult.java | 75 ------------ .../validation/CompositeValidationStatus.java | 103 +++++++++++++++++ .../utils/validation/CompositeValidator.java | 74 ++++++++++-- .../utils/validation/ValidationStatus.java | 25 +++- .../validation/CompositeValidatorTest.java | 107 +++++++++++++++++- 5 files changed, 290 insertions(+), 94 deletions(-) delete mode 100644 mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/CompositeValidationResult.java create mode 100644 mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/CompositeValidationStatus.java diff --git a/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/CompositeValidationResult.java b/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/CompositeValidationResult.java deleted file mode 100644 index 15be9e249..000000000 --- a/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/CompositeValidationResult.java +++ /dev/null @@ -1,75 +0,0 @@ -/******************************************************************************* - * Copyright 2015 Alexander Casall, Manuel Mauky - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - ******************************************************************************/ -package de.saxsys.mvvmfx.utils.validation; - -import javafx.beans.property.ListProperty; -import javafx.beans.property.SimpleListProperty; -import javafx.collections.FXCollections; -import javafx.collections.ListChangeListener; - -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -/** - * @author manuel.mauky - */ -class CompositeValidationResult extends ValidationStatus { - - private ListProperty subResults = new SimpleListProperty<>(FXCollections.observableArrayList()); - - private Map> changeListeners = new HashMap<>(); - - public CompositeValidationResult() { - subResults.addListener((ListChangeListener) c -> { - while (c.next()) { - c.getAddedSubList().forEach(result -> { - CompositeValidationResult.this.addMessage(result.getMessages()); - - final ListChangeListener changeListener = change -> { - while (change.next()) { - change.getAddedSubList().forEach(CompositeValidationResult.this::addMessage); - change.getRemoved().forEach(CompositeValidationResult.this::removeMessage); - } - }; - result.getMessages().addListener(changeListener); - changeListeners.put(result, changeListener); - }); - c.getRemoved().forEach(result -> { - CompositeValidationResult.this.removeMessage(result.getMessages()); - result.getMessages().removeListener(changeListeners.get(result)); - }); - } - }); - } - - void addResults(ValidationStatus... results) { - subResults.addAll(results); - } - - void addResults(List results) { - subResults.addAll(results); - } - - void removeResults(ValidationStatus... results) { - subResults.removeAll(results); - } - - void removeResults(List results) { - subResults.removeAll(results); - } - -} diff --git a/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/CompositeValidationStatus.java b/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/CompositeValidationStatus.java new file mode 100644 index 000000000..7a8133623 --- /dev/null +++ b/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/CompositeValidationStatus.java @@ -0,0 +1,103 @@ +/******************************************************************************* + * Copyright 2015 Alexander Casall, Manuel Mauky + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + ******************************************************************************/ +package de.saxsys.mvvmfx.utils.validation; + +import javafx.beans.property.ListProperty; +import javafx.beans.property.SimpleListProperty; +import javafx.collections.FXCollections; +import javafx.collections.ListChangeListener; + +import java.util.*; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +/** + * This class is used as {@link ValidationStatus} for {@link CompositeValidator}. + * + * In contrast to the basic {@link ValidationStatus} this class not only tracks + * {@link ValidationMessage} alone but also keeps track of the {@link Validator}s that + * the messages belong to. This is needed to be able to remove only those messages for + * a specific validator. + * + * @author manuel.mauky + */ +class CompositeValidationStatus extends ValidationStatus { + + /** + * This wrapper class is used to additionally store the information of the validator + * that the messages belong to. + * + * Instead of storing the validator instance itself we only store an {@link System#identityHashCode(Object)} + * because we don't need the validator itself but only a way to distinguish validator instances. + * Using an identity hashcode instead of the actual instance can minimize the possibility of memory leaks. + */ + private static class CompositeValidationMessageWrapper extends ValidationMessage { + + private Integer validatorCode; + + CompositeValidationMessageWrapper(ValidationMessage base, Validator validator) { + super(base.getSeverity(), base.getMessage()); + this.validatorCode = System.identityHashCode(validator); + } + + Integer getValidatorCode() { + return validatorCode; + } + } + + + void addMessage(Validator validator, List messages) { + /* + Instead of adding the messages directly to the message list ... + */ + getMessagesInternal().addAll( + messages.stream() + // ... we wrap them to keep track of the used validator. + .map(message -> new CompositeValidationMessageWrapper(message, validator)) + .collect(Collectors.toList())); + } + + /* + Remove all given messages for the given validator. + */ + void removeMessage(Validator validator, List messages) { + List messagesToRemove = + getMessagesInternal().stream() + .filter(messages::contains) // only the given messages + .filter(message -> (message instanceof CompositeValidationMessageWrapper)) + .map(message -> (CompositeValidationMessageWrapper) message) + .filter(message -> message.getValidatorCode().equals(System.identityHashCode(validator))) + .collect(Collectors.toList()); + + getMessagesInternal().removeAll(messagesToRemove); + } + + /* + * Remove all messages for this particular validator. + */ + void removeMessage(Validator validator) { + getMessagesInternal().removeIf(validationMessage -> { + if(validationMessage instanceof CompositeValidationMessageWrapper) { + CompositeValidationMessageWrapper wrapper = (CompositeValidationMessageWrapper) validationMessage; + + return wrapper.getValidatorCode().equals(System.identityHashCode(validator)); + } + + return false; + }); + } + +} diff --git a/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/CompositeValidator.java b/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/CompositeValidator.java index 167f42354..8c41c33f4 100644 --- a/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/CompositeValidator.java +++ b/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/CompositeValidator.java @@ -15,8 +15,14 @@ ******************************************************************************/ package de.saxsys.mvvmfx.utils.validation; -import java.util.stream.Collectors; -import java.util.stream.Stream; +import javafx.beans.property.ListProperty; +import javafx.beans.property.SimpleListProperty; +import javafx.collections.FXCollections; +import javafx.collections.ListChangeListener; +import javafx.collections.ObservableList; + +import java.util.HashMap; +import java.util.Map; /** * This {@link Validator} implementation is used to compose multiple other validators. @@ -27,31 +33,75 @@ * @author manuel.mauky */ public class CompositeValidator implements Validator { - - private CompositeValidationResult validationStatus = new CompositeValidationResult(); - + + CompositeValidationStatus status = new CompositeValidationStatus(); + + private ListProperty validators = new SimpleListProperty<>(FXCollections.observableArrayList()); + private Map> listenerMap = new HashMap<>(); + + public CompositeValidator() { + + validators.addListener(new ListChangeListener() { + @Override + public void onChanged(Change c) { + while(c.next()) { + + // When validators are added... + c.getAddedSubList().forEach(validator -> { + + ObservableList messages = validator.getValidationStatus().getMessages(); + // ... we first add all existing messages to our own validator messages. + status.addMessage(validator, messages); + + final ListChangeListener changeListener = change -> { + while(change.next()) { + // add/remove messages for this particular validator + status.addMessage(validator, change.getAddedSubList()); + status.removeMessage(validator, change.getRemoved()); + } + }; + + validator.getValidationStatus().getMessages().addListener(changeListener); + + // keep a reference to the listener for a specific validator so we can later use + // this reference to remove the listener + listenerMap.put(validator, changeListener); + }); + + + c.getRemoved().forEach(validator -> { + status.removeMessage(validator); + + if(listenerMap.containsKey(validator)){ + ListChangeListener changeListener = listenerMap.get(validator); + + validator.getValidationStatus().getMessages().removeListener(changeListener); + listenerMap.remove(validator); + } + }); + } + } + }); + } public CompositeValidator(Validator... validators) { + this(); // before adding the validators we need to setup the listeners in the default constructor addValidators(validators); } public void addValidators(Validator... validators) { - validationStatus.addResults(Stream.of(validators) - .map(Validator::getValidationStatus) - .collect(Collectors.toList())); + this.validators.addAll(validators); } public void removeValidators(Validator... validators) { - validationStatus.removeResults(Stream.of(validators) - .map(Validator::getValidationStatus) - .collect(Collectors.toList())); + this.validators.removeAll(validators); } @Override public ValidationStatus getValidationStatus() { - return validationStatus; + return status; } } diff --git a/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/ValidationStatus.java b/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/ValidationStatus.java index 7a268a5c9..0a3b64ca1 100644 --- a/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/ValidationStatus.java +++ b/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/validation/ValidationStatus.java @@ -43,7 +43,12 @@ public class ValidationStatus { new FilteredList<>(messages, message -> message.getSeverity().equals(Severity.ERROR))); private ObservableList warningMessages = FXCollections.unmodifiableObservableList( new FilteredList<>(messages, message -> message.getSeverity().equals(Severity.WARNING))); - + + + ListProperty getMessagesInternal() { + return messages; + } + void addMessage(ValidationMessage message) { messages.add(message); @@ -64,16 +69,26 @@ void removeMessage(Collection messages) { void clearMessages() { messages.clear(); } - - + + + /** + * @return an unmodifiable observable list of all messages. + */ public ObservableList getMessages() { return unmodifiableMessages; } - + + + /** + * @return an unmodifiable observable list of all messages of severity {@link Severity#ERROR}. + */ public ObservableList getErrorMessages() { return errorMessages; } - + + /** + * @return an unmodifiable observable list of all messages of severity {@link Severity#WARNING}. + */ public ObservableList getWarningMessages() { return warningMessages; } diff --git a/mvvmfx/src/test/java/de/saxsys/mvvmfx/utils/validation/CompositeValidatorTest.java b/mvvmfx/src/test/java/de/saxsys/mvvmfx/utils/validation/CompositeValidatorTest.java index 6358bb9bc..0bca51a22 100644 --- a/mvvmfx/src/test/java/de/saxsys/mvvmfx/utils/validation/CompositeValidatorTest.java +++ b/mvvmfx/src/test/java/de/saxsys/mvvmfx/utils/validation/CompositeValidatorTest.java @@ -17,6 +17,7 @@ import javafx.beans.property.BooleanProperty; import javafx.beans.property.SimpleBooleanProperty; +import javafx.beans.property.SimpleStringProperty; import org.junit.Before; import org.junit.Test; @@ -50,6 +51,19 @@ public void setup() { compositeValidator = new CompositeValidator(); status = compositeValidator.getValidationStatus(); } + + /** + * Test that the composite validator also works when the base validators are passed to the constructor. + */ + @Test + public void testValidatorConstructor() { + valid1.setValue(false); + valid2.setValue(false); + + CompositeValidator newValidator = new CompositeValidator(validator1, validator2); + assertThat(newValidator.getValidationStatus().isValid()).isFalse(); + assertThat(newValidator.getValidationStatus().getMessages()).hasSize(2); + } @Test public void testValidation() { @@ -103,8 +117,97 @@ public void testLazyRegistration() { compositeValidator.removeValidators(validator1); assertThat(status.isValid()).isTrue(); } - - + + + /** + * This test is used to verify a previously existing bug + * (398) is fixed. + * + * The wrong behaviour was happening when two validators where using the a ValidationMessage + * with the same values (for example {@link Severity#ERROR} and a string message "error"). + * In this case when one of the validators was removed the message for the second validator + * was also gone. + */ + @Test + public void testUpdateResultWhenValidatorsAreRemoved() { + final SimpleStringProperty stringPropertyOne = new SimpleStringProperty(""); + + final FunctionBasedValidator validatorOne = new FunctionBasedValidator<>(stringPropertyOne, s -> { + if (s.isEmpty()) { + return new ValidationMessage(Severity.ERROR, "empty1"); + } else { + return null; + } + }); + + final SimpleStringProperty stringPropertyTwo = new SimpleStringProperty(""); + + final FunctionBasedValidator validatorTwo = new FunctionBasedValidator<>(stringPropertyTwo, s -> { + if (s.isEmpty()) { + return new ValidationMessage(Severity.ERROR, "empty1"); + } else { + return null; + } + }); + + final CompositeValidator compositeValidator = new CompositeValidator(); + + + assertThat(compositeValidator.getValidationStatus().isValid()).isTrue(); + assertThat(compositeValidator.getValidationStatus().getErrorMessages()).isEmpty(); + + + compositeValidator.addValidators(validatorOne); + + assertThat(compositeValidator.getValidationStatus().isValid()).isFalse(); + assertThat(compositeValidator.getValidationStatus().getErrorMessages()).hasSize(1); + + compositeValidator.addValidators(validatorTwo); + + assertThat(compositeValidator.getValidationStatus().isValid()).isFalse(); + assertThat(compositeValidator.getValidationStatus().getErrorMessages()).hasSize(2); + + + compositeValidator.removeValidators(validatorTwo); + assertThat(compositeValidator.getValidationStatus().isValid()).isFalse(); + assertThat(compositeValidator.getValidationStatus().getErrorMessages()).hasSize(1); + + + compositeValidator.removeValidators(validatorOne); + assertThat(compositeValidator.getValidationStatus().isValid()).isTrue(); + assertThat(compositeValidator.getValidationStatus().getErrorMessages()).isEmpty(); + } + + /** + * Verify that it's easy possible to assert on existing validation messages + * by creating new ValidationMessage instances with the expected values on the fly. + * This is needed to write useful unit tests for users. + */ + @Test + public void testTestability() { + + final SimpleStringProperty stringPropertyOne = new SimpleStringProperty(""); + + final FunctionBasedValidator validatorOne = new FunctionBasedValidator<>(stringPropertyOne, s -> { + if (s.isEmpty()) { + return new ValidationMessage(Severity.ERROR, "empty1"); + } else { + return null; + } + }); + + + CompositeValidator compositeValidator = new CompositeValidator(); + compositeValidator.addValidators(validatorOne); + + // It's possible to write an assert for the existing of a test by creating a new validation message object with the same values. + // This makes writing unit tests easier and more readable. + assertThat(compositeValidator.getValidationStatus().getMessages()).contains(ValidationMessage.error("empty1")); + + + } + + private List asStrings(List messages) { return messages .stream()