From dc47a061efd362986c4477fc6cffc20b95d22772 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Thu, 4 Jun 2026 12:02:57 -0700 Subject: [PATCH] refactor(change): persist typed change details from the change provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? The change provider already produces rich per-URI facts (author, changed files, line counts), but its value types lived in the extension layer and the data was thrown away — validate fetched ChangeInfo only to log a file count, and ChangeRecord stored an opaque Metadata JSON string that was never written. Nothing downstream could read typed change facts. ### What? - Move the change value types into entities: entity.User, entity.ChangedFile (now with LinesModified), entity.ChangeDetails (the facts), and entity.ChangeInfo (URI -> Details), with aggregation helpers. The changeprovider extension and GitHub impl now produce these. - Replace ChangeRecord.Metadata (opaque string) with typed Details (ChangeDetails); the change table's metadata JSON column becomes details. - Add ChangeStore.UpdateDetails — a version-guarded conditional write, following the optimistic-locking contract (arithmetic in the controller). - validate now persists each fetched ChangeInfo onto the request's change records (per-URI, idempotent; ErrVersionMismatch is a benign no-op). This is the producer half: typed details now exist and are persisted. The score controller consumes them in a follow-up. ## Test Plan - ✅ `make build`, `make test`, `make lint`, `make check-mocks/gazelle/tidy` - ✅ `make integration-test` (storage contract suite round-trips Details and covers UpdateDetails create/update/version-mismatch) --- submitqueue/entity/BUILD.bazel | 1 + submitqueue/entity/change_provider.go | 76 +++++++++++++++++ submitqueue/entity/change_record.go | 29 +++---- .../changeprovider/change_provider.go | 37 ++------ .../changeprovider/github/convert.go | 31 +++---- .../changeprovider/github/graphql.go | 2 - .../changeprovider/github/graphql_test.go | 4 +- .../changeprovider/github/provider.go | 8 +- .../changeprovider/github/provider_test.go | 4 +- .../mock/change_provider_mock.go | 4 +- submitqueue/extension/storage/change_store.go | 14 +-- .../extension/storage/mysql/change_store.go | 28 ++++-- .../extension/storage/mysql/schema/change.sql | 2 +- .../orchestrator/controller/start/start.go | 45 ++-------- .../controller/start/start_test.go | 85 ++----------------- .../controller/validate/BUILD.bazel | 1 - .../controller/validate/validate.go | 49 +++++++++-- .../controller/validate/validate_test.go | 77 ++++++++++++++--- .../submitqueue/extension/storage/suite.go | 31 ++++--- 19 files changed, 294 insertions(+), 234 deletions(-) create mode 100644 submitqueue/entity/change_provider.go diff --git a/submitqueue/entity/BUILD.bazel b/submitqueue/entity/BUILD.bazel index 39a077dc..d99f079b 100644 --- a/submitqueue/entity/BUILD.bazel +++ b/submitqueue/entity/BUILD.bazel @@ -7,6 +7,7 @@ go_library( "batch_dependent.go", "build.go", "cancel_request.go", + "change_provider.go", "change_record.go", "land_request.go", "queue_config.go", diff --git a/submitqueue/entity/change_provider.go b/submitqueue/entity/change_provider.go new file mode 100644 index 00000000..9ffacc0a --- /dev/null +++ b/submitqueue/entity/change_provider.go @@ -0,0 +1,76 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package entity + +// Author represents the author of a change. +type Author struct { + // Name is the display name of the author. + Name string `json:"name"` + // Email is the email address of the author. + Email string `json:"email"` +} + +// ChangedFile represents a single file modification in a change. +type ChangedFile struct { + // Path is the file path relative to the repository root. + Path string `json:"path"` + // LinesAdded is the number of lines added in this file. + LinesAdded int `json:"lines_added"` + // LinesDeleted is the number of lines deleted in this file. + LinesDeleted int `json:"lines_deleted"` + // LinesModified is the number of lines modified in this file. Some providers + // (e.g. GitHub) report only additions and deletions and leave this zero. + LinesModified int `json:"lines_modified"` +} + +// TotalLines returns the total number of lines touched in this file. +func (f ChangedFile) TotalLines() int { + return f.LinesAdded + f.LinesDeleted + f.LinesModified +} + +// ChangeDetails holds the provider-supplied facts about a single change (author, +// modified files, line counts). It carries no identity — the owning URI lives on +// ChangeInfo (provider correlation) and ChangeRecord (persisted claim). +type ChangeDetails struct { + // Author is the author of the change. + Author Author `json:"author"` + // ChangedFiles is the list of files modified in this change. Order is unspecified. + ChangedFiles []ChangedFile `json:"changed_files,omitempty"` +} + +// TotalLinesChanged returns the total number of lines touched across all files in the change. +func (d ChangeDetails) TotalLinesChanged() int { + total := 0 + for _, f := range d.ChangedFiles { + total += f.TotalLines() + } + return total +} + +// FileCount returns the number of files touched in the change. +func (d ChangeDetails) FileCount() int { + return len(d.ChangedFiles) +} + +// ChangeInfo maps a change URI to its details. It is the change provider's return +// type: for a Change with multiple URIs (e.g. a stacked PR set), the provider +// returns one ChangeInfo per URI so callers can correlate results to inputs by URI. +type ChangeInfo struct { + // URI is the full change URI for correlation with the input request + // (e.g., "github://uber/repo/pull/98/c3a4d5e6f7890123456789abcdef0123456789ab"). + URI string `json:"uri"` + // Details is the provider-supplied facts for this URI. + Details ChangeDetails `json:"details"` +} diff --git a/submitqueue/entity/change_record.go b/submitqueue/entity/change_record.go index 8ba0ff7d..0024a7fa 100644 --- a/submitqueue/entity/change_record.go +++ b/submitqueue/entity/change_record.go @@ -15,9 +15,9 @@ package entity // ChangeRecord represents a single URI's claim by a request, persisted in the change store. -// The (Queue, URI, RequestID) triple is the identity and is immutable; Metadata may be -// updated over time as additional information about the change (e.g., PR title, author, -// mergeability) becomes available. +// The whole record is immutable: the (Queue, URI, RequestID) triple is its identity and the +// Details (author, changed files, line counts) are captured once at claim time from the +// change provider. There is no update path. type ChangeRecord struct { // URI identifies the change (RFC 3986). Same scheme/format as entity.Change.URIs. // Example: "github://uber/submitqueue/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab". @@ -29,7 +29,7 @@ type ChangeRecord struct { // RequestID participates in the change-store primary key so that concurrent claims // by different requests on the same URI coexist as distinct rows. Same-request // retries collide on the PK and are absorbed idempotently; different-request - // collisions surface as additional rows that callers detect via FindOverlapping. + // collisions surface as additional rows that callers detect via GetByURI. RequestID string `json:"request_id"` // Queue is the queue the owning request belongs to. It is the leading column of @@ -37,21 +37,20 @@ type ChangeRecord struct { // scans and the table is shardable by queue. Queue string `json:"queue"` - // Metadata is a JSON-encoded blob of provider-specific information about the change - // (e.g., PR title, author, mergeable state). Stored as `'{}'` when no metadata has - // been populated yet; updated by downstream enrichment. - Metadata string `json:"metadata,omitempty"` + // Details holds the provider-supplied facts about the change (author, changed + // files, line counts). It is captured at claim time (the validate controller, after + // fetching from the change provider) and written once with the record — records are + // immutable, so Details is never updated after Create. + Details ChangeDetails `json:"details"` - // CreatedAt is the Unix milliseconds timestamp when this record was first created. + // CreatedAt is the Unix milliseconds timestamp when this record was created. CreatedAt int64 `json:"created_at"` - // UpdatedAt is the Unix milliseconds timestamp when this record's Metadata was last updated. - // Equal to CreatedAt when the record has never been updated. + // UpdatedAt is the Unix milliseconds timestamp when this record was created. Records + // are immutable, so it always equals CreatedAt; retained for schema symmetry. UpdatedAt int64 `json:"updated_at"` - // Version is the optimistic-locking counter for mutable fields (Metadata). - // Starts at 1 on Create and is incremented by callers on every update. - // Mirrors the request-store convention: callers compute newVersion = oldVersion + 1 - // and pass both to the update method; the store performs a pure conditional write. + // Version is the record version. Records are immutable, so it is always 1; retained + // for schema symmetry with the other stores. Version int32 `json:"version"` } diff --git a/submitqueue/extension/changeprovider/change_provider.go b/submitqueue/extension/changeprovider/change_provider.go index 47a91144..9b4b7433 100644 --- a/submitqueue/extension/changeprovider/change_provider.go +++ b/submitqueue/extension/changeprovider/change_provider.go @@ -22,44 +22,17 @@ import ( "github.com/uber/submitqueue/submitqueue/entity" ) -// User represents the author of a change. -type User struct { - // Name is the display name of the user. - Name string - // Email is the email address of the user. - Email string -} - -// ChangedFile represents a single file modification in a change. -type ChangedFile struct { - // Path is the file path relative to the repository root. - Path string - // Patch is the diff patch content for this file. - Patch string - // LinesAdded is the number of lines added in this file. - LinesAdded int - // LinesDeleted is the number of lines deleted in this file. - LinesDeleted int -} - -// ChangeInfo contains metadata and file changes for a code change. -type ChangeInfo struct { - // URI is the full change URI for correlation with the input request - // (e.g., "github://uber/repo/pull/98/c3a4d5e6f7890123456789abcdef0123456789ab" or "phab://D123/xyz789"). - URI string - // User is the author of the change. - User User - // ChangedFiles is the list of files modified in this change. Order is unspecified. - ChangedFiles []ChangedFile -} - // ChangeProvider fetches change metadata from external systems // Each implementation is configured for a specific provider (GitHub, GitLab, Phabricator). +// +// The change value types it produces — entity.ChangeInfo, entity.ChangeDetails, +// entity.Author, entity.ChangedFile — live in the entity package so the same typed +// facts can be persisted (entity.ChangeRecord) and scored without re-declaration. type ChangeProvider interface { // Get retrieves change information for the provided Change. // For a Change with multiple URIs (e.g., stacked PRs), returns one ChangeInfo per URI. // Returns a slice of ChangeInfo, one for each change in the stack. - Get(ctx context.Context, change entity.Change) ([]ChangeInfo, error) + Get(ctx context.Context, change entity.Change) ([]entity.ChangeInfo, error) } // Config carries the per-queue identity handed to a Factory. The system knows diff --git a/submitqueue/extension/changeprovider/github/convert.go b/submitqueue/extension/changeprovider/github/convert.go index 40c28de1..474e500a 100644 --- a/submitqueue/extension/changeprovider/github/convert.go +++ b/submitqueue/extension/changeprovider/github/convert.go @@ -1,32 +1,33 @@ package github import ( + "github.com/uber/submitqueue/submitqueue/entity" entitygithub "github.com/uber/submitqueue/submitqueue/entity/github" - "github.com/uber/submitqueue/submitqueue/extension/changeprovider" ) -// convertToChangeInfo converts GitHub PR data to ChangeInfo. -func convertToChangeInfo(parsed entitygithub.ChangeID, prData *pullRequestData) changeprovider.ChangeInfo { - changedFiles := convertFiles(prData.Files.Nodes) - - return changeprovider.ChangeInfo{ +// convertToChangeInfo converts GitHub PR data to an entity.ChangeInfo. +func convertToChangeInfo(parsed entitygithub.ChangeID, prData *pullRequestData) entity.ChangeInfo { + return entity.ChangeInfo{ URI: parsed.String(), - User: changeprovider.User{ - Name: prData.Author.Name, - Email: prData.Author.Email, + Details: entity.ChangeDetails{ + Author: entity.Author{ + Name: prData.Author.Name, + Email: prData.Author.Email, + }, + ChangedFiles: convertFiles(prData.Files.Nodes), }, - ChangedFiles: changedFiles, } } -// convertFiles converts GitHub file nodes to ChangedFile structs. -func convertFiles(nodes []fileNode) []changeprovider.ChangedFile { - changedFiles := make([]changeprovider.ChangedFile, 0, len(nodes)) +// convertFiles converts GitHub file nodes to entity.ChangedFile structs. +// GitHub's API reports only additions and deletions per file, so LinesModified +// is left zero here. +func convertFiles(nodes []fileNode) []entity.ChangedFile { + changedFiles := make([]entity.ChangedFile, 0, len(nodes)) for _, file := range nodes { - changedFiles = append(changedFiles, changeprovider.ChangedFile{ + changedFiles = append(changedFiles, entity.ChangedFile{ Path: file.Path, - Patch: file.Patch, LinesAdded: file.Additions, LinesDeleted: file.Deletions, }) diff --git a/submitqueue/extension/changeprovider/github/graphql.go b/submitqueue/extension/changeprovider/github/graphql.go index 0d527710..fc24ee5b 100644 --- a/submitqueue/extension/changeprovider/github/graphql.go +++ b/submitqueue/extension/changeprovider/github/graphql.go @@ -34,7 +34,6 @@ query($owner: String!, $repo: String!, $prNumber: Int!, $filesCursor: String) { additions deletions changeType - patch } } } @@ -98,7 +97,6 @@ type fileNode struct { Additions int `json:"additions"` Deletions int `json:"deletions"` ChangeType string `json:"changeType"` - Patch string `json:"patch"` } // buildGraphQLRequest builds a GraphQL request for fetching pull request data. diff --git a/submitqueue/extension/changeprovider/github/graphql_test.go b/submitqueue/extension/changeprovider/github/graphql_test.go index 605f449d..7eab8e45 100644 --- a/submitqueue/extension/changeprovider/github/graphql_test.go +++ b/submitqueue/extension/changeprovider/github/graphql_test.go @@ -77,7 +77,7 @@ func TestParseGraphQLResponse(t *testing.T) { "files": { "totalCount": 1, "pageInfo": {"endCursor": "cur1", "hasNextPage": false}, - "nodes": [{"path": "main.go", "additions": 10, "deletions": 2, "changeType": "MODIFIED", "patch": "diff content"}] + "nodes": [{"path": "main.go", "additions": 10, "deletions": 2, "changeType": "MODIFIED"}] } } } @@ -90,7 +90,7 @@ func TestParseGraphQLResponse(t *testing.T) { Files: filesData{ TotalCount: 1, PageInfo: pageInfo{EndCursor: "cur1", HasNextPage: false}, - Nodes: []fileNode{{Path: "main.go", Additions: 10, Deletions: 2, ChangeType: "MODIFIED", Patch: "diff content"}}, + Nodes: []fileNode{{Path: "main.go", Additions: 10, Deletions: 2, ChangeType: "MODIFIED"}}, }, }, }, diff --git a/submitqueue/extension/changeprovider/github/provider.go b/submitqueue/extension/changeprovider/github/provider.go index 5c1274d1..6f86e621 100644 --- a/submitqueue/extension/changeprovider/github/provider.go +++ b/submitqueue/extension/changeprovider/github/provider.go @@ -44,7 +44,7 @@ func NewProvider(params Params) changeprovider.ChangeProvider { // Get retrieves change information from GitHub for the provided Change. // Returns one ChangeInfo per URI (one per PR in stacked changes). -func (p *provider) Get(ctx context.Context, change entity.Change) (_ []changeprovider.ChangeInfo, retErr error) { +func (p *provider) Get(ctx context.Context, change entity.Change) (_ []entity.ChangeInfo, retErr error) { op := coremetrics.Begin(p.metricsScope, "get") defer func() { op.Complete(retErr) }() @@ -85,8 +85,8 @@ func (p *provider) Get(ctx context.Context, change entity.Change) (_ []changepro func (p *provider) fetchAllPRs( ctx context.Context, changeIDs []entitygithub.ChangeID, -) ([]changeprovider.ChangeInfo, error) { - changeInfos := make([]changeprovider.ChangeInfo, 0, len(changeIDs)) +) ([]entity.ChangeInfo, error) { + changeInfos := make([]entity.ChangeInfo, 0, len(changeIDs)) for _, cid := range changeIDs { prData, err := p.fetchPullRequest(ctx, cid) @@ -109,7 +109,7 @@ func (p *provider) fetchAllPRs( "org", cid.Org, "repo", cid.Repo, "pr", cid.PRNumber, - "files_count", len(changeInfo.ChangedFiles), + "files_count", len(changeInfo.Details.ChangedFiles), "head_sha", prData.HeadRefOid, ) } diff --git a/submitqueue/extension/changeprovider/github/provider_test.go b/submitqueue/extension/changeprovider/github/provider_test.go index 3f57d51f..9ae9d0cb 100644 --- a/submitqueue/extension/changeprovider/github/provider_test.go +++ b/submitqueue/extension/changeprovider/github/provider_test.go @@ -115,7 +115,7 @@ func TestProvider_Get(t *testing.T) { require.NoError(t, err) require.Len(t, infos, 1) assert.Equal(t, tt.uris[0], infos[0].URI) - assert.Len(t, infos[0].ChangedFiles, 2) + assert.Len(t, infos[0].Details.ChangedFiles, 2) }) } } @@ -154,7 +154,7 @@ func TestProvider_Get_Pagination(t *testing.T) { require.NoError(t, err) assert.Equal(t, 2, callCount) require.Len(t, infos, 1) - assert.Len(t, infos[0].ChangedFiles, 2) + assert.Len(t, infos[0].Details.ChangedFiles, 2) } func TestProvider_Get_MultiplePRs(t *testing.T) { diff --git a/submitqueue/extension/changeprovider/mock/change_provider_mock.go b/submitqueue/extension/changeprovider/mock/change_provider_mock.go index 948c32a0..a1afc4ae 100644 --- a/submitqueue/extension/changeprovider/mock/change_provider_mock.go +++ b/submitqueue/extension/changeprovider/mock/change_provider_mock.go @@ -43,10 +43,10 @@ func (m *MockChangeProvider) EXPECT() *MockChangeProviderMockRecorder { } // Get mocks base method. -func (m *MockChangeProvider) Get(ctx context.Context, change entity.Change) ([]changeprovider.ChangeInfo, error) { +func (m *MockChangeProvider) Get(ctx context.Context, change entity.Change) ([]entity.ChangeInfo, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Get", ctx, change) - ret0, _ := ret[0].([]changeprovider.ChangeInfo) + ret0, _ := ret[0].([]entity.ChangeInfo) ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/submitqueue/extension/storage/change_store.go b/submitqueue/extension/storage/change_store.go index a995f33c..a95c460f 100644 --- a/submitqueue/extension/storage/change_store.go +++ b/submitqueue/extension/storage/change_store.go @@ -24,19 +24,19 @@ import ( // ChangeStore manages per-URI claim records for in-flight land requests. // Each row records that a specific URI was claimed by a specific request, scoped to a queue. -// The (Queue, URI, RequestID) triple is the immutable identity of a record. Metadata may -// evolve over time. +// The (Queue, URI, RequestID) triple is the immutable identity of a record, and a record's +// Details are captured at claim time and never updated — records are immutable. // // The interface is intentionally per-record / per-URI so that any backend (SQL, DynamoDB, // Bigtable, …) can implement it without needing batch-atomicity or multi-key query support. // Callers loop when they have multiple URIs to claim or check; the typical request has a // small number of URIs (a single PR or a short stack), so the loop overhead is negligible. type ChangeStore interface { - // Create persists a single ChangeRecord. A primary-key conflict on - // (Queue, URI, RequestID) is silently ignored, which makes the call - // idempotent under queue redeliveries of the same request. Records belonging - // to different requests do not conflict on the PK — cross-request overlap - // is detected by GetByURI, not by Create. + // Create persists a single ChangeRecord (identity + Details) in one write. A + // primary-key conflict on (Queue, URI, RequestID) is silently ignored, which makes + // the call idempotent under queue redeliveries of the same request (first write + // wins). Records belonging to different requests do not conflict on the PK — + // cross-request overlap is detected by GetByURI, not by Create. Create(ctx context.Context, record entity.ChangeRecord) error // GetByURI returns every ChangeRecord for the given (queue, uri). Multiple diff --git a/submitqueue/extension/storage/mysql/change_store.go b/submitqueue/extension/storage/mysql/change_store.go index c83ae92b..be25b18f 100644 --- a/submitqueue/extension/storage/mysql/change_store.go +++ b/submitqueue/extension/storage/mysql/change_store.go @@ -17,6 +17,7 @@ package mysql import ( "context" "database/sql" + "encoding/json" "fmt" "github.com/uber-go/tally/v4" @@ -43,16 +44,14 @@ func (s *changeStore) Create(ctx context.Context, record entity.ChangeRecord) (r op := metrics.Begin(s.scope, "create") defer func() { op.Complete(retErr) }() - // Use the empty JSON object as the canonical "no metadata yet" value. - // metadata is NOT NULL in the schema and the JSON column type rejects an empty string. - metadata := record.Metadata - if metadata == "" { - metadata = "{}" + detailsJSON, err := marshalDetails(record.Details) + if err != nil { + return fmt.Errorf("failed to marshal details for change record uri=%s request_id=%s: %w", record.URI, record.RequestID, err) } - const query = "INSERT IGNORE INTO `change` (uri, request_id, queue, metadata, created_at, updated_at, version) VALUES (?, ?, ?, ?, ?, ?, ?)" + const query = "INSERT IGNORE INTO `change` (uri, request_id, queue, details, created_at, updated_at, version) VALUES (?, ?, ?, ?, ?, ?, ?)" if _, err := s.db.ExecContext(ctx, query, - record.URI, record.RequestID, record.Queue, metadata, record.CreatedAt, record.UpdatedAt, record.Version, + record.URI, record.RequestID, record.Queue, detailsJSON, record.CreatedAt, record.UpdatedAt, record.Version, ); err != nil { return fmt.Errorf("failed to insert change record uri=%s request_id=%s: %w", record.URI, record.RequestID, err) } @@ -65,7 +64,7 @@ func (s *changeStore) GetByURI(ctx context.Context, queue string, uri string) (r op := metrics.Begin(s.scope, "get_by_uri") defer func() { op.Complete(retErr) }() - const query = "SELECT uri, request_id, queue, metadata, created_at, updated_at, version FROM `change` WHERE queue = ? AND uri = ?" + const query = "SELECT uri, request_id, queue, details, created_at, updated_at, version FROM `change` WHERE queue = ? AND uri = ?" rows, err := s.db.QueryContext(ctx, query, queue, uri) if err != nil { return nil, fmt.Errorf("failed to query change records for queue=%s uri=%s: %w", queue, uri, err) @@ -75,9 +74,13 @@ func (s *changeStore) GetByURI(ctx context.Context, queue string, uri string) (r var results []entity.ChangeRecord for rows.Next() { var rec entity.ChangeRecord - if err := rows.Scan(&rec.URI, &rec.RequestID, &rec.Queue, &rec.Metadata, &rec.CreatedAt, &rec.UpdatedAt, &rec.Version); err != nil { + var detailsJSON []byte + if err := rows.Scan(&rec.URI, &rec.RequestID, &rec.Queue, &detailsJSON, &rec.CreatedAt, &rec.UpdatedAt, &rec.Version); err != nil { return nil, fmt.Errorf("failed to scan change record for queue=%s uri=%s: %w", queue, uri, err) } + if err := json.Unmarshal(detailsJSON, &rec.Details); err != nil { + return nil, fmt.Errorf("failed to unmarshal details for change record queue=%s uri=%s request_id=%s: %w", queue, uri, rec.RequestID, err) + } results = append(results, rec) } if err := rows.Err(); err != nil { @@ -85,3 +88,10 @@ func (s *changeStore) GetByURI(ctx context.Context, queue string, uri string) (r } return results, nil } + +// marshalDetails serializes ChangeDetails to JSON for the NOT NULL `details` JSON +// column. A zero-value ChangeDetails marshals to a non-empty JSON object, so no +// empty-string special-casing is needed. +func marshalDetails(details entity.ChangeDetails) ([]byte, error) { + return json.Marshal(details) +} diff --git a/submitqueue/extension/storage/mysql/schema/change.sql b/submitqueue/extension/storage/mysql/schema/change.sql index d5397754..e808021b 100644 --- a/submitqueue/extension/storage/mysql/schema/change.sql +++ b/submitqueue/extension/storage/mysql/schema/change.sql @@ -7,7 +7,7 @@ CREATE TABLE IF NOT EXISTS `change` ( uri VARCHAR(255) NOT NULL, request_id VARCHAR(255) NOT NULL, queue VARCHAR(255) NOT NULL, - metadata JSON NOT NULL, + details JSON NOT NULL, created_at BIGINT NOT NULL, updated_at BIGINT NOT NULL, version INT NOT NULL, diff --git a/submitqueue/orchestrator/controller/start/start.go b/submitqueue/orchestrator/controller/start/start.go index f389d139..9a442929 100644 --- a/submitqueue/orchestrator/controller/start/start.go +++ b/submitqueue/orchestrator/controller/start/start.go @@ -18,7 +18,6 @@ import ( "context" "errors" "fmt" - "time" "github.com/uber-go/tally/v4" "github.com/uber/submitqueue/core/metrics" @@ -31,11 +30,11 @@ import ( ) // Controller handles start queue messages. -// It consumes requests, persists them to the request store, claims their URIs in -// the change store, and publishes to the validate stage. Both writes are idempotent -// on retries; the duplicate-detection check itself is performed downstream by the -// validate controller, which reads the change store and consults the request store -// for liveness. Implements consumer.Controller. +// It consumes requests, persists them to the request store, and publishes to the +// validate stage. The request write is idempotent on retries. URI claiming and +// duplicate detection are performed downstream by the validate controller (which +// claims each URI in the change store with its provider details and consults the +// request store for liveness). Implements consumer.Controller. type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope @@ -68,8 +67,8 @@ func NewController( } // Process processes a request delivery from the queue. -// Persists the request, claims its URIs in the change store, and publishes to validate. -// Returns nil to ack (success), or error to nack (retry). +// Persists the request and publishes to validate. Returns nil to ack (success), +// or error to nack (retry). func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (retErr error) { const opName = "process" @@ -113,15 +112,6 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r return fmt.Errorf("failed to create request: %w", err) } - // Claim each URI in the change store. Different requests on the same URI write - // distinct rows (different request_id), so cross-request URI overlap does not - // collide on insert; the validate controller surfaces it via GetByURI + a - // liveness check against the request store. - if err := c.claimURIs(ctx, request); err != nil { - metrics.NamedCounter(c.metricsScope, opName, "change_store_errors", 1) - return fmt.Errorf("failed to claim URIs for request %s: %w", request.ID, err) - } - // Record the "new" status in the request log. logEntry := entity.NewRequestLog(request.ID, entity.RequestStatusStarted, request.Version, "", nil) if err := corerequest.PublishLog(ctx, c.registry, logEntry, request.ID); err != nil { @@ -142,27 +132,6 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r return nil } -// claimURIs persists one ChangeRecord per URI in the request. Each Create call is -// independent; the change store's per-PK idempotency makes the loop safe under -// queue redelivery (same (Queue, URI, RequestID) is a no-op on retry). -func (c *Controller) claimURIs(ctx context.Context, request entity.Request) error { - now := time.Now().UnixMilli() - for _, uri := range request.Change.URIs { - record := entity.ChangeRecord{ - URI: uri, - RequestID: request.ID, - Queue: request.Queue, - CreatedAt: now, - UpdatedAt: now, - Version: 1, - } - if err := c.store.GetChangeStore().Create(ctx, record); err != nil { - return fmt.Errorf("failed to claim uri=%s for request %s: %w", uri, request.ID, err) - } - } - return nil -} - // publish publishes a request ID to the specified topic key. func (c *Controller) publish(ctx context.Context, key consumer.TopicKey, requestID string, partitionKey string) error { rid := entity.RequestID{ID: requestID} diff --git a/submitqueue/orchestrator/controller/start/start_test.go b/submitqueue/orchestrator/controller/start/start_test.go index 04d90286..fb5c4c62 100644 --- a/submitqueue/orchestrator/controller/start/start_test.go +++ b/submitqueue/orchestrator/controller/start/start_test.go @@ -38,14 +38,11 @@ func newTestController( t *testing.T, ctrl *gomock.Controller, store *storagemock.MockStorage, - cs *storagemock.MockChangeStore, publishErr error, ) *Controller { logger := zaptest.NewLogger(t).Sugar() scope := tally.NoopScope - store.EXPECT().GetChangeStore().Return(cs).AnyTimes() - mockPub := queuemock.NewMockPublisher(ctrl) mockPub.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( func(ctx context.Context, topic string, msg entityqueue.Message) error { @@ -77,13 +74,6 @@ func newMockStorage(ctrl *gomock.Controller) *storagemock.MockStorage { return store } -// newMockChangeStore returns a MockChangeStore that accepts any Create call. -func newMockChangeStore(ctrl *gomock.Controller) *storagemock.MockChangeStore { - cs := storagemock.NewMockChangeStore(ctrl) - cs.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - return cs -} - // makeDelivery builds a MockDelivery wrapping a serialized LandRequest. func makeDelivery(t *testing.T, ctrl *gomock.Controller, lr entity.LandRequest) *queuemock.MockDelivery { payload, err := lr.ToBytes() @@ -98,7 +88,7 @@ func makeDelivery(t *testing.T, ctrl *gomock.Controller, lr entity.LandRequest) func TestNewController(t *testing.T) { ctrl := gomock.NewController(t) - controller := newTestController(t, ctrl, newMockStorage(ctrl), newMockChangeStore(ctrl), nil) + controller := newTestController(t, ctrl, newMockStorage(ctrl), nil) require.NotNil(t, controller) assert.Equal(t, consumer.TopicKeyStart, controller.TopicKey()) @@ -108,7 +98,7 @@ func TestNewController(t *testing.T) { func TestController_Process_Success(t *testing.T) { ctrl := gomock.NewController(t) - controller := newTestController(t, ctrl, newMockStorage(ctrl), newMockChangeStore(ctrl), nil) + controller := newTestController(t, ctrl, newMockStorage(ctrl), nil) delivery := makeDelivery(t, ctrl, entity.LandRequest{ ID: "test-queue/123", @@ -122,7 +112,7 @@ func TestController_Process_Success(t *testing.T) { func TestController_Process_InvalidJSON(t *testing.T) { ctrl := gomock.NewController(t) - controller := newTestController(t, ctrl, newMockStorage(ctrl), newMockChangeStore(ctrl), nil) + controller := newTestController(t, ctrl, newMockStorage(ctrl), nil) invalidPayload := []byte(`{"invalid": json"}`) msg := entityqueue.NewMessage("invalid-msg", invalidPayload, "partition1", nil) @@ -149,7 +139,7 @@ func TestController_Process_ConstructsRequestWithStateAndVersion(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() - controller := newTestController(t, ctrl, store, newMockChangeStore(ctrl), nil) + controller := newTestController(t, ctrl, store, nil) landRequest := entity.LandRequest{ ID: "test-queue/42", @@ -181,7 +171,7 @@ func TestController_Process_AllStrategies(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctrl := gomock.NewController(t) - controller := newTestController(t, ctrl, newMockStorage(ctrl), newMockChangeStore(ctrl), nil) + controller := newTestController(t, ctrl, newMockStorage(ctrl), nil) delivery := makeDelivery(t, ctrl, entity.LandRequest{ ID: fmt.Sprintf("queue/%s", tt.strategy), @@ -195,48 +185,9 @@ func TestController_Process_AllStrategies(t *testing.T) { } } -func TestController_Process_MultipleChanges(t *testing.T) { - ctrl := gomock.NewController(t) - - cs := storagemock.NewMockChangeStore(ctrl) - var captured []entity.ChangeRecord - cs.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( - func(ctx context.Context, record entity.ChangeRecord) error { - captured = append(captured, record) - return nil - }, - ).Times(3) - - controller := newTestController(t, ctrl, newMockStorage(ctrl), cs, nil) - - uris := []string{ - "github://uber/monorepo/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - "github://uber/monorepo/pull/2/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", - "github://uber/monorepo/pull/3/cccccccccccccccccccccccccccccccccccccccc", - } - delivery := makeDelivery(t, ctrl, entity.LandRequest{ - ID: "queue/999", - Queue: "test-queue", - Change: entity.Change{URIs: uris}, - LandStrategy: entity.RequestLandStrategySquashRebase, - }) - - require.NoError(t, controller.Process(context.Background(), delivery)) - - require.Len(t, captured, len(uris)) - for i, r := range captured { - assert.Equal(t, uris[i], r.URI) - assert.Equal(t, "queue/999", r.RequestID) - assert.Equal(t, "test-queue", r.Queue) - assert.Equal(t, int32(1), r.Version) - assert.Positive(t, r.CreatedAt) - assert.Equal(t, r.CreatedAt, r.UpdatedAt) - } -} - func TestController_Process_PublishFailure(t *testing.T) { ctrl := gomock.NewController(t) - controller := newTestController(t, ctrl, newMockStorage(ctrl), newMockChangeStore(ctrl), fmt.Errorf("publish failed")) + controller := newTestController(t, ctrl, newMockStorage(ctrl), fmt.Errorf("publish failed")) delivery := makeDelivery(t, ctrl, entity.LandRequest{ ID: "test-queue/123", @@ -256,7 +207,7 @@ func TestController_Process_StorageFailure(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() - controller := newTestController(t, ctrl, store, newMockChangeStore(ctrl), nil) + controller := newTestController(t, ctrl, store, nil) delivery := makeDelivery(t, ctrl, entity.LandRequest{ ID: "test-queue/123", @@ -278,7 +229,7 @@ func TestController_Process_AlreadyExistsSucceeds(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes() - controller := newTestController(t, ctrl, store, newMockChangeStore(ctrl), nil) + controller := newTestController(t, ctrl, store, nil) delivery := makeDelivery(t, ctrl, entity.LandRequest{ ID: "test-queue/123", @@ -290,27 +241,9 @@ func TestController_Process_AlreadyExistsSucceeds(t *testing.T) { require.NoError(t, controller.Process(context.Background(), delivery)) } -func TestController_Process_ChangeStoreFailure(t *testing.T) { - ctrl := gomock.NewController(t) - - cs := storagemock.NewMockChangeStore(ctrl) - cs.EXPECT().Create(gomock.Any(), gomock.Any()).Return(fmt.Errorf("change store down")) - - controller := newTestController(t, ctrl, newMockStorage(ctrl), cs, nil) - - delivery := makeDelivery(t, ctrl, entity.LandRequest{ - ID: "test-queue/123", - Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/1/789abc1234567890abcdef1234567890abcdef12"}}, - LandStrategy: entity.RequestLandStrategyRebase, - }) - - require.Error(t, controller.Process(context.Background(), delivery)) -} - func TestController_InterfaceImplementation(t *testing.T) { ctrl := gomock.NewController(t) - controller := newTestController(t, ctrl, newMockStorage(ctrl), newMockChangeStore(ctrl), nil) + controller := newTestController(t, ctrl, newMockStorage(ctrl), nil) var _ consumer.Controller = controller } diff --git a/submitqueue/orchestrator/controller/validate/BUILD.bazel b/submitqueue/orchestrator/controller/validate/BUILD.bazel index 5880202e..967490a3 100644 --- a/submitqueue/orchestrator/controller/validate/BUILD.bazel +++ b/submitqueue/orchestrator/controller/validate/BUILD.bazel @@ -29,7 +29,6 @@ go_test( "//extension/messagequeue/mock", "//submitqueue/core/consumer", "//submitqueue/entity", - "//submitqueue/extension/changeprovider", "//submitqueue/extension/changeprovider/mock", "//submitqueue/extension/mergechecker", "//submitqueue/extension/mergechecker/mock", diff --git a/submitqueue/orchestrator/controller/validate/validate.go b/submitqueue/orchestrator/controller/validate/validate.go index 4aa738e9..6f9a5d7d 100644 --- a/submitqueue/orchestrator/controller/validate/validate.go +++ b/submitqueue/orchestrator/controller/validate/validate.go @@ -18,6 +18,7 @@ import ( "context" "errors" "fmt" + "time" "github.com/uber-go/tally/v4" "github.com/uber/submitqueue/core/errs" @@ -172,6 +173,16 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r "total_files", totalFiles(changeInfos), ) + // Claim each URI in the change store with its provider details. The claim is + // created here — after duplicate detection and the merge/provider checks — so a + // rejected request never leaves a claim, and the record is written once with its + // details (immutable thereafter; no separate enrichment update). Create is + // idempotent per (queue, uri, request_id), so redelivery is a no-op. + if err := c.claimChanges(ctx, request, changeInfos); err != nil { + coremetrics.NamedCounter(c.metricsScope, "process", "change_store_errors", 1) + return fmt.Errorf("failed to claim change records for request %s: %w", request.ID, err) + } + // Publish to batch topic if err := c.publish(ctx, consumer.TopicKeyBatch, request.ID, request.Queue); err != nil { coremetrics.NamedCounter(c.metricsScope, "process", "publish_errors", 1) @@ -187,10 +198,12 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r } // checkDuplicate looks for any other in-flight request whose URIs overlap with this -// request's. The change rows are written upstream by the start controller; validate -// is read-only here. For each URI it queries the change store, walks the returned -// candidates skipping self/duplicates/orphans/terminals, and short-circuits on the -// first live duplicate. Returns that request_id, or "" if none. +// request's. It reads the change store before this request claims its own URIs +// (claimChanges runs later in Process), so it only sees rows written by other +// requests. For each URI it queries the change store, walks the returned candidates +// skipping self/duplicates/orphans/terminals, and short-circuits on the first live +// duplicate. Returns that request_id, or "" if none. Per-queue partition leasing +// serializes validate within a queue, so this read-then-claim sequence is race-free. // // Per-URI / per-record reads keep the contract backend-agnostic; the typical request // has 1-5 URIs, so the loop is cheap. @@ -254,11 +267,35 @@ func (c *Controller) publish(ctx context.Context, key consumer.TopicKey, request return nil } +// claimChanges persists one ChangeRecord per fetched ChangeInfo, capturing the +// provider details at claim time. The record's identity (queue, uri, request_id) +// and its Details are written together in a single immutable Create — there is no +// later mutation. Create is idempotent on its primary key, so a redelivery (or a +// prior partial attempt) is a no-op and the first write wins. +func (c *Controller) claimChanges(ctx context.Context, request entity.Request, infos []entity.ChangeInfo) error { + now := time.Now().UnixMilli() + for _, info := range infos { + record := entity.ChangeRecord{ + URI: info.URI, + RequestID: request.ID, + Queue: request.Queue, + Details: info.Details, + CreatedAt: now, + UpdatedAt: now, + Version: 1, + } + if err := c.store.GetChangeStore().Create(ctx, record); err != nil { + return fmt.Errorf("failed to claim uri=%s for request %s: %w", info.URI, request.ID, err) + } + } + return nil +} + // totalFiles returns the total number of files across all changeInfos. -func totalFiles(infos []changeprovider.ChangeInfo) int { +func totalFiles(infos []entity.ChangeInfo) int { total := 0 for _, info := range infos { - total += len(info.ChangedFiles) + total += info.Details.FileCount() } return total } diff --git a/submitqueue/orchestrator/controller/validate/validate_test.go b/submitqueue/orchestrator/controller/validate/validate_test.go index 4426e9a8..a8edf034 100644 --- a/submitqueue/orchestrator/controller/validate/validate_test.go +++ b/submitqueue/orchestrator/controller/validate/validate_test.go @@ -27,7 +27,6 @@ import ( queuemock "github.com/uber/submitqueue/extension/messagequeue/mock" "github.com/uber/submitqueue/submitqueue/core/consumer" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/changeprovider" changeprovidermock "github.com/uber/submitqueue/submitqueue/extension/changeprovider/mock" "github.com/uber/submitqueue/submitqueue/extension/mergechecker" mergecheckermock "github.com/uber/submitqueue/submitqueue/extension/mergechecker/mock" @@ -47,16 +46,18 @@ func requestIDPayload(t *testing.T, id string) []byte { // mockChangeProvider is a simple mock that returns test data. type mockChangeProvider struct{} -func (m *mockChangeProvider) Get(ctx context.Context, change entity.Change) ([]changeprovider.ChangeInfo, error) { - return []changeprovider.ChangeInfo{ +func (m *mockChangeProvider) Get(ctx context.Context, change entity.Change) ([]entity.ChangeInfo, error) { + return []entity.ChangeInfo{ { URI: "github://org/repo/pull/123/abcdef0123456789abcdef0123456789abcdef01", - User: changeprovider.User{ - Name: "Test User", - Email: "test@example.com", - }, - ChangedFiles: []changeprovider.ChangedFile{ - {Path: "main.go"}, + Details: entity.ChangeDetails{ + Author: entity.Author{ + Name: "Test User", + Email: "test@example.com", + }, + ChangedFiles: []entity.ChangedFile{ + {Path: "main.go"}, + }, }, }, }, nil @@ -80,12 +81,13 @@ func newMockStorage(ctrl *gomock.Controller, request entity.Request) (*storagemo return store, mockReqStore } -// newMockChangeStore creates a MockChangeStore with default no-overlap behavior. -// Tests that need to simulate overlap can override GetByURI with their own EXPECT. -// Validate is read-only against the change store — it never calls Create. +// newMockChangeStore creates a MockChangeStore with default no-overlap behavior +// (GetByURI returns nothing) and accepts the claim Create. Tests that need to +// simulate overlap or assert the claim override these with their own EXPECTs. func newMockChangeStore(ctrl *gomock.Controller) *storagemock.MockChangeStore { cs := storagemock.NewMockChangeStore(ctrl) cs.EXPECT().GetByURI(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() + cs.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() return cs } @@ -171,6 +173,54 @@ func TestController_Process_Success(t *testing.T) { require.NoError(t, controller.Process(context.Background(), delivery)) } +// TestController_Process_ClaimsChangeRecordsWithDetails verifies that, on the happy +// path, validate creates a change record per fetched change, capturing the provider +// details in a single immutable Create. +func TestController_Process_ClaimsChangeRecordsWithDetails(t *testing.T) { + ctrl := gomock.NewController(t) + mc := newMergeableMock(ctrl) + + // The request's URI matches the URI the mock change provider returns, so the + // claim carries that change's details. + const uri = "github://org/repo/pull/123/abcdef0123456789abcdef0123456789abcdef01" + request := entity.Request{ + ID: "test-queue/123", + Queue: "test-queue", + Change: entity.Change{URIs: []string{uri}}, + LandStrategy: entity.RequestLandStrategyRebase, + State: entity.RequestStateStarted, + Version: 1, + } + store, _ := newMockStorage(ctrl, request) + + wantDetails := entity.ChangeDetails{ + Author: entity.Author{Name: "Test User", Email: "test@example.com"}, + ChangedFiles: []entity.ChangedFile{{Path: "main.go"}}, + } + cs := storagemock.NewMockChangeStore(ctrl) + // Duplicate-detection read finds no overlap. + cs.EXPECT().GetByURI(gomock.Any(), request.Queue, uri).Return(nil, nil).AnyTimes() + // Capture the record passed to Create; assert identity + details. + cs.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, rec entity.ChangeRecord) error { + assert.Equal(t, uri, rec.URI) + assert.Equal(t, request.ID, rec.RequestID) + assert.Equal(t, request.Queue, rec.Queue) + assert.Equal(t, wantDetails, rec.Details) + return nil + }, + ) + + controller := newTestController(t, ctrl, store, cs, mc, nil) + + msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) + delivery := queuemock.NewMockDelivery(ctrl) + delivery.EXPECT().Message().Return(msg).AnyTimes() + delivery.EXPECT().Attempt().Return(1).AnyTimes() + + require.NoError(t, controller.Process(context.Background(), delivery)) +} + func TestController_Process_StorageFailure(t *testing.T) { ctrl := gomock.NewController(t) mc := newMergeableMock(ctrl) @@ -428,6 +478,9 @@ func TestController_Process_DuplicateDetection(t *testing.T) { for _, u := range uris { cs.EXPECT().GetByURI(gomock.Any(), queueName, u).Return(tt.byURI[u], nil).MaxTimes(1) } + // When no duplicate is found, the controller continues to fetch change info + // and claims each fetched change via Create. Accept any Create. + cs.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() controller := newTestController(t, ctrl, store, cs, mc, nil) diff --git a/test/integration/submitqueue/extension/storage/suite.go b/test/integration/submitqueue/extension/storage/suite.go index 32bf66ce..4b092e8b 100644 --- a/test/integration/submitqueue/extension/storage/suite.go +++ b/test/integration/submitqueue/extension/storage/suite.go @@ -336,28 +336,39 @@ func (s *StorageContractSuite) TestStorage_ChangeCreate_DifferentRequestSameURI( assert.Equal(t, []string{queue + "/1", queue + "/2"}, ids) } -// TestStorage_ChangeCreate_PreservesMetadata verifies metadata round-trips through the store. -func (s *StorageContractSuite) TestStorage_ChangeCreate_PreservesMetadata() { +// sampleDetails is a representative ChangeDetails reused across change-store contract tests. +func sampleDetails() entity.ChangeDetails { + return entity.ChangeDetails{ + Author: entity.Author{Name: "Ada Lovelace", Email: "ada@example.com"}, + ChangedFiles: []entity.ChangedFile{ + {Path: "main.go", LinesAdded: 10, LinesDeleted: 3, LinesModified: 2}, + {Path: "main_test.go", LinesAdded: 20, LinesDeleted: 0}, + }, + } +} + +// TestStorage_ChangeCreate_PreservesDetails verifies typed Details round-trip through the store. +func (s *StorageContractSuite) TestStorage_ChangeCreate_PreservesDetails() { t := s.T() ctx := s.ctx - const queue = "cq-meta" - const meta = `{"title":"add new feature"}` + const queue = "cq-details" + details := sampleDetails() require.NoError(t, s.storage.GetChangeStore().Create(ctx, entity.ChangeRecord{ - URI: changeURI, RequestID: queue + "/1", Queue: queue, Metadata: meta, CreatedAt: 1, UpdatedAt: 1, Version: 1, + URI: changeURI, RequestID: queue + "/1", Queue: queue, Details: details, CreatedAt: 1, UpdatedAt: 1, Version: 1, })) got, err := s.storage.GetChangeStore().GetByURI(ctx, queue, changeURI) require.NoError(t, err) require.Len(t, got, 1) - assert.JSONEq(t, meta, got[0].Metadata) + assert.Equal(t, details, got[0].Details) } -// TestStorage_ChangeCreate_EmptyMetadataStoredAsObject verifies empty metadata is stored as "{}". -func (s *StorageContractSuite) TestStorage_ChangeCreate_EmptyMetadataStoredAsObject() { +// TestStorage_ChangeCreate_EmptyDetails verifies a zero-value Details round-trips (stored as a JSON object). +func (s *StorageContractSuite) TestStorage_ChangeCreate_EmptyDetails() { t := s.T() ctx := s.ctx - const queue = "cq-emptymeta" + const queue = "cq-emptydetails" require.NoError(t, s.storage.GetChangeStore().Create(ctx, entity.ChangeRecord{ URI: changeURI, RequestID: queue + "/1", Queue: queue, CreatedAt: 1, UpdatedAt: 1, Version: 1, @@ -366,5 +377,5 @@ func (s *StorageContractSuite) TestStorage_ChangeCreate_EmptyMetadataStoredAsObj got, err := s.storage.GetChangeStore().GetByURI(ctx, queue, changeURI) require.NoError(t, err) require.Len(t, got, 1) - assert.JSONEq(t, "{}", got[0].Metadata) + assert.Equal(t, entity.ChangeDetails{}, got[0].Details) }