Make Binder.setReadonly ignore effectively readonly bindings (#10368)

Binder.setReadonly will ignore bindings with null setter
or property without an accessible setter when changing
field read only flag.

Fixes #10252

3 files changed, 56 insertions, 5 deletions

diff --git a/server/src/main/java/com/vaadin/data/Binder.java b/server/src/main/java/com/vaadin/data/Binder.java
index a443421a0f..26354138ce 100644
--- a/server/src/main/java/com/vaadin/data/Binder.java
+++ b/server/src/main/java/com/vaadin/data/Binder.java
@@ -33,6 +33,7 @@ import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.BiFunction;
+import java.util.logging.Logger;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -762,10 +763,10 @@ public class Binder<BEAN> implements Serializable {
                                     + " from " + getBinder().propertySet));
 
             ValueProvider<BEAN, ?> getter = definition.getGetter();
-            Setter<BEAN, ?> setter = definition.getSetter()
-                    .orElse((bean, value) -> {
-                        // Setter ignores value
-                    });
+            Setter<BEAN, ?> setter = definition.getSetter().orElse(null);
+            if (setter == null) {
+                getLogger().fine(() -> propertyName + " does not have an accessible setter");
+            }
 
             BindingBuilder<BEAN, ?> finalBinding = withConverter(
                     createConverter(definition.getType()), false);
@@ -2287,7 +2288,9 @@ public class Binder<BEAN> implements Serializable {
      *            write
      */
     public void setReadOnly(boolean fieldsReadOnly) {
-        getBindings().stream().map(BindingImpl::getField)
+        getBindings().stream()
+            .filter(binding -> Objects.nonNull(binding.setter))
+            .map(BindingImpl::getField)
                 .forEach(field -> field.setReadOnly(fieldsReadOnly));
     }
 
@@ -2740,4 +2743,9 @@ public class Binder<BEAN> implements Serializable {
         Optional.ofNullable(boundProperties.get(propertyName))
                 .ifPresent(Binding::unbind);
     }
+
+    private static final Logger getLogger() {
+        return Logger.getLogger(Binder.class.getName());
+    }
+
 }
diff --git a/server/src/test/java/com/vaadin/data/BeanBinderTest.java b/server/src/test/java/com/vaadin/data/BeanBinderTest.java
index 45610754d5..567bdcc3e8 100644
--- a/server/src/test/java/com/vaadin/data/BeanBinderTest.java
+++ b/server/src/test/java/com/vaadin/data/BeanBinderTest.java
@@ -311,6 +311,24 @@ public class BeanBinderTest
     }
 
     @Test
+    public void setReadonlyShouldIgnoreBindingsForReadOnlyProperties() {
+        binder.bind(nameField, "readOnlyProperty");
+
+        binder.setReadOnly(true);
+        assertFalse("Name field should be ignored and not be readonly", nameField.isReadOnly());
+
+        binder.setReadOnly(false);
+        assertFalse("Name field should be ignored and not be readonly", nameField.isReadOnly());
+
+        nameField.setReadOnly(true);
+        binder.setReadOnly(true);
+        assertTrue("Name field should be ignored and be readonly", nameField.isReadOnly());
+
+        binder.setReadOnly(false);
+        assertTrue("Name field should be ignored and be readonly", nameField.isReadOnly());
+    }
+
+    @Test
     public void beanBound_setInvalidFieldValue_validationError() {
         binder.setBean(item);
         binder.bind(nameField, "firstname");
diff --git a/server/src/test/java/com/vaadin/data/BinderTest.java b/server/src/test/java/com/vaadin/data/BinderTest.java
index 36301afcfa..5c6c9657a5 100644
--- a/server/src/test/java/com/vaadin/data/BinderTest.java
+++ b/server/src/test/java/com/vaadin/data/BinderTest.java
@@ -609,6 +609,31 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
     }
 
     @Test
+    public void setReadonlyShouldIgnoreBindingsWithNullSetter() {
+        binder.bind(nameField, Person::getFirstName, null);
+        binder.forField(ageField)
+            .withConverter(new StringToIntegerConverter(""))
+            .bind(Person::getAge, Person::setAge);
+
+        binder.setReadOnly(true);
+        assertFalse("Name field should be ignored and not be readonly", nameField.isReadOnly());
+        assertTrue("Age field should be readonly", ageField.isReadOnly());
+
+        binder.setReadOnly(false);
+        assertFalse("Name field should be ignored and remain not readonly", nameField.isReadOnly());
+        assertFalse("Age field should not be readonly", ageField.isReadOnly());
+
+        nameField.setReadOnly(false);
+        binder.setReadOnly(false);
+        assertFalse("Name field should be ignored and remain not readonly", nameField.isReadOnly());
+        assertFalse("Age field should not be readonly", ageField.isReadOnly());
+
+        binder.setReadOnly(true);
+        assertFalse("Name field should be ignored and remain not readonly", nameField.isReadOnly());
+        assertTrue("Age field should be readonly", ageField.isReadOnly());
+    }
+
+    @Test
     public void isValidTest_bound_binder() {
         binder.forField(nameField)
                 .withValidator(Validator.from(