review changes part 2

Author: rashmi-prithyani

entity/request.go

@@ -33,14 +33,18 @@ const (
 	RequestStateError RequestState = "error"
 )
 
-// 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.
+// Change represents a code change identified by a URI from a code change provider (e.g., GitHub Pull Request, Phabricator Diff).
+// The provider is extracted from the URI scheme. The object is immutable after creation.
 type Change struct {
-	// 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"`
+	// URI identifies the change(s) to land (RFC 3986 compliant).
+	// The scheme identifies the change provider, and the path contains provider-specific resource identifiers.
+	//
+	// By default, the GitHub format is supported (though other providers can be added):
+	//   Single PR: "github://<org>/<repo>/pull/<pr>/<hash>"
+	//   Stacked PRs: "github://<org>/<repo>/pull/<pr1>/<hash1>/<pr2>/<hash2>/..."
+	//   Example: "github://uber/submitqueue/pull/123/abc123def"
+	//
+	URI string `json:"uri"`
 }
 
 // Request defines a request to land (merge into target branch of the source control repository) a set of code changes.

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{Provider: "github", URIs: []string{"github.com/uber/submitqueue/456/abc123def", "github.com/uber/submitqueue/789/def456abc"}},
+		Change:       Change{URI: "github://uber/submitqueue/pull/456/abc123def/789/def456abc"},
 		LandStrategy: RequestLandStrategyRebase,
 		State:        RequestStateNew,
 		Version:      1,
@@ -24,8 +24,7 @@ func TestRequest_ToBytes(t *testing.T) {
 	// Verify JSON contains expected fields
 	jsonStr := string(data)
 	assert.Contains(t, jsonStr, "test-queue/123")
-	assert.Contains(t, jsonStr, "github")
-	assert.Contains(t, jsonStr, "github.com/uber/submitqueue/456/abc123def")
+	assert.Contains(t, jsonStr, "github://uber/submitqueue/pull/456/abc123def/789/def456abc")
 	assert.Contains(t, jsonStr, "rebase")
 	assert.Contains(t, jsonStr, "new")
 }
@@ -34,7 +33,7 @@ func TestRequestFromBytes(t *testing.T) {
 	original := Request{
 		ID:           "my-queue/999",
 		Queue:        "my-queue",
-		Change:       Change{Provider: "phabricator", URIs: []string{"phabricator.uber.com/D111/fedcba987"}},
+		Change:       Change{URI: "code.uber.internal.com/D111"},
 		LandStrategy: RequestLandStrategyMerge,
 		State:        RequestStateProcessing,
 		Version:      3,
@@ -51,8 +50,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.Provider, deserialized.Change.Provider)
-	assert.Equal(t, original.Change.URIs, deserialized.Change.URIs)
+	assert.Equal(t, original.Change.URI, deserialized.Change.URI)
 	assert.Equal(t, original.LandStrategy, deserialized.LandStrategy)
 	assert.Equal(t, original.State, deserialized.State)
 	assert.Equal(t, original.Version, deserialized.Version)
@@ -85,11 +83,11 @@ func TestRequest_SerializationRoundTrip(t *testing.T) {
 		req  Request
 	}{
 		{
-			name: "full request with multiple PRs",
+			name: "github stacked diff",
 			req: Request{
 				ID:           "queue1/100",
 				Queue:        "queue1",
-				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"}},
+				Change:       Change{URI: "github://uber/repo-a/pull/101/aaa111/102/bbb222/103/ccc333"},
 				LandStrategy: RequestLandStrategySquashRebase,
 				State:        RequestStateLanded,
 				Version:      5,
@@ -100,7 +98,7 @@ func TestRequest_SerializationRoundTrip(t *testing.T) {
 			req: Request{
 				ID:           "queue2/200",
 				Queue:        "queue2",
-				Change:       Change{Provider: "phabricator", URIs: []string{"phabricator.uber.com/D12345/abc123def456"}},
+				Change:       Change{URI: "code.uber.internal.com/D12345"},
 				LandStrategy: RequestLandStrategyRebase,
 				State:        RequestStateNew,
 				Version:      1,
@@ -111,7 +109,7 @@ func TestRequest_SerializationRoundTrip(t *testing.T) {
 			req: Request{
 				ID:           "queue3/300",
 				Queue:        "queue3",
-				Change:       Change{Provider: "github-enterprise", URIs: []string{"github.uber.com/internal/service/999/deadbeef12"}},
+				Change:       Change{URI: "github.uber.com/internal/service/999/deadbeef12"},
 				LandStrategy: RequestLandStrategyMerge,
 				State:        RequestStateError,
 				Version:      10,

extension/changeprovider/BUILD.bazel

@@ -0,0 +1,8 @@
+load("@rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "changeprovider",
+    srcs = ["change_provider.go"],
+    importpath = "github.com/uber/submitqueue/extension/changeprovider",
+    visibility = ["//visibility:public"],
+)

extension/changeprovider/change_provider.go

@@ -34,12 +34,15 @@ type ChangeInfo struct {
 	ChangedFiles []ChangedFile
 }
 
-// ChangeProvider fetches change information from external code review providers.
+// ChangeProvider fetches change metadata from external systems
 // 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")
+	// Get retrieves change information for the provided URI (RFC 3986 compliant).
+	// The URI scheme identifies the provider, and the path contains provider-specific resource identifiers.
+	//
+	// By default, the GitHub format is supported (though other providers can be added):
+	//   Single PR: "github://<org>/<repo>/pull/<pr>/<hash>"
+	//   Stacked PRs: "github://<org>/<repo>/pull/<pr1>/<hash1>/<pr2>/<hash2>/..."
 	// Returns the change info containing metadata and file changes.
-	Get(ctx context.Context, uris []string) (ChangeInfo, error)
+	Get(ctx context.Context, uri string) (ChangeInfo, error)
 }

extension/storage/mysql/request_store.go

@@ -3,7 +3,6 @@ package mysql
 import (
 	"context"
 	"database/sql"
-	"encoding/json"
 	"errors"
 	"fmt"
 
@@ -25,12 +24,11 @@ func NewRequestStore(db *sql.DB) storage.RequestStore {
 // Get retrieves a land request by ID. Returns ErrNotFound if the request is not found.
 func (r *requestStore) Get(ctx context.Context, id string) (entity.Request, error) {
 	var req entity.Request
-	var changeURIsJSON []byte
 
 	err := r.db.QueryRowContext(ctx,
-		"SELECT id, queue, change_source, change_uris, land_strategy, state, version FROM request WHERE id = ?",
+		"SELECT id, queue, change_uri, land_strategy, state, version FROM request WHERE id = ?",
 		id,
-	).Scan(&req.ID, &req.Queue, &req.Change.Provider, &changeURIsJSON, &req.LandStrategy, &req.State, &req.Version)
+	).Scan(&req.ID, &req.Queue, &req.Change.URI, &req.LandStrategy, &req.State, &req.Version)
 
 	if errors.Is(err, sql.ErrNoRows) {
 		return entity.Request{}, storage.WrapNotFound(err)
@@ -39,23 +37,14 @@ func (r *requestStore) Get(ctx context.Context, id string) (entity.Request, erro
 		return entity.Request{}, fmt.Errorf("failed to get request entity id=%s from the database: %w", id, err)
 	}
 
-	if err := json.Unmarshal(changeURIsJSON, &req.Change.URIs); err != nil {
-		return entity.Request{}, fmt.Errorf("failed to unmarshal change URIs for request entity id=%s from the database: %w", id, err)
-	}
-
 	return req, nil
 }
 
 // Create creates a new land request. The request must have a unique ID already assigned. Returns ErrAlreadyExists if the request ID already exists.
 func (r *requestStore) Create(ctx context.Context, request entity.Request) error {
-	changeURIsJSON, err := json.Marshal(request.Change.URIs)
-	if err != nil {
-		return fmt.Errorf("failed to marshal change URIs=%v id=%s for Create request entity: %w", request.Change.URIs, request.ID, err)
-	}
-
-	_, 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.Provider, changeURIsJSON, request.LandStrategy, request.State, request.Version,
+	_, err := r.db.ExecContext(ctx,
+		"INSERT INTO request (id, queue, change_uri, land_strategy, state, version) VALUES (?, ?, ?, ?, ?, ?)",
+		request.ID, request.Queue, request.Change.URI, request.LandStrategy, request.State, request.Version,
 	)
 	if err != nil {
 		var mysqlErr *mysql.MySQLError

extension/storage/mysql/schema/request.sql

@@ -1,8 +1,7 @@
 CREATE TABLE IF NOT EXISTS request (
     id VARCHAR(255) NOT NULL,
     queue VARCHAR(255) NOT NULL,
-    change_source VARCHAR(255) NOT NULL,
-    change_uris JSON NOT NULL,
+    change_uri TEXT NOT NULL,
     land_strategy VARCHAR(64) NOT NULL,
     state VARCHAR(64) NOT NULL,
     version INT NOT NULL,

gateway/controller/land.go

@@ -61,16 +61,12 @@ func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (*pb.Lan
 	if req.Queue == "" {
 		return nil, fmt.Errorf("LandController requires the request to have a queue name specified: %w", ErrInvalidRequest)
 	}
-	if req.Change == nil || req.Change.Source == "" {
-		return nil, fmt.Errorf("LandController requires the request to have a change source specified: %w", ErrInvalidRequest)
-	}
-	if len(req.Change.GetUris()) == 0 {
-		return nil, fmt.Errorf("LandController requires the request to have at least one change URI specified: %w", ErrInvalidRequest)
+	if req.Change == nil || req.Change.Uri == "" {
+		return nil, fmt.Errorf("LandController requires the request to have a change URI specified: %w", ErrInvalidRequest)
 	}
 
 	change := entity.Change{
-		Provider: req.Change.GetSource(),
-		URIs:     req.Change.GetUris(),
+		URI: req.Change.GetUri(),
 	}
 
 	// TODO: validate that queue is configured. Return error if not.
@@ -104,8 +100,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.Provider,
-		"change_uris", change.URIs,
+		"change_uri", change.URI,
 		"strategy", string(strategy),
 	)
 

gateway/controller/land_test.go

@@ -216,7 +216,7 @@ func TestLand_ReturnsSqid(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "test-queue",
-		Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}},
+		Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"},
 	}
 	resp, err := controller.Land(ctx, req)
 
@@ -248,16 +248,15 @@ func TestLand_PassesCorrectParametersToStore(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:    "my-queue",
-		Change:   &pb.Change{Source: "github", Uris: []string{"github.com/uber/myservice/1/abc111", "github.com/uber/myservice/2/def222"}},
+		Change:   &pb.Change{Uri: "github://uber/myservice/pull/1/abc111/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.Provider)
-	assert.Equal(t, []string{"github.com/uber/myservice/1/abc111", "github.com/uber/myservice/2/def222"}, capturedRequest.Change.URIs)
+	assert.Equal(t, "github://uber/myservice/pull/1/abc111/2/def222", capturedRequest.Change.URI)
 	assert.Equal(t, entity.RequestLandStrategyRebase, capturedRequest.LandStrategy)
 	assert.Equal(t, entity.RequestStateNew, capturedRequest.State)
 	assert.Equal(t, int32(1), capturedRequest.Version)
@@ -285,7 +284,7 @@ func TestLand_ReturnsErrorOnStorageFailure(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "test-queue",
-		Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}},
+		Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"},
 	}
 	_, err := controller.Land(ctx, req)
 
@@ -313,7 +312,7 @@ func TestLand_ReturnsErrorOnCounterFailure(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "test-queue",
-		Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}},
+		Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"},
 	}
 	_, err := controller.Land(ctx, req)
 
@@ -344,7 +343,7 @@ func TestLand_CounterDomainIncludesQueue(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "my-queue",
-		Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}},
+		Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"},
 	}
 	_, err := controller.Land(ctx, req)
 
@@ -366,15 +365,15 @@ func TestLand_ReturnsErrorOnEmptyQueue(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "",
-		Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}},
+		Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"},
 	}
 	_, err := controller.Land(ctx, req)
 
 	require.Error(t, err)
 	assert.True(t, IsInvalidRequest(err))
 }
 
-func TestLand_ReturnsErrorOnEmptyChangeSource(t *testing.T) {
+func TestLand_ReturnsErrorOnEmptyChangeUri(t *testing.T) {
 	store := &mockStorage{requestStore: &mockRequestStore{
 		createFunc: func(ctx context.Context, request entity.Request) error {
 			return nil
@@ -388,7 +387,7 @@ func TestLand_ReturnsErrorOnEmptyChangeSource(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "test-queue",
-		Change: &pb.Change{Source: "", Uris: []string{"github.com/uber/test-repo/123/abc123def"}},
+		Change: &pb.Change{Uri: ""},
 	}
 	_, err := controller.Land(ctx, req)
 
@@ -418,28 +417,6 @@ func TestLand_ReturnsErrorOnNilChange(t *testing.T) {
 	assert.True(t, IsInvalidRequest(err))
 }
 
-func TestLand_ReturnsErrorOnEmptyChangeIDs(t *testing.T) {
-	store := &mockStorage{requestStore: &mockRequestStore{
-		createFunc: func(ctx context.Context, request entity.Request) error {
-			return nil
-		},
-	}}
-	cnt := &mockCounter{nextFunc: func(ctx context.Context, domain string) (int64, error) {
-		return 1, nil
-	}}
-	controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, store, cnt, noopPublisher(), "request")
-	ctx := context.Background()
-
-	req := &pb.LandRequest{
-		Queue:  "test-queue",
-		Change: &pb.Change{Source: "github", Uris: []string{}},
-	}
-	_, err := controller.Land(ctx, req)
-
-	require.Error(t, err)
-	assert.True(t, IsInvalidRequest(err))
-}
-
 func TestLand_PublishesToQueue(t *testing.T) {
 	var publishedTopic string
 	var publishedMessage queue.Message
@@ -463,7 +440,7 @@ func TestLand_PublishesToQueue(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:    "test-queue",
-		Change:   &pb.Change{Source: "github", Uris: []string{"github.com/uber/backend/456/fed987cba"}},
+		Change:   &pb.Change{Uri: "github://uber/backend/pull/456/fed987cba"},
 		Strategy: pb.Strategy_REBASE,
 	}
 	resp, err := controller.Land(ctx, req)
@@ -481,8 +458,7 @@ 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.Provider)
-	assert.Equal(t, []string{"github.com/uber/backend/456/fed987cba"}, deserializedReq.Change.URIs)
+	assert.Equal(t, "github://uber/backend/pull/456/fed987cba", deserializedReq.Change.URI)
 	assert.Equal(t, entity.RequestLandStrategyRebase, deserializedReq.LandStrategy)
 	assert.Equal(t, entity.RequestStateNew, deserializedReq.State)
 	assert.Equal(t, int32(1), deserializedReq.Version)
@@ -506,7 +482,7 @@ func TestLand_ContinuesWhenPublishFails(t *testing.T) {
 
 	req := &pb.LandRequest{
 		Queue:  "test-queue",
-		Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/service/1/abc123def"}},
+		Change: &pb.Change{Uri: "github://uber/service/pull/1/abc123def"},
 	}
 	_, err := controller.Land(ctx, req)
 

gateway/proto/gateway.proto

@@ -41,16 +41,21 @@ enum Strategy {
     MERGE = 3;
 }
 
-// Change represents a set of related code changes identified by one or more URIs from a particular code change provider, like Github Pull Requests.
+// Change represents a code change identified by a URI from a code change provider (e.g., GitHub Pull Request, Phabricator Diff).
+// The provider is extracted from the URI scheme.
 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 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.
-    repeated string uris = 2;
+    // URI identifying the change(s) to land (RFC 3986 compliant).
+    // The scheme identifies the change provider, and the path contains provider-specific resource identifiers.
+    //
+    // By default, the GitHub format is supported (though other providers can be added):
+    //   Single PR: "github://<org>/<repo>/pull/<pr>/<hash>"
+    //   Stacked PRs: "github://<org>/<repo>/pull/<pr1>/<hash1>/<pr2>/<hash2>/..."
+    //   Example: "github://uber/submitqueue/pull/123/abc123def"
+    //
+    // SubmitQueue guarantees changes are landed in order with no other changes in between.
+    // SubmitQueue does not guarantee each change is individually valid, but produces a validity marker on such changes.
+    // The user is responsible for including all changes in a stack in order, starting from the earliest change.
+    string uri = 1;
 }
 
 // LandRequest defines a request to land (merge into target branch of the source control repository) a set of code changes.