diff --git a/cmd/ateapi/internal/store/ateredis/ateredis.go b/cmd/ateapi/internal/store/ateredis/ateredis.go index 523dd6820..3b981ca6c 100644 --- a/cmd/ateapi/internal/store/ateredis/ateredis.go +++ b/cmd/ateapi/internal/store/ateredis/ateredis.go @@ -307,6 +307,27 @@ func (s *Persistence) DebugClearAll(ctx context.Context) error { return err } +// marshalStoredActor serializes dbActor for storage, clearing the identity +// fields as they are stored in the key instead of the value. It mutates +// dbActor: callers pass a clone they own. +func marshalStoredActor(dbActor *ateapipb.Actor) ([]byte, error) { + dbActor.Atespace = "" + dbActor.ActorId = "" + return protojson.Marshal(dbActor) +} + +// unmarshalStoredActor parses a stored actor value and backfills the identity +// from the key components. +func unmarshalStoredActor(data []byte, atespace, id string) (*ateapipb.Actor, error) { + actor := &ateapipb.Actor{} + if err := protojson.Unmarshal(data, actor); err != nil { + return nil, err + } + actor.Atespace = atespace + actor.ActorId = id + return actor, nil +} + func (s *Persistence) GetActor(ctx context.Context, atespace, id string) (*ateapipb.Actor, error) { dbKey := actorDBKey(atespace, id) @@ -318,15 +339,11 @@ func (s *Persistence) GetActor(ctx context.Context, atespace, id string) (*ateap return nil, fmt.Errorf("while getting actor key %q: %w", dbKey, err) } - actor := &ateapipb.Actor{} - if err := protojson.Unmarshal(dbActorBytes, actor); err != nil { + actor, err := unmarshalStoredActor(dbActorBytes, atespace, id) + if err != nil { return nil, fmt.Errorf("while unmarshaling actor: %w", err) } - if actor.GetActorId() != id || actor.GetAtespace() != atespace { - return nil, fmt.Errorf("(impossible) mismatch between stored id/atespace and key") - } - return actor, nil } @@ -338,9 +355,9 @@ func (s *Persistence) CreateActor(ctx context.Context, actor *ateapipb.Actor) er dbActor := proto.Clone(actor).(*ateapipb.Actor) dbActor.Version = 1 - dbActorBytes, err := protojson.Marshal(dbActor) + dbActorBytes, err := marshalStoredActor(dbActor) if err != nil { - return fmt.Errorf("in protojson.Marshal: %w", err) + return fmt.Errorf("while marshaling actor: %w", err) } ok, err := s.rdb.SetNX(ctx, dbKey, dbActorBytes, 0).Result() @@ -538,12 +555,6 @@ func (s *Persistence) UpdateActor(ctx context.Context, actor *ateapipb.Actor, ex return store.ErrPersistenceRetry } dbActor.Version = currentActor.GetVersion() + 1 - if currentActor.GetActorId() != dbActor.GetActorId() { - return fmt.Errorf("actor_id is immutable") - } - if currentActor.GetAtespace() != dbActor.GetAtespace() { - return fmt.Errorf("atespace is immutable") - } if currentActor.GetActorTemplateNamespace() != dbActor.GetActorTemplateNamespace() { return fmt.Errorf("actor_template_namespace is immutable") } @@ -551,9 +562,9 @@ func (s *Persistence) UpdateActor(ctx context.Context, actor *ateapipb.Actor, ex return fmt.Errorf("actor_template_name is immutable") } - newVal, err := protojson.Marshal(dbActor) + newVal, err := marshalStoredActor(dbActor) if err != nil { - return fmt.Errorf("in protojson.Marshal: %w", err) + return fmt.Errorf("while marshaling actor: %w", err) } _, err = tx.TxPipelined(ctx, func(pipe redis.Pipeliner) error { @@ -768,7 +779,7 @@ func (s *Persistence) fetchActors(ctx context.Context, master *redis.Client, key } var actors []*ateapipb.Actor - for _, cmd := range cmds { + for i, cmd := range cmds { getCmd, ok := cmd.(*redis.StringCmd) if !ok { continue @@ -780,9 +791,13 @@ func (s *Persistence) fetchActors(ctx context.Context, master *redis.Client, key return nil, fmt.Errorf("while getting actor: %w", getCmd.Err()) } - actor := &ateapipb.Actor{} - if err := protojson.Unmarshal([]byte(getCmd.Val()), actor); err != nil { - return nil, fmt.Errorf("in protojson.Unmarshal: %w", err) + parts := strings.Split(keys[i], ":") + if len(parts) != 3 { + return nil, fmt.Errorf("bad key format %q", keys[i]) + } + actor, err := unmarshalStoredActor([]byte(getCmd.Val()), parts[1], parts[2]) + if err != nil { + return nil, fmt.Errorf("while unmarshaling actor: %w", err) } actors = append(actors, actor) } diff --git a/cmd/ateapi/internal/store/ateredis/ateredis_test.go b/cmd/ateapi/internal/store/ateredis/ateredis_test.go index 559d794e6..4203134ff 100644 --- a/cmd/ateapi/internal/store/ateredis/ateredis_test.go +++ b/cmd/ateapi/internal/store/ateredis/ateredis_test.go @@ -19,6 +19,7 @@ import ( "errors" "fmt" "sort" + "strings" "sync" "testing" "time" @@ -1070,6 +1071,56 @@ func TestDeleteAtespace_EmptyAfterActorsRemoved(t *testing.T) { } } +func TestActorValue_OmitsIdentity(t *testing.T) { + mr, s, ctx := setupTest(t) + defer mr.Close() + + actor := &ateapipb.Actor{ + ActorId: "a1", + Atespace: "team-a", + ActorTemplateNamespace: "default", + ActorTemplateName: "tmpl", + Status: ateapipb.Actor_STATUS_SUSPENDED, + } + if err := s.CreateActor(ctx, actor); err != nil { + t.Fatalf("CreateActor failed: %v", err) + } + + assertNoIdentity := func(operation string) { + t.Helper() + raw, err := mr.Get("actor:team-a:a1") + if err != nil { + t.Fatalf("raw get after %s failed: %v", operation, err) + } + if strings.Contains(raw, `"actorId"`) || strings.Contains(raw, `"atespace"`) { + t.Errorf("value after %s duplicates key identity: %s", operation, raw) + } + } + assertNoIdentity("create") + + got, err := s.GetActor(ctx, "team-a", "a1") + if err != nil { + t.Fatalf("GetActor failed: %v", err) + } + if got.GetAtespace() != "team-a" || got.GetActorId() != "a1" { + t.Errorf("GetActor identity = (%q, %q), want (team-a, a1)", got.GetAtespace(), got.GetActorId()) + } + + got.Status = ateapipb.Actor_STATUS_RUNNING + if err := s.UpdateActor(ctx, got, got.GetVersion()); err != nil { + t.Fatalf("UpdateActor failed: %v", err) + } + assertNoIdentity("update") + + actors, _, err := s.ListActors(ctx, "team-a", 10, "") + if err != nil { + t.Fatalf("ListActors failed: %v", err) + } + if len(actors) != 1 || actors[0].GetAtespace() != "team-a" || actors[0].GetActorId() != "a1" { + t.Errorf("ListActors did not re-populate identity: %v", actors) + } +} + func TestDeleteAtespace_EmptyWhileOtherAtespaceNonEmpty(t *testing.T) { mr, s, ctx := setupTest(t) defer mr.Close()