CHASM: tree node serialization/deserialization methods #7409

alexshtin · 2025-03-01T01:30:36Z

What changed?

Added methods to serialized/deserialized CHASM components to its proto representation.
Added many TODOs to address later.

Why?

Part of CHASM worksream.

How did you test it?

Added unit tests.

Potential risks

No risks. Feature is still at the very early stage.

Documentation

I wish.

Is hotfix candidate?

No.

chasm/tree.go

yycptt

Haven't review the serialize logic yet but I feel it's better to split it into a separate PR given how complex it is (we need to update tree structure as well). Let's focusing on getting the decoding part fully implemented?

chasm/field.go

chasm/tree.go

alexshtin · 2025-03-15T00:14:35Z

Sorry, I did rebase and most of the comments are "Outdated" now. Which is actually true because I changed a lot. I addressed all comments though.

yycptt

Would you mind give an overview of what are the main helper methods introduced by this PR and how they should be used by the closeTransaction() implementation?

yycptt · 2025-03-17T21:06:47Z

chasm/tree.go

 // IsDirty returns true if any node rooted at Node n has been modified.
 // The result will be reset to false after a call to CloseTransaction().
 func (n *Node) IsDirty() bool {
-	panic("not implemented")
+	return n.dirty


this needs to recursively check all child nodes. The dirty flag is only for the current node, otherwise we can't compute mutations at the end of a transaction.

Makes sense. Not part of this PR though. I am adding your comment as TODO there.

yycptt · 2025-03-17T21:12:21Z

chasm/tree.go

+			continue
+		}
+
+		// chasm.Field field must be a pointer, i.e. *chasm.Field[T].


I don't think it has to be a pointer. As long as the registered component that's using chasm.Field is a pointer we should be good I think.
Is there a case you have in mind that doesn't work when Field is used without a pointer?

I just don't want to support both cases and existing Field constructors return *Field. I agree that Field look more natural in component definition than *Field. I am flipping it to use Field and forbid *Field.

yycptt · 2025-03-17T21:20:09Z

chasm/tree.go

+		// TODO: error or just go around?
+		// return serviceerror.NewInternal("unsupported field type in component " + fieldT.String())


let's be strict in the beginning I'd say. It's always easier to relax a check than enforcing one.

yycptt · 2025-03-17T21:32:20Z

chasm/tree.go

+		}
+
+		n.serializedValue.GetComponentAttributes().Data = blob
+		n.serializedValue.GetComponentAttributes().Type = nodeValueT.Elem().String() // type name w/o *


This should be the registered component name, no go type name.

yycptt · 2025-03-17T21:35:10Z

chasm/tree.go

+
+		n.serializedValue.GetComponentAttributes().Data = blob
+		n.serializedValue.GetComponentAttributes().Type = nodeValueT.Elem().String() // type name w/o *
+		if n.serializedValue.GetInitialVersionedTransition() == nil {


initial versioned transition should only be set when the creating a new chasm node. It looks like you can do this as part of setValue(), though I am not 100% sure when that method will be called.

Then LastUpdateVersionedTransition is updated every time the node is mutated. Guess you can do it here.

yycptt · 2025-03-17T21:45:46Z

chasm/tree.go

+			if err := childNode.deleteChildren(removedPaths, nil, append(currentPath, childName)); err != nil {
+				return err
+			}
+			path, err := n.pathEncoder.Encode(n, append(currentPath, childName))


Suggested change

path, err := n.pathEncoder.Encode(n, append(currentPath, childName))

path, err := n.pathEncoder.Encode(childNode, append(currentPath, childName))

🤦‍♂️

yycptt · 2025-03-17T21:48:08Z

chasm/tree.go

+			panic("not implemented")
+		}
+	}
+


What about newly added fields? when will we add them to the child map?

Is that logic reflectSubcomponents or setValues()? I guess I am a bit confused where/when those two methods will be invoked.

lina-temporal · 2025-03-17T21:29:18Z

chasm/test_component.go

+
+type (
+	// TestComponent is a sample CHASM component used in tests.
+	// It would be nice to move it another package, but this creates a circular dependency.


Could you move the tests that need these and these test components into a separate package (like chasm_test)?

I tried to do it. Unfortunately in this case I will need to export almost everything which I don't want to do. I hope later, I will figure out how to write better tests w/o accessing many unexported fields.
But I am 100% with you on this (registry_test.go is in chasm_test), and I will do it later.

lina-temporal · 2025-03-17T21:43:16Z

chasm/tree.go

+	return f.Name
+}
+
+func (n *Node) reflectSubcomponents() error {


(minor) How about, updateNodeTree or updateSubcomponents?

can we add a doc on what this method does? Maybe something like, "creates the internal node tree structure based on the root node's value?"

Renamed to setChildNodes (similar to setValue). Added doc.

lina-temporal · 2025-03-17T22:18:45Z

chasm/tree.go

+//   - when a child is removed, all its children are removed too.
+//
+// Returns slice of paths to removed nodes.
+func (n *Node) syncChildren() ([]string, error) {


When is this called in a node's/component's lifecycle? Is it just within CloseTransaction to get a list of removed paths, after an application deletes some of its nodes?

lina-temporal · 2025-03-17T22:20:54Z

chasm/tree.go

+	for i := 0; i < nodeValueT.Elem().NumField(); i++ {
+		fieldV := nodeValueV.Elem().Field(i)
+		fieldT := fieldV.Type()


Have you considered creating a Walk method for walking a node's tree, to share this logic between reflectSubcomponents/{de}serializeComponentNode/syncChildrenInternal?

I haven't but I guess I should. I am trying to make it simple and dumb for now. Adding TODO for now.

alexshtin requested a review from a team as a code owner March 1, 2025 01:30

alexshtin marked this pull request as draft March 1, 2025 01:32

lina-temporal reviewed Mar 3, 2025

View reviewed changes

yycptt reviewed Mar 4, 2025

View reviewed changes

alexshtin added 3 commits March 14, 2025 16:58

CHASM: basic serialization/deserialization helpers

f05bd06

Renames

06e5af1

Save progress

f727af1

alexshtin force-pushed the feature/chasm-tree branch from 79920a2 to f727af1 Compare March 15, 2025 00:01

alexshtin changed the title ~~CHASM: basic serialization/deserialization helpers~~ CHASM: tree node serialization/deserialization methods Mar 15, 2025

alexshtin requested review from yycptt and lina-temporal March 15, 2025 00:17

alexshtin marked this pull request as ready for review March 15, 2025 00:17

alexshtin added 2 commits March 14, 2025 17:24

Fix linter

8480012

Fix header

a127ba6

yycptt reviewed Mar 17, 2025

View reviewed changes

lina-temporal reviewed Mar 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CHASM: tree node serialization/deserialization methods #7409

CHASM: tree node serialization/deserialization methods #7409

alexshtin commented Mar 1, 2025 •

edited

Loading

yycptt left a comment

alexshtin commented Mar 15, 2025

yycptt left a comment

yycptt Mar 17, 2025

alexshtin Mar 20, 2025

yycptt Mar 17, 2025

alexshtin Mar 24, 2025

yycptt Mar 17, 2025

yycptt Mar 17, 2025

yycptt Mar 17, 2025

yycptt Mar 17, 2025

alexshtin Mar 19, 2025

yycptt Mar 17, 2025

lina-temporal Mar 17, 2025

alexshtin Mar 19, 2025

lina-temporal Mar 17, 2025

alexshtin Mar 19, 2025

lina-temporal Mar 17, 2025

lina-temporal Mar 17, 2025

alexshtin Mar 19, 2025

		// TODO: error or just go around?
		// return serviceerror.NewInternal("unsupported field type in component " + fieldT.String())

	path, err := n.pathEncoder.Encode(n, append(currentPath, childName))
	path, err := n.pathEncoder.Encode(childNode, append(currentPath, childName))

CHASM: tree node serialization/deserialization methods #7409

Are you sure you want to change the base?

CHASM: tree node serialization/deserialization methods #7409

Conversation

alexshtin commented Mar 1, 2025 • edited Loading

What changed?

Why?

How did you test it?

Potential risks

Documentation

Is hotfix candidate?

yycptt left a comment

Choose a reason for hiding this comment

alexshtin commented Mar 15, 2025

yycptt 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

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

alexshtin commented Mar 1, 2025 •

edited

Loading