From a05f7ce3fc864358ce3ef5cdd93386162eac5b25 Mon Sep 17 00:00:00 2001 From: Stefan Benz <46600784+stebenz@users.noreply.github.com> Date: Wed, 30 Apr 2025 14:58:10 +0200 Subject: [PATCH] fix: correct handling of removed targets (#9824) # Which Problems Are Solved In Actions v2, if a target is removed, which is still used in an execution, the target is still listed when list executions. # How the Problems Are Solved Removed targets are now also removed from the executions. # Additional Changes To be sure the list executions include a check if the target is still existing. # Additional Context None Co-authored-by: Livio Spring --- internal/query/execution.go | 12 ++-- internal/query/execution_targets.sql | 22 ++++--- internal/query/execution_test.go | 71 +++++++++++++++++++-- internal/query/projection/execution.go | 25 ++++++++ internal/query/projection/execution_test.go | 30 +++++++++ 5 files changed, 141 insertions(+), 19 deletions(-) diff --git a/internal/query/execution.go b/internal/query/execution.go index 0a2a989918..4739a5839e 100644 --- a/internal/query/execution.go +++ b/internal/query/execution.go @@ -1,11 +1,13 @@ package query import ( + "cmp" "context" "database/sql" _ "embed" "encoding/json" "errors" + "slices" "time" sq "github.com/Masterminds/squirrel" @@ -301,13 +303,15 @@ func executionTargetsUnmarshal(data []byte) ([]*exec.Target, error) { } targets := make([]*exec.Target, len(executionTargets)) - // position starts with 1 - for _, item := range executionTargets { + slices.SortFunc(executionTargets, func(a, b *executionTarget) int { + return cmp.Compare(a.Position, b.Position) + }) + for i, item := range executionTargets { if item.Target != "" { - targets[item.Position-1] = &exec.Target{Type: domain.ExecutionTargetTypeTarget, Target: item.Target} + targets[i] = &exec.Target{Type: domain.ExecutionTargetTypeTarget, Target: item.Target} } if item.Include != "" { - targets[item.Position-1] = &exec.Target{Type: domain.ExecutionTargetTypeInclude, Target: item.Include} + targets[i] = &exec.Target{Type: domain.ExecutionTargetTypeInclude, Target: item.Include} } } return targets, nil diff --git a/internal/query/execution_targets.sql b/internal/query/execution_targets.sql index 32257f4a1f..a6e6dd6caa 100644 --- a/internal/query/execution_targets.sql +++ b/internal/query/execution_targets.sql @@ -1,11 +1,15 @@ -SELECT instance_id, - execution_id, +SELECT et.instance_id, + et.execution_id, JSONB_AGG( JSON_OBJECT( - 'position' : position, - 'include' : include, - 'target' : target_id - ) - ) as targets -FROM projections.executions1_targets -GROUP BY instance_id, execution_id \ No newline at end of file + 'position' : et.position, + 'include' : et.include, + 'target' : et.target_id + ) + ) as targets +FROM projections.executions1_targets AS et + INNER JOIN projections.targets2 AS t + ON et.instance_id = t.instance_id + AND et.target_id IS NOT NULL + AND et.target_id = t.id +GROUP BY et.instance_id, et.execution_id \ No newline at end of file diff --git a/internal/query/execution_test.go b/internal/query/execution_test.go index eaaac1e9ba..64f9a4849f 100644 --- a/internal/query/execution_test.go +++ b/internal/query/execution_test.go @@ -22,9 +22,10 @@ var ( ` COUNT(*) OVER ()` + ` FROM projections.executions1` + ` JOIN (` + - `SELECT instance_id, execution_id, JSONB_AGG( JSON_OBJECT( 'position' : position, 'include' : include, 'target' : target_id ) ) as targets` + - ` FROM projections.executions1_targets` + - ` GROUP BY instance_id, execution_id` + + `SELECT et.instance_id, et.execution_id, JSONB_AGG( JSON_OBJECT( 'position' : et.position, 'include' : et.include, 'target' : et.target_id ) ) as targets` + + ` FROM projections.executions1_targets AS et` + + ` INNER JOIN projections.targets2 AS t ON et.instance_id = t.instance_id AND et.target_id IS NOT NULL AND et.target_id = t.id` + + ` GROUP BY et.instance_id, et.execution_id` + `)` + ` AS execution_targets` + ` ON execution_targets.instance_id = projections.executions1.instance_id` + @@ -45,9 +46,10 @@ var ( ` execution_targets.targets` + ` FROM projections.executions1` + ` JOIN (` + - `SELECT instance_id, execution_id, JSONB_AGG( JSON_OBJECT( 'position' : position, 'include' : include, 'target' : target_id ) ) as targets` + - ` FROM projections.executions1_targets` + - ` GROUP BY instance_id, execution_id` + + `SELECT et.instance_id, et.execution_id, JSONB_AGG( JSON_OBJECT( 'position' : et.position, 'include' : et.include, 'target' : et.target_id ) ) as targets` + + ` FROM projections.executions1_targets AS et` + + ` INNER JOIN projections.targets2 AS t ON et.instance_id = t.instance_id AND et.target_id IS NOT NULL AND et.target_id = t.id` + + ` GROUP BY et.instance_id, et.execution_id` + `)` + ` AS execution_targets` + ` ON execution_targets.instance_id = projections.executions1.instance_id` + @@ -179,6 +181,63 @@ func Test_ExecutionPrepares(t *testing.T) { }, }, }, + { + name: "prepareExecutionsQuery multiple result, removed target, position missing", + prepare: prepareExecutionsQuery, + want: want{ + sqlExpectations: mockQueries( + regexp.QuoteMeta(prepareExecutionsStmt), + prepareExecutionsCols, + [][]driver.Value{ + { + "ro", + "id-1", + testNow, + testNow, + []byte(`[{"position" : 1, "target" : "target"}, {"position" : 3, "include" : "include"}]`), + }, + { + "ro", + "id-2", + testNow, + testNow, + []byte(`[{"position" : 2, "target" : "target"}, {"position" : 1, "include" : "include"}]`), + }, + }, + ), + }, + object: &Executions{ + SearchResponse: SearchResponse{ + Count: 2, + }, + Executions: []*Execution{ + { + ObjectDetails: domain.ObjectDetails{ + ID: "id-1", + EventDate: testNow, + CreationDate: testNow, + ResourceOwner: "ro", + }, + Targets: []*exec.Target{ + {Type: domain.ExecutionTargetTypeTarget, Target: "target"}, + {Type: domain.ExecutionTargetTypeInclude, Target: "include"}, + }, + }, + { + ObjectDetails: domain.ObjectDetails{ + ID: "id-2", + EventDate: testNow, + CreationDate: testNow, + ResourceOwner: "ro", + }, + Targets: []*exec.Target{ + {Type: domain.ExecutionTargetTypeInclude, Target: "include"}, + {Type: domain.ExecutionTargetTypeTarget, Target: "target"}, + }, + }, + }, + }, + }, { name: "prepareExecutionsQuery sql err", prepare: prepareExecutionsQuery, diff --git a/internal/query/projection/execution.go b/internal/query/projection/execution.go index 9001fcd3ba..1bd7f2e7f5 100644 --- a/internal/query/projection/execution.go +++ b/internal/query/projection/execution.go @@ -9,6 +9,7 @@ import ( "github.com/zitadel/zitadel/internal/eventstore/handler/v2" exec "github.com/zitadel/zitadel/internal/repository/execution" "github.com/zitadel/zitadel/internal/repository/instance" + "github.com/zitadel/zitadel/internal/repository/target" ) const ( @@ -78,6 +79,15 @@ func (p *executionProjection) Reducers() []handler.AggregateReducer { }, }, }, + { + Aggregate: target.AggregateType, + EventReducers: []handler.EventReducer{ + { + Event: target.RemovedEventType, + Reduce: p.reduceTargetRemoved, + }, + }, + }, { Aggregate: instance.AggregateType, EventReducers: []handler.EventReducer{ @@ -152,6 +162,21 @@ func (p *executionProjection) reduceExecutionSet(event eventstore.Event) (*handl return handler.NewMultiStatement(e, stmts...), nil } +func (p *executionProjection) reduceTargetRemoved(event eventstore.Event) (*handler.Statement, error) { + e, err := assertEvent[*target.RemovedEvent](event) + if err != nil { + return nil, err + } + return handler.NewDeleteStatement( + e, + []handler.Condition{ + handler.NewCond(ExecutionTargetInstanceIDCol, e.Aggregate().InstanceID), + handler.NewCond(ExecutionTargetTargetIDCol, e.Aggregate().ID), + }, + handler.WithTableSuffix(ExecutionTargetSuffix), + ), nil +} + func (p *executionProjection) reduceExecutionRemoved(event eventstore.Event) (*handler.Statement, error) { e, err := assertEvent[*exec.RemovedEvent](event) if err != nil { diff --git a/internal/query/projection/execution_test.go b/internal/query/projection/execution_test.go index 27d6e89258..aecae6905a 100644 --- a/internal/query/projection/execution_test.go +++ b/internal/query/projection/execution_test.go @@ -7,6 +7,7 @@ import ( "github.com/zitadel/zitadel/internal/eventstore/handler/v2" exec "github.com/zitadel/zitadel/internal/repository/execution" "github.com/zitadel/zitadel/internal/repository/instance" + "github.com/zitadel/zitadel/internal/repository/target" "github.com/zitadel/zitadel/internal/zerrors" ) @@ -79,6 +80,35 @@ func TestExecutionProjection_reduces(t *testing.T) { }, }, }, + { + name: "reduceTargetRemoved", + args: args{ + event: getEvent( + testEvent( + target.RemovedEventType, + target.AggregateType, + []byte(`{}`), + ), + eventstore.GenericEventMapper[target.RemovedEvent], + ), + }, + reduce: (&executionProjection{}).reduceTargetRemoved, + want: wantReduce{ + aggregateType: eventstore.AggregateType("target"), + sequence: 15, + executer: &testExecuter{ + executions: []execution{ + { + expectedStmt: "DELETE FROM projections.executions1_targets WHERE (instance_id = $1) AND (target_id = $2)", + expectedArgs: []interface{}{ + "instance-id", + "agg-id", + }, + }, + }, + }, + }, + }, { name: "reduceExecutionRemoved", args: args{