Skip to content

Commit 45000b8

Browse files
authored
CHASM: Fix validation context (#8562)
## What changed? - Use immutable chasm context to access node when doing validations ## Why? - Validation doesn't change component state and nothing needs to be persisted. - Some parts of the system skips persistence when component access fail on validation step, if node is marked dirty, a dirty mutable state panic will be triggered upon releasing the execution lock. ## How did you test it? - [x] built - [ ] run locally and tested manually - [ ] covered by existing tests - [x] added new unit test(s) - [ ] added new functional test(s)
1 parent 1d9a163 commit 45000b8

File tree

2 files changed

+74
-37
lines changed

2 files changed

+74
-37
lines changed

chasm/tree.go

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -373,7 +373,8 @@ func (n *Node) Component(
373373
return nil, errComponentNotFound
374374
}
375375

376-
if err := node.prepareComponentValue(chasmContext); err != nil {
376+
validationContext := NewContext(chasmContext.getContext(), node)
377+
if err := node.prepareComponentValue(validationContext); err != nil {
377378
return nil, err
378379
}
379380

@@ -388,18 +389,22 @@ func (n *Node) Component(
388389
// Access check always begins on the target node's parent, and ignored for nodes
389390
// without ancestors.
390391
if node.parent != nil {
391-
err := node.parent.validateAccess(chasmContext)
392+
err := node.parent.validateAccess(validationContext)
392393
if err != nil {
393394
return nil, err
394395
}
395396
}
396397

397398
if ref.validationFn != nil {
398-
if err := ref.validationFn(node.root().backend, chasmContext, componentValue); err != nil {
399+
if err := ref.validationFn(node.root().backend, validationContext, componentValue); err != nil {
399400
return nil, err
400401
}
401402
}
402403

404+
// prepare component value again using incoming context to mark node as dirty if needed.
405+
if err := node.prepareComponentValue(chasmContext); err != nil {
406+
return nil, err
407+
}
403408
return componentValue, nil
404409
}
405410

@@ -2737,14 +2742,12 @@ func (n *Node) ExecutePureTask(
27372742
return false, fmt.Errorf("ExecutePureTask called on a SideEffect task '%s'", registrableTask.fqType())
27382743
}
27392744

2740-
ctx := NewMutableContext(
2741-
newContextWithOperationIntent(baseCtx, OperationIntentProgress),
2742-
n,
2743-
)
2745+
progressIntentCtx := newContextWithOperationIntent(baseCtx, OperationIntentProgress)
2746+
validationContext := NewContext(progressIntentCtx, n)
27442747

27452748
// Ensure this node's component value is hydrated before execution. Component
27462749
// will also check access rules.
2747-
component, err := n.Component(ctx, ComponentRef{})
2750+
_, err := n.Component(validationContext, ComponentRef{})
27482751
if err != nil {
27492752
// NotFound errors are expected here and we can safely skip the task execution.
27502753
if errors.As(err, new(*serviceerror.NotFound)) {
@@ -2754,16 +2757,22 @@ func (n *Node) ExecutePureTask(
27542757
}
27552758

27562759
// Run the task's registered value before execution.
2757-
valid, err := n.validateTask(ctx, taskAttributes, taskInstance)
2760+
valid, err := n.validateTask(validationContext, taskAttributes, taskInstance)
27582761
if err != nil {
27592762
return false, err
27602763
}
27612764
if !valid {
27622765
return false, nil
27632766
}
27642767

2768+
executionContext := NewMutableContext(progressIntentCtx, n)
2769+
component, err := n.Component(executionContext, ComponentRef{})
2770+
if err != nil {
2771+
return false, err
2772+
}
2773+
27652774
result := registrableTask.executeFn.Call([]reflect.Value{
2766-
reflect.ValueOf(ctx),
2775+
reflect.ValueOf(executionContext),
27672776
reflect.ValueOf(component),
27682777
reflect.ValueOf(taskAttributes),
27692778
reflect.ValueOf(taskInstance),

chasm/tree_test.go

Lines changed: 55 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1477,10 +1477,8 @@ func (s *nodeSuite) TestValidateAccess() {
14771477
}
14781478

14791479
func (s *nodeSuite) TestGetComponent() {
1480-
root, err := s.newTestTree(testComponentSerializedNodes())
1481-
s.NoError(err)
1482-
14831480
errValidation := errors.New("some random validation error")
1481+
14841482
expectedTestComponent := &TestComponent{}
14851483
setTestComponentFields(expectedTestComponent, s.nodeBackend)
14861484
assertTestComponent := func(component Component) {
@@ -1494,39 +1492,47 @@ func (s *nodeSuite) TestGetComponent() {
14941492

14951493
testCases := []struct {
14961494
name string
1497-
chasmContext Context
1495+
chasmContextFn func(root *Node) Context
14981496
ref ComponentRef
14991497
expectedErr error
1500-
valueState valueState
1498+
nodeDirty bool
15011499
assertComponent func(Component)
15021500
}{
15031501
{
1504-
name: "path not found",
1505-
chasmContext: NewContext(context.Background(), root),
1502+
name: "path not found",
1503+
chasmContextFn: func(root *Node) Context {
1504+
return NewContext(context.Background(), root)
1505+
},
15061506
ref: ComponentRef{
15071507
componentPath: []string{"unknownComponent"},
15081508
},
15091509
expectedErr: errComponentNotFound,
15101510
},
15111511
{
1512-
name: "archetype mismatch",
1513-
chasmContext: NewContext(context.Background(), root),
1512+
name: "archetype mismatch",
1513+
chasmContextFn: func(root *Node) Context {
1514+
return NewContext(context.Background(), root)
1515+
},
15141516
ref: ComponentRef{
15151517
archetype: "TestLibrary.test_sub_component_1",
15161518
},
15171519
expectedErr: errComponentNotFound,
15181520
},
15191521
{
1520-
name: "entityGoType mismatch",
1521-
chasmContext: NewContext(context.Background(), root),
1522+
name: "entityGoType mismatch",
1523+
chasmContextFn: func(root *Node) Context {
1524+
return NewContext(context.Background(), root)
1525+
},
15221526
ref: ComponentRef{
15231527
entityGoType: reflect.TypeFor[*TestSubComponent2](),
15241528
},
15251529
expectedErr: errComponentNotFound,
15261530
},
15271531
{
1528-
name: "initialVT mismatch",
1529-
chasmContext: NewContext(context.Background(), root),
1532+
name: "initialVT mismatch",
1533+
chasmContextFn: func(root *Node) Context {
1534+
return NewMutableContext(context.Background(), root)
1535+
},
15301536
ref: ComponentRef{
15311537
componentPath: []string{"SubComponent1", "SubComponent11"},
15321538
// should be (1, 1) but we set it to (2, 2)
@@ -1538,8 +1544,10 @@ func (s *nodeSuite) TestGetComponent() {
15381544
expectedErr: errComponentNotFound,
15391545
},
15401546
{
1541-
name: "validation failure",
1542-
chasmContext: NewContext(context.Background(), root),
1547+
name: "validation failure",
1548+
chasmContextFn: func(root *Node) Context {
1549+
return NewMutableContext(context.Background(), root)
1550+
},
15431551
ref: ComponentRef{
15441552
componentPath: []string{"SubComponent1"},
15451553
componentInitialVT: &persistencespb.VersionedTransition{
@@ -1553,8 +1561,10 @@ func (s *nodeSuite) TestGetComponent() {
15531561
expectedErr: errValidation,
15541562
},
15551563
{
1556-
name: "success readonly access",
1557-
chasmContext: NewContext(context.Background(), root),
1564+
name: "success readonly access",
1565+
chasmContextFn: func(root *Node) Context {
1566+
return NewContext(context.Background(), root)
1567+
},
15581568
ref: ComponentRef{
15591569
componentPath: []string{}, // root
15601570
componentInitialVT: &persistencespb.VersionedTransition{
@@ -1566,32 +1576,42 @@ func (s *nodeSuite) TestGetComponent() {
15661576
},
15671577
},
15681578
expectedErr: nil,
1569-
valueState: valueStateSynced,
15701579
assertComponent: assertTestComponent,
15711580
},
15721581
{
1573-
name: "success mutable access",
1574-
chasmContext: NewMutableContext(context.Background(), root),
1582+
name: "success mutable access",
1583+
chasmContextFn: func(root *Node) Context {
1584+
return NewMutableContext(context.Background(), root)
1585+
},
15751586
ref: ComponentRef{
15761587
componentPath: []string{}, // root
15771588
},
15781589
expectedErr: nil,
1579-
valueState: valueStateNeedSyncStructure,
1590+
nodeDirty: true,
15801591
assertComponent: assertTestComponent,
15811592
},
15821593
}
15831594

15841595
for _, tc := range testCases {
15851596
s.Run(tc.name, func() {
1586-
component, err := root.Component(tc.chasmContext, tc.ref)
1597+
root, err := s.newTestTree(testComponentSerializedNodes())
1598+
s.NoError(err)
1599+
1600+
component, err := root.Component(tc.chasmContextFn(root), tc.ref)
15871601
s.Equal(tc.expectedErr, err)
1588-
if tc.expectedErr == nil {
1589-
// s.Equal(tc.expectedComponent, component)
15901602

1591-
node, ok := root.findNode(tc.ref.componentPath)
1603+
node, ok := root.findNode(tc.ref.componentPath)
1604+
if tc.expectedErr == nil {
15921605
s.True(ok)
1593-
s.Equal(component, node.value)
1594-
s.Equal(tc.valueState, node.valueState)
1606+
tc.assertComponent(component)
1607+
}
1608+
1609+
if ok {
1610+
if tc.nodeDirty {
1611+
s.Greater(node.valueState, valueStateSynced)
1612+
} else {
1613+
s.LessOrEqual(node.valueState, valueStateSynced)
1614+
}
15951615
}
15961616
})
15971617
}
@@ -2739,30 +2759,38 @@ func (s *nodeSuite) TestExecutePureTask() {
27392759
}
27402760

27412761
// Succeed task execution and validation (happy case).
2762+
root.setValueState(valueStateSynced)
27422763
expectExecute(nil)
27432764
expectValidate(true, nil)
27442765
executed, err := root.ExecutePureTask(ctx, taskAttributes, pureTask)
27452766
s.NoError(err)
27462767
s.True(executed)
2768+
s.Equal(valueStateNeedSyncStructure, root.valueState)
27472769

27482770
expectedErr := errors.New("dummy")
27492771

27502772
// Succeed validation, fail execution.
2773+
root.setValueState(valueStateSynced)
27512774
expectExecute(expectedErr)
27522775
expectValidate(true, nil)
27532776
_, err = root.ExecutePureTask(ctx, taskAttributes, pureTask)
27542777
s.ErrorIs(expectedErr, err)
2778+
s.Equal(valueStateNeedSyncStructure, root.valueState)
27552779

27562780
// Fail task validation (no execution occurs).
2781+
root.setValueState(valueStateSynced)
27572782
expectValidate(false, nil)
27582783
executed, err = root.ExecutePureTask(ctx, taskAttributes, pureTask)
27592784
s.NoError(err)
27602785
s.False(executed)
2786+
s.Equal(valueStateSynced, root.valueState) // task not executed, so node is clean
27612787

27622788
// Error during task validation (no execution occurs).
2789+
root.setValueState(valueStateSynced)
27632790
expectValidate(false, expectedErr)
27642791
_, err = root.ExecutePureTask(ctx, taskAttributes, pureTask)
27652792
s.ErrorIs(expectedErr, err)
2793+
s.Equal(valueStateSynced, root.valueState) // task not executed, so node is clean
27662794
}
27672795

27682796
func (s *nodeSuite) TestExecuteSideEffectTask() {

0 commit comments

Comments
 (0)