dmitri.shuralyov.com/service/change/...

gerritapi: use project~id change ID format

Use the more explicit project~id change ID format. It's not deprecated,
and lets us detect and return a not exist error when the project doesn't
match the CL.

Previously, just the numeric change ID was used. That format is
deprecated, and doesn't tell us when the project doesn't match the CL.

References:

• https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id
• https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#change.api.allowedIdentifier
dmitshur committed 9 months ago commit 7c09d163af9b65a0f95bf6dc84a4acf9fdd82fc5
gerritapi/gerritapi.go
@@ -2,10 +2,11 @@
 package gerritapi
 
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"os"
 	"sort"
 	"strings"
 	"unicode"
 
@@ -26,32 +27,35 @@ func NewService(client *gerrit.Client) change.Service {
 type service struct {
 	cl     *gerrit.Client
 	domain string
 }
 
-func (s service) List(ctx context.Context, rs string, opt change.ListOptions) ([]change.Change, error) {
-	project := project(rs)
+func (s service) List(ctx context.Context, repo string, opt change.ListOptions) ([]change.Change, error) {
+	project := project(repo)
 	var query string
 	switch opt.Filter {
 	case change.FilterOpen:
 		query = fmt.Sprintf("project:%s status:open", project)
 	case change.FilterClosedMerged:
 		// "status:closed" is equivalent to "(status:abandoned OR status:merged)".
 		query = fmt.Sprintf("project:%s status:closed", project)
 	case change.FilterAll:
 		query = fmt.Sprintf("project:%s", project)
 	}
-	cs, _, err := s.cl.Changes.QueryChanges(&gerrit.QueryChangeOptions{
+	cs, resp, err := s.cl.Changes.QueryChanges(&gerrit.QueryChangeOptions{
 		QueryOptions: gerrit.QueryOptions{
 			Query: []string{query},
 			Limit: 25,
 		},
 		ChangeOptions: gerrit.ChangeOptions{
 			AdditionalFields: []string{"DETAILED_ACCOUNTS", "MESSAGES"},
 		},
 	})
 	if err != nil {
+		if resp != nil && resp.StatusCode == http.StatusNotFound {
+			return nil, os.ErrNotExist
+		}
 		return nil, err
 	}
 	var is []change.Change
 	for _, chg := range *cs {
 		if chg.Status == "DRAFT" {
@@ -76,15 +80,19 @@ func (s service) List(ctx context.Context, rs string, opt change.ListOptions) ([
 func (s service) Count(_ context.Context, repo string, opt change.ListOptions) (uint64, error) {
 	// TODO.
 	return 0, nil
 }
 
-func (s service) Get(ctx context.Context, _ string, id uint64) (change.Change, error) {
-	chg, _, err := s.cl.Changes.GetChange(fmt.Sprint(id), &gerrit.ChangeOptions{
+func (s service) Get(ctx context.Context, repo string, id uint64) (change.Change, error) {
+	project := project(repo)
+	chg, resp, err := s.cl.Changes.GetChange(fmt.Sprintf("%s~%d", project, id), &gerrit.ChangeOptions{
 		AdditionalFields: []string{"DETAILED_ACCOUNTS", "MESSAGES", "ALL_REVISIONS"},
 	})
 	if err != nil {
+		if resp != nil && resp.StatusCode == http.StatusNotFound {
+			return change.Change{}, os.ErrNotExist
+		}
 		return change.Change{}, err
 	}
 	if chg.Status == "DRAFT" {
 		return change.Change{}, os.ErrNotExist
 	}
@@ -113,16 +121,20 @@ func state(status string) change.State {
 	default:
 		panic("unreachable")
 	}
 }
 
-func (s service) ListCommits(ctx context.Context, _ string, id uint64) ([]change.Commit, error) {
-	chg, _, err := s.cl.Changes.GetChange(fmt.Sprint(id), &gerrit.ChangeOptions{
+func (s service) ListCommits(ctx context.Context, repo string, id uint64) ([]change.Commit, error) {
+	project := project(repo)
+	chg, resp, err := s.cl.Changes.GetChange(fmt.Sprintf("%s~%d", project, id), &gerrit.ChangeOptions{
 		AdditionalFields: []string{"DETAILED_ACCOUNTS", "ALL_REVISIONS"},
 		//AdditionalFields: []string{"ALL_REVISIONS", "ALL_COMMITS"}, // TODO: Consider using git committer/author instead...
 	})
 	if err != nil {
+		if resp != nil && resp.StatusCode == http.StatusNotFound {
+			return nil, os.ErrNotExist
+		}
 		return nil, err
 	}
 	if chg.Status == "DRAFT" {
 		return nil, os.ErrNotExist
 	}
@@ -137,23 +149,30 @@ func (s service) ListCommits(ctx context.Context, _ string, id uint64) ([]change
 		}
 	}
 	return commits, nil
 }
 
-func (s service) GetDiff(ctx context.Context, _ string, id uint64, opt *change.GetDiffOptions) ([]byte, error) {
+func (s service) GetDiff(ctx context.Context, repo string, id uint64, opt *change.GetDiffOptions) ([]byte, error) {
+	project := project(repo)
 	switch opt {
 	case nil:
-		diff, _, err := s.cl.Changes.GetPatch(fmt.Sprint(id), "current", nil)
+		diff, resp, err := s.cl.Changes.GetPatch(fmt.Sprintf("%s~%d", project, id), "current", nil)
 		if err != nil {
+			if resp != nil && resp.StatusCode == http.StatusNotFound {
+				return nil, os.ErrNotExist
+			}
 			return nil, err
 		}
 		return []byte(*diff), nil
 	default:
-		chg, _, err := s.cl.Changes.GetChange(fmt.Sprint(id), &gerrit.ChangeOptions{
+		chg, resp, err := s.cl.Changes.GetChange(fmt.Sprintf("%s~%d", project, id), &gerrit.ChangeOptions{
 			AdditionalFields: []string{"ALL_REVISIONS"},
 		})
 		if err != nil {
+			if resp != nil && resp.StatusCode == http.StatusNotFound {
+				return nil, os.ErrNotExist
+			}
 			return nil, err
 		}
 		if chg.Status == "DRAFT" {
 			return nil, os.ErrNotExist
 		}
@@ -166,11 +185,11 @@ func (s service) GetDiff(ctx context.Context, _ string, id uint64, opt *change.G
 		case 1:
 			base = ""
 		default:
 			base = fmt.Sprint(r.Number - 1)
 		}
-		files, _, err := s.cl.Changes.ListFiles(fmt.Sprint(id), opt.Commit, &gerrit.FilesOptions{
+		files, _, err := s.cl.Changes.ListFiles(fmt.Sprintf("%s~%d", project, id), opt.Commit, &gerrit.FilesOptions{
 			Base: base,
 		})
 		if err != nil {
 			return nil, err
 		}
@@ -179,11 +198,11 @@ func (s service) GetDiff(ctx context.Context, _ string, id uint64, opt *change.G
 			sortedFiles = append(sortedFiles, file)
 		}
 		sort.Strings(sortedFiles)
 		var diff string
 		for _, file := range sortedFiles {
-			diffInfo, _, err := s.cl.Changes.GetDiff(fmt.Sprint(id), opt.Commit, file, &gerrit.DiffOptions{
+			diffInfo, _, err := s.cl.Changes.GetDiff(fmt.Sprintf("%s~%d", project, id), opt.Commit, file, &gerrit.DiffOptions{
 				Base:    base,
 				Context: "5",
 			})
 			if err != nil {
 				return nil, err
@@ -231,18 +250,22 @@ func (s service) GetDiff(ctx context.Context, _ string, id uint64, opt *change.G
 		}
 		return []byte(diff), nil
 	}
 }
 
-func (s service) ListTimeline(ctx context.Context, _ string, id uint64, opt *change.ListTimelineOptions) ([]interface{}, error) {
+func (s service) ListTimeline(ctx context.Context, repo string, id uint64, opt *change.ListTimelineOptions) ([]interface{}, error) {
 	// TODO: Pagination. Respect opt.Start and opt.Length, if given.
 
-	chg, _, err := s.cl.Changes.GetChangeDetail(fmt.Sprint(id), nil)
+	project := project(repo)
+	chg, resp, err := s.cl.Changes.GetChangeDetail(fmt.Sprintf("%s~%d", project, id), nil)
 	if err != nil {
+		if resp != nil && resp.StatusCode == http.StatusNotFound {
+			return nil, os.ErrNotExist
+		}
 		return nil, err
 	}
-	comments, _, err := s.cl.Changes.ListChangeComments(fmt.Sprint(id))
+	comments, _, err := s.cl.Changes.ListChangeComments(fmt.Sprintf("%s~%d", project, id))
 	if err != nil {
 		return nil, err
 	}
 	var timeline []interface{}
 	timeline = append(timeline, change.Comment{ // CL description.