code review changes

Author: rashmi-prithyani

entity/request.go

@@ -36,11 +36,10 @@ const (
 // Change represents a set of related code changes identified by one or more URIs from a particular code change provider, like Github Pull Requests.
 // The object is immutable after creation.
 type Change struct {
-	// Source is the code change provider ID that maps to a registered provider (e.g., "github", "github-enterprise", "phabricator").
-	Source string `json:"source"`
-	// URIs is a list of change URIs that should be landed together. The format is provider-specific:
-	//   - GitHub: "owner/repo/pr_number@commit_hash" (e.g., "uber/submitqueue/123@abc123def")
-	//   - Phabricator: "revision_id@commit_hash" (e.g., "D12345@abc123def")
+	// Provider is the code change provider that maps to a registered provider (e.g., "github", "github-enterprise", "phabricator").
+	Provider string `json:"provider"`
+	// URIs is a list of change URIs that should be landed together. The format is determined by the change-provider implementation.
+	// Default format: "github.com/<org>/<repo>/<pr>/<hash>" (e.g., "github.com/uber/submitqueue/123/abc123def")
 	URIs []string `json:"uris"`
 }
 

entity/request_test.go

@@ -11,7 +11,7 @@ func TestRequest_ToBytes(t *testing.T) {
 	req := Request{
 		ID:           "test-queue/123",
 		Queue:        "test-queue",
-		Change:       Change{Source: "github", URIs: []string{"uber/submitqueue/456@abc123def", "uber/submitqueue/789@def456abc"}},
+		Change:       Change{Provider: "github", URIs: []string{"github.com/uber/submitqueue/456/abc123def", "github.com/uber/submitqueue/789/def456abc"}},
 		LandStrategy: RequestLandStrategyRebase,
 		State:        RequestStateNew,
 		Version:      1,
@@ -25,7 +25,7 @@ func TestRequest_ToBytes(t *testing.T) {
 	jsonStr := string(data)
 	assert.Contains(t, jsonStr, "test-queue/123")
 	assert.Contains(t, jsonStr, "github")
-	assert.Contains(t, jsonStr, "uber/submitqueue/456@abc123def")
+	assert.Contains(t, jsonStr, "github.com/uber/submitqueue/456/abc123def")
 	assert.Contains(t, jsonStr, "rebase")
 	assert.Contains(t, jsonStr, "new")
 }
@@ -34,7 +34,7 @@ func TestRequestFromBytes(t *testing.T) {
 	original := Request{
 		ID:           "my-queue/999",
 		Queue:        "my-queue",
-		Change:       Change{Source: "phabricator", URIs: []string{"D111@fedcba987"}},
+		Change:       Change{Provider: "phabricator", URIs: []string{"phabricator.uber.com/D111/fedcba987"}},
 		LandStrategy: RequestLandStrategyMerge,
 		State:        RequestStateProcessing,
 		Version:      3,
@@ -51,7 +51,7 @@ func TestRequestFromBytes(t *testing.T) {
 	// Verify all fields match
 	assert.Equal(t, original.ID, deserialized.ID)
 	assert.Equal(t, original.Queue, deserialized.Queue)
-	assert.Equal(t, original.Change.Source, deserialized.Change.Source)
+	assert.Equal(t, original.Change.Provider, deserialized.Change.Provider)
 	assert.Equal(t, original.Change.URIs, deserialized.Change.URIs)
 	assert.Equal(t, original.LandStrategy, deserialized.LandStrategy)
 	assert.Equal(t, original.State, deserialized.State)
@@ -89,7 +89,7 @@ func TestRequest_SerializationRoundTrip(t *testing.T) {
 			req: Request{
 				ID:           "queue1/100",
 				Queue:        "queue1",
-				Change:       Change{Source: "github", URIs: []string{"uber/repo-a/101@aaa111", "uber/repo-a/102@bbb222", "uber/repo-a/103@ccc333"}},
+				Change:       Change{Provider: "github", URIs: []string{"github.com/uber/repo-a/101/aaa111", "github.com/uber/repo-a/102/bbb222", "github.com/uber/repo-a/103/ccc333"}},
 				LandStrategy: RequestLandStrategySquashRebase,
 				State:        RequestStateLanded,
 				Version:      5,
@@ -100,7 +100,7 @@ func TestRequest_SerializationRoundTrip(t *testing.T) {
 			req: Request{
 				ID:           "queue2/200",
 				Queue:        "queue2",
-				Change:       Change{Source: "phabricator", URIs: []string{"D12345@abc123def456"}},
+				Change:       Change{Provider: "phabricator", URIs: []string{"phabricator.uber.com/D12345/abc123def456"}},
 				LandStrategy: RequestLandStrategyRebase,
 				State:        RequestStateNew,
 				Version:      1,
@@ -111,7 +111,7 @@ func TestRequest_SerializationRoundTrip(t *testing.T) {
 			req: Request{
 				ID:           "queue3/300",
 				Queue:        "queue3",
-				Change:       Change{Source: "github-enterprise", URIs: []string{"internal/service/999@deadbeef12"}},
+				Change:       Change{Provider: "github-enterprise", URIs: []string{"github.uber.com/internal/service/999/deadbeef12"}},
 				LandStrategy: RequestLandStrategyMerge,
 				State:        RequestStateError,
 				Version:      10,

extension/change_provider/change_provider.go

@@ -0,0 +1,45 @@
+package changeprovider
+
+import "context"
+
+// 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
+	// LinesModified is the number of lines modified in this file.
+	LinesModified int
+}
+
+// ChangeInfo contains metadata and file changes for a code change.
+type ChangeInfo struct {
+	// ID is the change identifier (e.g., "PR: uber-code/go-code/1" or "diff: uber-code/go-code/D1").
+	ID string
+	// User is the author of the change.
+	User User
+	// ChangedFiles is the list of files modified in this change.
+	ChangedFiles []ChangedFile
+}
+
+// ChangeProvider fetches change information from external code review providers.
+// Each implementation is configured for a specific provider (GitHub, GitLab, Phabricator).
+type ChangeProvider interface {
+	// Get retrieves change information for the provided URIs.
+	// The URI format is determined by the implementation.
+	// Default format: "github.com/<org>/<repo>/<pr>/<hash>" (e.g., "github.com/uber/submitqueue/123/abc123def")
+	// Returns the change info containing metadata and file changes.
+	Get(ctx context.Context, uris []string) (ChangeInfo, error)
+}

extension/changeprovider/change_provider.go

@@ -30,7 +30,7 @@ func (r *requestStore) Get(ctx context.Context, id string) (entity.Request, erro
 	err := r.db.QueryRowContext(ctx,
 		"SELECT id, queue, change_source, change_uris, land_strategy, state, version FROM request WHERE id = ?",
 		id,
-	).Scan(&req.ID, &req.Queue, &req.Change.Source, &changeURIsJSON, &req.LandStrategy, &req.State, &req.Version)
+	).Scan(&req.ID, &req.Queue, &req.Change.Provider, &changeURIsJSON, &req.LandStrategy, &req.State, &req.Version)
 
 	if errors.Is(err, sql.ErrNoRows) {
 		return entity.Request{}, storage.WrapNotFound(err)
@@ -55,7 +55,7 @@ func (r *requestStore) Create(ctx context.Context, request entity.Request) error
 
 	_, err = r.db.ExecContext(ctx,
 		"INSERT INTO request (id, queue, change_source, change_uris, land_strategy, state, version) VALUES (?, ?, ?, ?, ?, ?, ?)",
-		request.ID, request.Queue, request.Change.Source, changeURIsJSON, request.LandStrategy, request.State, request.Version,
+		request.ID, request.Queue, request.Change.Provider, changeURIsJSON, request.LandStrategy, request.State, request.Version,
 	)
 	if err != nil {
 		var mysqlErr *mysql.MySQLError

extension/storage/mysql/request_store.go

@@ -69,8 +69,8 @@ func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (*pb.Lan
 	}
 
 	change := entity.Change{
-		Source: req.Change.GetSource(),
-		URIs:   req.Change.GetUris(),
+		Provider: req.Change.GetSource(),
+		URIs:     req.Change.GetUris(),
 	}
 
 	// TODO: validate that queue is configured. Return error if not.
@@ -104,7 +104,7 @@ func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (*pb.Lan
 	c.logger.Debugw("land request created",
 		"queue", req.Queue,
 		"sqid", request.ID,
-		"change_source", change.Source,
+		"change_source", change.Provider,
 		"change_uris", change.URIs,
 		"strategy", string(strategy),
 	)

gateway/controller/land.go

@@ -216,7 +216,7 @@ func TestLand_ReturnsSqid(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "test-queue",
-		Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}},
+		Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}},
 	}
 	resp, err := controller.Land(ctx, req)
 
@@ -248,16 +248,16 @@ func TestLand_PassesCorrectParametersToStore(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:    "my-queue",
-		Change:   &pb.Change{Source: "github", Uris: []string{"uber/myservice/1@abc111", "uber/myservice/2@def222"}},
+		Change:   &pb.Change{Source: "github", Uris: []string{"github.com/uber/myservice/1/abc111", "github.com/uber/myservice/2/def222"}},
 		Strategy: pb.Strategy_REBASE,
 	}
 	resp, err := controller.Land(ctx, req)
 
 	require.NoError(t, err)
 	assert.Equal(t, "my-queue/42", capturedRequest.ID)
 	assert.Equal(t, "my-queue", capturedRequest.Queue)
-	assert.Equal(t, "github", capturedRequest.Change.Source)
-	assert.Equal(t, []string{"uber/myservice/1@abc111", "uber/myservice/2@def222"}, capturedRequest.Change.URIs)
+	assert.Equal(t, "github", capturedRequest.Change.Provider)
+	assert.Equal(t, []string{"github.com/uber/myservice/1/abc111", "github.com/uber/myservice/2/def222"}, capturedRequest.Change.URIs)
 	assert.Equal(t, entity.RequestLandStrategyRebase, capturedRequest.LandStrategy)
 	assert.Equal(t, entity.RequestStateNew, capturedRequest.State)
 	assert.Equal(t, int32(1), capturedRequest.Version)
@@ -285,7 +285,7 @@ func TestLand_ReturnsErrorOnStorageFailure(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "test-queue",
-		Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}},
+		Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}},
 	}
 	_, err := controller.Land(ctx, req)
 
@@ -313,7 +313,7 @@ func TestLand_ReturnsErrorOnCounterFailure(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "test-queue",
-		Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}},
+		Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}},
 	}
 	_, err := controller.Land(ctx, req)
 
@@ -344,7 +344,7 @@ func TestLand_CounterDomainIncludesQueue(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "my-queue",
-		Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}},
+		Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}},
 	}
 	_, err := controller.Land(ctx, req)
 
@@ -366,7 +366,7 @@ func TestLand_ReturnsErrorOnEmptyQueue(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "",
-		Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}},
+		Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}},
 	}
 	_, err := controller.Land(ctx, req)
 
@@ -388,7 +388,7 @@ func TestLand_ReturnsErrorOnEmptyChangeSource(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "test-queue",
-		Change: &pb.Change{Source: "", Uris: []string{"uber/test-repo/123@abc123def"}},
+		Change: &pb.Change{Source: "", Uris: []string{"github.com/uber/test-repo/123/abc123def"}},
 	}
 	_, err := controller.Land(ctx, req)
 
@@ -463,7 +463,7 @@ func TestLand_PublishesToQueue(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:    "test-queue",
-		Change:   &pb.Change{Source: "github", Uris: []string{"uber/backend/456@fed987cba"}},
+		Change:   &pb.Change{Source: "github", Uris: []string{"github.com/uber/backend/456/fed987cba"}},
 		Strategy: pb.Strategy_REBASE,
 	}
 	resp, err := controller.Land(ctx, req)
@@ -481,8 +481,8 @@ func TestLand_PublishesToQueue(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, "test-queue/123", deserializedReq.ID)
 	assert.Equal(t, "test-queue", deserializedReq.Queue)
-	assert.Equal(t, "github", deserializedReq.Change.Source)
-	assert.Equal(t, []string{"uber/backend/456@fed987cba"}, deserializedReq.Change.URIs)
+	assert.Equal(t, "github", deserializedReq.Change.Provider)
+	assert.Equal(t, []string{"github.com/uber/backend/456/fed987cba"}, deserializedReq.Change.URIs)
 	assert.Equal(t, entity.RequestLandStrategyRebase, deserializedReq.LandStrategy)
 	assert.Equal(t, entity.RequestStateNew, deserializedReq.State)
 	assert.Equal(t, int32(1), deserializedReq.Version)
@@ -506,7 +506,7 @@ func TestLand_ContinuesWhenPublishFails(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "test-queue",
-		Change: &pb.Change{Source: "github", Uris: []string{"uber/service/1@abc123def"}},
+		Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/service/1/abc123def"}},
 	}
 	_, err := controller.Land(ctx, req)
 

gateway/controller/land_test.go

@@ -45,9 +45,8 @@ enum Strategy {
 message Change {
     // The code change provider ID that maps to a registered provider (e.g., "github", "github-enterprise", "phabricator").
     string source = 1;
-    // List of change URIs that should be landed together. The format is provider-specific:
-    //   - GitHub: "owner/repo/pr_number@commit_hash" (e.g., "uber/submitqueue/123@abc123def")
-    //   - Phabricator: "revision_id@commit_hash" (e.g., "D12345@abc123def")
+    // List of change URIs that should be landed together. The format is determined by the change-provider implementation.
+    // Default format: "github.com/<org>/<repo>/<pr>/<hash>" (e.g., "github.com/uber/submitqueue/123/abc123def")
     // SubmitQueue guarantees that the changes are landed in the order of the list, and no other changes are landed in between.
     // SubmitQueue does not guarantee that each change is individually valid, but produces a special validity marker on such changes.
     // The user is responsible to include all changes in a change stack in the order of the list, starting from the earliest change.

gateway/proto/gateway.proto

@@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) {
 	request := entity.Request{
 		ID:           "test-queue/123",
 		Queue:        "test-queue",
-		Change:       entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}},
+		Change:       entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}},
 		LandStrategy: entity.RequestLandStrategyRebase,
 		State:        entity.RequestStateNew,
 		Version:      1,
@@ -100,7 +100,7 @@ func TestController_Process_PublishFailure(t *testing.T) {
 	request := entity.Request{
 		ID:           "test-queue/123",
 		Queue:        "test-queue",
-		Change:       entity.Change{Source: "github", URIs: []string{"uber/service/1@xyz789abc"}},
+		Change:       entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/1/xyz789abc"}},
 		LandStrategy: entity.RequestLandStrategyRebase,
 		State:        entity.RequestStateNew,
 		Version:      1,