[Signals] ValueSignal with unmodifiable default leads to error on restore / page reload

[stefanuebe]

trafficstars

Description of the bug

I created a simple ValueSignal<Set<>> to store a collection of items and initialized it with an empty set using the factory method with the default value. When reloading the page, I get a jackson deserialization exception.

Note: the factory method with the class parameter works fine in this scenario.

Caused by: java.lang.RuntimeException: com.fasterxml.jackson.databind.JsonMappingException: (was java.lang.UnsupportedOperationException) (through reference chain: java.util.Collections$EmptySet[0])
	at com.vaadin.signals.Signal.fromJson(Signal.java:526) ~[signals-24.8.0.rc1.jar:24.8.0.rc1]
	at com.vaadin.signals.Signal.nodeValue(Signal.java:545) ~[signals-24.8.0.rc1.jar:24.8.0.rc1]
	at com.vaadin.signals.ValueSignal.extractValue(ValueSignal.java:115) ~[signals-24.8.0.rc1.jar:24.8.0.rc1]
	at com.vaadin.signals.Signal.peek(Signal.java:151) ~[signals-24.8.0.rc1.jar:24.8.0.rc1]
	at com.vaadin.signals.SignalFactory.value(SignalFactory.java:104) ~[signals-24.8.0.rc1.jar:24.8.0.rc1]
	at com.vaadin.signals.SignalFactory.value(SignalFactory.java:132) ~[signals-24.8.0.rc1.jar:24.8.0.rc1]
	at net.azaberlin.test.AbstractTestView.<init>(AbstractTestView.java:25) ~[classes/:na]
	at net.azaberlin.test.AbstractTestView$TestView1.<init>(AbstractTestView.java:69) ~[classes/:na]
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[na:na]
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77) ~[na:na]
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) ~[na:na]
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500) ~[na:na]
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481) ~[na:na]
	at org.springframework.beans.BeanUtils.instantiateClass(BeanUtils.java:196) ~[spring-beans-6.2.5.jar:na]
	... 90 common frames omitted
Caused by: com.fasterxml.jackson.databind.JsonMappingException: (was java.lang.UnsupportedOperationException) (through reference chain: java.util.Collections$EmptySet[0])
	at com.fasterxml.jackson.databind.JsonMappingException.wrapWithPath(JsonMappingException.java:401) ~[jackson-databind-2.18.3.jar:2.18.3]
	at com.fasterxml.jackson.databind.JsonMappingException.wrapWithPath(JsonMappingException.java:372) ~[jackson-databind-2.18.3.jar:2.18.3]
	at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer._deserializeFromArray(CollectionDeserializer.java:381) ~[jackson-databind-2.18.3.jar:2.18.3]
	at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.deserialize(CollectionDeserializer.java:246) ~[jackson-databind-2.18.3.jar:2.18.3]
	at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.deserialize(CollectionDeserializer.java:30) ~[jackson-databind-2.18.3.jar:2.18.3]
	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:342) ~[jackson-databind-2.18.3.jar:2.18.3]
	at com.fasterxml.jackson.databind.ObjectMapper._readValue(ObjectMapper.java:4904) ~[jackson-databind-2.18.3.jar:2.18.3]
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3036) ~[jackson-databind-2.18.3.jar:2.18.3]
	at com.fasterxml.jackson.databind.ObjectMapper.treeToValue(ObjectMapper.java:3500) ~[jackson-databind-2.18.3.jar:2.18.3]
	at com.vaadin.signals.Signal.fromJson(Signal.java:523) ~[signals-24.8.0.rc1.jar:24.8.0.rc1]
	... 103 common frames omitted
Caused by: java.lang.UnsupportedOperationException: null
	at java.base/java.util.AbstractCollection.add(AbstractCollection.java:251) ~[na:na]
	at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer._deserializeFromArray(CollectionDeserializer.java:369) ~[jackson-databind-2.18.3.jar:2.18.3]
	... 110 common frames omitted

Expected behavior

Based on the signal factory method docs, that says, that

If the implementation returns an existing signal instance that already had a non-null value, then the provided default value is ignored I would not expect and exception. Either that behavior should be improved or the docs need to clarify, what the default value is used for.

It seems like the factory method takes the default value as a base for the deserialization. When I pass a new HashSet instead, it works.

Minimal reproducible example

Create a new 24.8. app and enable signals.

Add a view and add there the following code:

public class YourView ... {
    private final ValueSignal<Set<String>> sSelection = SignalFactory.IN_MEMORY_SHARED.value("selection", Collections.emptySet());

    public YourView() {
        sSelection.value(Set.of("Hello"));
    }
}

Open the view. Reload the page. The exception should pop up.

Versions

• Vaadin / Flow version: 24.8.0.rc1
• Java version: 17
• OS version: Windows

[Legioth]

I suspect that the problem in this case is that Collections.emptySet().getClass() returns a class that Jackson cannot deserialize (since that's the internal implementation class used by Collections.emptySet() rather than Set.class). This means that any attempt to read the value of the signal instance will fail and it's just a coincidence that this in your case happened through the peek() that happened when the factory method was run again at page reload.

The shorthand factory method that accepts only a default value is not appropriate for cases where the default value is not a "proper" representative of the value type. I don't think we can do anything else to deal with this other than eagerly trying to deserialize the provided value with the detected class so that the exception at least in this case would be thrown eagerly rather than only when reloading.

On the other hand, your case is relatively broken anyways since there's no way of expressing Set<String> as a Class. This in turn means that even though we would get Set.class correctly, there would still not be anything that tells Jackson that the item type should be String.

I would instead recommend using ListSignal<String> for tracking selection.

[stefanuebe]

I would instead recommend using ListSignal<String> for tracking selection.

At least for now that seems not like a suitable replacement. Beside the currently missing "batch operations", that allow setting a collection into the ListSignal, there is the disadvantage of set being not ordered by nature. This means, that the dev must always sort the selected items first, then convert them to a list and then set them to the list signal (or use a mass call of insertXyz).

While this might be doable for cases, where the used data type is known and therefore the id can be hopefully used to sort the data, it will be harder to implement that for generic cases (where the data type of the grid might be not known or is not easy sortable).

[mshabarov]

New SetSignal class ? 😄