diff --git a/cmd/ateapi/internal/store/atepg/atepg.go b/cmd/ateapi/internal/store/atepg/atepg.go index 791ac1f32c..a17c31c12c 100644 --- a/cmd/ateapi/internal/store/atepg/atepg.go +++ b/cmd/ateapi/internal/store/atepg/atepg.go @@ -188,19 +188,6 @@ type querier interface { Exec(ctx context.Context, sql string, args ...any) (pgconn.CommandTag, error) } -// TODO: EOL this in favor of setCreateMetadata -func newCreateMetadata(atespace, name string) *ateapipb.ResourceMetadata { - now := timestamppb.Now() - return &ateapipb.ResourceMetadata{ - Atespace: atespace, - Name: name, - Uid: uuid.NewString(), - Version: 1, - CreateTime: now, - UpdateTime: now, - } -} - func setCreateMetadata(metadata *ateapipb.ResourceMetadata) { metadata.Uid = uuid.NewString() metadata.Version = 1 @@ -208,14 +195,6 @@ func setCreateMetadata(metadata *ateapipb.ResourceMetadata) { metadata.UpdateTime = metadata.CreateTime } -// TODO: EOL this in favor of setUpdateMetadata -func newUpdateMetadata(current *ateapipb.ResourceMetadata) *ateapipb.ResourceMetadata { - metadata := proto.Clone(current).(*ateapipb.ResourceMetadata) - metadata.Version++ - metadata.UpdateTime = timestamppb.Now() - return metadata -} - // validateProtoMetadataMatchesColumns verifies that the metadata in the database // matches the metadata in the proto. func validateProtoMetadataMatchesColumns(resource string, metadata *ateapipb.ResourceMetadata, uid string, version int64) error { @@ -271,8 +250,14 @@ func pgErrConstraint(err error) string { func (p *Persistence) CreateAtespace(ctx context.Context, atespace *ateapipb.Atespace) (*ateapipb.Atespace, error) { name := atespace.GetMetadata().GetName() - dbAtespace := proto.Clone(atespace).(*ateapipb.Atespace) - dbAtespace.Metadata = newCreateMetadata("", name) + // The atespace is mutated in place: callers pass a dedicated object. + dbAtespace := atespace + if dbAtespace.Metadata == nil { + dbAtespace.Metadata = &ateapipb.ResourceMetadata{} + } + // Atespaces are global-scoped, so the atespace is always empty. + dbAtespace.Metadata.Atespace = "" + setCreateMetadata(dbAtespace.Metadata) protoBytes, err := proto.Marshal(dbAtespace) if err != nil { @@ -383,7 +368,8 @@ func (p *Persistence) DeleteAtespace(ctx context.Context, name string) (*ateapip func (p *Persistence) CreateActorTemplate(ctx context.Context, template *ateapipb.ActorTemplate) (*ateapipb.ActorTemplate, error) { atespace, name := template.GetMetadata().GetAtespace(), template.GetMetadata().GetName() - dbTemplate := proto.Clone(template).(*ateapipb.ActorTemplate) + // The template is mutated in place: callers pass a dedicated object. + dbTemplate := template if dbTemplate.Metadata == nil { dbTemplate.Metadata = &ateapipb.ResourceMetadata{} } @@ -588,11 +574,12 @@ func (p *Persistence) CreateActor(ctx context.Context, actor *ateapipb.Actor) (* atespace := actor.GetMetadata().GetAtespace() name := actor.GetMetadata().GetName() - // TODO: doing a full clone here is wasteful - the caller already has to - // make modifications to the actor before passing it in, so we can safely - // mutate it in place. This breaks some of the contract tests, so we can - // fix it later. - dbActor := proto.Clone(actor).(*ateapipb.Actor) + // The actor is mutated in place: the caller already builds a dedicated + // object to pass in, so a defensive clone is wasted work. + dbActor := actor + if dbActor.Metadata == nil { + dbActor.Metadata = &ateapipb.ResourceMetadata{} + } setCreateMetadata(dbActor.Metadata) protoBytes, err := proto.Marshal(dbActor) @@ -845,8 +832,16 @@ func (p *Persistence) listActorsGlobal(ctx context.Context, pageSize int32, page // --- Actor egress policies --- func (p *Persistence) CreateEgressPolicy(ctx context.Context, actorRef resources.ActorRef, policy *ateapipb.EgressPolicy) (*ateapipb.EgressPolicy, error) { - dbPolicy := proto.Clone(policy).(*ateapipb.EgressPolicy) - dbPolicy.Metadata = newCreateMetadata(actorRef.Atespace, "default") + // The policy is mutated in place: callers pass a dedicated object. + dbPolicy := policy + if dbPolicy.Metadata == nil { + dbPolicy.Metadata = &ateapipb.ResourceMetadata{} + } + // The policy is a singleton nested under its actor: its identity comes + // from the actor, not from caller-supplied metadata. + dbPolicy.Metadata.Atespace = actorRef.Atespace + dbPolicy.Metadata.Name = "default" + setCreateMetadata(dbPolicy.Metadata) protoBytes, err := proto.Marshal(dbPolicy) if err != nil { return nil, fmt.Errorf("marshaling egress policy: %w", err) @@ -960,7 +955,10 @@ func (p *Persistence) CreateActorSnapshot(ctx context.Context, snapshot *ateapip atespace := snapshot.GetMetadata().GetAtespace() name := snapshot.GetMetadata().GetName() dbSnapshot := proto.Clone(snapshot).(*ateapipb.ActorSnapshot) - dbSnapshot.Metadata = newCreateMetadata(atespace, name) + if dbSnapshot.Metadata == nil { + dbSnapshot.Metadata = &ateapipb.ResourceMetadata{} + } + setCreateMetadata(dbSnapshot.Metadata) protoBytes, err := proto.Marshal(dbSnapshot) if err != nil { @@ -1129,7 +1127,10 @@ func (p *Persistence) CreateActorSnapshotTag(ctx context.Context, snapshotRef re tagAtespace := tag.GetMetadata().GetAtespace() tagName := tag.GetMetadata().GetName() dbTag := proto.Clone(tag).(*ateapipb.ActorSnapshotTag) - dbTag.Metadata = newCreateMetadata(tagAtespace, tagName) + if dbTag.Metadata == nil { + dbTag.Metadata = &ateapipb.ResourceMetadata{} + } + setCreateMetadata(dbTag.Metadata) dbTag.Snapshot = &ateapipb.ObjectRef{Atespace: snapshotAtespace, Name: snapshotName} protoBytes, err := proto.Marshal(dbTag) if err != nil { @@ -1238,9 +1239,12 @@ func (p *Persistence) UpdateActorSnapshotTag(ctx context.Context, tagRef resourc if err := validateUpdateActorSnapshotTagMutation(tagBeforeMutation, dbTag); err != nil { return nil, fmt.Errorf("%w: %w", store.ErrImmutableField, err) } - // Stored metadata is authoritative; discard any metadata edits made by the - // closure and derive the next revision from the state this attempt read. - dbTag.Metadata = newUpdateMetadata(tagBeforeMutation.GetMetadata()) + // Stored server-assigned metadata is authoritative; the next revision is + // derived from the state this attempt read. + if dbTag.Metadata == nil { + dbTag.Metadata = &ateapipb.ResourceMetadata{} + } + setUpdateMetadata(dbTag.Metadata, tagBeforeMutation.GetMetadata()) updatedBytes, err := proto.Marshal(dbTag) if err != nil { @@ -1285,7 +1289,8 @@ func (p *Persistence) DeleteActorSnapshotTag(ctx context.Context, tagRef resourc // --- Workers --- func (p *Persistence) CreateWorker(ctx context.Context, worker *ateapipb.Worker) (*ateapipb.Worker, error) { - dbWorker := proto.Clone(worker).(*ateapipb.Worker) + // The worker is mutated in place: callers pass a dedicated object. + dbWorker := worker if dbWorker.Metadata == nil { dbWorker.Metadata = &ateapipb.ResourceMetadata{} } diff --git a/cmd/ateapi/internal/store/store.go b/cmd/ateapi/internal/store/store.go index bc1d055fb7..1cbfbe5482 100644 --- a/cmd/ateapi/internal/store/store.go +++ b/cmd/ateapi/internal/store/store.go @@ -67,9 +67,10 @@ var ( // Interface defines the contract for the persistence layer storing actor state. type Interface interface { // Stores a new actor in suspended state and returns the stored resource with - // server-assigned metadata (uid, version, timestamps). The input is not - // mutated. Returns ErrAlreadyExists if key is taken, or - // ErrFailedPrecondition if the actor's atespace does not exist. + // server-assigned metadata (uid, version, timestamps). The input may be + // mutated in place: the caller passes a dedicated object, so the store + // stamps its metadata rather than cloning. Returns ErrAlreadyExists if key + // is taken, or ErrFailedPrecondition if the actor's atespace does not exist. CreateActor(ctx context.Context, actor *ateapipb.Actor) (*ateapipb.Actor, error) // Fetches an actor by reference. Returns ErrNotFound if missing. @@ -159,8 +160,8 @@ type Interface interface { DeleteActorSnapshotTag(ctx context.Context, tagRef resources.ActorSnapshotTagRef) (*ateapipb.ActorSnapshotTag, error) // Stores a new atespace and returns the stored resource with server-assigned - // metadata (uid, version, timestamps). The input is not mutated. Returns - // ErrAlreadyExists if the name is taken. + // metadata (uid, version, timestamps). The input may be mutated in place. + // Returns ErrAlreadyExists if the name is taken. CreateAtespace(ctx context.Context, atespace *ateapipb.Atespace) (*ateapipb.Atespace, error) // Fetches an atespace by name. Returns ErrNotFound if missing. @@ -175,8 +176,8 @@ type Interface interface { DeleteAtespace(ctx context.Context, name string) (*ateapipb.Atespace, error) // Stores a new ActorTemplate and returns the stored resource with - // server-assigned metadata (uid, version, timestamps). The input is not - // mutated. Returns ErrAlreadyExists if the (atespace, name) is taken. + // server-assigned metadata (uid, version, timestamps). The input may be + // mutated in place. Returns ErrAlreadyExists if the (atespace, name) is taken. CreateActorTemplate(ctx context.Context, template *ateapipb.ActorTemplate) (*ateapipb.ActorTemplate, error) // Fetches an ActorTemplate by reference. Returns ErrNotFound if missing. @@ -199,8 +200,8 @@ type Interface interface { DeleteActorTemplate(ctx context.Context, templateRef resources.ActorTemplateRef) (*ateapipb.ActorTemplate, error) // Registers a new idle worker and returns the stored resource with - // server-assigned metadata (uid, version, timestamps). The input is not - // mutated. Returns ErrAlreadyExists if already registered. + // server-assigned metadata (uid, version, timestamps). The input may be + // mutated in place. Returns ErrAlreadyExists if already registered. CreateWorker(ctx context.Context, worker *ateapipb.Worker) (*ateapipb.Worker, error) // Fetches worker state by name. Returns ErrNotFound if missing. diff --git a/cmd/ateapi/internal/store/storecontract/contract.go b/cmd/ateapi/internal/store/storecontract/contract.go index b28d3e73e5..fd25479747 100644 --- a/cmd/ateapi/internal/store/storecontract/contract.go +++ b/cmd/ateapi/internal/store/storecontract/contract.go @@ -356,9 +356,8 @@ func runActorContractTests(t *testing.T, setup func(t *testing.T) store.Interfac t.Errorf("CreateActor returned unset create/update time") } - if actor.GetMetadata().GetUid() != "" || actor.GetMetadata().GetVersion() != 0 { - t.Errorf("CreateActor must not mutate its input, got metadata %v", actor.GetMetadata()) - } + // CreateActor may stamp the input's metadata in place instead of + // cloning; the caller passes a dedicated object. got, err := s.GetActor(ctx, resources.ActorRefFromActor(actor)) if err != nil {