feat: Add signal commands #20876

Legioth · 2025-01-17T16:31:32Z

This is not yet a complete signal implementation but only the low-level core data manipulation logic.

github-actions · 2025-01-17T16:47:44Z

Test Results

1 170 files + 5 1 170 suites +5 1h 39m 39s ⏱️ + 4m 36s
7 749 tests +124 7 691 ✅ +122 56 💤 ±0 0 ❌ ±0 2 🔥 +2
7 992 runs + 4 7 928 ✅ + 5 62 💤 - 3 0 ❌ ±0 2 🔥 +2

For more details on these errors, see this check.

Results for commit 2fbd578. ± Comparison against base commit 32c4914.

♻️ This comment has been updated with latest results.

taefi

Tried to do a fast review, except for the following bigger files, that will be reviewed in a second round:

MutableTreeRevision
TreeRevision
MutableTreeRevisionTest
TreeRevisionTest

signals/src/main/java/com/vaadin/signals/Node.java

taefi · 2025-01-20T09:39:44Z

signals/src/main/java/com/vaadin/signals/SignalCommand.java

+     * A signal command that doesn't apply any change but only performs a test
+     * that will be part of determining whether a transaction passes.
+     */
+    sealed interface TestCommand extends SignalCommand {


Maybe I'm a bit biased, but the name TestCommand (and further down ValueTest, PositionTest, and etc.) itches my mind. At first glance, it looks like a class that belongs to src/main/test, but in reality it is used to verify a transaction. Could this be named as VerifyCommand or something like that?

What about ConditionCommand for the base type and then naming the specific ones e.g. ValueCondition?

signals/src/main/java/com/vaadin/signals/Node.java

taefi · 2025-01-20T09:45:25Z

signals/src/main/java/com/vaadin/signals/Node.java

+         * @param listChildren
+         *            a list of child ids, or the an list if the node has no
+         *            list children


Same here. Though, I prefer to get rid of this almost duplicate block.

signals/src/main/java/com/vaadin/signals/ListSignal.java

taefi · 2025-01-20T11:55:15Z

signals/src/main/java/com/vaadin/signals/ListSignal.java

+        /**
+         * Gets the insertion position that corresponds to the beginning of the
+         * list.
+         *
+         * @return a list position for the beginning of the list, not
+         *         <code>null</code>
+         */
+        public static ListPosition first() {
+            // After edge
+            return new ListPosition(Id.ZERO, null);
+        }
+
+        /**
+         * Gets the insertion position that corresponds to the end of the list.
+         *
+         * @return a list position for the end of the list, not
+         *         <code>null</code>
+         */
+        public static ListPosition last() {
+            // Before edge
+            return new ListPosition(null, Id.ZERO);
+        }


Not sure if I easily understand this part:
The description of the after param says: id of the node to insert immediately after...
and description of the before param says: id of the node to insert immediately before...

What is the role of the ZERO? Is the ZERO an always existing root pointer that will store the head position (or the head == tail == ZERO in case of an empty list)?

If so, calling first() returns the before first (after the ZERO) position which makes sense. But, the last() returns before the ZERO, which I don't simply get :) Was it supposed to be Id.MAX?

Probably, something in the implementation is being optimized by this(?), but this representation seems a bit confusing.

This is a bit of (premature ?) optimization: ZERO is widely used also in other contexts and has therefore a custom JSON representation to be as compact as practically possible ("").

ZERO represents both edges of the list, i.e. both head and tail. This should be safe since they can never be mixed up with each other. Furthermore, ZERO also represents the root node but that node can never have siblings.

I agree that it can be confusing. I'm just not sure if that should be addressed by additional documentation or by removing the optimization? Or maybe just introduce an EDGE constant that refers to the same Id instance? It would probably be dangerous to introduce HEAD and TAIL constants with identical values since someone might assume them to be different?

Well, at this point, not seeing the rest of the implementation / usages makes it hard to make a practical decision about which approach to pick. Maybe I was mentioned and I forgot: is the List implementation going to be a circular linked list? In the case, it makes sense to have only one EDGE and the before / after seems to be enough.

What would first() and last() mean in a circular linked list?

I think, when I was writing the previous comment, I got confused for a sec with having one EDGE constant for both head and tail (for the before first and after last positions), vs. head and tail pointing to the same location all the time (which wasn't the case). I would say, having a separate constant such as EDGE is enough for not getting confused with the ZERO thingy. Having separate HEAD and TAIL constants with different values might not add much value.

signals/src/main/java/com/vaadin/signals/impl/OperationResult.java

taefi · 2025-01-20T12:21:25Z

signals/src/main/java/com/vaadin/signals/impl/OperationResult.java

+     * @return an accepted result if the condition is <code>true</code>, a
+     *         rejected result if the condition is <code>false</code>
+     */
+    public static OperationResult test(boolean condition,


Minor: suggesting to rename to something like verify:

Suggested change

public static OperationResult test(boolean condition,

public static OperationResult verify(boolean condition,

Or from, of, fromCondition, ofCondition.

taefi · 2025-01-20T13:10:54Z

signals/src/test/java/com/vaadin/signals/impl/OperationResultTest.java

+
+import com.vaadin.signals.Id;
+
+public class OperationResultTest {


There could be more tests here to have better coverage of all the implementations, especially verifying the Accept#onlyUpdate, and the test behaviour.

taefi

In this round I started by looking at Node (again), TreeRevision, and TreeRevisionTest classes, and I found myself a bit confused over the idea of mapChildren and listChildren of the Node, since in the TreeRevisionTest it distinctly uses different Ids for these two children collections, so decided to pause here for some clarifications before looking into the MutableTreeRevision.

Also, would it make sense to use either JSpecify or Jetbrains annotations for marking the NotNull params everywhere?

taefi · 2025-01-27T11:21:30Z

signals/src/main/java/com/vaadin/signals/SignalCommand.java

+     * The created node will be automatically removed if the owner is
+     * disconnected.
+     */
+    sealed interface ScopeOwnerCommand extends SignalCommand {


Suggested change

sealed interface ScopeOwnerCommand extends SignalCommand {

sealed interface ScopeOwnedCommand extends SignalCommand {

It has a field for the owner but it's owned only if that field has a non-null value.

"Owner" also doesn't completely feel right. To me it means like a command that owns the scope. I must admit it is a hard to name concept.

Based on the JavaDoc descriptions, the key ideas behind the interface are:

It creates a signal node.

The node might have an owner.

The node is automatically removed if the owner disconnects.

Fed this to GPT4o, and it suggests the following:

If the key focus is scoping the node to an owner, ScopedSignalCommand is a clear and concise option. If the focus is on automatic removal, OwnerBoundCommand or EphemeralSignalCommand could work well.

Would you like a name that emphasizes ownership, lifecycle behavior, or signal creation?

Another thing to consider is: if the owner is null, why the caller code still wants to issues this type of command to create the signal? In other words, why the owner is nullable?

In other words, why the owner is nullable?

Would be quite redundant to have separate "insert without owner" and "insert with owner" commands and similarly also for "put if absent without owner" and "put if absent with owner".

I have in general tried to keep the number of commands down and instead opted for some branching in the logic that handles those commands. That's why there's also a KeyTest command that can test for absence, presence or a specific value based on a parameter value. Those different variants will be expressed as separate methods in the high-level application-facing API even though they share the same underlying low-level command type.

ScopedCommand might be an option even though it has the same problem as ScopeOwnedCommand in not reflecting the fact that the scoping / ownership is optional.

This type is anyways mostly an internal concern that exists mainly to avoid doing two separate instanceof checks in assertValidTree. The type is exposed to application code only through the validator functionality.

signals/src/main/java/com/vaadin/signals/impl/TreeRevision.java

taefi · 2025-01-27T12:00:16Z

signals/src/main/java/com/vaadin/signals/impl/TreeRevision.java

+     *         optional of there is no node with the given id
+     */
+    public Optional<Data> data(Id id) {
+        Node node = nodes.get(id);


If the id is not supposed to be null, why not explicitly guard against it:

Suggested change

Node node = nodes.get(id);

Node node = nodes.get(Objects.requireNonNull(id, "id cannot be null."));

All paths to this method should be guarded by unit tests. Application code is not supposed to call this method.

taefi · 2025-01-27T12:43:54Z

signals/src/main/java/com/vaadin/signals/Node.java

+         *            a list of child ids, or the an list if the node has no
+         *            list children
+         * @param mapChildren
+         *            a sequenced map from key to child id, or an empty map if


What does a sequenced map mean here? Is it referring to the Java 21's SequencedMap or an ordered map? If so, shouldn't we define with a more specific interface e.g. SequencedMap or the good old LinkedHashMap?

Cannot use SequencedMap since we still support Java 17. Don't want to use LinkedHashMap in any signature since that's an implementation detail.

Then what prevents the caller code from passing an unordered map?

If caller code doesn't care about ordering, then it can do whatever it wants. But we should probably make sure there are tests to verify that order remains preserved once it has been implicitly or explicitly established.

Are those tests going to be added to this PR?

Yes. I will add those tests along with some other test omissions that have been pointed out in other comments.

taefi · 2025-01-27T14:09:38Z

signals/src/main/java/com/vaadin/signals/Node.java

+    public record Data(Id parent, Id lastUpdate, Id scopeOwner, JsonNode value,
+            List<Id> listChildren,
+            Map<String, Id> mapChildren) implements Node {


Looking at the implementation of TreeRevision # assertValidTree method, and the way it had to concat the listChildren and mapChildren, seems like a flag to me. Enabling the caller code to provide both, can easily result in creating an invalid Data node. One should be calculated based on the other, for instance, If the mapChildren changes to an orderedMap such as LinkedHashMap, then the listChildren could be just a public method that calculates based on mapChildren.values(). Or maybe I misunderstood the purpose of having both the mapChildren and listChildren side by side.

Some child nodes are accessed by key and some by order. There's no reason why a single node couldn't have children of both types even though that's not the typical case.

What I'm not understanding is: should or shouldn't the listChildren and mapChildren.values() contain the same set of Ids?

There should be no overlap. Every child should be in exactly one location - either addressable by ListPosition or by a String key but never both at the same time.

All operations that move a child remove the child from its current location before attaching it back again, even if moving within the same parent. Any accidental overlap should trigger an error in TreeRevision.assertValidTree() from the !visited.add(id) check (since the concatenation doesn't do distinct()). Should maybe add a unit test for that case as well.

There should be no overlap. Every child should be in exactly one location - either addressable by ListPosition or by a String key but never both at the same time.

To me, the it definitely worth adding this to the javadocs.

signals/src/main/java/com/vaadin/signals/impl/TreeRevision.java

Co-authored-by: Soroosh Taefi <taefi.soroosh@gmail.com>

signals/src/main/java/com/vaadin/signals/SignalCommand.java

mshabarov

First round of review.
Will continue with the impl next and tests.

Please confirm that my following assumptions are more or less true:

Id is needed in signals to uniquely identify a signal object within some context. Whereas key is for storing signals in a Map, e.g. for signals representing each field of an entity object like Person. The signals in list and map are usually different, because in the "list" case they are of the same rank, and in the "map" case they belong to the parent object, e.g. Person.
the signal command is a command that is applied to one or more nodes and basically this is something that happens under the hood what an API user tries to get or set a value to signal or for example manipulates the collection of signals. Though the commands are not only scoped to the value operations, but also to modifications in hierarchy structure, access and transactions.
scope owners are needed for security / access layer, so that each signal allows modifications only for a given owner.
"test" commands are needed for transactions engine.

signals/src/main/java/com/vaadin/signals/Id.java

mshabarov · 2025-01-27T16:05:29Z

signals/src/main/java/com/vaadin/signals/ListSignal.java

+     *            id of the node to insert immediately before, nor
+     *            <code>null</code> to not define a constraint
+     */
+    public record ListPosition(Id after, Id before) {


Can be marked with @Nullable from JSpecify (it should be in Flow codebase already, it was added with the CRUD repos feature)

signals/src/main/java/com/vaadin/signals/SignalCommand.java

Legioth · 2025-01-28T13:32:30Z

Id is needed in signals to uniquely identify a signal object within some context. Whereas key is for storing signals in a Map, e.g. for signals representing each field of an entity object like Person.

Correct. The id is always randomly generated (or a hardcoded constant for the root node) and carries no meaning to application logic. Application logic will very rarely even have to deal with Id values. The key is always a string provided by the application.

The signals in list and map are usually different, because in the "list" case they are of the same rang, and in the "map" case they belong to the parent object, e.g. Person.

They are all children. The only difference is whether the child is "addressed" based its position relative to other list children or addressed based on an application-provided key.

In most cases, a node has either a value, list children, or map children but not multiple types of content at the same time. This corresponds to a JSON structure where [] is a node with only list children, {} is a node with only map children and the primitive types (string, number, boolean and null) are stored as value`.

But the signal data structure is more flexible than that. Another example is XML that can be expressed so that different nodes have different meaning (though XML can also be expressed as JSON with some gotchas). A signal node that is a "list child" of another node represents an element and the signal node's value is the XML tag name. A signal node that is a "map child" of another node represents an attribute - they map key is the attribute name and the node value is the attribute value. In this way, an XML element that has both child elements and attributes will have both list children and map children.

the signal command is a command that is applied to one or more nodes and basically this is something that happens under the hood what an API user tries to get or set a value to signal or for example manipulates the collection of signals. Though the commands are not only scoped to the value operations, but also to modifications in hierarchy structure, access and transactions.

Reading values or hierarchy does not involve any commands. Commands are only for making changes (to values and/or hierarchy) and for defining conditions for transactions. And this makes me think that the XyzTest commands should be renamed to XyzCondition rather than XyZVerify that was suggested previously.

scope owners are needed for security / access layer, so that each signal allows modifications only for a given owner.

Nope. Scope owners are for automatically deleting nodes when the owner disappears (i.e. when a Hilla client is disconnected or if a Flow component is detached), even if the owner disappears in an abrupt way where it has no chance of sending cleanup commands. The main use case for this is to automatically clean up an avatar group. What would be a better name for this concept?

"test" commands are needed for transactions engine.

Confirmed. Those commands have no use on their own but only as part of a transaction that also applies some changes through "regular" commands.

Legioth · 2025-01-28T13:34:32Z

signals/src/main/java/com/vaadin/signals/SignalCommand.java

+     * A signal command that doesn't apply any change but only performs a test
+     * that will be part of determining whether a transaction passes.
+     */
+    sealed interface TestCommand extends SignalCommand {


What about ConditionCommand for the base type and then naming the specific ones e.g. ValueCondition?

signals/src/main/java/com/vaadin/signals/Id.java

signals/src/main/java/com/vaadin/signals/ListSignal.java

signals/src/main/java/com/vaadin/signals/SignalCommand.java

Legioth · 2025-01-28T14:07:25Z

signals/src/main/java/com/vaadin/signals/SignalCommand.java

+     * @param nodeId
+     *            id of the node to check, not <code>null</code>
+     * @param expectedLastUpdate
+     *            the expected id of the command hat last updated this node, not


Suggested change

* the expected id of the command hat last updated this node, not

* the expected id of the command that last updated this node, not

signals/src/main/java/com/vaadin/signals/SignalCommand.java

mshabarov · 2025-01-30T10:54:29Z

Nope. Scope owners are for automatically deleting nodes when the owner disappears (i.e. when a Hilla client is disconnected or if a Flow component is detached), even if the owner disappears in an abrupt way where it has no chance of sending cleanup commands. The main use case for this is to automatically clean up an avatar group. What would be a better name for this concept?

Well, with this explanation the name makes sense. I'd think of "Lifecycle" in the name, but it's nearly the same thing.

taefi

Added some questions and suggestions. The MutableTreeRevisionTest would be reviewed in the next round.

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

taefi · 2025-01-31T11:37:23Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+    public void apply(List<SignalCommand> commands) {
+        for (SignalCommand command : commands) {
+            apply(command, null);
+        }
+    }


This method has no test coverage.

signals/src/test/java/com/vaadin/signals/impl/MutableTreeRevisionTest.java

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

signals/src/test/java/com/vaadin/signals/impl/MutableTreeRevisionTest.java

taefi · 2025-01-31T15:55:10Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+        if (result instanceof Accept accept) {
+            accept.updates().forEach((nodeId, update) -> {
+                Node newNode = update.newNode();
+
+                if (newNode == null) {
+                    nodes().remove(nodeId);
+                    originalInserts().remove(nodeId);
+                } else {
+                    nodes().put(nodeId, newNode);
+                }
+            });
+
+            originalInserts().putAll(accept.originalInserts());
+        }


As I don't see what happens to a MutableTreeRevision instance after a command is applied to it, I still don't have a clear understanding of why originalInserts().remove(nodeId); should happen when a command ends up removing a node from the tree.

When the scope object that owns a node no longer exists, If the node(s) owned by that owner should be removed from the tree. Then, according to the javadoc of TreeRevision # originalInserts, some node that is no longer exist as a Data node should be added back again when the owner is back, by replaying the same command from the original insert collection.

I grasp that If there's no removal from that collection, it grows forever, and it doesn't make sense, but at the same time, without knowing how the system is going to notify the absence of an owner to a tree, I cannot digest the logic of removing from originalInserts() every time a command results in removing a data node.

When commented out the originalInserts().remove(nodeId);, only those tests that are related to the removal of scope owned nodes are failing, so it is a bit confusing.

I couldn't get why originalInserts().remove(nodeId); is needed too.
Original inserts in accept.originalInserts() are only added in createNode() by PutIfAbsentCommand or InsertCommand, but never removed from there, so even if some inserts are removed from map of MutableTreeRevision, they are still held in map of TreeManipulator and re-added.

Maybe they are for different nodes, but I couldn't deduct this from the codes.

Would leak memory if we don't remove those entires when the associated node is removed. The "happy path" is that the node is indeed supposed to be removed for good and then this is the right place to do it.

The other case is when it's only a temporary removal due to cleanup when the node owner is disconnected which means that the node owner may need it again later if it gets reconnected. To deal with that, the node owner needs to create a copy of its original inserts in the logic where it handles being disconnected and reconnected so it's outside the logic that exists in this PR.

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

taefi · 2025-01-31T16:27:45Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+            }
+
+            useData(parentId, (node, id) -> {
+                detachedNodes.remove(resolvedChildId);


Should detachedNodes.remove(resolvedChildId); for some reason be happening within the lambda that has no dependency to?

There's no direct data relationship but useData checks for an error condition and we shouldn't apply any changes before that check has been performed.

Maybe worth having a comment about why it is called there.

mshabarov

Second round of review.
I'm focusing now on the MutableTreeRevision.

signals/src/main/java/com/vaadin/signals/impl/TreeRevision.java

signals/src/main/java/com/vaadin/signals/SignalCommand.java

mshabarov · 2025-01-30T12:32:53Z

signals/src/main/java/com/vaadin/signals/impl/OperationResult.java

+ * @see SignalCommand
+ * @see TreeRevision
+ */
+public sealed interface OperationResult {


The name could be CommandResult to have a better match with the SignalCommand.

mshabarov · 2025-01-30T13:01:58Z

signals/src/main/java/com/vaadin/signals/impl/OperationResult.java

+     *            the new node instance or null if the operation removed the
+     *            node
+     */
+    record TreeModification(Node oldNode, Node newNode) {


I would prefer NodeModification name instead, but this is minor.

mshabarov · 2025-01-30T13:32:13Z

signals/src/main/java/com/vaadin/signals/impl/OperationResult.java

+     * @return an accepted result if the condition is <code>true</code>, a
+     *         rejected result if the condition is <code>false</code>
+     */
+    public static OperationResult test(boolean condition,


Or from, of, fromCondition, ofCondition.

signals/src/main/java/com/vaadin/signals/impl/Snapshot.java

mshabarov · 2025-02-03T07:49:28Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+        if (result instanceof Accept accept) {
+            accept.updates().forEach((nodeId, update) -> {
+                Node newNode = update.newNode();
+
+                if (newNode == null) {
+                    nodes().remove(nodeId);
+                    originalInserts().remove(nodeId);
+                } else {
+                    nodes().put(nodeId, newNode);
+                }
+            });
+
+            originalInserts().putAll(accept.originalInserts());
+        }


I couldn't get why originalInserts().remove(nodeId); is needed too.
Original inserts in accept.originalInserts() are only added in createNode() by PutIfAbsentCommand or InsertCommand, but never removed from there, so even if some inserts are removed from map of MutableTreeRevision, they are still held in map of TreeManipulator and re-added.

Maybe they are for different nodes, but I couldn't deduct this from the codes.

mshabarov

Last round of reviews.
I don't really have any blocker comments, but rather some worries about maintainability of MutableTreeRevision class which is huge at the moment. My concerns:

Would be good to extract methods from TreeManipulator that don't use state fields, e.g. findInsertIndex, to the util class.
Would be good if possible to extract updatedNodes and detachedNodes to a separate state holder class and move there some basic operations with the state, like data(), value(), useData(), attach(), detach().
childResult seems to be used only for accumulating results into one collection, I'd remove it from class field and added to the state holder class.
I'd avoid checking the result in the commands, because else it's squashed through the whole TreeManipulator, could we have it as a separate verification phase in the command execution loop (outside of the commands handlers)?

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

signals/src/main/java/com/vaadin/signals/impl/TreeRevision.java

mshabarov · 2025-02-03T13:54:56Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+            MutableTreeRevision child = new MutableTreeRevision(
+                    MutableTreeRevision.this);


treeRevisionCopy would be a better name.
Secondly, why we need a copy here, wouldn't it be just fine to make changes to the MutableTreeRevision.this object?

I think the name should be consistent with the childResults field. What should we name that field if this variable would be treeRevisionCopy?

The copy is needed to make it easy to discard the already applied changes if the transaction fails.

What should we name that field if this variable would be treeRevisionCopy?

child or childResults means to me that these are sub-trees for child nodes, but in fact these are modifications for the whole tree but triggered by single commands.

I'd expect the naming like this:

MutableTreeRevision treeRevisionCopy = new MutableTreeRevision( MutableTreeRevision.this); signalCommandResults = new HashMap<Id, OperationResult>();

I still don't get why we are creating another instance of MutableTreeRevision from other instance (MutableTreeRevision.this).

I thought that the workflow is like this:
Snapshot -> new mutual copy -> MutableTreeRevision -> run transaction -> MutableTreeRevision with changes -> update Snapshot, so we create a mutable tree revision on top of immutable, make changes in transaction and commit these changes as a new immutable snapshot.

But looks like we are trying to make a copy of MutableTreeRevision for each transaction in a chain that already has it's master MutableTreeRevision instance.

I'm thinking of "child" in context of the transaction hierarchy (since transaction commands can be nested) but I see how it can be confused with the data node hierarchy. Would it be clearer with "sub"?

The Snapshot -> MutableTreeRevision -> Snapshot cycle that you describe is the basic one but it needs to be slightly more complex for transactions. To apply a transaction, we need to apply each sub command to a MutableTreeRevision instance so that subsequent commands can be applied based on the result of previous commands. If a sub command fails, then the changes performed by the previous commands in the transaction should also be discarded so that the resulting state of the MutableTreeRevision is as though the transaction never happened.

This doesn't matter for regular transactions that are anyways "dead" as soon as any sub command fails but there are also the high-level transactions used for repeatable reads. (Note that there's no corresponding transaction command for a read transaction itself but regular transactions may be nested inside the read transaction.) In that case, all changes are applied both to the underlying signal tree and to a MutableTreeRevision that acts as a buffer for that read transaction. In that case, we want to make sure the data in that buffer is correct even if a failing regular transaction is applied inside the read transaction.

You could thus think of the copy as a scratchpad that is used only for evaluating whether the transaction command should be applied but it doesn't participate in the main flow of data. The actual changes in case the transaction is applied come through the values that are collected into the Accept instance that is set as the result of processing the transaction command. Maybe something like scratchpad would be a clearer variable name?

mshabarov · 2025-02-04T08:47:20Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+     * @param command
+     *            the command to apply, not <code>null</code>
+     * @param resultHandler
+     *            the result handler callback, or <code>null</code> to ignore


Not clear what is the purpose of the result handler.
Is it mainly for collecting all the results into one single collection? Or do we foreseen any other logic later?

I only see it as a collector, but defining it as a BiConsumer instead of a Map to emphasize that the receiver shouldn't need to read anything from the map.

resultCollector would be better then.

Name makes sense here. At the beginning I was thinking that this handler makes some processing of the result, e.g. iterates through the tree modifications and so on, but the purpose is simpler. It's easier to read the code when you better understand the purpose of a class or object.

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

mshabarov · 2025-02-04T14:52:45Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+
+        private void handleClear(ClearCommand clear) {
+            updateData(clear.nodeId(), node -> {
+                assert detachedNodes.isEmpty();


I don't get why detachedNodes should be empty at this moment.
Don't other commands, RemoveByKey for example, add nodes to detachedNodes ?

Every command is applied in its own TreeManipulator instance. At the same time, I don't see why this line is needed specifically here while not having similar checks in other similar methods.

mshabarov · 2025-02-05T13:12:17Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+
+            Id before = position.before();
+            if (before != null) {
+                if (before.equals(Id.ZERO)) {


Some other constant would be better, because Id.ZERO has an association with the very first item in the list, but in fact it's the last item.

There was a similar discussion related to ListPosition and the conclusion was that we should introduce an alias named e.g. EDGE instead.

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

Legioth

Would be good to extract methods from TreeManipulator that don't use state fields, e.g. findInsertIndex, to the util class.

I don't see the benefit of that if those util methods are anyways only used from a single class. The main impact then would be that you have to jump back and forth between multiple files when you try to understand what goes on.

I think the other top-level comments were addressed in the comments for individual code locations.

Legioth · 2025-02-06T08:00:19Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+            MutableTreeRevision child = new MutableTreeRevision(
+                    MutableTreeRevision.this);


I think the name should be consistent with the childResults field. What should we name that field if this variable would be treeRevisionCopy?

The copy is needed to make it easy to discard the already applied changes if the transaction fails.

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

Legioth · 2025-02-06T08:50:01Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+
+        private void handleClear(ClearCommand clear) {
+            updateData(clear.nodeId(), node -> {
+                assert detachedNodes.isEmpty();


Every command is applied in its own TreeManipulator instance. At the same time, I don't see why this line is needed specifically here while not having similar checks in other similar methods.

signals/src/main/java/com/vaadin/signals/SignalCommand.java

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

Legioth · 2025-02-06T12:03:05Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+     * @param command
+     *            the command to apply, not <code>null</code>
+     * @param resultHandler
+     *            the result handler callback, or <code>null</code> to ignore


I only see it as a collector, but defining it as a BiConsumer instead of a Map to emphasize that the receiver shouldn't need to read anything from the map.

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

Legioth · 2025-02-06T12:13:44Z

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

+
+            Id before = position.before();
+            if (before != null) {
+                if (before.equals(Id.ZERO)) {


There was a similar discussion related to ListPosition and the conclusion was that we should introduce an alias named e.g. EDGE instead.

signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com> Co-authored-by: Soroosh Taefi <taefi.soroosh@gmail.com>

vaadin-bot · 2025-02-07T10:41:54Z

TC Format Checker Report - 10:41 - 07 - Feb

There are 2 files with format errors

To see a complete report of formatting issues, download the differences file
To fix the build, please run mvn formatter:format in your branch and commit the changes.
Optionally you might add the following line in your .git/hooks/pre-commit file:
```
mvn formatter:format
```

Here is the list of files with format issues in your PR:

signals/src/main/java/com/vaadin/signals/Id.java
signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java

sonarqubecloud · 2025-02-07T10:56:01Z

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Add signal commands

95949da

This is not yet a complete signal implementation but only the low-level core data manipulation logic.

Legioth requested review from mshabarov and taefi January 17, 2025 16:32

vaadin-bot added the +0.0.1 label Jan 17, 2025

Legioth added 2 commits January 17, 2025 18:56

Add some missing Javadocs

eea8740

Add license headers

371348d

mshabarov changed the title ~~Add signal commands~~ feat: Add signal commands Jan 20, 2025

taefi requested changes Jan 20, 2025

View reviewed changes

taefi requested changes Jan 27, 2025

View reviewed changes

Apply suggestions from code review

da62c09

Co-authored-by: Soroosh Taefi <taefi.soroosh@gmail.com>

vaadin-bot added +1.0.0 and removed +0.0.1 labels Jan 28, 2025

taefi reviewed Jan 28, 2025

View reviewed changes

signals/src/main/java/com/vaadin/signals/SignalCommand.java Outdated Show resolved Hide resolved

mshabarov reviewed Jan 28, 2025

View reviewed changes

Legioth commented Jan 28, 2025

View reviewed changes

taefi requested changes Jan 31, 2025

View reviewed changes

mshabarov reviewed Feb 3, 2025

View reviewed changes

mshabarov reviewed Feb 5, 2025

View reviewed changes

Legioth commented Feb 6, 2025

View reviewed changes

Apply suggestions from code review

2fbd578

Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com> Co-authored-by: Soroosh Taefi <taefi.soroosh@gmail.com>

	public static OperationResult test(boolean condition,
	public static OperationResult verify(boolean condition,


		import com.vaadin.signals.Id;

		public class OperationResultTest {

	sealed interface ScopeOwnerCommand extends SignalCommand {
	sealed interface ScopeOwnedCommand extends SignalCommand {

	Node node = nodes.get(id);
	Node node = nodes.get(Objects.requireNonNull(id, "id cannot be null."));

	* the expected id of the command hat last updated this node, not
	* the expected id of the command that last updated this node, not

		MutableTreeRevision child = new MutableTreeRevision(
		MutableTreeRevision.this);

feat: Add signal commands #20876

Are you sure you want to change the base?

feat: Add signal commands #20876

Conversation

Legioth commented Jan 17, 2025

github-actions bot commented Jan 17, 2025 • edited Loading

Test Results

taefi left a comment • edited Loading

Choose a reason for hiding this comment

taefi Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taefi Jan 20, 2025 • edited Loading

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

taefi Jan 28, 2025 • edited Loading

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

taefi left a comment • edited Loading

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

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

taefi Jan 28, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taefi Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

mshabarov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Legioth commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshabarov commented Jan 30, 2025

taefi 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

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

mshabarov 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

Choose a reason for hiding this comment

mshabarov 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2025 •

edited

Loading

taefi left a comment •

edited

Loading

taefi Jan 20, 2025 •

edited

Loading

taefi Jan 20, 2025 •

edited

Loading

taefi Jan 28, 2025 •

edited

Loading

taefi left a comment •

edited

Loading

taefi Jan 28, 2025 •

edited

Loading

taefi Jan 28, 2025 •

edited

Loading

mshabarov left a comment •

edited

Loading